Bug 199813 - Make all users of ThrowUserError pass $vars in explicitly.

Bugzilla/Error.pm

+# -*- Mode: perl; indent-tabs-mode: nil -*-
+#
+# The contents of this file are subject to the Mozilla Public
+# License Version 1.1 (the "License"); you may not use this file
+# except in compliance with the License. You may obtain a copy of
+# the License at http://www.mozilla.org/MPL/
+#
+# Software distributed under the License is distributed on an "AS
+# IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
+# implied. See the License for the specific language governing
+# rights and limitations under the License.
+#
+# The Original Code is the Bugzilla Bug Tracking System.
+#
+# 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): Bradley Baetz <bbaetz@acm.org>
+
+use strict;
+
+package Bugzilla::Error;
+
+use base qw(Exporter);
+
+@Bugzilla::Error::EXPORT = qw(ThrowUserError);
+
+sub ThrowUserError {
+    my ($error, $vars, $unlock_tables) = @_;
+
+    $vars ||= {};
+
+    $vars->{error} = $error;
+
+    # Need to do this until User.pm goes in, so that the footer is correct
+    $vars->{user} = $::vars->{user};
+
+    Bugzilla->dbh->do("UNLOCK TABLES") if $unlock_tables;
+
+    # XXX - mod_perl
+    print "Content-type: text/html\n\n" if !$::vars->{'header_done'};
+
+    my $template = Bugzilla->template;
+    $template->process("global/user-error.html.tmpl", $vars)
+      || &::ThrowTemplateError($template->error());
+
+    exit;
+}
+
+1;
+
+__END__
+
+=head1 NAME
+
+Bugzilla::Error - Error handling utilities for Bugzilla
+
+=head1 SYNOPSIS
+
+  use Bugzilla::Error;
+
+  ThrowUserError("error_tag",
+                 { foo => 'bar' });
+
+=head1 DESCRIPTION
+
+Various places throughout the Bugzilla codebase need to report errors to the
+user. The C<Throw*Error> family of functions allow this to be done in a
+generic and localisable manner.
+
+=head1 FUNCTIONS
+
+=over 4
+
+=item C<ThrowUserError>
+
+This function takes an error tag as the first argument, and an optional hashref
+of variables as a second argument. These are used by the
+I<global/user-error.html.tmpl> template to format the error, using the passed
+in variables as required.
+
+An optional third argument may be supplied. If present (and defined), then the
+error handling code will unlock the database tables. In the long term, this
+argument will go away, to be replaced by transactional C<rollback> calls. There
+is no timeframe for doing so, however.
+
+=back
+
+=head1 SEE ALSO
+
+L<Bugzilla|Bugzilla>

Bugzilla/Search.pm

