From: <Chr...@he...> - 2002-10-09 16:22:02
|
Hi, as the implemetation suggestions you've sent looked exactly like that what we are needing I've implemented the custom_fields_api.php. This file is attached as well as the required database change and one additional function to the bug_api.php. Note: This code hasn't been tested so far (but it doesn't create runtime errors when called directly). I'll test it as I'm implementing/changing the necessary pages (like bug_report) It would be great if someone can look at it or even place it in CVS till Monday. That's the next day I'm at work and I'll try to improove the code further. CU, Christian |
From: <Chr...@he...> - 2002-10-14 16:28:03
Attachments:
custom_fields_API.php
additional_tables.sql
|
> -----Original Message----- > From: ju...@be... [mailto:ju...@be...] > Sent: Wednesday, October 09, 2002 9:58 PM > To: man...@li... > Subject: Re: [Mantisbt-dev] Custom Fields > > > * db_unprepare_*() are not needed... we thought they were for a while > but no code should actually be using them anymore > * can you set your editor to use tabs instead of spaces and > tab width of > 4? That's what the rest of the code uses and we'll have to fix it > before committing if you don't get that straightened out > * can we use custom_field_definition_* and > custom_field_value_* instead > of the abbreviations? I know they're a little longer but I've grown > away from using abbeviations for function names in most > cases. This may > just be my opinion, others may disagree. > * Can you look at, for example, project_api and copy the way the > functions are organized (boolean queries, then > creation/deletion, etc)? > Also, you could group the definition functions together and the value > one together? > * bug_project_id() should be called bug_get_project_id() but > note that > you're probably better just using bug_get_field( 'project_id' ) or at > very least definining the new function in terms of that code if you > really think it's needed. That will make sure the bug is loaded from > the cache if appropriate. Hi Julian, thanks for the comments. I think I've implemented all of them in the attached code. Thanks for the hint with the "bug_get_field( 'project_id' )". I wasn't aware of that function. As it's exactly doing what I need I got rid of the additional bug_api function :) Now I hope it's passing the peer review soon, so that I can start implementing the web pages next week and use a fresh CVS checkout... CU, Christian |
From: Julian F. <ju...@be...> - 2002-10-14 19:05:12
|
Chr...@he... wrote: >>-----Original Message----- >>From: ju...@be... [mailto:ju...@be...] >>Sent: Wednesday, October 09, 2002 9:58 PM >>To: man...@li... >>Subject: Re: [Mantisbt-dev] Custom Fields >> >> >>* db_unprepare_*() are not needed... we thought they were for a while >>but no code should actually be using them anymore >>* can you set your editor to use tabs instead of spaces and >>tab width of >>4? That's what the rest of the code uses and we'll have to fix it >>before committing if you don't get that straightened out >>* can we use custom_field_definition_* and >>custom_field_value_* instead >>of the abbreviations? I know they're a little longer but I've grown >>away from using abbeviations for function names in most >>cases. This may >>just be my opinion, others may disagree. >>* Can you look at, for example, project_api and copy the way the >>functions are organized (boolean queries, then >>creation/deletion, etc)? >>Also, you could group the definition functions together and the value >>one together? >>* bug_project_id() should be called bug_get_project_id() but >>note that >>you're probably better just using bug_get_field( 'project_id' ) or at >>very least definining the new function in terms of that code if you >>really think it's needed. That will make sure the bug is loaded from >>the cache if appropriate. > > > Hi Julian, > > thanks for the comments. I think I've implemented all of them in the > attached code. > > Thanks for the hint with the "bug_get_field( 'project_id' )". I wasn't > aware of that function. As it's exactly doing what I need I got rid of > the additional bug_api function :) > > Now I hope it's passing the peer review soon, so that I can start > implementing the web pages next week and use a fresh CVS checkout... > > CU, > Christian Hi Christian... just a heads up... I'm going out for Thanksgiving dinner now but I'll try to look at this tonight or tomorrow. I'd still like to hear from Victor on it too, since he was working on the db schema, etc. Julian -- ju...@be... Beta4 Productions (http://www.beta4.com) |
From: Julian F. <ju...@be...> - 2002-10-15 22:06:39
|
Detailed comments are mixed in. I haven't looked at the SQL schema at all since that's truly victor's baby. :) I think we can go through one or two more iterations here and then commit it and iron out the details with patches on that... Also, in general, some of the function comments could do with a line describing what they do in general before giving the return values. Julian Chr...@he... wrote: > Hi Julian, > > thanks for the comments. I think I've implemented all of them in the > attached code. > > Thanks for the hint with the "bug_get_field( 'project_id' )". I wasn't > aware of that function. As it's exactly doing what I need I got rid of > the additional bug_api function :) > > Now I hope it's passing the peer review soon, so that I can start > implementing the web pages next week and use a fresh CVS checkout... > > CU, > Christian > <?php > # Mantis - a php based bugtracking system > # Copyright (C) 2002 Mantis Team - man...@li... > # This program is distributed under the terms and conditions of the GPL > # See the files README and LICENSE for details > > # -------------------------------------------------------- > # $Id: custom_fields_API.php,v 1.4 2002/10/14 15:01:25 mayerch Exp $ > # -------------------------------------------------------- > > ########################################################################### > # Custom Fields API > ########################################################################### > > #=================================== > # Boolean queries and ensures > #=================================== > > # -------------------- > # returns false - bug doesn't exist, invalid field id, ...etc > # return true - otherwise > function custom_field_exists( $p_bug_id, $p_field_id ) { > $c_bug_id = db_prepare_int( $p_bug_id ); > $c_field_id = db_prepare_int( $p_field_id ); We like to align the ='s with tabs when doing several variable assignments in a row... This applies throughout the file. > # figure out if this bug_id/field_id combination exists > $query = "SELECT project_id FROM " . > config_get( 'mantis_custom_fields_strings' ) . > " WHERE id='$c_field_id'"; We've been trying to remove the extra quotes in these select statements. The double quotes allow strings to wrap onto the next line so you can remove the quote-dot-quote stuff throughout this file. In this case, since you're inserting the table name has been to store config_get( 'mantis_foo_table' ) in a variable called $t_foo_table and then use that inside the quotes. I'd like to keep this consistent throughout, if we can. > $result = db_querry( $query ); > > if( false == $result ) > return false; # field does not exist mantis code always uses braces for conditional blocks, even for one line blocks. I don't actually personally agree with it because it makes it longer but it is more explicit and again, we need to do it for consistency. > > $t_project_id = bug_get_field( $c_bug_id, 'project_id' ); > > if( 0 == $t_project_id ) > return false; # bug does not exist again. Also, if the bug doesn't exist, bug_get_field() should generate an error and this call will never return, right? > $row = db_get_row( $result ); > > if( $row[0] != $t_project_id ) > return false; # this bug does not have this field id rather than getting the row and checking the project_id, why not include "project_id = $t_project_id" in the query and select COUNT(*)? (or SELECT 1 and LIMIT 1, as victor suggested elsewhere - though that won't work when we abstract the db so maybe better to stay away from it). I prefer to use a SELECT COUNT when doing an exists check because it's a more literal translation of what you're asking. > return true; > } > > > #=================================== > # Functions for definitions > #=================================== I'm trying to decide about the function naming here... In general I'd prefer custom_field_get_*() and custom_field_set_*(). I understand you're splitting up the definition and value stuff, though, so I'm undecided. But I think custom_field_get_value(), etc are perfectly clear and read better to me (since everywhere else, after the file prefix, the function name reads from left to right). I'm open to debate on this... > # -------------------- > # returns an array of ids of custom fields related to the specified project. > function custom_field_definition_get_ids ( $p_project_id ) { custom_field_get_ids()? is that ambiguous? > $c_project_id = db_prepare_int( $p_project_id ); > > $query = "SELECT id FROM " . > config_get( 'mantis_custom_fields' ) . > " WHERE project_id='$c_project_id'"; > $result = db_querry( $query ); > > return db_fetch_array( $result ); Hmm... this doesn't return what you want, I don't think... db_fetch_array() just gets one row as an array and you want to return all the ids, right? Take a look at user_get_accessible_projects() which does something similar. > } > > # -------------------- > # returns an associative array with all fields. > # returns false if project id / field id combination is not found. > function custom_field_definition_get ( $p_project_id, $p_field_id ) { I think I'd say custom_field_get_definition() or custom_field_get_definition_row(). Most functions that return an entire DB row end with '_row' (user_get_profile_row() for example)... not sure if we need to stick with that in all cases... > $c_project_id = db_prepare_int( $p_project_id ); > $c_field_id = db_prepare_int( $p_field_id ); > > $query = "SELECT * FROM " . > config_get( 'mantis_custom_fieldss' ) . > " WHERE id='$c_field_id'" . > " AND project_id='$c_project_id'"; > $result = db_querry( $query ); > > if( false == $result ) > return false; > > return db_fetch_array( $result ); > } > > # -------------------- > # $p_def_array is an associative array that contains the following > # information: 'caption', 'type', 'values', 'default', 'regexp', > # 'access_level_r', 'access_level_rw', 'length_min', > # 'length_max', 'advanced' > # should add the value with the default id for this field for all existing > # bugs belonging to the project. What will happen if the default was changed > # later? Should the bug fields change? (i.e. save actual default value or > # --flag--default), ideas? > # return false - error during adding field definition > # return true - field added successfully > function custom_field_definition_add ( $p_project_id, $p_def_array ) { I think custom_field_add() is sufficient? You get/set a value, you add/delete a definition... The use of an associative array to simulate named parameters is something I have considered (for example when creating a bug). But we've never done it so we should discuss it. I would argue, I think, that it might be nicer just to create field struct (class) that has all the proper fields in it. Then the values can just be initialized to their default values and we don't need all the code below. I'm not sure if this fits into Ken's acceptable use of classes policy or not - though we've sort of discussed it before. My argument is that we're not using classes in the sense of having complex inheritance or even having method calls, just using them as cleaner data storage mechanisms (and structs are familiar even to C programmers). > $c_project_id = db_prepare_int($p_project_id ); > > if( array_key_exists('caption', $p_def_array) ) > $c_caption = db_prepare_string( $p_def_array['caption'] ); > else > return false; > > if( array_key_exists('type', $p_def_array) ) > $c_type = db_prepare_int( $p_def_array['type'] ); > else > $c_type = 0; If we do decide to keep the associative array, consider adding a helper_array_key() function or something that takes the array, the key, and a default value. That way you can reduce all of these 4-line blocks to 1 line: $c_foo = helper_array_key( 'foo', $p_def_array, 0 ); for example. Also note that there should be a space between 'if' and the bracket, and that there should be spaces before the first (and after the last) parameter in a function call. (see http://mantisbt.sourceforge.net/guidelines.php3 for the complete coding guidelines) > if( array_key_exists('values', $p_def_array) ) > $c_values = db_prepare_string( $p_def_array['values'] ); > else > $c_values = ''; > > if( array_key_exists('default', $p_def_array) ) > $c_default = db_prepare_string( $p_def_array['default'] ); > else > $c_default = ''; > > if( array_key_exists('regexp', $p_def_array) ) > $c_regexp = db_prepare_string( $p_def_array['regexp'] ); > else > $c_regexp = ''; > > if( array_key_exists('access_level_r', $p_def_array) ) > $c_access_level_r = db_prepare_int( $p_def_array['access_level_r'] ); > else > $c_access_level_r = 0; > > if( array_key_exists('access_level_rw', $p_def_array) ) > $c_access_level_rw = db_prepare_int( $p_def_array['access_level_rw'] ); > else > $c_access_level_rw = 0; > > if( array_key_exists('length_min', $p_def_array) ) > $c_length_min = db_prepare_int( $p_def_array['length_min'] ); > else > $c_length_min = 0; > > if( array_key_exists('length_max', $p_def_array) ) > $c_length_max = db_prepare_int( $p_def_array['length_max'] ); > else > $c_length_max = 0; > > if( array_key_exists('advanced', $p_def_array) ) > $c_advanced = db_prepare_int( $p_def_array['advanced'] ); > else > $c_advanced = 0; > > $query = "INSERT INTO " . > config_get( 'mantis_custom_fields' ) . > " ( project_id, " . > " caption, type, values, default, regexp, " . > " access_level_r, access_level_rw, length_min, " . > " length_max, advanced )" . > " VALUES " . > " ( '$c_project_id', " . > " '$c_caption', '$c_type', 'c_values', '$c_default', '$c_regexp', " . > " '$c_access_level_r', '$c_access_level_rw', '$c_length_min', " . > " '$c_length_max', '$c_advanced' )"; > if ( false == db_querry( $query ) ) db_query (typo) > return false; > else > return true; you don't need this conditional. db_query() exits if there is an error so it will never return in that case. I use the following comment and code at the end of such functions for consistency: # db_query() errors on failure so: return true; > } > > # -------------------- > # returns false project id / field id not found or update failed. > # returns true success > function custom_field_definition_update ( $p_project_id, $p_field_id, $p_def_array ) { again, I'd prefer custom_field_update() or custom_field_update_definition() Again we need to think about whether we want associative arrays for parameters. They tend to make longer code in the caller. Since the caller is likely a viewed page and we want to keep those files short, I'm not sure the trade-off is worth it. > $c_project_id = db_prepare_int($p_project_id ); > $c_field_id = db_prepare_int($p_field_id ); > $c_caption = db_prepare_string( $p_def_array['caption'] ); > $c_type = db_prepare_int( $p_def_array['type'] ); > $c_values = db_prepare_string( $p_def_array['values'] ); > $c_default = db_prepare_string( $p_def_array['default'] ); > $c_regexp = db_prepare_string( $p_def_array['regexp'] ); > $c_access_level_r = db_prepare_int( $p_def_array['access_level_r'] ); > $c_access_level_rw = db_prepare_int( $p_def_array['access_level_rw'] ); > $c_length_min = db_prepare_int( $p_def_array['length_min'] ); > $c_length_max = db_prepare_int( $p_def_array['length_max'] ); > $c_advanced = db_prepare_int( $p_def_array['advanced'] ); You db_prepare the values here and then don't use them and do it again below... > $query = "UPDATE " . > config_get( 'mantis_custom_fields' ); > > if( array_key_exists('caption', $p_def_array) ) > $query .= " SET caption='" . db_prepare_string( $p_def_array['caption'] ) . "',"; > if( array_key_exists('type', $p_def_array) ) > $query .= " SET type='" . db_prepare_int( $p_def_array['type'] ) . "',"; > if( array_key_exists('values', $p_def_array) ) > $query .= " SET values='" . db_prepare_string( $p_def_array['values'] ) . "',"; > if( array_key_exists('default', $p_def_array) ) > $query .= " SET default='" . db_prepare_string( $p_def_array['default'] ) . "',"; > if( array_key_exists('regexp', $p_def_array) ) > $query .= " SET regexp='" . db_prepare_string( $p_def_array['regexp'] ) . "',"; > if( array_key_exists('access_level_r', $p_def_array) ) > $query .= " SET access_level_r='" . db_prepare_int( $p_def_array['access_level_r'] ) . "',"; > if( array_key_exists('access_level_rw', $p_def_array) ) > $query .= " SET access_level_rw='" . db_prepare_int( $p_def_array['access_level_rw'] ) . "',"; > if( array_key_exists('length_min', $p_def_array) ) > $query .= " SET length_min='" . db_prepare_int( $p_def_array['length_min'] ) . "',"; > if( array_key_exists('length_max', $p_def_array) ) > $query .= " SET length_max='" . db_prepare_int( $p_def_array['length_max'] ) . "',"; > if( array_key_exists('advanced', $p_def_array) ) > $query .= " SET advanced='" . db_prepare_int( $p_def_array['advanced'] ) . "',"; > > $query .= " WHERE id='$c_field_id'" . > " AND project_id='$c_project_id'"; > > if ( false == db_querry( $query ) ) > return false; > else > return true; same note as above about how to return from these functions > } > > # -------------------- > # deletes the field definition and all values for it (for all bugs). > # returns false if project_id and field_id don't match > # returns true success > function custom_field_definition_delete ( $p_project_id, $p_field_id ) { I'd call this custom_field_delete(), particularly since we aren't just deleting the definition but every value associated with it... > $c_project_id = db_prepare_int($p_project_id ); > $c_field_id = db_prepare_int($p_field_id ); aha! alignment!! :) > # do I need to update or insert this value? > $query = "SELECT COUNT(*) FROM " . > config_get( 'mantis_custom_fields' ) . > " WHERE id='$c_field_id'" . > " AND project_id='$c_project_id'"; > $result = db_querry( $query ); db_query() > if ( 0 == db_result( $result ) ) > return false; this would all be cleaner by having a custom_field_exists() that took the project_id and field_id. Perhaps you should rename your existing custom_field_exists() to custom_field_is_value_defined() or something... until I got here and went back and looked I assumed it was checking whether the definition existed. > # delete all values > $query = "DELETE FROM " . > config_get( 'mantis_custom_fields_strings' ) . > " WHERE field_id='$c_field_id'"; > db_query( $query ); > > # delete the definition > $query = "DELETE FROM " . > config_get( 'mantis_custom_fields' ) . > " WHERE id='$c_field_id' " . > " AND project_id='$c_project_id'"; > db_query( $query ); > > return true; > } > > # -------------------- > # deletes all custom fields defined for the specified project including the > # values associated with the project bugs. To be called from within > # project_delete(). > function custom_field_definition_delete_all ( $p_project_id ) { custom_field_delete_all() ? > $c_project_id = db_prepare_int( $p_project_id ); > > $t_field_ids = custom_field_definition_get_ids( $p_project_id ); > > for( $index=0; $index < count( $t_field_ids ); $index++ ) > custom_field_definition_delete ( $p_project_id, $t_field_ids[$index] ); Maybe use foreach as victor suggested. Also, need braces and indentation. > } > > #=================================== > # Data Access > #=================================== > > # -------------------- > # returns false - bug doesn't exist, invalid field id, ...etc > # return value - otherwise (value found or default returned) > function custom_field_value_get( $p_bug_id, $p_field_id ) { custom_field_get() (should be unambiguous) or custom_field_get_value() at least. > $c_bug_id = db_prepare_int( $p_bug_id ); > $c_field_id = db_prepare_int( $p_field_id ); > > if ( !custom_field_exists( $p_bug_id, $p_field_id ) ) > return false; you could just check the number of rows returned below to see if it's 0... that way you only need one query > $query = "SELECT project_id, access_level_r FROM " . > config_get( 'mantis_custom_fields_strings' ) . > " WHERE bug_id='$c_bug_id'" . > " AND field_id='$c_field_id'"; > $result = db_querry( $query ); > $row = db_get_row( $result ); hmm... have you tested this code? :) there's no such thing as db_get_row(); you want db_fetch_array() > $t_project_id = $row['project_id']; > $t_access_level_r = $row['access_level_r']; > > if( !access_level_check_greater_or_equal( $t_access_level_r, $t_project_id ) ) > return false; hmm.. I'd prefer to see the access level being checked for the project obtained from bug_get_field( $p_bug_id, 'project_id' )... since we're modifying the bug, the permissions should be based on the actual values in the bug. I don't actually understand why we need the project_id in this DB table at all actually... Victor, isn't the field_id globally unique? Can't the field_id by itself be a primary key so we can just look up the project id in the definition table if we need it? This seems like duplication of information and could accidentaly get out of date. > $query = "SELECT value FROM " . > config_get( 'mantis_custom_fields_strings' ) . > " WHERE bug_id='$c_bug_id'" . > " AND field_id='$c_field_id'"; > $result = db_querry( $query ); db_query() > $row = db_fetch_row( $result ); db_fetch_array() > if( false != $result ) > return $row[0]; again, result cannot be false, we would have already exited with an error > $query = "SELECT default FROM " . > config_get( 'mantis_custom_fields' ) . > " WHERE id='$c_field_id'"; > $result = db_querry( $query ); db_query() (I'm going to stop marking these :) > $row = db_fetch_row( $result ); db_fetch_array() (gonna stop marking these too :) > if( false != $result ) > return $row[0]; and again, result won't be false > } > > #=================================== > # Data Modification > #=================================== > > # -------------------- > # returns false - bug doesn't exist, invalid field id, invalid value, ...etc > # return true - otherwise > function custom_field_value_set( $p_bug_id, $p_field_id, $p_value ) { custom_field_set() or custom_field_set_value() > $c_bug_id = db_prepare_int( $p_bug_id ); > $c_field_id = db_prepare_int( $p_field_id ); > $c_value = db_prepare_string( $p_value ); > > if ( !custom_field_exists( $p_bug_id, $p_field_id ) ) > return false; hmm... are you confused yourself here? I think this is a case where you'd want to use the new custom_field_exists() I was proposing above. The current implementation checks for a value which isn't what you want right? If there's no value defined, that's not an error, we're just going to define one now. > $query = "SELECT project_id, type, values, regexp, access_level_rw, " . > " length_min, length_max FROM " . > config_get( 'mantis_custom_fields_strings' ) . > " WHERE bug_id='$c_bug_id'" . > " AND field_id='$c_field_id'"; > $result = db_querry( $query ); > $row = db_get_row( $result ); > > $t_project_id = $row['project_id']; > $t_type = $row['type']; > $t_values = $row['values']; > $t_regexp = $row['regexp']; > $t_access_level_rw = $row['access_level_rw']; > $t_length_min = $row['length_min']; > $t_length_max = $row['length_max']; We usually use something like: for($i=0; $i < sizeof( $rows ); $i++) { # prefix bug data with v_ extract( $rows[$i], EXTR_PREFIX_ALL, 'v' ); } For stuff like this... ( you could use foreach of course ) > # check for valid value > if ( !ereg( $t_regexp, $c_value ) ) > return false; for all of these we should think whether we want to return false or trigger_error()... I'm not sure: it depends how it's being used. > if ( strlen( $c_value ) < $t_length_min ) > return false; > > if (( strlen( $c_value ) > $t_length_max ) && (0 != $t_length_max)) > return false; > > if( !access_level_check_greater_or_equal( $t_access_level_rw, $t_project_id ) ) > return false; > > # do I need to update or insert this value? > $query = "SELECT COUNT(*) FROM " . > config_get( 'mantis_custom_fields_strings' ) . > " WHERE bug_id='$c_bug_id'" . > " AND field_id='$c_field_id'"; > $result = db_querry( $query ); > if ( db_result( $result ) > 0 ) { You can use custom_field_exists (or custom_field_is_value_defined() as I proposed it) here instead of this select statement. > $query = "UPDATE " . > config_get( 'mantis_custom_fields_strings' ) . > " SET value='$c_value'" . > " WHERE bug_id='$c_bug_id'" . > " AND field_id='$c_field_id'"; > db_querry( $query ); > } else { > $query = "INSERT INTO " . > config_get( 'mantis_custom_fields_strings' ) . > " ( bug_id, field_id, value )" . > " VALUES " . > " ( '$c_bug_id', '$c_field_id', '$c_value' )"; > db_querry( $query ); > } > > return true; > } > > # -------------------- > # deletes all custom values associated with the specified bug. To be called > # from bug_delete(). > function custom_field_value_delete_all ( $p_bug_id ) { hmm... custom_field_delete_all_values() I guess... > $c_bug_id = db_prepare_int($p_bug_id ); > > $query = "DELETE FROM " . > config_get( 'mantis_custom_fields_strings' ) . > " WHERE bug_id='$c_bug_id'"; > db_query( $query ); > } > ?> -- ju...@be... Beta4 Productions (http://www.beta4.com) |
From: <Chr...@he...> - 2002-10-21 16:29:55
|
Thanks! This week my time was too limited to incooperate all these changes :( But I'll try to finish ist next monday. CU, Christian > -----Original Message----- > From: ju...@be... [mailto:ju...@be...] > Sent: Wednesday, October 16, 2002 12:02 AM > To: man...@li... > Subject: Re: [Mantisbt-dev] Custom Fields > > > Detailed comments are mixed in. I haven't looked at the SQL > schema at > all since that's truly victor's baby. :) I think we can go > through one > or two more iterations here and then commit it and iron out > the details > with patches on that... > [...] |
From: Julian F. <ju...@be...> - 2002-10-09 19:58:01
|
Chr...@he... wrote: > Hi, > > as the implemetation suggestions you've sent looked exactly like that > what we are needing I've implemented the custom_fields_api.php. > > This file is attached as well as the required database change and one > additional function to the bug_api.php. > > Note: This code hasn't been tested so far (but it doesn't create > runtime errors when called directly). > > I'll test it as I'm implementing/changing the necessary pages (like > bug_report) > > It would be great if someone can look at it or even place it in CVS > till Monday. That's the next day I'm at work and I'll try to improove > the code further. > > CU, > Christian Hi Christian, I'm hoping Victor will review this in more detail for you but I thought I'd throw a few of my own thoughts into the mix anyway. * db_unprepare_*() are not needed... we thought they were for a while but no code should actually be using them anymore * can you set your editor to use tabs instead of spaces and tab width of 4? That's what the rest of the code uses and we'll have to fix it before committing if you don't get that straightened out * can we use custom_field_definition_* and custom_field_value_* instead of the abbreviations? I know they're a little longer but I've grown away from using abbeviations for function names in most cases. This may just be my opinion, others may disagree. * Can you look at, for example, project_api and copy the way the functions are organized (boolean queries, then creation/deletion, etc)? Also, you could group the definition functions together and the value one together? * bug_project_id() should be called bug_get_project_id() but note that you're probably better just using bug_get_field( 'project_id' ) or at very least definining the new function in terms of that code if you really think it's needed. That will make sure the bug is loaded from the cache if appropriate. Those are my comments from a quick perusal, I haven't actually looked at the validity of the code. It looks like in general you've done a pretty good job of figuring out all the coding conventions, etc... good work. Now I'll hand it over to Victor... Julian -- ju...@be... Beta4 Productions (http://www.beta4.com) |