AddFDef would be useful for custom fields as well as being useful for checksetup, and so should be moved into the upcoming "Field.pm" module. We should also rename the function, probably something like add_fielddef or add_field_def.

Okay, here we go. Note that this changes how Bugzilla generates the "sortkey" field for fielddefs. Before, it used to generate it by the order of the AddFDef statements, and regenerate it if you added a new AddFDef statement. Now, it generates it by adding 100 to the highest sortkey currently in existence, and never regenerates it. I don't think this matters much, and it's much simpler. I don't think it matters because fielddefs.sortkey is not used anywhere extremely important, that I know of.

Comment on attachment 229641 [details] [diff] [review] v1 bitrotten. Apologies for waiting so long.

Okay, I fixed the bitrot, but this patch is broken. The problem is that create_or_update won't work before the database is upgraded, because it depends on fielddefs having its modern structure. But ideally I'd like to be able to call create_or_update throughout checksetup, before the table is ugpraded fully. So I have to figure that out, somehow.

Okay, this version works! I just moved all of the fielddefs schema changes to the top of checksetup. This is fine, I tested it and it works now. I'll say as module owner that the checksetup changes are okay, and you don't have to review those if you don't want to. The only thing I'd like review on is any of the Bugzilla::Field changes that I made, and anything outside of checksetup that I touched.

Comment on attachment 231560 [details] [diff] [review] v2 >Index: checksetup.pl Assumed this was ok, only glanced over it. >@@ -329,6 +329,8 @@ > > require Bugzilla::DB; > require Bugzilla::Template; >+# For create_or_update >+require Bugzilla::Field; Nit: bitrot. >@@ -443,101 +445,138 @@ >+# NOTE: All of these entries are unconditional, from when get_field_id >+# used to create an entry if it wasn't found. New fielddef columns should >+# be created with their associated schema change. >+use constant OLD_FIELD_DEFS => ( >+ {name => 'bug_group', desc => 'Group'}, >@@ -1778,7 +1820,7 @@ > } > # Replace old activity log groupset records with lists of names of groups. > # Start by defining the bug_group field and getting its id. >- AddFDef("bug_group", "Group", 0); >+ Bugzilla::Field::create_or_update({name => "bug_group", desc => "Group"}); Isn't this already done? >Index: customfield.pl In another bug: Might be nice to allow someone to update the description now it is possible. >Index: Bugzilla/Field.pm >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v >retrieving revision 1.14 >diff -u -r1.14 Field.pm >--- Bugzilla/Field.pm 25 Jul 2006 23:20:22 -0000 1.14 >+++ Bugzilla/Field.pm 1 Aug 2006 08:34:44 -0000 >@@ -176,44 +183,61 @@ > > =over > >-=item C<create($name, $desc)> >+=item C<create_or_update({name => $name, desc => $desc, in_new_bugmail => 1, custom => 0})> If this is meant to show the defaults, then in_new_bugmail should be 0. + C<in_new_bugmail> - boolean - Whether this field appears at the + top of the bugmail for a newly-filed bug. That currently only works for custom fields.

Okay, I fixed most of your comments on checkin. I'll attach a new patch showing what was checked in. Checking in checksetup.pl; /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v <-- checksetup.pl new revision: 1.532; previous revision: 1.531 done Checking in customfield.pl; /cvsroot/mozilla/webtools/bugzilla/customfield.pl,v <-- customfield.pl new revision: 1.7; previous revision: 1.6 done Checking in Bugzilla/Field.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v <-- Field.pm new revision: 1.15; previous revision: 1.14 done

Okay, here's what I checked in.