@@ -103,8 +103,8 @@ sub init {
         my $c = trim($params->param('votes'));
         if ($c ne "") {
             if ($c !~ /^[0-9]*$/) {
-                $::vars->{'value'} = $c;
-                &::ThrowUserError("illegal_at_least_x_votes");
+                &::ThrowUserError("illegal_at_least_x_votes",
+                                  { value => $c });
             }
             push(@specialchart, ["votes", "greaterthan", $c - 1]);
         }
@@ -207,8 +207,8 @@ sub init {
         if (@clist) {
             push(@specialchart, \@clist);
         } else {
-            $::vars->{'email'} = $email;
-            &::ThrowUserError("missing_email_type");
+            ThrowUserError("missing_email_type",
+                           { email => $email });
         }
     }
 
@@ -217,8 +217,8 @@ sub init {
         my $c = trim($params->param('changedin'));
         if ($c ne "") {
             if ($c !~ /^[0-9]*$/) {
-                $::vars->{'value'} = $c;
-                &::ThrowUserError("illegal_changed_in_last_x_days");
+                &::ThrowUserError("illegal_changed_in_last_x_days",
+                                 { value => $c });
             }
             push(@specialchart, ["changedin",
                                  "lessthan", $c + 1]);
@@ -558,8 +558,8 @@ sub init {
                      push(@list, "$table.keywordid = $id");
                  }
                  else {
-                     $::vars->{'keyword'} = $v;
-                     &::ThrowUserError("unknown_keyword");
+                     ThrowUserError("unknown_keyword",
+                                    { keyword => $v });
                  }
              }
              my $haveawordterm;
@@ -992,8 +992,7 @@ sub SqlifyDate {
     }
     my $date = str2time($str);
     if (!defined($date)) {
-        $::vars->{'date'} = $str;
-        &::ThrowUserError("illegal_date");
+        &::ThrowUserError("illegal_date", { date => $str });
     }
     return time2str("%Y-%m-%d %H:%M:%S", $date);
 }

CGI.pl

@@ -35,6 +35,7 @@ use lib ".";
 use Bugzilla::Util;
 use Bugzilla::Config;
 use Bugzilla::Constants;
+use Bugzilla::Error;
 
 # Shut up misguided -w warnings about "used only once".  For some reason,
 # "use vars" chokes on me when I try it here.
@@ -252,8 +253,7 @@ sub CheckEmailSyntax {
     my ($addr) = (@_);
     my $match = Param('emailregexp');
     if ($addr !~ /$match/ || $addr =~ /[\\\(\)<>&,;:"\[\] \t\r\n]/) {
-        $vars->{'addr'} = $addr;
-        ThrowUserError("illegal_email_address");
+        ThrowUserError("illegal_email_address", { addr => $addr });
     }
 }
 
@@ -327,24 +327,6 @@ sub ThrowCodeError {
   exit;
 }
 
-# For errors made by the user.
-sub ThrowUserError {
-  ($vars->{'error'}, my $extra_vars, my $unlock_tables) = (@_);
-
-  SendSQL("UNLOCK TABLES") if $unlock_tables;
- 
-  # Copy the extra_vars into the vars hash 
-  foreach my $var (keys %$extra_vars) {
-      $vars->{$var} = $extra_vars->{$var};
-  }
-  
-  print "Content-type: text/html\n\n" if !$vars->{'header_done'};
-  $template->process("global/user-error.html.tmpl", $vars)
-    || ThrowTemplateError($template->error());
-    
-  exit;
-}
-
 # This function should only be called if a template->process() fails.
 # It tries another template first, because often one template being
 # broken or missing doesn't mean that they all are. But it falls back on

attachment.cgi

@@ -171,7 +171,8 @@ sub validateCanEdit
             "attach_id = $attach_id AND submitter_id = $::userid");
 
     FetchSQLData()
-      || ThrowUserError("illegal_attachment_edit");
+      || ThrowUserError("illegal_attachment_edit",
+                        { attach_id => $attach_id });
 }
 
 sub validateCanChangeAttachment 
@@ -183,7 +184,8 @@ sub validateCanChangeAttachment
              AND bugs.bug_id = attachments.bug_id");
     my $productid = FetchOneColumn();
     CanEditProductId($productid)
-      || ThrowUserError("illegal_attachment_edit");
+      || ThrowUserError("illegal_attachment_edit",
+                        { attach_id => $attachid });
 }
 
 sub validateCanChangeBug
@@ -194,7 +196,8 @@ sub validateCanChangeBug
              WHERE bug_id = $bugid");
     my $productid = FetchOneColumn();
     CanEditProductId($productid)
-      || ThrowUserError("illegal_attachment_edit");
+      || ThrowUserError("illegal_attachment_edit_bug",
+                        { bug_id => $bugid });
 }
 
 sub validateDescription
@@ -249,8 +252,8 @@ sub validateContentType
 
   if ( $::FORM{'contenttype'} !~ /^(application|audio|image|message|model|multipart|text|video)\/.+$/ )
   {
-    $vars->{'contenttype'} = $::FORM{'contenttype'};
-    ThrowUserError("invalid_content_type");
+    ThrowUserError("invalid_content_type",
+                   { contenttype => $::FORM{'contenttype'} });
   }
 }
 
