Bug 440612 – Use Bugzilla::Bug->check everywhere instead of ValidateBugID

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

13 files changed, 47 insertions, 60 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index c32672ce2..e2620203e 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -49,7 +49,7 @@ use Storable qw(dclone);
 
 use base qw(Bugzilla::Object Exporter);
 @Bugzilla::Bug::EXPORT = qw(
-    bug_alias_to_id ValidateBugID
+    bug_alias_to_id
     RemoveVotes CheckIfVotedConfirmed
     LogActivityEntry
     editable_bug_fields
@@ -1091,8 +1091,8 @@ sub _check_dependencies {
         my @bug_ids = split(/[\s,]+/, $deps_in{$type});
         # Eliminate nulls.
         @bug_ids = grep {$_} @bug_ids;
-        # We do Validate up here to make sure all aliases are converted to IDs.
-        ValidateBugID($_, $type) foreach @bug_ids;
+        # We do this up here to make sure all aliases are converted to IDs.
+        @bug_ids = map { $invocant->check($_, $type)->id } @bug_ids;
        
         my @check_access = @bug_ids;
         # When we're updating a bug, only added or removed bug_ids are 
@@ -1114,11 +1114,10 @@ sub _check_dependencies {
 
         my $user = Bugzilla->user;
         foreach my $modified_id (@check_access) {
-            ValidateBugID($modified_id);
+            my $delta_bug = $invocant->check($modified_id);
             # Under strict isolation, you can't modify a bug if you can't
             # edit it, even if you can see it.
             if (Bugzilla->params->{"strict_isolation"}) {
-                my $delta_bug = new Bugzilla::Bug($modified_id);
                 if (!$user->can_edit_product($delta_bug->{'product_id'})) {
                     ThrowUserError("illegal_change_deps", {field => $type});
                 }
@@ -1142,7 +1141,7 @@ sub _check_dup_id {
     $dupe_of = trim($dupe_of);
     $dupe_of || ThrowCodeError('undefined_field', { field => 'dup_id' });
     # Make sure we can change the original bug (issue A on bug 96085)
-    ValidateBugID($dupe_of, 'dup_id');
+    my $dupe_of_bug = $self->check($dupe_of, 'dup_id');
     
     # Make sure a loop isn't created when marking this bug
     # as duplicate.
@@ -1174,7 +1173,6 @@ sub _check_dup_id {
     # Should we add the reporter to the CC list of the new bug?
     # If he can see the bug...
     if ($self->reporter->can_see_bug($dupe_of)) {
-        my $dupe_of_bug = new Bugzilla::Bug($dupe_of);
         # We only add him if he's not the reporter of the other bug.
         $self->{_add_dup_cc} = 1
             if $dupe_of_bug->reporter->id != $self->reporter->id;
@@ -1199,9 +1197,7 @@ sub _check_dup_id {
             my $vars = {};
             my $template = Bugzilla->template;
             # Ask the user what they want to do about the reporter.
-            $vars->{'cclist_accessible'} = $dbh->selectrow_array(
-                q{SELECT cclist_accessible FROM bugs WHERE bug_id = ?},
-                undef, $dupe_of);
+            $vars->{'cclist_accessible'} = $dupe_of_bug->cclist_accessible;
             $vars->{'original_bug_id'} = $dupe_of;
             $vars->{'duplicate_bug_id'} = $self->id;
             print $cgi->header();
diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm
index dd866a67b..3323fd5dd 100755
--- a/Bugzilla/WebService/Bug.pm
+++ b/Bugzilla/WebService/Bug.pm
@@ -123,7 +123,8 @@ sub get_history {
     my @return;
     foreach my $bug_id (@$ids) {
         my %item;
-        ValidateBugID($bug_id);
+        my $bug = Bugzilla::Bug->check($bug_id);
+        $bug_id = $bug->id;
 
         my ($activity) = Bugzilla::Bug::GetBugActivity($bug_id);
         $item{$bug_id} = [];
@@ -155,7 +156,6 @@ sub get_history {
         # alias is returned in case users passes a mixture of ids and aliases
         # then they get to know which bug activity relates to which value  
         # they passed
-        my $bug = new Bugzilla::Bug($bug_id);
         if (Bugzilla->params->{'usebugaliases'}) {
             $item{alias} = type('string')->value($bug->alias);
         }
diff --git a/attachment.cgi b/attachment.cgi
index 2520c0032..c28a300a0 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -165,8 +165,10 @@ sub validateID {
       || ThrowUserError("invalid_attach_id", { attach_id => $attach_id });
 
     # Make sure the user is authorized to access this attachment's bug.
-    ValidateBugID($attachment->bug_id);
-    if ($attachment->isprivate && $user->id != $attachment->attacher->id && !$user->is_insider) {
+    Bugzilla::Bug->check($attachment->bug_id);
+    if ($attachment->isprivate && $user->id != $attachment->attacher->id 
+        && !$user->is_insider) 
+    {
         ThrowUserError('auth_failure', {action => 'access',
                                         object => 'attachment'});
     }
@@ -281,9 +283,8 @@ sub diff {
 # HTML page.
 sub viewall {
     # Retrieve and validate parameters
-    my $bugid = $cgi->param('bugid');
-    ValidateBugID($bugid);
-    my $bug = new Bugzilla::Bug($bugid);
+    my $bug = Bugzilla::Bug->check(scalar $cgi->param('bugid'));
+    my $bugid = $bug->id;
 
     my $attachments = Bugzilla::Attachment->get_attachments_by_bug($bugid);
 
@@ -301,13 +302,12 @@ sub viewall {
 # Display a form for entering a new attachment.
 sub enter {
   # Retrieve and validate parameters
-  my $bugid = $cgi->param('bugid');
-  ValidateBugID($bugid);
+  my $bug = Bugzilla::Bug->check(scalar $cgi->param('bugid'));
+  my $bugid = $bug->id;
   validateCanChangeBug($bugid);
   my $dbh = Bugzilla->dbh;
   my $user = Bugzilla->user;
 
-  my $bug = new Bugzilla::Bug($bugid, $user->id);
   # Retrieve the attachments the user can edit from the database and write
   # them into an array of hashes where each hash represents one attachment.
   my $canEdit = "";
@@ -344,8 +344,8 @@ sub insert {
     $dbh->bz_start_transaction;
 
     # Retrieve and validate parameters
-    my $bugid = $cgi->param('bugid');
-    ValidateBugID($bugid);
+    my $bug = Bugzilla::Bug->check(scalar $cgi->param('bugid'));
+    my $bugid = $bug->id;
     validateCanChangeBug($bugid);
     my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()");
 
@@ -373,7 +373,6 @@ sub insert {
         }
     }
 
-    my $bug = new Bugzilla::Bug($bugid);
     my $attachment =
         Bugzilla::Attachment->insert_attachment_for_bug(THROW_ERROR, $bug, $user,
                                                         $timestamp, $vars);
diff --git a/buglist.cgi b/buglist.cgi
index 1ae9f84a7..2d4e323b2 100755
--- a/buglist.cgi
+++ b/buglist.cgi
@@ -562,8 +562,8 @@ elsif (($cgi->param('cmdtype') eq "doit") && defined $cgi->param('remtype')) {
             my $changes = 0;
             foreach my $bug_id (split(/[\s,]+/, $cgi->param('bug_ids'))) {
                 next unless $bug_id;
-                ValidateBugID($bug_id);
-                $bug_ids{$bug_id} = $keep_bug;
+                my $bug = Bugzilla::Bug->check($bug_id);
+                $bug_ids{$bug->id} = $keep_bug;
                 $changes = 1;
             }
             ThrowUserError('no_bug_ids',
diff --git a/email_in.pl b/email_in.pl
index 12be24471..742b84f5f 100644
--- a/email_in.pl
+++ b/email_in.pl
@@ -41,7 +41,7 @@ use Pod::Usage;
 use Encode;
 
 use Bugzilla;
-use Bugzilla::Bug qw(ValidateBugID);
+use Bugzilla::Bug;
 use Bugzilla::Constants qw(USAGE_MODE_EMAIL);
 use Bugzilla::Error;
 use Bugzilla::Mailer;
@@ -172,8 +172,7 @@ sub process_bug {
 
     debug_print("Updating Bug $fields{id}...");
 
-    ValidateBugID($bug_id);
-    my $bug = new Bugzilla::Bug($bug_id);
+    my $bug = Bugzilla::Bug->check($bug_id);
 
     if ($fields{'bug_status'}) {
         $fields{'knob'} = $fields{'bug_status'};
diff --git a/enter_bug.cgi b/enter_bug.cgi
index ee362e302..9ee58ea53 100755
--- a/enter_bug.cgi
+++ b/enter_bug.cgi
@@ -350,8 +350,8 @@ my $has_canconfirm = $user->in_group('canconfirm', $product->id);
 $cloned_bug_id = $cgi->param('cloned_bug_id');
 
 if ($cloned_bug_id) {
-    ValidateBugID($cloned_bug_id);
-    $cloned_bug = new Bugzilla::Bug($cloned_bug_id);
+    $cloned_bug = Bugzilla::Bug->check($cloned_bug_id);
+    $cloned_bug_id = $cloned_bug->id;
 }
 
 if (scalar(@{$product->components}) == 1) {
diff --git a/process_bug.cgi b/process_bug.cgi
index bb680fab2..814ff5612 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -112,23 +112,16 @@ sub should_set {
 # Create a list of objects for all bugs being modified in this request.
 my @bug_objects;
 if (defined $cgi->param('id')) {
-  my $id = $cgi->param('id');
-  ValidateBugID($id);
-
-  # Store the validated, and detainted id back in the cgi data, as
-  # lots of later code will need it, and will obtain it from there
-  $cgi->param('id', $id);
-  push(@bug_objects, new Bugzilla::Bug($id));
+  my $bug = Bugzilla::Bug->check(scalar $cgi->param('id'));
+  $cgi->param('id', $bug->id);
+  push(@bug_objects, $bug);
 } else {
-    my @ids;
     foreach my $i ($cgi->param()) {
         if ($i =~ /^id_([1-9][0-9]*)/) {
             my $id = $1;
-            ValidateBugID($id);
-            push(@ids, $id);
+            push(@bug_objects, Bugzilla::Bug->check($id));
         }
     }
-    @bug_objects = @{Bugzilla::Bug->new_from_list(\@ids)};
 }
 
 # Make sure there are bugs to process.
diff --git a/show_activity.cgi b/show_activity.cgi
index d2570f8b1..f7db3dd0b 100755
--- a/show_activity.cgi
+++ b/show_activity.cgi
@@ -43,17 +43,17 @@ Bugzilla->login();
 
 # Make sure the bug ID is a positive integer representing an existing
 # bug that the user is authorized to access.
-my $bug_id = $cgi->param('id');
-ValidateBugID($bug_id);
+my $id = $cgi->param('id');
+my $bug = Bugzilla::Bug->check($id);
 
 ###############################################################################
 # End Data/Security Validation
 ###############################################################################
 
 ($vars->{'operations'}, $vars->{'incomplete_data'}) = 
-    Bugzilla::Bug::GetBugActivity($bug_id);
+    Bugzilla::Bug::GetBugActivity($bug->id);
 
-$vars->{'bug'} = new Bugzilla::Bug($bug_id);
+$vars->{'bug'} = $bug;
 
 print $cgi->header();
 
diff --git a/show_bug.cgi b/show_bug.cgi
index 782293af5..b3251b9d1 100755
--- a/show_bug.cgi
+++ b/show_bug.cgi
@@ -57,10 +57,7 @@ my %marks;
 
 if ($single) {
     my $id = $cgi->param('id');
-    # Its a bit silly to do the validation twice - that functionality should
-    # probably move into Bug.pm at some point
-    ValidateBugID($id);
-    push @bugs, new Bugzilla::Bug($id);
+    push @bugs, Bugzilla::Bug->check($id);
     if (defined $cgi->param('mark')) {
         foreach my $range (split ',', $cgi->param('mark')) {
             if ($range =~ /^(\d+)-(\d+)$/) {
diff --git a/showdependencygraph.cgi b/showdependencygraph.cgi
index f7977446e..327f73c9d 100755
--- a/showdependencygraph.cgi
+++ b/showdependencygraph.cgi
@@ -135,8 +135,8 @@ if ($display eq 'doall') {
     }
 } else {
     foreach my $i (split('[\s,]+', $cgi->param('id'))) {
-        ValidateBugID($i);
-        $baselist{$i} = 1;
+        my $bug = Bugzilla::Bug->check($i);
+        $baselist{$bug->id} = 1;
     }
 
     my @stack = keys(%baselist);
diff --git a/showdependencytree.cgi b/showdependencytree.cgi
index 80e67716a..8683c190c 100755
--- a/showdependencytree.cgi
+++ b/showdependencytree.cgi
@@ -49,9 +49,8 @@ my $dbh = Bugzilla->switch_to_shadow_db();
 
 # Make sure the bug ID is a positive integer representing an existing
 # bug that the user is authorized to access.
-my $id = $cgi->param('id') || ThrowUserError('improper_bug_id_field_value');
-ValidateBugID($id);
-my $current_bug = new Bugzilla::Bug($id);
+my $bug = Bugzilla::Bug->check(scalar $cgi->param('id'));
+my $id = $bug->id;
 
 local our $hide_resolved = $cgi->param('hide_resolved') ? 1 : 0;
 
@@ -67,7 +66,7 @@ local our $realdepth = 0;
 
 # Generate the tree of bugs that this bug depends on and a list of IDs
 # appearing in the tree.
-my $dependson_tree = { $id => $current_bug };
+my $dependson_tree = { $id => $bug };
 my $dependson_ids = {};
 GenerateTree($id, "dependson", 1, $dependson_tree, $dependson_ids);
 $vars->{'dependson_tree'} = $dependson_tree;
@@ -75,7 +74,7 @@ $vars->{'dependson_ids'} = [keys(%$dependson_ids)];
 
 # Generate the tree of bugs that this bug blocks and a list of IDs
 # appearing in the tree.
-my $blocked_tree = { $id => $current_bug };
+my $blocked_tree = { $id => $bug };
 my $blocked_ids = {};
 GenerateTree($id, "blocked", 1, $blocked_tree, $blocked_ids);
 $vars->{'blocked_tree'} = $blocked_tree;
diff --git a/summarize_time.cgi b/summarize_time.cgi
index 071f89a67..0330f9dcf 100755
--- a/summarize_time.cgi
+++ b/summarize_time.cgi
@@ -251,7 +251,7 @@ $user->in_group(Bugzilla->params->{"timetrackinggroup"})
                                        object => "timetracking_summaries"});
 
 my @ids = split(",", $cgi->param('id'));
-map { ValidateBugID($_) } @ids;
+@ids = map { Bugzilla::Bug->check($_)->id } @ids;
 scalar(@ids) || ThrowUserError('no_bugs_chosen', {action => 'view'});
 
 my $group_by = $cgi->param('group_by') || "number";
diff --git a/votes.cgi b/votes.cgi
index 961db7aa5..af41af0e4 100755
--- a/votes.cgi
+++ b/votes.cgi
@@ -67,7 +67,10 @@ else {
 # Make sure the bug ID is a positive integer representing an existing
 # bug that the user is authorized to access.
 
-ValidateBugID($bug_id) if defined $bug_id;
+if (defined $bug_id) {
+    my $bug = Bugzilla::Bug->check($bug_id);
+    $bug_id = $bug->id;
+}
 
 ################################################################################
 # End Data/Security Validation
@@ -244,14 +247,15 @@ sub record_votes {
         }
     }
 
-    # Call ValidateBugID on each bug ID to make sure it is a positive
+    # Call check() on each bug ID to make sure it is a positive
     # integer representing an existing bug that the user is authorized 
     # to access, and make sure the number of votes submitted is also
     # a non-negative integer (a series of digits not preceded by a
     # minus sign).
     my %votes;
     foreach my $id (@buglist) {
-      ValidateBugID($id);
+      my $bug = Bugzilla::Bug->check($id);
+      $id = $bug->id;
       $votes{$id} = $cgi->param($id);
       detaint_natural($votes{$id}) 
         || ThrowUserError("votes_must_be_nonnegative");