Bug 539865: Make Bugzilla::Object pass $params to validators during create()

(implement VALIDATOR_DEPENDENCIES)
r=LpSolit, a=LpSolit

5 files changed, 65 insertions, 16 deletions

diff --git a/Bugzilla/Comment.pm b/Bugzilla/Comment.pm
index ba33ba5f3..be10329d9 100644
--- a/Bugzilla/Comment.pm
+++ b/Bugzilla/Comment.pm
@@ -30,6 +30,8 @@ use Bugzilla::Error;
 use Bugzilla::User;
 use Bugzilla::Util;
 
+use Scalar::Util qw(blessed);
+
 ###############################
 ####    Initialization     ####
 ###############################
@@ -57,11 +59,12 @@ use constant ID_FIELD => 'comment_id';
 use constant LIST_ORDER => 'bug_when';
 
 use constant VALIDATORS => {
+    extra_data => \&_check_extra_data,
     type => \&_check_type,
 };
 
-use constant UPDATE_VALIDATORS => {
-    extra_data => \&_check_extra_data,
+use constant VALIDATOR_DEPENDENCIES => {
+    extra_data => ['type'],
 };
 
 #########################
@@ -154,9 +157,8 @@ sub body_full {
 sub set_extra_data { $_[0]->set('extra_data', $_[1]); }
 
 sub set_type {
-    my ($self, $type, $extra_data) = @_;
+    my ($self, $type) = @_;
     $self->set('type', $type);
-    $self->set_extra_data($extra_data);
 }
 
 ##############
@@ -164,8 +166,9 @@ sub set_type {
 ##############
 
 sub _check_extra_data {
-    my ($invocant, $extra_data, $type) = @_;
-    $type = $invocant->type if ref $invocant;
+    my ($invocant, $extra_data, undef, $params) = @_;
+    my $type = blessed($invocant) ? $invocant->type : $params->{type};
+
     if ($type == CMT_NORMAL) {
         if (defined $extra_data) {
             ThrowCodeError('comment_extra_data_not_allowed',
diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index 2477244df..e1c7661ed 100644
--- a/Bugzilla/Object.pm
+++ b/Bugzilla/Object.pm
@@ -37,6 +37,7 @@ use constant LIST_ORDER => NAME_FIELD;
 use constant UPDATE_VALIDATORS => {};
 use constant NUMERIC_COLUMNS   => ();
 use constant DATE_COLUMNS      => ();
+use constant VALIDATOR_DEPENDENCIES => {};
 
 # This allows the JSON-RPC interface to return Bugzilla::Object instances
 # as though they were hashes. In the future, this may be modified to return
@@ -313,7 +314,10 @@ sub set {
 
 sub set_all {
     my ($self, $params) = @_;
-    foreach my $key (keys %$params) {
+
+    my @sorted_names = sort { $self->_cmp_dependency($a, $b) }
+                            (keys %$params);
+    foreach my $key (@sorted_names) {
         my $method = "set_$key";
         $self->$method($params->{$key});
     }
@@ -447,19 +451,21 @@ sub run_create_validators {
     my ($class, $params) = @_;
 
     my $validators = $class->_get_validators;
+    my %field_values = %$params;
 
-    my %field_values;
-    # We do the sort just to make sure that validation always
-    # happens in a consistent order.
-    foreach my $field (sort keys %$params) {
+    my @sorted_names = sort { $class->_cmp_dependency($a, $b) } 
+                            (keys %field_values);
+    foreach my $field (@sorted_names) {
         my $value;
         if (exists $validators->{$field}) {
             my $validator = $validators->{$field};
-            $value = $class->$validator($params->{$field}, $field);
+            $value = $class->$validator($field_values{$field}, $field,
+                                        \%field_values);
         }
         else {
-            $value = $params->{$field};
+            $value = $field_values{$field};
         }
+
         # We want people to be able to explicitly set fields to NULL,
         # and that means they can be set to undef.
         trick_taint($value) if defined $value && !ref($value);
@@ -503,6 +509,30 @@ sub get_all {
 
 sub check_boolean { return $_[1] ? 1 : 0 }
 
+###################
+# General Helpers #
+###################
+
+# Helps sort fields according to VALIDATOR_DEPENDENCIES.
+sub _cmp_dependency {
+    my ($invocant, $a, $b) = @_;
+    my $dependencies = $invocant->VALIDATOR_DEPENDENCIES;
+    # If $a is a key in the hash and $b is one of its dependencies, then
+    # $b should come first (meaning $a is "greater" than $b).
+    if (my $b_first = $dependencies->{$a}) {
+        return 1 if grep { $_ eq $b } @$b_first;
+    }
+    # If $b is a key in the hash and $a is one of its dependencies,
+    # then $a should come first (meaning $a is "less" than $b).
+    if (my $a_first = $dependencies->{$b}) {
+        return -1 if grep { $_ eq $a } @$a_first;
+    }
+
+    # Sort alphabetically so that we get a consistent order for fields
+    # that don't have dependencies.
+    return $a cmp $b;
+}
+
 ####################
 # Constant Helpers #
 ####################
@@ -651,6 +681,20 @@ here must not appear in L</VALIDATORS>.
 
 L<Bugzilla::Bug> has good examples in its code of when to use this.
 
+=item C<VALIDATOR_DEPENDENCIES>
+
+During L</create> and L</set_all>, validators are normally called in
+a somewhat-random order.  If you need one field to be validated and set
+before another field, this constant is how you do it, by saying that
+one field "depends" on the value of other fields.
+
+This is a hashref, where the keys are field names and the values are
+arrayrefs of field names. You specify what fields a field depends on using
+the arrayrefs. So, for example, to say that a C<component> field depends
+on the C<product> field being set, you would do:
+
+ component => ['product']
+
 =item C<UPDATE_COLUMNS>
 
 A list of columns to update when L</update> is called.
diff --git a/email_in.pl b/email_in.pl
index 38ff17cc5..8e2c00053 100755
--- a/email_in.pl
+++ b/email_in.pl
@@ -243,7 +243,8 @@ sub handle_attachments {
         # and this is our first attachment, then we make the comment an 
         # "attachment created" comment.
         if ($comment and !$comment->type and !$update_comment) {
-            $comment->set_type(CMT_ATTACHMENT_CREATED, $obj->id);
+            $comment->set_all({ type       => CMT_ATTACHMENT_CREATED, 
+                                extra_data => $obj->id });
             $update_comment = 1;
         }
         else {
diff --git a/extensions/Example/Extension.pm b/extensions/Example/Extension.pm
index 26a91b789..87061aa06 100644
--- a/extensions/Example/Extension.pm
+++ b/extensions/Example/Extension.pm
@@ -415,7 +415,7 @@ sub object_end_of_set {
 sub object_end_of_set_all {
     my ($self, $args) = @_;
     
-    my $object = $args->{'class'};
+    my $object = $args->{'object'};
     my $object_params = $args->{'params'};
     
     # Note that this is a made-up class, for this example.
diff --git a/post_bug.cgi b/post_bug.cgi
index 881568298..0f78cc5cd 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -217,7 +217,8 @@ if (defined($cgi->upload('data')) || $cgi->param('attachurl')) {
         $attachment->set_flags($flags, $new_flags);
         $attachment->update($timestamp);
         my $comment = $bug->comments->[0];
-        $comment->set_type(CMT_ATTACHMENT_CREATED, $attachment->id);
+        $comment->set_all({ type => CMT_ATTACHMENT_CREATED, 
+                            extra_data => $attachment->id });
         $comment->update();
     }
     else {