287325 - Ability to add custom plain-text fields to a Bug

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P1)

Description

[Max Kanat-Alexander]

The most basic type of custom field is a plain-text field. This field would
basically be just like the status whiteboard.

It would be nice to be able to add more fields like this one to Bugzilla. This
is a good place to start with custom fields, because plain-text fields are so
simple.

I'm not entirely certain whether this should block or depend on bug 287323, but
for now I'm saying it depends on it, because it seems like it'd be easier to
separate out the actual creation of the framework from the creation of the
interface for adding fields.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: implements plain-text custom fields

Here's a basic implementation.	It comes with a command-line script for adding
a custom field, lets you search custom fields via the boolean chart, displays
them on bug pages, lets you edit them, commits your changes, and includes them
in XML bug output.

Comment 2

[Max Kanat-Alexander]

Comment on attachment 179782 [details] [diff] [review]
patch v1: implements plain-text custom fields

You know, I actually like this implementation. However, there are some things
that I'd like to see changed about it.

I'd like to have a Field.pm module to start with, so that we don't later have
to have a "move things to Field.pm" later. For now it doesn't have to be a real
object, but it should contain things like add_field.

Ideally, though, a Field would be an object, and it would know its type and
other attributes from the fielddefs table. If it was really cool, it would even
know its current value on a Bug.

We could also put the custom_fields sub that you have in there.

You also need a "WHERE custom = 1" for cross-db compatibility, in that sub.

Also, that raw ALTER TABLE statement needs to be replaced with a
$dbh->bz_add_field, which will actually be $dbh->bz_add_column in a few days,
so you might as well start with that anyhow (It's already in Bugzilla::DB, just
read the POD docs and ask me if you have any questions).

I'd also like to see a standard naming scheme for custom fields, so that they
all start with something like cf_ when they're in the database.

The FORM stuff in process_bug will probably be going away before this patch is
checked in, also, so if we could use $cgi there that would be better.

There's no need to import :deprecated from Bugzilla::DB in show_bug.

That little "Custom Fields" block in the template should probably be its own
template, and it should probably be either passed a list of custom fields, or
should render one field and be passed one field (that's my preference).

You should move AddFDef from checksetup.pm into Field.pm, and then use that to
add your new field to the fielddefs table, in the command-line script.

OK, those are all the comments that I can come up with right now. Any from you,
Joel?

Comment 3

[Max Kanat-Alexander]

I'd like to do the Field.pm stuff as part of bug 287324, if possible, just to
keep it split up.

Comment 4

[Joel Peshkin]

Comment on attachment 179782 [details] [diff] [review]
patch v1: implements plain-text custom fields


I like this approach.  Let's get mkanat's comments addressed and plug away at
landing this.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress

Comment 6

[Max Kanat-Alexander]

Comment on attachment 181235 [details] [diff] [review]
work in progress

Looks good!

Hrm... perhaps we could have a Bugzilla->fields method to get the current list
of custom fields, that would replace get_custom_fields (at least for regular
callers)?

Is get_fields intended to be public, or just a private subroutine for
Bugzilla::Field?

Is the "$criterion != "1=1";" line in get_fields doing anything?

Oh, and I think that getting most of addcustomfield into a sub in Field.pm
would be good, too.

Those are just a few of my thoughts on a brief glance. :-) I understand it's a
work-in-progress, too; just thought I'd give it a real quick look-over.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: addresses mkanat's review comments

>I'd like to have a Field.pm module to start with

Ok, done.

>You also need a "WHERE custom = 1" for cross-db compatibility

Fixed.

>Also, that raw ALTER TABLE statement needs to be replaced with a
>$dbh->bz_add_field, which will actually be $dbh->bz_add_column in a few days

I've added in the function call but commented out until the function is ready.

>I'd also like to see a standard naming scheme for custom fields, so that they
>all start with something like cf_ when they're in the database.

Ok, done.  They start with cf_.

>The FORM stuff in process_bug will probably be going away before this patch is

>checked in, also, so if we could use $cgi there that would be better.

Roger that.  I use $cgi there now.

>There's no need to import :deprecated from Bugzilla::DB in show_bug.

Removed (as is all modification of show_bug.cgi now that I can access the list
of custom fields within the template via the Bugzilla object).

