Bug 339380: Make Bugzilla::Component use Bugzilla::Object

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

7 files changed, 122 insertions, 91 deletions

diff --git a/Bugzilla/Component.pm b/Bugzilla/Component.pm
index 657d0f728..1b2e8f8d2 100644
--- a/Bugzilla/Component.pm
+++ b/Bugzilla/Component.pm
@@ -21,6 +21,8 @@ use strict;
 
 package Bugzilla::Component;
 
+use base qw(Bugzilla::Object);
+
 use Bugzilla::Util;
 use Bugzilla::Error;
 use Bugzilla::User;
@@ -30,68 +32,50 @@ use Bugzilla::FlagType;
 ####    Initialization     ####
 ###############################
 
+use constant DB_TABLE => 'components';
+
 use constant DB_COLUMNS => qw(
-    components.id
-    components.name
-    components.product_id
-    components.initialowner
-    components.initialqacontact
-    components.description
+    id
+    name
+    product_id
+    initialowner
+    initialqacontact
+    description
 );
 
-our $columns = join(", ", DB_COLUMNS);
-
 ###############################
 ####       Methods         ####
 ###############################
 
 sub new {
-    my $invocant = shift;
-    my $class = ref($invocant) || $invocant;
-    my $self = {};
-    bless($self, $class);
-    return $self->_init(@_);
-}
-
-sub _init {
-    my $self = shift;
-    my ($param) = (@_);
+    my $class = shift;
+    my $param = shift;
     my $dbh = Bugzilla->dbh;
 
-    my $id = $param unless (ref $param eq 'HASH');
-    my $component;
-
-    if (defined $id) {
-        detaint_natural($id)
-          || ThrowCodeError('param_must_be_numeric',
-                            {function => 'Bugzilla::Component::_init'});
-
-        $component = $dbh->selectrow_hashref(qq{
-            SELECT $columns FROM components
-            WHERE id = ?}, undef, $id);
-
-    } elsif (defined $param->{'product_id'}
-        && detaint_natural($param->{'product_id'})
-        && defined $param->{'name'}) {
-
-        trick_taint($param->{'name'});
-
-        $component = $dbh->selectrow_hashref(qq{
-            SELECT $columns FROM components
-            WHERE name = ? AND product_id = ?}, undef,
-            ($param->{'name'}, $param->{'product_id'}));
-    } else {
-        ThrowCodeError('bad_arg',
-            {argument => 'param',
-             function => 'Bugzilla::Component::_init'});
+    my $product;
+    if (ref $param) {
+        $product = $param->{product};
+        my $name = $param->{name};
+        if (!defined $product) {
+            ThrowCodeError('bad_arg',
+                {argument => 'product',
+                 function => "${class}::new"});
+        }
+        if (!defined $name) {
+            ThrowCodeError('bad_arg',
+                {argument => 'name',
+                 function => "${class}::new"});
+        }
+
+        my $condition = 'product_id = ? AND name = ?';
+        my @values = ($product->id, $name);
+        $param = { condition => $condition, values => \@values };
     }
 
-    return undef unless (defined $component);
-
-    foreach my $field (keys %$component) {
-        $self->{$field} = $component->{$field};
-    }
-    return $self;
+    unshift @_, $param;
+    my $component = $class->SUPER::new(@_);
+    $component->{product} = $product if $product;
+    return $component;
 }
 
 sub bug_count {
@@ -204,8 +188,8 @@ sub check_component {
     }
 
     my $component =
-        new Bugzilla::Component({product_id => $product->id,
-                                 name       => $comp_name});
+        new Bugzilla::Component({product => $product,
+                                 name    => $comp_name});
     unless ($component) {
         ThrowUserError('component_not_valid',
                        {'product' => $product->name,
@@ -227,8 +211,8 @@ Bugzilla::Component - Bugzilla product component class.
     use Bugzilla::Component;
 
     my $component = new Bugzilla::Component(1);
-    my $component = new Bugzilla::Component({product_id => 1,
-                                             name       => 'AcmeComp'});
+    my $component = new Bugzilla::Component({product => $product,
+                                             name    => 'AcmeComp'});
 
     my $bug_count          = $component->bug_count();
     my $bug_ids            = $component->bug_ids();
diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index a2ca8ff94..30ecc77e2 100644
--- a/Bugzilla/Object.pm
+++ b/Bugzilla/Object.pm
@@ -60,6 +60,8 @@ sub _init {
     my $object;
 
     if (defined $id) {
+        # We special-case if somebody specifies an ID, so that we can
+        # validate it as numeric.
         detaint_natural($id)
           || ThrowCodeError('param_must_be_numeric',
                             {function => $class . '::_init'});
@@ -67,23 +69,40 @@ sub _init {
         $object = $dbh->selectrow_hashref(qq{
             SELECT $columns FROM $table
              WHERE $id_field = ?}, undef, $id);
-    } elsif (defined $param->{'name'}) {
-        trick_taint($param->{'name'});
-        $object = $dbh->selectrow_hashref(qq{
-            SELECT $columns FROM $table
-             WHERE } . $dbh->sql_istrcmp($name_field, '?'), 
-            undef, $param->{'name'});
     } else {
-        ThrowCodeError('bad_arg',
-            {argument => 'param',
-             function => $class . '::_init'});
+        unless (defined $param->{name} || (defined $param->{'condition'} 
+                                           && defined $param->{'values'}))
+        {
+            ThrowCodeError('bad_arg', { argument => 'param',
+                                        function => $class . '::new' });
+        }
+
+        my ($condition, @values);
+        if (defined $param->{name}) {
+            $condition = $dbh->sql_istrcmp($name_field, '?');
+            push(@values, $param->{name});
+        }
+        elsif (defined $param->{'condition'} && defined $param->{'values'}) {
+            caller->isa('Bugzilla::Object')
+                || ThrowCodeError('protection_violation',
+                       { caller    => caller, 
+                         function  => $class . '::new',
+                         argument  => 'condition/values' });
+            $condition = $param->{'condition'};
+            push(@values, @{$param->{'values'}});
+        }
+
+        map { trick_taint($_) } @values;
+        $object = $dbh->selectrow_hashref(
+            "SELECT $columns FROM $table WHERE $condition", undef, @values);
     }
 
     return $object;
 }
 
 sub new_from_list {
-    my $class = shift;
+    my $invocant = shift;
+    my $class = ref($invocant) || $invocant;
     my ($id_list) = @_;
     my $dbh = Bugzilla->dbh;
     my $columns = join(',', $class->DB_COLUMNS);
@@ -363,17 +382,47 @@ the L</ID_FIELD> usually can't be updated.)
 
 =item C<new($param)>
 
- Description: The constructor is used to load an existing object
-              from the database, by id or by name.
+=over
+
+=item B<Description>
+
+The constructor is used to load an existing object from the database,
+by id or by name.
 
- Params:      $param - If you pass an integer, the integer is the
-                       id of the object, from the database, that we 
-                       want to read in. If you pass in a hash with 
-                       C<name> key, then the value of the name key 
-                       is the case-insensitive name of the object from 
-                       the DB.
+=item B<Params>
+
+If you pass an integer, the integer is the id of the object, 
+from the database, that we  want to read in. (id is defined
+as the value in the L</ID_FIELD> column).
+
+If you pass in a hash, you can pass a C<name> key. The 
+value of the C<name> key is the case-insensitive name of the object 
+(from L</NAME_FIELD>) in the DB.
+
+B<Additional Parameters Available for Subclasses>
+
+If you are a subclass of C<Bugzilla::Object>, you can pass
+C<condition> and C<values> as hash keys, instead of the above.
+
+C<condition> is a set of SQL conditions for the WHERE clause, which contain
+placeholders.
+
+C<values> is a reference to an array. The array contains the values
+for each placeholder in C<condition>, in order.
 
- Returns:     A fully-initialized object.
+This is to allow subclasses to have complex parameters, and then to
+translate those parameters into C<condition> and C<values> when they
+call C<$self->SUPER::new> (which is this function, usually).
+
+If you try to call C<new> outside of a subclass with the C<condition>
+and C<values> parameters, Bugzilla will throw an error. These parameters
+are intended B<only> for use by subclasses.
+
+=item B<Returns>
+
+A fully-initialized object.
+
+=back
 
 =item C<new_from_list(\@id_list)>
 
diff --git a/Bugzilla/Product.pm b/Bugzilla/Product.pm
index 4edc7ef85..465d12515 100644
--- a/Bugzilla/Product.pm
+++ b/Bugzilla/Product.pm
@@ -65,12 +65,8 @@ sub components {
             WHERE product_id = ?
             ORDER BY name}, undef, $self->id);
 
-        my @components;
         require Bugzilla::Component;
-        foreach my $id (@$ids) {
-            push @components, new Bugzilla::Component($id);
-        }
-        $self->{components} = \@components;
+        $self->{components} = Bugzilla::Component->new_from_list($ids);
     }
     return $self->{components};
 }
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 05b96dd29..a4ff44331 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -931,10 +931,7 @@ sub product_responsibilities {
 
     # We cannot |use| it, because Component.pm already |use|s User.pm.
     require Bugzilla::Component;
-    my @components;
-    push(@components, new Bugzilla::Component($_)) foreach (@$comp_ids);
-
-    $self->{'product_resp'} = \@components;
+    $self->{'product_resp'} = Bugzilla::Component->new_from_list($comp_ids);
     return $self->{'product_resp'};
 }
 
diff --git a/editcomponents.cgi b/editcomponents.cgi
index 17ad290c5..f28a87b7a 100755
--- a/editcomponents.cgi
+++ b/editcomponents.cgi
@@ -168,7 +168,7 @@ if ($action eq 'new') {
     }
 
     my $component =
-        new Bugzilla::Component({product_id => $product->id,
+        new Bugzilla::Component({product => $product,
                                  name => $comp_name});
 
     if ($component) {
@@ -200,7 +200,7 @@ if ($action eq 'new') {
              ($product->id, $comp_name, $description,
               $default_assignee_id, $default_qa_contact_id));
 
-    $component = new Bugzilla::Component({ product_id => $product->id,
+    $component = new Bugzilla::Component({ product => $product,
                                            name => $comp_name });
 
     my $sth = $dbh->prepare("INSERT INTO component_cc 
@@ -383,7 +383,7 @@ if ($action eq 'update') {
 
     if ($comp_name ne $component_old->name) {
         my $component =
-            new Bugzilla::Component({product_id => $product->id,
+            new Bugzilla::Component({product => $product,
                                      name => $comp_name});
         if ($component) {
             ThrowUserError('component_already_exists',
diff --git a/importxml.pl b/importxml.pl
index b637f98fd..989eb31e0 100755
--- a/importxml.pl
+++ b/importxml.pl
@@ -326,8 +326,8 @@ sub init() {
                   moved-default-product. \n", "REOPEN", $exporter);
     my $def_component = new Bugzilla::Component(
         {
-            product_id => $def_product->id,
-            name       => $params->{"moved-default-component"}
+            product => $def_product,
+            name    => $params->{"moved-default-component"}
         })
     || Error("Cannot import these bugs because an invalid default 
               component was defined for the target db."
@@ -622,8 +622,8 @@ sub process_bug {
       new Bugzilla::Product( { name => $params->{"moved-default-product"} } );
     my $def_component = new Bugzilla::Component(
         {
-            product_id => $def_product->id,
-            name       => $params->{"moved-default-component"}
+            product => $def_product,
+            name    => $params->{"moved-default-component"}
         }
     );
     my $product;
@@ -643,8 +643,8 @@ sub process_bug {
     if ( defined $bug_fields{'component'} ) {
         $component = new Bugzilla::Component(
             {
-                product_id => $product->id,
-                name       => $bug_fields{'component'}
+                product => $product,
+                name    => $bug_fields{'component'}
             }
         );
         unless ($component) {
diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl
index a0e9bd9b9..d48b89993 100644
--- a/template/en/default/global/code-error.html.tmpl
+++ b/template/en/default/global/code-error.html.tmpl
@@ -325,6 +325,11 @@
 
   [% ELSIF error == "protection_violation" %]
     The function <code>[% function FILTER html %]</code> was called
+
+    [% IF argument %]
+        with the argument <code>[% argument FILTER html %]</code>
+    [% END %]
+
     from
    
     [% IF caller %]