@@ -292,11 +295,11 @@ sub validateData
   # Make sure the attachment does not exceed the maximum permitted size
   my $len = length($data);
   if ($maxsize && $len > $maxsize) {
-      $vars->{'filesize'} = sprintf("%.0f", $len/1024);
+      my $vars = { filesize => sprintf("%.0f", $len/1024) };
       if ( $::FORM{'ispatch'} ) {
-          ThrowUserError("patch_too_large");
+          ThrowUserError("patch_too_large", $vars);
       } else {
-          ThrowUserError("file_too_large");
+          ThrowUserError("file_too_large", $vars);
       }
   }
 
@@ -328,6 +331,9 @@ sub validateObsolete
   # Make sure the attachment id is valid and the user has permissions to view
   # the bug to which it is attached.
   foreach my $attachid (@{$::MFORM{'obsolete'}}) {
+    # my $vars after ThrowCodeError is updated to not use the global
+    # vars hash
+
     $vars->{'attach_id'} = $attachid;
     
     detaint_natural($attachid)
@@ -338,7 +344,7 @@ sub validateObsolete
 
     # Make sure the attachment exists in the database.
     MoreSQLData()
-      || ThrowUserError("invalid_attach_id");
+      || ThrowUserError("invalid_attach_id", $vars);
 
     my ($bugid, $isobsolete, $description) = FetchSQLData();
 
@@ -663,8 +669,8 @@ sub update
   my $bugid = FetchSQLData();
   unless ($bugid) 
   {
-    $vars->{'bug_id'} = $bugid;
-    ThrowUserError("invalid_bug_id");
+    ThrowUserError("invalid_bug_id",
+                   { bug_id => $bugid });
   }
   
   # Lock database tables in preparation for updating the attachment.

describecomponents.cgi

@@ -83,8 +83,8 @@ my $product = $::FORM{'product'};
 my $product_id = get_product_id($product);
 
 if (!$product_id) {
-    $::vars->{'product'} = $product;
-    ThrowUserError("invalid_product_name");
+    ThrowUserError("invalid_product_name",
+                   { product => $product });
 }
 
 # Make sure the user is authorized to access this product.

duplicates.cgi

@@ -83,8 +83,8 @@ my $product_id;
 if ($product) {
     $product_id = get_product_id($product);
     if (!$product_id) {
-        $vars->{'product'} = $product;
-        ThrowUserError("invalid_product_name");
+        ThrowUserError("invalid_product_name",
+                       { product => $product });
     }
 }
 
@@ -109,17 +109,17 @@ if (!tie(%dbmcount, 'AnyDBM_File', "data/duplicates/dupes$today",
     if ($!{ENOENT}) {
         if (!tie(%dbmcount, 'AnyDBM_File', "data/duplicates/dupes$yesterday",
                  O_RDONLY, 0644)) {
-            $vars->{'today'} = $today;
+            my $vars = { today => $today };
             if ($!{ENOENT}) {
-                ThrowUserError("no_dupe_stats");
+                ThrowUserError("no_dupe_stats", $vars);
             } else {
                 $vars->{'error_msg'} = $!;
-                ThrowUserError("no_dupe_stats_error_yesterday");
+                ThrowUserError("no_dupe_stats_error_yesterday", $vars);
             }
         }
     } else {
-        $vars->{'error_msg'} = $!;
-        ThrowUserError("no_dupe_stats_error_today");
+        ThrowUserError("no_dupe_stats_error_today",
+                       { error_msg => $! });
     }
 }
 
@@ -146,10 +146,11 @@ if (!tie(%before, 'AnyDBM_File', "data/duplicates/dupes$whenever",
          O_RDONLY, 0644)) {
     # Ignore file not found errors
     if (!$!{ENOENT}) {
-        $vars->{'error_msg'} = $!;
-        $vars->{'changedsince'} = $changedsince;
-        $vars->{'whenever'} = $whenever;    
-        ThrowUserError("no_dupe_stats_error_whenever");
+        ThrowUserError("no_dupe_stats_error_whenever",
+                       { error_msg => $!,
+                         changedsince => $changedsince,
+                         whenever => $whenever,
+                       });
     }
 } else {
     # Calculate the deltas

globals.pl

@@ -127,7 +127,6 @@ sub AppendComment {
        # zero or more digits, OR we have a period followed by one or more digits
        if ($work_time !~ /^-?(?:\d+(?:\.\d*)?|\.\d+)$/) { 
           ThrowUserError("need_numeric_value");
-          return;
        }
     } else { $work_time = 0 };
 
@@ -863,8 +862,8 @@ sub DBNameToIdAndCheck {
         return $result;
     }
 
-    $::vars->{'name'} = $name;
-    ThrowUserError("invalid_username");
+    ThrowUserError("invalid_username",
+                   { name => $name });
 }
 
 

post_bug.cgi

@@ -78,8 +78,8 @@ ValidateComment($comment);
 my $product = $::FORM{'product'};
 my $product_id = get_product_id($product);
 if (!$product_id) {
-    $vars->{'product'} = $product;
-    ThrowUserError("invalid_product_name");
+    ThrowUserError("invalid_product_name",
+                   { product => $product });
 }
 
 # Set cookies
@@ -230,8 +230,8 @@ if ($::FORM{'keywords'} && UserInGroup("editbugs")) {
         }
         my $i = GetKeywordIdFromName($keyword);
         if (!$i) {
-            $vars->{'keyword'} = $keyword;
-            ThrowUserError("unknown_keyword");
+            ThrowUserError("unknown_keyword",
+                           { keyword => $keyword });
         }
         if (!$keywordseen{$i}) {
             push(@keywordlist, $i);
@@ -301,8 +301,10 @@ if (UserInGroup("editbugs") && defined($::FORM{'dependson'})) {
                 foreach my $i (@isect) {
                     $both = $both . GetBugLink($i, "#" . $i) . " ";
                 }
-                $vars->{'both'} = $both;
-                ThrowUserError("dependency_loop_multi", undef, "abort");
+
+                ThrowUserError("dependency_loop_multi",
+                               { both => $both },
+                               "abort");
             }
         }
         my $tmp = $me;
@@ -337,8 +339,8 @@ if (UserInGroup(Param("timetrackinggroup")) &&
     if ($est_time =~ /^(?:\d+(?:\.\d*)?|\.\d+)$/) {
         $sql .= SqlQuote($est_time) . "," . SqlQuote($est_time);
     } else {
-        $vars->{'field'} = "estimated_time";
-        ThrowUserError("need_positive_number");
+        ThrowUserError("need_positive_number",
+                       { field => 'estimated_time' });
     }
 } else {
     $sql .= "0, 0";

process_bug.cgi

@@ -223,7 +223,12 @@ if ((($::FORM{'id'} && $::FORM{'product'} ne $::oldproduct)
         $vars->{'oldvalue'} = $::oldproduct;
         $vars->{'newvalue'} = $::FORM{'product'};
         $vars->{'field'} = 'product';
-        ThrowUserError("illegal_change", undef, "abort");            
+        ThrowUserError("illegal_change",
+                       { oldvalue => $::oldproduct,
+                         newvalue => $::FORM{'product'},
+                         field => 'product',
+                       },
+                       "abort");
     }
  
     CheckFormField(\%::FORM, 'product', \@::legal_product);
@@ -689,7 +694,7 @@ if (Param("usebugaliases") && defined($::FORM{'alias'})) {
 
             # Make sure the alias is unique.
             my $escaped_alias = SqlQuote($alias);
-            $vars->{'alias'} = $alias;
+            my $vars = { alias => $alias };
             
             SendSQL("SELECT bug_id FROM bugs WHERE alias = $escaped_alias " . 
                     "AND bug_id != $idlist[0]");
@@ -697,17 +702,17 @@ if (Param("usebugaliases") && defined($::FORM{'alias'})) {
             
             if ($id) {
                 $vars->{'bug_link'} = GetBugLink($id, "Bug $id");
-                ThrowUserError("alias_in_use");
+                ThrowUserError("alias_in_use", $vars);
             }
 
             # Make sure the alias isn't just a number.
             if ($alias =~ /^\d+$/) {
-                ThrowUserError("alias_is_numeric");
+                ThrowUserError("alias_is_numeric", $vars);
             }
 
             # Make sure the alias has no commas or spaces.
             if ($alias =~ /[, ]/) {
-                ThrowUserError("alias_has_comma_or_space");
+                ThrowUserError("alias_has_comma_or_space", $vars);
             }
         }
         
@@ -750,8 +755,8 @@ if (UserInGroup(Param('timetrackinggroup'))) {
                     DoComma();
                     $::query .= "$field = " . SqlQuote($er_time);
                 } else {
-                    $vars->{'field'} = $field;
-                    ThrowUserError("need_positive_number");
+                    ThrowUserError("need_positive_number",
+                                   field => $field);
                 }
             }
         }
@@ -946,8 +951,8 @@ SWITCH: for ($::FORM{'knob'}) {
         SendSQL("SELECT bug_id FROM bugs where bug_id = " .  SqlQuote($checkid));
         $checkid = FetchOneColumn();
         if (!$checkid) {
-            $vars->{'bug_id'} = $checkid;
-            ThrowUserError("invalid_bug_id");
+            ThrowUserError("invalid_bug_id",
+                           { bug_id => $checkid });
         }
         $::FORM{'comment'} .= "\n\n*** This bug has been marked as a duplicate of $num ***";
         $duplicate = $num;
@@ -975,8 +980,8 @@ if ($::FORM{'keywords'}) {
         }
         my $i = GetKeywordIdFromName($keyword);
         if (!$i) {
-            $vars->{keyword} = $keyword;
-            ThrowUserError("unknown_keyword");
+            ThrowUserError("unknown_keyword",
+                           { keyword => $keyword });
         }
         if (!$keywordseen{$i}) {
             push(@keywordlist, $i);
@@ -1098,6 +1103,7 @@ foreach my $id (@idlist) {
         if (exists $::FORM{$col}) {
             if (!CheckCanChangeField($col, $id, $oldvalues[$i], $::FORM{$col})) {
                 # More fun hacking... don't display component_id
+                my $vars;
                 if ($col eq 'component_id') {
                     $vars->{'oldvalue'} = get_component_name($oldhash{'component_id'});
                     $vars->{'newvalue'} = $::FORM{'component'};
@@ -1108,23 +1114,23 @@ foreach my $id (@idlist) {
                     $vars->{'newvalue'} = $::FORM{$col};
                     $vars->{'field'} = $col;
                 }
-                ThrowUserError("illegal_change", undef, "abort");            
+                ThrowUserError("illegal_change", $vars, "abort");
             }
         }
         $i++;
     }
     $oldhash{'product'} = get_product_name($oldhash{'product_id'});
     if (!CanEditProductId($oldhash{'product_id'})) {
-        $vars->{'product'} = $oldhash{'product'};
-        ThrowUserError("product_edit_denied");
+        ThrowUserError("product_edit_denied",
+                      { product => $oldhash{'product'} });
     }
 
     if (defined $::FORM{'product'} 
         && $::FORM{'product'} ne $::FORM{'dontchange'} 
         && $::FORM{'product'} ne $oldhash{'product'}
         && !CanEnterProduct($::FORM{'product'})) {
-        $vars->{'product'} = $::FORM{'product'};
-        ThrowUserError("entry_access_denied");
+        ThrowUserError("entry_access_denied",
+                       { product => $::FORM{'product'} });
     }
     if ($requiremilestone) {
         my $value = $::FORM{'target_milestone'};
@@ -1135,8 +1141,9 @@ foreach my $id (@idlist) {
                 SqlQuote($oldhash{'product'}));
         if ($value eq FetchOneColumn()) {
             SendSQL("UNLOCK TABLES");
-            $vars->{'bug_id'} = $id;
-            ThrowUserError("milestone_required", undef, "abort");
+            ThrowUserError("milestone_required",
+                           { bug_id => $id },
+                           "abort");
         }
     }   
     if (defined $::FORM{'delta_ts'} && $::FORM{'delta_ts'} ne $delta_ts) {
@@ -1214,8 +1221,9 @@ foreach my $id (@idlist) {
                        $both = $both . GetBugLink($i, "#" . $i) . " ";
                     }
 
-                    $vars->{'both'} = $both;
-                    ThrowUserError("dependency_loop_multi", undef, "abort");
+                    ThrowUserError("dependency_loop_multi",
+                                   { both => $both },
+                                   "abort");
                 }
             }
             my $tmp = $me;

template/en/default/global/user-error.html.tmpl

@@ -34,14 +34,7 @@
 [% DEFAULT title = "Error" %]
 
 [% error_message = BLOCK %]
-  [% IF    error == "aaa_example_error_tag" %]
-    [% title = "Example Error" %]    
-    This is an example error. The title is set above. This text is the body
-    of the error. It can contain arbitrary <b>HTML</b>, and also references
-    to any [% parameters %] which you may have set before calling
-    ThrowUserError.
-
-  [% ELSIF error == "account_creation_disabled" %]
+  [% IF    error == "account_creation_disabled" %]
     [% title = "Account Creation Disabled" %]
     Account self-creation has been disabled or restricted.
     <hr>
@@ -207,7 +200,7 @@
   [% ELSIF error == "flag_type_sortkey_invalid" %]
     [% title = "Flag Type Sort Key Invalid" %]
     The sort key must be an integer between 0 and 32767 inclusive.
-    It cannot be <em>[% variables.sortkey %]</em>.
+    It cannot be <em>[% sortkey FILTER html %]</em>.
   
   [% ELSIF error == "illegal_at_least_x_votes" %]
     [% title = "Your Query Makes No Sense" %]
@@ -218,6 +211,10 @@
     [% title = "Unauthorised Action" %]
     You are not authorised to edit attachment [% attach_id %].
 
+  [% ELSIF error == "illegal_attachment_edit_bug" %]
+    [% title = "Unauthorised Action" %]
+    You are not authorised to edit attachments on bug [% bug_id %].
+         
   [% ELSIF error == "illegal_attachment_is_patch" %]
     [% title = "Your Query Makes No Sense" %]
     The only legal values for the <em>Attachment is patch</em> field are
@@ -507,18 +504,6 @@
     [% title = "Access Denied" %]
     You do not have the permissions necessary to view reports for this product.
 
-  [% ELSIF error == "requestee_too_short" %]
-    [% title = "Requestee Name Too Short" %]
-    One or two characters match too many users, so please enter at least 
-    three characters of the name/email address of the user you want to set 
-    the flag.
-  
-  [% ELSIF error == "requestee_too_many_matches" %]
-    [% title = "Requestee String Matched Too Many Times" %]
-    The string <em>[% requestee FILTER html %]</em> matched more than 
-    100 users.  Enter more of the name to bring the number of matches 
-    down to a reasonable amount.
-  
   [% ELSIF error == "require_component" %]
     [% title = "Component Needed" %]
     You must choose a component to file this bug in. If necessary, 
@@ -596,11 +581,10 @@
     The file you are trying to attach is empty!
     
   [% ELSE %]
-    [%# Cope with legacy calling convention, where "error" was the string
-      # to print. 
-      #%]
-        
-    [% error %]
+    [% title = "Error string not found" %]
+    The user error string <code>[% error FILTER html %]</code> was not found.
+    Please send email to [% Param("maintainer") %] describing the steps taken
+    to obtain this message.
   [% END %]
 [% END %]
 

token.cgi

@@ -231,9 +231,9 @@ sub changeEmail {
     # The new email address should be available as this was 
     # confirmed initially so cancel token if it is not still available
     if (! ValidateNewUser($new_email,$old_email)) {
-        $vars->{'email'} = $new_email;
+        $vars->{'email'} = $new_email; # Needed for Token::Cancel's mail
         Token::Cancel($::token,"account_exists");
-        ThrowUserError("account_exists");
+        ThrowUserError("account_exists", { email => $new_email } );
     } 
 
     # Update the user's login name in the profiles table and delete the token

userprefs.cgi

@@ -385,7 +385,8 @@ SWITCH: for ($current_tab_name) {
         DoPermissions();
         last SWITCH;
     };
-    ThrowUserError("current_tab_name");
+    ThrowUserError("unknown_tab",
+                   { current_tab_name => $current_tab_name });
 }
 
 # Generate and return the UI (HTML page) from the appropriate template.