Bug 455632: Add Bugzilla::Field::Choice->create and have editvalues.cgi use it

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=bbaetz, a=mkanat

1 files changed, 148 insertions, 89 deletions

diff --git a/Bugzilla/Field/Choice.pm b/Bugzilla/Field/Choice.pm
index 6ef911941..7705258eb 100644
--- a/Bugzilla/Field/Choice.pm
+++ b/Bugzilla/Field/Choice.pm
@@ -26,6 +26,8 @@ use base qw(Bugzilla::Object);
 
 use Bugzilla::Constants;
 use Bugzilla::Error;
+use Bugzilla::Field;
+use Bugzilla::Util qw(trim detaint_natural);
 
 use Scalar::Util qw(blessed);
 
@@ -42,104 +44,141 @@ use constant DB_COLUMNS => qw(
 use constant NAME_FIELD => 'value';
 use constant LIST_ORDER => 'sortkey, value';
 
-##########################################
-# Constructors and Database Manipulation #
-##########################################
+use constant REQUIRED_CREATE_FIELDS => qw(value);
 
-# When calling class methods, we aren't based on just one table,
-# so we need some slightly hacky way to do DB_TABLE. We do it by overriding
-# class methods and making them specify $_new_table. This is possible
-# because we're always called from inside Bugzilla::Field, so it always
-# has a $self to pass us which contains info about which field we're
-# attached to.
-#
-# This isn't thread safe, but Bugzilla isn't a threaded application.
-our $_new_table;
-our $_current_field;
-sub DB_TABLE {
-    my $invocant = shift;
-    if (blessed $invocant) {
-        return $invocant->field->name;
+use constant VALIDATORS => {
+    value   => \&_check_value,
+    sortkey => \&_check_sortkey,
+};
+
+use constant CLASS_MAP => {
+    bug_status => 'Bugzilla::Status',
+};
+
+#################
+# Class Factory #
+#################
+
+# Bugzilla::Field::Choice is actually an abstract base class. Every field
+# type has its own dynamically-generated class for its values. This allows
+# certain fields to have special types, like how bug_status's values
+# are Bugzilla::Status objects.
+
+sub type {
+    my ($class, $field) = @_;
+    my $field_obj = blessed $field ? $field : Bugzilla::Field->check($field);
+    my $field_name = $field_obj->name;
+
+    if ($class->CLASS_MAP->{$field_name}) {
+        return $class->CLASS_MAP->{$field_name};
     }
-    return $_new_table;
+
+    # For generic classes, we use a lowercase class name, so as
+    # not to interfere with any real subclasses we might make some day.
+    my $package = "Bugzilla::Field::Choice::$field_name";
+
+    # We check defined so that the package and the stored field are only
+    # created once globally (at least per request). We prefix it with
+    # field_ (instead of suffixing it) so that we don't somehow conflict
+    # with the names of custom fields.
+    if (!defined Bugzilla->request_cache->{"field_$package"}) {
+        eval <<EOC;
+            package $package;
+            use base qw(Bugzilla::Field::Choice);
+            use constant DB_TABLE => '$field_name';
+EOC
+        Bugzilla->request_cache->{"field_$package"} = $field_obj;
+    }
+
+    return $package;
 }
 
+################
+# Constructors #
+################
+
+# We just make new() enforce this, which should give developers 
+# the understanding that you can't use Bugzilla::Field::Choice
+# without calling type().
 sub new {
     my $class = shift;
-    my ($params) = @_;
-    _check_field_arg($params);
-    my $self = $class->SUPER::new($params);
-    _fix_return($self);
-    return $self;
+    if ($class eq 'Bugzilla::Field::Choice') {
+        ThrowCodeError('field_choice_must_use_type');
+    }
+    $class->SUPER::new(@_);
 }
 
-sub new_from_list {
-    my $class = shift;
-    my ($ids, $params) = @_;
-    _check_field_arg($params);
-    my $list = $class->SUPER::new_from_list(@_);
-    _fix_return($list);
-    return $list;
-}
+#########################
+# Database Manipulation #
+#########################
 
-sub match {
+# Our subclasses can take more arguments than we normally accept.
+# So, we override create() to remove arguments that aren't valid
+# columns. (Normally Bugzilla::Object dies if you pass arguments
+# that aren't valid columns.)
+sub create {
     my $class = shift;
     my ($params) = @_;
-    _check_field_arg($params);
-    my $results = $class->SUPER::match(@_);
-    _fix_return($results);
-    return $results;
+    foreach my $key (keys %$params) {
+        if (!grep {$_ eq $key} $class->DB_COLUMNS) {
+            delete $params->{$key};
+        }
+    }
+    return $class->SUPER::create(@_);
 }
 
-sub get_all {
-    my $class = shift;
-    _check_field_arg(@_);
-    my @list = $class->SUPER::get_all(@_);
-    _fix_return(\@list);
-    return @list;
-}
+#############
+# Accessors #
+#############
 
-sub _check_field_arg {
-    my ($params) = @_;
-    my ($class, undef, undef, $func) = caller(1);
-    if (!defined $params->{field}) {
-        ThrowCodeError('param_required',
-                       { function => "${class}::$func",
-                         param    => 'field' });
-    }
-    elsif (!blessed $params->{field}) {
-        ThrowCodeError('bad_arg', { function => "${class}::$func",
-                                    argument => 'field' });
-    }
-    $_new_table = $params->{field}->name;
-    $_current_field = $params->{field};
-    delete $params->{field};
+sub sortkey { return $_[0]->{'sortkey'}; }
+sub field {
+    my $invocant = shift;
+    my $class = ref $invocant || $invocant;
+    my $cache = Bugzilla->request_cache;
+    # This is just to make life easier for subclasses. Our auto-generated
+    # subclasses from type() already have this set.
+    $cache->{"field_$class"} ||=  
+        new Bugzilla::Field({ name => $class->DB_TABLE });
+    return $cache->{"field_$class"};
 }
 
-sub _fix_return {
-    my $retval = shift;
-    if (ref $retval eq 'ARRAY') {
-        foreach my $obj (@$retval) {
-            $obj->{field} = $_current_field;
-        }
-    }
-    elsif (defined $retval) {
-        $retval->{field} = $_current_field;
+##############
+# Validators #
+##############
+
+sub _check_value {
+    my ($invocant, $value) = @_;
+
+    my $field = $invocant->field;
+
+    $value = trim($value);
+    ThrowUserError('fieldvalue_undefined') if !defined $value || $value eq "";
+    ThrowUserError('fieldvalue_name_too_long', { value => $value })
+        if length($value) > MAX_FIELD_VALUE_SIZE;
+
+    my $exists = $invocant->type($field)->new({ name => $value });
+    if ($exists) {
+        ThrowUserError('fieldvalue_already_exists', 
+                       { field => $field, value => $value });
     }
 
-    # We do this just to avoid any possible bugs where $_new_table or
-    # $_current_field are set from a previous call. It also might save
-    # a little memory under mod_perl by releasing $_current_field explicitly.
-    undef $_new_table;
-    undef $_current_field;
+    return $value;
 }
 
-#############
-# Accessors #
-#############
+sub _check_sortkey {
+    my ($invocant, $value) = @_;
+    $value = trim($value);
+    return 0 if !$value;
+    # Store for the error message in case detaint_natural clears it.
+    my $orig_value = $value;
+    detaint_natural($value)
+        || ThrowUserError('fieldvalue_sortkey_invalid',
+                          { sortkey => $orig_value,
+                            field   => $invocant->field });
+    return $value;
+}
 
-sub sortkey { return $_[0]->{'sortkey'}; }
-sub field   { return $_[0]->{'field'};   }
 
 1;
 
@@ -153,27 +192,47 @@ Bugzilla::Field::Choice - A legal value for a <select>-type field.
 
  my $field = new Bugzilla::Field({name => 'bug_status'});
 
- my $choice = new Bugzilla::Field::Choice({ field => $field, id => 1 });
- my $choice = new Bugzilla::Field::Choice({ field => $field, name => 'NEW' });
+ my $choice = new Bugzilla::Field::Choice->type($field)->new(1);
 
- my $choices = Bugzilla::Field::Choice->new_from_list([1,2,3], 
-                                                      { field => $field});
- my $choices = Bugzilla::Field::Choice->get_all({ field => $field });
- my $choices = Bugzilla::Field::Choice->match({ sortkey => 10, 
-                                                field => $field });
+ my $choices = Bugzilla::Field::Choice->type($field)->new_from_list([1,2,3]);
+ my $choices = Bugzilla::Field::Choice->type($field)->get_all();
+ my $choices = Bugzilla::Field::Choice->type($field->match({ sortkey => 10 }); 
 
 =head1 DESCRIPTION
 
 This is an implementation of L<Bugzilla::Object>, but with a twist.
-All the class methods require that you specify an additional C<field>
-argument, which is a L<Bugzilla::Field> object that represents the
-field whose value this is.
+You can't call any class methods (such as C<new>, C<create>, etc.) 
+directly on C<Bugzilla::Field::Choice> itself. Instead, you have to
+call C<Bugzilla::Field::Choice-E<gt>type($field)> to get the class
+you're going to instantiate, and then you call the methods on that.
+
+We do that because each field has its own database table for its values, so
+each value type needs its own class.
 
-You can look at the L</SYNOPSIS> to see where this extra C<field>
-argument goes in each function.
+See the L</SYNOPSIS> for examples of how this works.
 
 =head1 METHODS
 
+=head2 Class Factory
+
+In object-oriented design, a "class factory" is a method that picks
+and returns the right class for you, based on an argument that you pass.
+
+=over
+
+=item C<type>
+
+Takes a single argument, which is either the name of a field from the
+C<fielddefs> table, or a L<Bugzilla::Field> object representing a field.
+
+Returns an appropriate subclass of C<Bugzilla::Field::Choice> that you
+can now call class methods on (like C<new>, C<create>, C<match>, etc.)
+
+B<NOTE>: YOU CANNOT CALL CLASS METHODS ON C<Bugzilla::Field::Choice>. You
+must call C<type> to get a class you can call methods on.
+
+=back
+
 =head2 Accessors
 
 These are in addition to the standard L<Bugzilla::Object> accessors.