>That little "Custom Fields" block in the template should probably be its own
>template, and it should probably be either passed a list of custom fields, or
>should render one field and be passed one field (that's my preference).

It's now field.html.tmpl, and it gets passed one field.

>You should move AddFDef from checksetup.pm into Field.pm, and then use that to

>add your new field to the fielddefs table, in the command-line script.

I haven't done this yet.  AddFDef updates the fielddefs table for standard
fields, and integrating custom field creation into it (and making checksetup.pl
use it) will require some thought.  This might be better as a separate bug.

>I'd like to do the Field.pm stuff as part of bug 287324, if possible, just to
>keep it split up.

Necessity is the mother of invention, and in this case I think we're better off
creating the Field object in a patch that actually uses it for something rather
than separately inventing one divorced from known use cases.

>perhaps we could have a Bugzilla->fields method to get the current list
>of custom fields, that would replace get_custom_fields (at least for regular
>callers)?

Good idea, but I created separate Bugzilla->fields and Bugzilla->custom_fields
methods, since callers will sometimes want the complete list (f.e. whatever
populates the list of fields in the boolean chart), and the concept of "fields"
encompasses both types.  We will probably also want "current_fields" and
"standard_fields" accessors in the future.

The other option here is to create a parameterized interface to fields so that
we can say things like:

Bugzilla->fields({custom=>1,obsolete=>0})

But then we have to call fields as a function; we can't use it as a property of
the Bugzilla object.  Not sure what the consequences are there; perhaps
minimal.

>Is get_fields intended to be public, or just a private subroutine for
>Bugzilla::Field?

It was public, which has some advantages, but I've made it private, which has
others, in this iteration.

>Is the "$criterion != "1=1";" line in get_fields doing anything?

Yes.  It made get_fields work when no criterion was passed to the function,
since we blindly added $criterion to the WHERE clause of the query, and an
empty $criterion would generate an empty--and thus illegal--WHERE clause.  I've
eliminated it in the current version, however, in favor of checking to see if
we get passed any criteria before generating the WHERE clause.

>Oh, and I think that getting most of addcustomfield into a sub in Field.pm
>would be good, too.

Ok, done.

In addition to addressing the review comments, I also made activity logging
work for custom fields.

Comment 8

[Nick Barnes]

In edit.html.tmpl, given that you say

   [% FOREACH field = Bugzilla.custom_fields %]

why do you then say:	

    [% NEXT IF !field.custom %]

?
All of the custom_fields by definition have field.custom == 1.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

> All of the custom_fields by definition have field.custom == 1.

Right.  The NEXT statement is a remnant from an earlier version of the patch. 
I'll remove it in the next version.

Comment 10

[Max Kanat-Alexander]

Comment on attachment 181359 [details] [diff] [review]
patch v2: addresses mkanat's review comments

> Bugzilla->fields({custom=>1,obsolete=>0})
>
> But then we have to call fields as a function; we can't use it as a property
> of the Bugzilla object.  Not sure what the consequences are there; perhaps
> minimal.

  I like that idea. If performance becomes a problem ever in the future, we
could just cache the DB results and filter them in perl based on the criteria.
I don't think we need to worry about that now, though.

>+    push(@fields, map($_->{name}, Bugzilla->custom_fields));

  Perhaps we could use a convenience function for that, that just returns a
list of field names?

>@@ -149,6 +151,12 @@
> 
>   $self->{'who'} = new Bugzilla::User($user_id);
> 
>+    my $custom_fields = "";
>+    if (length(Bugzilla->custom_fields) > 0) {

  Why the length() check and not just a direct check?

>Index: Bugzilla/Field.pm

  I think we could use some accessors for the fields, instead of calling them
directly. That allows us the convenience of later modifying the accessor
subroutines without modifying the calling code.

> [snip]
>+# Contributor(s): Shane H. W. Travis <[e-mail]>
>+#                 Max Kanat-Alexander <[e-mail]>

  And you! :-D In fact, just you. :-)

>+our @attributes = ("fieldid AS id",

  In some new patches (newer than this patch), we've been making this a
constant called DB_COLUMNS, and then also having an "our" variable called
$columns to put into SQL statements.

>+sub new {
>+    my $self = shift || Bugzilla->dbh->selectrow_hashref("$select

  Why would you ever be passing an object to itself for new()? (The "shift.")

>+                                                          FROM fielddefs
>+                                                          WHERE name = ?",

  Also, it seems like we should attempt to use ids instead of names, where we
can. Or use the type of mechanism we have for the new Product.pm stuff (in bug
294160), where you can pass in either a name or an id.

>+    # XXX Replace ALTER TABLE query with call to bz_add_column once the latter is available.
>+    #$dbh->bz_add_column("bugs", $name, { TYPE => 'TINYTEXT' });

  Oh, and just as a note, it's available now, so we're good.

>+__END__
>+
>+=head1 NAME
>+Bugzilla::Field - a discrete piece of information about bugs

  I think I generally like having the POD docs throughout the file instead of
at the end. The convenience is that then when I'm looking at a function and
trying to work on it, I have its docs on the same screen.

>+
>+=head1 SYNOPSIS

  The SYNOPSIS and DESCRIPTION sections are reversed, for this module. (That
is, the contents of one should be the contents of the other.)

>+Field.pm defines field objects, which represent the discrete pieces
>+of information that Bugzilla stores about bugs.  Bugzilla stores Fields
>+in the fielddefs table, and callers access them through field objects
>+and the Bugzilla::fields/Bugzilla::custom_fields properties.

  Make that Bugzilla->fields and Bugzilla->custom_fields, I think, just so
readers don't get confused on how to call them.

  Also, details of how we store fields might not be good for API docs, but I'll
leave that up to you.

>Index: Bugzilla/DB/Schema.pm
>@@ -435,6 +435,9 @@
>             fieldid     => {TYPE => 'MEDIUMSERIAL', NOTNULL => 1,
>                             PRIMARYKEY => 1},
>             name        => {TYPE => 'varchar(64)', NOTNULL => 1},
>+            type        => {TYPE => 'INT2', NOTNULL => 1, DEFAULT => 0},

  Perhaps we should actually put the UNKNOWN_FIELD_TYPE constant, there? Or
perhaps not... hrm... I'm not too sure.


  Everything else looks pretty good to me! :-) I think I'm getting a bit
overworked on this patch, so I think I'd like Jouni to look over it, also. My
r- isn't really a "hard" r-, it's just "I had a lot of nit-like comments, so
some of them should probably be fixed."

Comment 11

[Jouni Heikniemi]

Comment on attachment 181359 [details] [diff] [review]
patch v2: addresses mkanat's review comments

"Hard r-" for bitrot; not being able to test makes this particularly difficult
for me, as I'm not as well versed in custom fields as you guys. Miscellaneous
comments follow:


How do we handle localizing the custom field labels (descriptions)? OK, we
don't do it for product descriptions (and certain other fields) either, but
OTOH, we should avoid building localization traps for ourselves. If you guys
agree l10n is not a worthy issue here, I'm fine with it... but let's remember
we allow standard field names to be mainly localized anyway (through
templates).


>Index: Bugzilla.pm
>===================================================================

>+sub _get_fields {
>+    my $class = shift;
>+    my $criteria = shift;

Nit: Please add a comment to explain the meaning of the criteria param... I
know it's private, but still. :-)

>Index: addcustomfield.pl
>===================================================================

>+# The Initial Developer of the Original Code is Netscape Communications
>+# Corporation. Portions created by Netscape are
>+# Copyright (C) 1998 Netscape Communications Corporation. All
>+# Rights Reserved.
>+# Contributor(s): Myk Melez <[e-mail]>

I'm not the boilerplate expert, but which part of the code is developed by
Netscape (or its employees) here?

>Index: checksetup.pl
>===================================================================

>+# 2005-02-01 Myk Melez <[e-mail]> bug 91037

Wrong bug number.

>Index: process_bug.cgi
>===================================================================
>+    if (defined $cgi->param($field->{name})) {
>+        if (!$cgi->param('dontchange')
>+            || $cgi->param($field->{name}) ne $cgi->param('dontchange')) {
>+            DoComma();
>+            $::query .= "$field->{name} = " . SqlQuote(trim($cgi->param($field->{name})));
>+        }
>+    }

Nit: The need for nested ifs instead of a single statement? Shouldn't we check
for dontchange prior to looping through the fields anyway; what's the point in
verifying on every iteration of the loop?

>+      <label for="[% field.name %]">[% field.description %]:</label>
>+    </th>
>+    <td>
>+      <input name="[% field.name %]" value="[% value FILTER html %]" size="60">

... remind me why we're not FILTER html:ing the field names?

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: addresses review comments

Thanks for the reviews, guys!  Here's an updated patch that addresses the
issues you raised.

> > Bugzilla->fields({custom=>1,obsolete=>0})
  ...
>   I like that idea. If performance becomes a problem ever in the future, we
> could just cache the DB results and filter them in perl based on the
criteria.
> I don't think we need to worry about that now, though.

Ok, implemented as a public Bugzilla->get_fields().  I made it "get_fields"
instead of "fields" to reflect that it's a method which takes parameters.


> >+	push(@fields, map($_->{name}, Bugzilla->custom_fields));
> 
>   Perhaps we could use a convenience function for that, that just returns a
> list of field names?

Good idea; added as Bugzilla->custom_field_names


> >+	my $custom_fields = "";
> >+	if (length(Bugzilla->custom_fields) > 0) {
> 
>   Why the length() check and not just a direct check?

Because we don't want to add any fields to the SELECT clause of the query
unless there are some custom fields.  This should actually be "scalar"; fixed. 
What's a "direct check"?


>   I think we could use some accessors for the fields, instead of calling them

> directly. That allows us the convenience of later modifying the accessor
> subroutines without modifying the calling code.

I'm not sure I understand; what do you have in mind?


> >+# Contributor(s): Shane H. W. Travis <[e-mail]>
> >+#		      Max Kanat-Alexander <[e-mail]>
> 
>   And you! :-D In fact, just you. :-)

Ok, fixed, although since there is now already a Field.pm with dmose and
lpsolit contributions, I've included them as well.


> >+our @attributes = ("fieldid AS id",
> 
>   In some new patches (newer than this patch), we've been making this a
> constant called DB_COLUMNS, and then also having an "our" variable called
> $columns to put into SQL statements.

Ok, this patch conforms to that.


> >+sub new {
> >+	my $self = shift || Bugzilla->dbh->selectrow_hashref("$select
> 
>   Why would you ever be passing an object to itself for new()? (The "shift.")


get_fields() retrieves the same hash new() does, and it saves some cycles to
pass it to new() for blessing into the object rather than making new() retrieve
the data from the database for each found field, since it requires only a
single query instead of one per field.


>   Also, it seems like we should attempt to use ids instead of names, where we

> can. Or use the type of mechanism we have for the new Product.pm stuff (in
bug
> 294160), where you can pass in either a name or an id.

Perhaps, but our codebase is heavily name-based when it comes to fields, so
this is probably something better tackled in a separate patch that can touch
all those other parts of the code.


> >+	# XXX Replace ALTER TABLE query with call to bz_add_column once the
latter is available.
> >+	#$dbh->bz_add_column("bugs", $name, { TYPE => 'TINYTEXT' });
> 
>   Oh, and just as a note, it's available now, so we're good.

Ok, changed.


>   I think I generally like having the POD docs throughout the file instead of

> at the end. The convenience is that then when I'm looking at a function and
> trying to work on it, I have its docs on the same screen.

Makes sense to me; done.


>   The SYNOPSIS and DESCRIPTION sections are reversed, for this module. (That
> is, the contents of one should be the contents of the other.)

Urk.  Ok, fixed.


> >+Field.pm defines field objects, which represent the discrete pieces
> >+of information that Bugzilla stores about bugs.  Bugzilla stores Fields
> >+in the fielddefs table, and callers access them through field objects
> >+and the Bugzilla::fields/Bugzilla::custom_fields properties.
> 
>   Make that Bugzilla->fields and Bugzilla->custom_fields, I think, just so
> readers don't get confused on how to call them.

Ok, updated.


>   Also, details of how we store fields might not be good for API docs, but
I'll
> leave that up to you.

Ok, removed.


> >Index: Bugzilla/DB/Schema.pm
> >@@ -435,6 +435,9 @@
> >		fieldid     => {TYPE => 'MEDIUMSERIAL', NOTNULL => 1,
> >				PRIMARYKEY => 1},
> >		name	    => {TYPE => 'varchar(64)', NOTNULL => 1},
> >+		type	    => {TYPE => 'INT2', NOTNULL => 1, DEFAULT => 0},
> 
>   Perhaps we should actually put the UNKNOWN_FIELD_TYPE constant, there? Or
> perhaps not... hrm... I'm not too sure.

Yes, we should.  Done.


(In reply to comment #11)
> (From update of attachment 181359 [details] [diff] [review] [edit])
> "Hard r-" for bitrot; not being able to test makes this particularly
difficult
> for me, as I'm not as well versed in custom fields as you guys. Miscellaneous

> comments follow:

Sorry about that; bitrot corrected.


> How do we handle localizing the custom field labels (descriptions)?

At the moment we don't, just like we don't for regular fields, but ultimately
we should use field_descs for both.


> Nit: Please add a comment to explain the meaning of the criteria param... I
> know it's private, but still. :-)

Ok, done.

> I'm not the boilerplate expert, but which part of the code is developed by
> Netscape (or its employees) here?

Oops.  Since Field.pm now exists, I'll just use its license.


> 
> >Index: checksetup.pl
> >===================================================================
> 
> >+# 2005-02-01 Myk Melez <[e-mail]> bug 91037
> 
> Wrong bug number.

Ok, fixed.


> >Index: process_bug.cgi
> >===================================================================
> >+	if (defined $cgi->param($field->{name})) {
> >+	    if (!$cgi->param('dontchange')
> >+		|| $cgi->param($field->{name}) ne $cgi->param('dontchange')) {
> >+		DoComma();
> >+		$::query .= "$field->{name} = " .
SqlQuote(trim($cgi->param($field->{name})));
> >+	    }
> >+	}
> 
> Nit: The need for nested ifs instead of a single statement? Shouldn't we
check
> for dontchange prior to looping through the fields anyway; what's the point
in
> verifying on every iteration of the loop?

Replaced the nested ifs with a single one, but I can't factor out the
dontchange check because it's not determinative (even if it's true, we might
still run the code within the conditional).


> 
> >+	  <label for="[% field.name %]">[% field.description %]:</label>
> >+	</th>
> >+	<td>
> >+	  <input name="[% field.name %]" value="[% value FILTER html %]"
size="60">
> 
> ... remind me why we're not FILTER html:ing the field names?

Probably because they're not likely to need filtering.	But perhaps that's
brittle; OK, filtering them now.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: unhorks

This is the same as the previous patch except unrotted.

Comment 14

[Jouni Heikniemi]

Comment on attachment 193533 [details] [diff] [review]
patch v4: unhorks

Uh... Is this really the correct version of the patch? It causes several
failures of the following sort:

[Sat Sep  3 08:02:01 2005] attachment.cgi: Can't locate object method
"custom_field_names" via package "Bugzilla" at Bugzilla/Bug.pm line 91.

(testing suite is completely broken with this)

Comment 15

[Max Kanat-Alexander]

Just so you know, I'm pretty keen on getting this into 2.22, so if you upload a
new patch sometime soon, I'll put it high on my priority list for reviewing.

Comment 16

[Frédéric Buclin]

The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: unhorks again and "resolves" jouni's issue

>Uh... Is this really the correct version of the patch? It causes several
>failures of the following sort:
>
>[Sat Sep  3 08:02:01 2005] attachment.cgi: Can't locate object method
>"custom_field_names" via package "Bugzilla" at Bugzilla/Bug.pm line 91.

Hmm, since Bugzilla::Bug->fields now calls Bugzilla->custom_field_names, it needs "use Bugzilla" to happen, and that doesn't currently happen in all CGIs that "use Bugzilla::Bug".  The list of CGIs that "use Bugzilla::Bug" and don't "use Bugzilla" is:

attachment.cgi
editcomponents.cgi
editmilestones.cgi
editproducts.cgi
show_activity.cgi
showdependencytree.cgi
votes.cgi

Seems like it would make more sense to "use Bugzilla" in Bugzilla/Bug.pm, and that's what I've done in this patch, but I recall people complaining about such things before, so is there a better way, or is the best way to actually add "use Bugzilla" to a bunch of scripts that never really need to use it?

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Ping.  I'll be around and ready to update this patch if you find issues with it in the next week and a half.  After that I'll be on vacation until the new year.

Comment 19

[Max Kanat-Alexander]

For reference, we resolved jouni's issue in an entirely separate bug. The rule now is actually that no .pm file may ever "use Bugzilla" -- .cgi scripts must do it for themselves.

Comment 20

[Max Kanat-Alexander]

Comment on attachment 203604 [details] [diff] [review]
patch v5: unhorks again and "resolves" jouni's issue

  Okay, I'm going to review this in pieces. The size of the patch is somewhat daunting to review, which is why I originally split this task up into several bugs, each of which would have had an easy review cycle.

  For this review, I'll go over Field.pm and make some general comments about the patch.

  I think the "addcustomfield" script should probably just be called "customfield.pl" so that some day it could also be used to delete, modify, etc. custom fields.

  Also, I'd sort of like it to print out a warning about how adding custom fields to your installation can greatly decrease usability.

>--- Bugzilla/Constants.pm	31 Oct 2005 23:09:28 -0000	1.31
>+++ Bugzilla/Constants.pm	19 Nov 2005 01:52:19 -0000
>@@ -91,6 +91,9 @@
>     ADMIN_GROUP_NAME
> 
>     SENDMAIL_EXE
>+
>+    UNKNOWN_FIELD_TYPE
>+    FREETEXT_FIELD_TYPE

  I think it would be clearer to reverse these, so that they're FIELD_TYPE_FREETEXT and FIELD_TYPE_UNKNOWN. Makes it easier to read, I think.

  Also, eventually all of the integer-based Type stuff will probably go away in favor of class names, once these are all real objects with subclasses. But we don't need to worry about that, at this point. So you can ignore this paragraph entirely. :-) Just making a note for the future.

>Index: Bugzilla/Field.pm

>+Bugzilla::Field - a discrete piece of information about bugs

  Nit: I think we should find some other word to use besides "discrete" that would be easily understood by all developers of all levels of English proficiency.

>+                  and useful routines for form field manipulation

  You know, it is sort of funny that those are in here, now that we really see what a Bugzilla::Field represents. I suppose we ought to move them somewhere else, for the future, like Bugzilla::CGI. (Then they could be CGI methods anyhow, which would make way more sense.)

>+sub new {
>+    my $invocant = shift;
>+    my $name = shift;
>+    my $self = shift || Bugzilla->dbh->selectrow_hashref("SELECT $columns
>+                                                          FROM fielddefs
>+                                                          WHERE name = ?",
>+                                                         undef,
>+                                                         $name);;

  Nit: The spacing there is really odd. You could just put the SQL statement on one line. Also, there's two semicolons.

>+    bless($self, ref($invocant) || $invocant);

  I don't think you ned the ref($invocant) || $invocant, there. Nobody calls "new" statically.

>+Description: creates a new custom field.
>+
>+Params:      C<$name> - string - the name of the field;
>+             C<$desc> - string - the field label to display in the UI.
>+
>+Returns:     a field object.

  Nit: We don't usually have extra newlines between Description/Params/Returns. It's okay if you you like them, though.

>+sub create {

>+    $name =~ /^cf_/
>+      or $name = "cf_" . $name;
> 
>-    ThrowCodeError('invalid_field_name', {field => $name}) unless $id;
>-    return $id
>+    my ($sortkey) = $dbh->selectrow_array("SELECT MAX(sortkey) + 1 FROM fielddefs");

  Nit: Line longer than 80 chars.

  Also, you might want to put a comment there that someday we'll allow people to specify the sortkey, we just don't, yet.

>+    my $type = FREETEXT_FIELD_TYPE;
>+
>+    # Create the database column that stores the data for this field.
>+    $dbh->bz_add_column("bugs", $name, { TYPE => 'TINYTEXT' });

  Let's actually make them varchar(255), instead. That works better in a cross-DB sense.

>+    # Add the field to the list of fields at this Bugzilla installation.
>+    my $sth = $dbh->prepare("INSERT INTO fielddefs
>+                             (name, description, sortkey, type, custom, mailhead)
>+                             VALUES (?, ?, ?, ?, 1, 1)");

  We really should move the AddFDef function instead of putting this SQL here... but I suppose if we do that quickly after this bug is checked-in, I'm okay with this being here for now.

>+sub match {
>+  my ($criteria) = @_;
> 
>+  my $records = Bugzilla->dbh->selectall_arrayref("SELECT $columns
>+                                                   FROM fielddefs
>+                                                   $where
>+                                                   ORDER BY sortkey",
>+                                                  { Slice => {}});

  Nit: Again, spacing's a bit awkward. Seems like all the SQL could go on one line. The code is good, though. I like this function.

>+  my @fields = map( new Bugzilla::Field(undef, $_), @$records );

  "map" is frequently hard to understand easily, by looking at it. Could you add a comment above this line explaining what it does?


  Overall, Field.pm is really well-written. I definitely am liking this patch. :-)

  I'll do the rest of it, or you can post an updated version and I'll do the rest of that.

Comment 21

[Max Kanat-Alexander]

Oh, and also -- All the Field.pm object fields should have accessors, instead of being accessed directly.

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v6: addresses review comments

>  Okay, I'm going to review this in pieces. The size of the patch is somewhat
>daunting to review, which is why I originally split this task up into several
>bugs, each of which would have had an easy review cycle.

I understand the pain involved in reviewing a patch this size, but I think it's important to bundle at least one implementation of a custom field type along with the infrastructure to support custom fields, since it provides us with a practical use case to inform our infrastructure design.  Thanks for sticking with it!


> For reference, we resolved jouni's issue in an entirely separate bug. The rule
> now is actually that no .pm file may ever "use Bugzilla" -- .cgi scripts must
> do it for themselves.

Ok, I've added "use Bugzilla" to all scripts that "use Bugzilla::Bug" and
removed "use Bugzilla" from Bug.pm.  To compensate, I had to add "use Bugzilla"
to six additional scripts that never touch custom fields but do "use Bugzilla::Bug" and hence must also "use Bugzilla".


>   I think the "addcustomfield" script should probably just be called
> "customfield.pl" so that some day it could also be used to delete, modify, 
> etc. custom fields.

Ok, renamed.


>   Also, I'd sort of like it to print out a warning about how adding custom
> fields to your installation can greatly decrease usability.

I have added such a warning to the usage output.


>   I think it would be clearer to reverse these, so that they're
> FIELD_TYPE_FREETEXT and FIELD_TYPE_UNKNOWN. Makes it easier to read, I think.

Makes sense, done.


> >+Bugzilla::Field - a discrete piece of information about bugs
> 
>   Nit: I think we should find some other word to use besides "discrete" that
> would be easily understood by all developers of all levels of English
> proficiency.

I switched it to "particular".


>   You know, it is sort of funny that those are in here, now that we really see
> what a Bugzilla::Field represents. I suppose we ought to move them somewhere
> else, for the future, like Bugzilla::CGI. (Then they could be CGI methods
> anyhow, which would make way more sense.)

Indeed.  I haven't done this, since it seems orthogonal to this patch (now that
this patch exists, anyway) and because I wanted to generate a patch with the
minimal changes needed to get custom fields working, but I can fix it here if you'd rather we do that now.


> >+sub new {
> >+    my $invocant = shift;
> >+    my $name = shift;
> >+    my $self = shift || Bugzilla->dbh->selectrow_hashref("SELECT $columns
> >+                                                          FROM fielddefs
> >+                                                          WHERE name = ?",
> >+                                                         undef,
> >+                                                         $name);;
> 
>   Nit: The spacing there is really odd. You could just put the SQL statement 
> on one line. Also, there's two semicolons.

Ok, I reformatted the list of function parameters to fit the SQL statement on one line, and I removed the extra semicolon.


> >+    bless($self, ref($invocant) || $invocant);
> 
>   I don't think you ned the ref($invocant) || $invocant, there. Nobody calls
> "new" statically.

Ok, removed.


> >+Description: creates a new custom field.
> >+
> >+Params:      C<$name> - string - the name of the field;
> >+             C<$desc> - string - the field label to display in the UI.
> >+
> >+Returns:     a field object.
> 
>   Nit: We don't usually have extra newlines between 
> Description/Params/Returns. It's okay if you you like them, though.

Without them, the three sections wrap poorly.  But I'm a novice with the perldoc
format, so perhaps I'm doing something wrong.  I have left it as is for now, but let me know if I'm missing something.


> >+sub create {
> 
> >+    $name =~ /^cf_/
> >+      or $name = "cf_" . $name;
> > 
> >-    ThrowCodeError('invalid_field_name', {field => $name}) unless $id;
> >-    return $id
> >+    my ($sortkey) = $dbh->selectrow_array("SELECT MAX(sortkey) + 1 FROM fielddefs");
> 
>   Nit: Line longer than 80 chars.

Fixed (as was another line that was 81 characters).


>   Also, you might want to put a comment there that someday we'll allow people
> to specify the sortkey, we just don't, yet.

Ok, done.


> >+    my $type = FREETEXT_FIELD_TYPE;
> >+
> >+    # Create the database column that stores the data for this field.
> >+    $dbh->bz_add_column("bugs", $name, { TYPE => 'TINYTEXT' });
> 
>   Let's actually make them varchar(255), instead. That works better in a
> cross-DB sense.

Ok, done.


> >+    # Add the field to the list of fields at this Bugzilla installation.
> >+    my $sth = $dbh->prepare("INSERT INTO fielddefs
> >+                             (name, description, sortkey, type, custom, mailhead)
> >+                             VALUES (?, ?, ?, ?, 1, 1)");
> 
>   We really should move the AddFDef function instead of putting this SQL
> here... but I suppose if we do that quickly after this bug is checked-in, I'm
> okay with this being here for now.


> >+  my @fields = map( new Bugzilla::Field(undef, $_), @$records );
> 
>   "map" is frequently hard to understand easily, by looking at it. Could you
> add a comment above this line explaining what it does?

Sure, added.


> Oh, and also -- All the Field.pm object fields should have accessors, instead
> of being accessed directly.

Ok, they all have accessors now.

In addition to the above fixes, I also improved the customfield.pl usage info, filtered the field name and description in field.html.tmpl so the filter test stops complaining, and made sure the changes pass all tests.

Comment 23

[Max Kanat-Alexander]

Comment on attachment 207461 [details] [diff] [review]
patch v6: addresses review comments

  Okay, great. Yeah, all your changes sound good. Yeah, you don't need to move the CGI functions -- I was mostly just noting that so we could do it in another bug.

>Index: Bugzilla.pm
> [snip]
>+sub get_fields {
>+    my $class = shift;
>+    my $criteria = shift;
>+    return (Bugzilla::Field::match($criteria));

  Nit: I'm pretty sure you don't need the extra parens there.

>+sub custom_field_names {
>+    return map($_->{name}, Bugzilla::Field::match({ custom=>1, obsolete=>0 }));

  And could this map() get a comment, also? I'm pretty sure I understand what it does, but it's always nice to have a little comment with map().

>Index: checksetup.pl
>
>+# 2005-08-10 Myk Melez <[e-mail]> bug 287325
>+# Record each field's type and whether or not it's a custom field in fielddefs.
>+$dbh->bz_add_column('fielddefs', 'type',
>+                    { TYPE => 'INT2', NOTNULL => 1, DEFAULT => 0 });
>+$dbh->bz_add_column('fielddefs', 'custom',
>+                    { TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE' });

  I was thinking about perhaps calling this is_custom to keep with conventions? I'm not sure.

>Index: Bugzilla/Field.pm
> [snip]
>+sub id {
>+    my $self = shift;
>+    return $self->{id};
> }

  Nit: Usually we just write these on one line as sub id { return $_[0]->{id}; }.

  I like that they all have POD docs, that's great.

  Everything else *looks* basically great, and I have only nits, so that's r+ as far as inspection goes.

  Now I'm going to call out to developers@ to get some testing on this thing. :-)

Comment 24

[Myk Melez [:myk] [@mykmelez]]

Frederic points out in IRC that I need to remove the prototypes from the check_form_field and check_form_field_defined functions.

Comment 25

[:glob ✱]

omg :)


the usage says:
  ./customfield.pl --name=<field_name> --desc="<field description>"
for windows this needs to say:
  perl -T customfield.pl --name=<field_name> --desc="<field description>"
it won't work without -T


when set the value of a custom field there were two identical entries in the activity log against the same time.


i'd like to see custom fields show listed somewhere on the advanced search page, not just in boolean charts.


adding the same custom field twice results in a nasty sql error -- we should check for duplicates and report a nicer entry:

C:\Bugzilla\dev\287325>perl -T customfield.pl --name=monkeys --desc="number of monkeys"
Creating custom field monkeys... <h1>Software error:</h1>
<pre>DBD::mysql::st execute failed: Duplicate entry 'cf_monkeys' for key 2 [for Statement &quot;INSERT INTO fielddefs (name, descrip
tion, sortkey, type,
                                          custom, mailhead)
                   VALUES (?, ?, ?, ?, 1, 1)&quot;] at Bugzilla/Field.pm line 247
        Bugzilla::Field::create('monkeys', 'number of monkeys') called at customfield.pl line 72
</pre>
<p>
For help, please send mail to this site's webmaster, giving this error message
and the time and date of the error.

</p>
[Wed Jan  4 10:46:36 2006] customfield.pl: DBD::mysql::st execute failed: Duplicate entry 'cf_monkeys' for key 2 [for Statement "INS
ERT INTO fielddefs (name, description, sortkey, type,
[Wed Jan  4 10:46:36 2006] customfield.pl:                                           custom, mailhead)
[Wed Jan  4 10:46:36 2006] customfield.pl:                    VALUES (?, ?, ?, ?, 1, 1)"] at Bugzilla/Field.pm line 247
[Wed Jan  4 10:46:36 2006] customfield.pl:      Bugzilla::Field::create('monkeys', 'number of monkeys') called at customfield.pl lin
e 72


also as you can see error messages are html.

Comment 26

[Frédéric Buclin]

Field names are unique. I agree with glob, this is the kind of frequent mistakes we do when we add many fields, we forget we had one already.

Comment 27

[:glob ✱]

oh, the usage says:

  Custom fields can make Bugzilla less usable.  See this URL
  for alternatives to custom fields:
  
  http://www.gerv.net/hacking/custom-fields.html
  
  You should try to implement applicable alternatives before using
  this script to add a custom field.

ideally gerv should update that page to reflect what's happening here, or better yet move it to bugzilla.org.

Comment 28

[Max Kanat-Alexander]

(In reply to comment #25)
> i'd like to see custom fields show listed somewhere on the advanced search
> page, not just in boolean charts.

  That would be a different bug than this.

> also as you can see error messages are html.

  That's actually a bug we already have filed. It was a side-effect of moving stuff out of CGI.pl.

(In reply to comment #27)
> ideally gerv should update that page to reflect what's happening here, or
> better yet move it to bugzilla.org.

  Yeah, I agree, it would probably be good to move it to bugzilla.org, after we have this patch. It's still very valid for pre-2.22 versions, though.

Comment 29

[Max Kanat-Alexander]

(In reply to comment #28)
> It's still very valid for pre-2.22 versions, though.

  Oops, meant "pre-2.24 versions." Oh, and also, I agreed with all the rest of glob's comments: good catch.

Comment 30

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v7: addresses additional review comments

mkanat says:

>   Nit: I'm pretty sure you don't need the extra parens there.

Ok, removed.


>   And could this map() get a comment, also? I'm pretty sure I understand what
> it does, but it's always nice to have a little comment with map().

Yup, added.


>   I was thinking about perhaps calling this is_custom to keep with
> conventions?  I'm not sure.

Me neither.  Left it as it is for now.


>   Nit: Usually we just write these on one line as
> sub id { return $_[0]->{id}; }.

Ok, done, but without the semicolon per the Perl style guide, which says to omit the trailing semicolon in single statement single line blocks.


myk says:

> Frederic points out in IRC that I need to remove the prototypes from the
> check_form_field and check_form_field_defined functions.

Ok, removed.


glob says:

> the usage says:
>   ./customfield.pl --name=<field_name> --desc="<field description>"
> for windows this needs to say:
>   perl -T customfield.pl --name=<field_name> --desc="<field description>"
> it won't work without -T

Ok, usage updated to mention the Windows-specific syntax.


> when set the value of a custom field there were two identical entries in the
> activity log against the same time.

This is a regression, probably from bug 311434.  Fixed.


> adding the same custom field twice results in a nasty sql error -- we should
> check for duplicates and report a nicer entry:

Yup, we now do.


I also fixed the indentation of the "match" method, and made the "create"
method generic (moving custom field-specific code into customfield.pl).

Comment 31

[Max Kanat-Alexander]

Comment on attachment 207545 [details] [diff] [review]
patch v7: addresses additional review comments

  Looks all good to me. :-) I think that create() could use a hashref for its last argument, as I mentioned in IRC.

Comment 32

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 207545 [details] [diff] [review]
patch v7: addresses additional review comments

Byron, how does this version work for you?  Note that although this is a "review" request I'm looking more for testing feedback, since Max says his review plus someone else's testing is sufficient for this patch.

Comment 33

[:glob ✱]

Comment on attachment 207545 [details] [diff] [review]
patch v7: addresses additional review comments

r=glob

result of testing ..

custom fields aren't shown on the "format for printing" format.  fodder for another bug methinks.

i also think it'd be nice to have somewhere where an admin can view the current custom field names and descriptions, with a blurb about how to add them.  in another bug :)

nit picking the code:

>+  Use this script to add a custom field to your Bugzilla installation
>+  by invoking it with the --name and --desc command-line options:
>+
>+  ./customfield.pl --name=<field_name> --desc="<field description>"
>+
>+  Or, on Windows:
>+  
>+  perl -T customfield.pl --name=<field_name> --desc="<field description>"

how about displaying the correct command line based on the current operating system.

>+  <field description> is a short string describing the field.  It will
>+  be displayed to Bugzilla users in several parts of Bugzilla's UI,
>+  f.e. as the form field label on the "show bug" page.

f.e. isn't common everywhere; perhaps use e.g. or spell it out

also the message you get when a field is added is..

  Creating custom field cf_burlap... Adding new column cf_burlap to table bugs ...
  done; created as column cf_burlap.

i think it'd look better with a \n after the first "..."
and there isn't a space before the first ... but there's one before the second.

>+print "done; created as column $field->{name}.\n";

s/done/Done/

Comment 34

[Gervase Markham [:gerv]]

I'm happy to update the custom fields page when a patch gets checked in, and if someone can summarise for me what it does :-)

Gerv

Comment 35

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v8: fixes nits

> custom fields aren't shown on the "format for printing" format.  fodder for
> another bug methinks.

Indeed.


> i also think it'd be nice to have somewhere where an admin can view the 
> current custom field names and descriptions, with a blurb about how to add 
> them.  in another bug :)

Yeah.  My goal here was to write the smallest (and hence safest, easiest to review, etc.) possible patch implementing custom fields that we can use as a base for iterative improvement.


> how about displaying the correct command line based on the current operating
> system.

Good idea, done.


> f.e. isn't common everywhere; perhaps use e.g. or spell it out

Ok, I have spelled it out.


> also the message you get when a field is added is..
> 
>   Creating custom field cf_burlap... Adding new column cf_burlap to table bugs
> ...
>   done; created as column cf_burlap.
> 
> i think it'd look better with a \n after the first "..."
> and there isn't a space before the first ... but there's one before the 
> second.

Good points.  Now the messages read:

Creating custom field test ...
Adding new column test to table bugs ...
Custom field test created.

Comment 36

[:glob ✱]

Comment on attachment 207623 [details] [diff] [review]
patch v8: fixes nits

r=glob

Comment 37

[Myk Melez [:myk] [@mykmelez]]

Thanks all for your feedback and reviews.

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.27; previous revision: 1.26
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.465; previous revision: 1.464
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/customfield.pl,v
done
Checking in customfield.pl;
/cvsroot/mozilla/webtools/bugzilla/customfield.pl,v  <--  customfield.pl
initial revision: 1.1
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.67; previous revision: 1.66
done
Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi
new revision: 1.48; previous revision: 1.47
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.109; previous revision: 1.108
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.299; previous revision: 1.298
done
Checking in show_activity.cgi;
/cvsroot/mozilla/webtools/bugzilla/show_activity.cgi,v  <--  show_activity.cgi
new revision: 1.19; previous revision: 1.18
done
Checking in showdependencytree.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencytree.cgi,v  <--  showdependencytree.cgi
new revision: 1.39; previous revision: 1.38
done
Checking in summarize_time.cgi;
/cvsroot/mozilla/webtools/bugzilla/summarize_time.cgi,v  <--  summarize_time.cgi
new revision: 1.12; previous revision: 1.11
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.102; previous revision: 1.101
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.32; previous revision: 1.31
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.8; previous revision: 1.7
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.44; previous revision: 1.43
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.67; previous revision: 1.66
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v
done
Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
initial revision: 1.1
done

Comment 38

[Myk Melez [:myk] [@mykmelez]]

Looks like I spoke too soon; the tree isn't yet open for 2.24 checkins.  Backing out the patch until it is, which lpsolit says will be in about a month.

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.28; previous revision: 1.27
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.466; previous revision: 1.465
done
Removing customfield.pl;
/cvsroot/mozilla/webtools/bugzilla/customfield.pl,v  <--  customfield.pl
new revision: delete; previous revision: 1.1
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.68; previous revision: 1.67
done
Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi
new revision: 1.49; previous revision: 1.48
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.110; previous revision: 1.109
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.300; previous revision: 1.299
done
Checking in show_activity.cgi;
/cvsroot/mozilla/webtools/bugzilla/show_activity.cgi,v  <--  show_activity.cgi
new revision: 1.20; previous revision: 1.19
done
Checking in showdependencytree.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencytree.cgi,v  <--  showdependencytree.cgi
new revision: 1.40; previous revision: 1.39
done
Checking in summarize_time.cgi;
/cvsroot/mozilla/webtools/bugzilla/summarize_time.cgi,v  <--  summarize_time.cgi
new revision: 1.13; previous revision: 1.12
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.103; previous revision: 1.102
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.33; previous revision: 1.32
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.9; previous revision: 1.8
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.45; previous revision: 1.44
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.68; previous revision: 1.67
done
Removing template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: delete; previous revision: 1.1
done

Comment 39

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v9: fixes tinderbox horkage caused by forced DB access on compile

Bug.pm's main execution block generates a list of valid bug attributes.  My previous patches have added custom fields to that list, forcing a DB call on compile, which causes tinderboxes to fail (since they don't have access to an actual DB).  Here's the minimal fix for the problem: instead of generating the list of valid attributes at compile time, we lazily generate it at run time the first time the list is needed.

Comment 40

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v10: the real fix for tinderbox compile time db access

Patch v9 was only a partial solution that broke other things.  Here's the complete solution.  Interdiff this against v8 to see what's changed.

Comment 41

[Douglas Calvert]

Custom field changes do not show up in the bug activity page.

Comment 42

[Max Kanat-Alexander]

Comment on attachment 207726 [details] [diff] [review]
patch v10: the real fix for tinderbox compile time db access

Hrm. Unfortunately, my understanding is that your "my @_fields" is something that won't work in mod_perl. I'm not certain, but I'm fairly sure.

Would it be possible just use $invocant or $self to store a {fields} in, or something like that?

Comment 43

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 207726 [details] [diff] [review]
patch v10: the real fix for tinderbox compile time db access

>Hrm. Unfortunately, my understanding is that your "my @_fields" is something
>that won't work in mod_perl.

Perhaps, but then neither does the existing code, which uses a global %ok_fields hash.


>Would it be possible just use $invocant or $self to store a {fields} in, or
>something like that?

Unfortunately it wouldn't, since "fields" is a class property, so there's no $self, and $invocant is just a string representation of the class, not the class itself.  Besides, if it was the class, writing to it would probably be the same as defining a global package variable, I reckon, and would have the same mod_perl compatibility issues.

Is there some other request-specific place to cache this data?  Otherwise I can just regenerate it every time "fields" and "valid_attributes" are invoked, which probably isn't too expensive given current usage.

But perhaps this is something that can be dealt with in another mod_perl compatibility bug, given that the existing code is also non-compliant.  Let me know how I should proceed.

Comment 44

[Max Kanat-Alexander]

(In reply to comment #43)
> Otherwise I can
> just regenerate it every time "fields" and "valid_attributes" are invoked,
> which probably isn't too expensive given current usage.

  Yeah, I think that's what we should do.

Comment 45

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v11: no longer caches lists of fields and valid attributes

Ok, this patch is like the previous one, except it doesn't cache the lists of fields and valid attributes.  It also clarifies that the "fields" accessor is a class accessor and the _valid_attributes function is internal.

Comment 46

[Max Kanat-Alexander]

Comment on attachment 208146 [details] [diff] [review]
patch v11: no longer caches lists of fields and valid attributes

Yeah, this looks great to me.

I think that some day we should just get rid of that AUTOLOAD, if possible, and make all the accessors explicit. I've been caused some debugging trouble by that AUTOLOAD before.

Comment 47

[Dave Miller [:justdave]]

This looks like it's first up in the post-freeze checkins.

Comment 48

[Frédéric Buclin]

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.30; previous revision: 1.29
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.470; previous revision: 1.469
done
Checking in customfield.pl;
/cvsroot/mozilla/webtools/bugzilla/customfield.pl,v  <--  customfield.pl
new revision: 1.3; previous revision: 1.2
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.69; previous revision: 1.68
done
Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi
new revision: 1.50; previous revision: 1.49
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.111; previous revision: 1.110
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.303; previous revision: 1.302
done
Checking in show_activity.cgi;
/cvsroot/mozilla/webtools/bugzilla/show_activity.cgi,v  <--  show_activity.cgi
new revision: 1.21; previous revision: 1.20
done
Checking in show_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/show_bug.cgi,v  <--  show_bug.cgi
new revision: 1.39; previous revision: 1.38
done
Checking in showdependencytree.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencytree.cgi,v  <--  showdependencytree.cgi
new revision: 1.41; previous revision: 1.40
done
Checking in summarize_time.cgi;
/cvsroot/mozilla/webtools/bugzilla/summarize_time.cgi,v  <--  summarize_time.cgi
new revision: 1.14; previous revision: 1.13
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.105; previous revision: 1.104
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.35; previous revision: 1.34
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.10; previous revision: 1.9
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.46; previous revision: 1.45
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.70; previous revision: 1.69
done
Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: 1.3; previous revision: 1.2
done

Comment 49

[Vlad Dascalu]

Attachment: Proposed patch for fixing tree

This caused tinderbox to fail - http://tinderbox.mozilla.org/showlog.cgi?log=Bugzilla/1140528902.21543.gz .

Justdave noticed that the SQL-generated query has an extra comma:

[Tue Feb 21 05:36:40 2006] test-checksetup.pl:     SELECT
[Tue Feb 21 05:36:40 2006] test-checksetup.pl:       bugs.bug_id, alias, products.classification_id, classifications.name,
[Tue Feb 21 05:36:40 2006] test-checksetup.pl:       bugs.product_id, products.name, version,
[Tue Feb 21 05:36:40 2006] test-checksetup.pl:       rep_platform, op_sys, bug_status, resolution, priority,
[Tue Feb 21 05:36:40 2006] test-checksetup.pl:       bug_severity, bugs.component_id, components.name, 
[Tue Feb 21 05:36:40 2006] test-checksetup.pl:       assigned_to AS assigned_to_id, reporter AS reporter_id,
[Tue Feb 21 05:36:40 2006] test-checksetup.pl:       bug_file_loc, short_desc, target_milestone,
[Tue Feb 21 05:36:40 2006] test-checksetup.pl:       qa_contact AS qa_contact_id, status_whiteboard, DATE_FORMAT(creation_ts, '%Y.%m.%d %H:%i'),
[Tue Feb 21 05:36:40 2006] test-checksetup.pl:       delta_ts, COALESCE(SUM(votes.vote_count), 0), everconfirmed,
[Tue Feb 21 05:36:40 2006] test-checksetup.pl:       reporter_accessible, cclist_accessible,
[Tue Feb 21 05:36:40 2006] test-checksetup.pl:       estimated_time, remaining_time, DATE_FORMAT(deadline, '%Y-%m-%d'), 
[Tue Feb 21 05:36:40 2006] test-checksetup.pl:     FROM bugs

right before the FROM clause.

I think it's because length was used incorrectly. perldoc -f LENGTH says:

     length EXPR
     length  Returns the length in *characters* of the value of EXPR. If EXPR
             is omitted, returns length of $_. Note that this cannot be used
             on an entire array or hash to find out how many elements these
             have.

Scalar should be used instead.

Comment 50

[Vlad Dascalu]

Comment on attachment 212580 [details] [diff] [review]
Proposed patch for fixing tree

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.106; previous revision: 1.105
done

Hopefully tinderbox becomes green with this, otherwise we need to investigate further or backout the patch.

Comment 51

[Vlad Dascalu]

Yeap, it did the trick.

Comment 52

[P P N]

I am new to bugzilla and hence would require some help from you all here.

Could anyone let me know if this enhancement will be available for the current release if I download bugzilla now? If no, how to get the current updates / patches for bugzilla 2.20? In such a case will all the new customization [custom field] etc get merged with bugzilla 2.20 release?

The customizations mentioned here would really come handy for us too. Please
check BUG #344086 about the same. Looking forward to your replies on the same.

Thanks,

Comment 53

[Ray Booysen]

By the looks of the bug, it'll be available in 2.24

Comment 54

[Myk Melez [:myk] [@mykmelez]]

That's right, this feature is making its first appearance in 2.24.  There aren't any plans to backport it to earlier releases.

Comment 55

[Max Kanat-Alexander]

Added to the relnotes currently attached to bug 349423.