Bug 163114 - Templatise all calls to DisplayError. Patch D (the last one). Patch by gerv; r=burnus.

Bugzilla/Search.pm

@@ -746,12 +746,7 @@ sub init {
                 # chart -1 is generated by other code above, not from the user-
                 # submitted form, so we'll blindly accept any values in chart -1
                 if ((!$chartfields{$f}) && ($chart != -1)) {
-                    my $errstr = "Can't use $f as a field name.  " .
-                        "If you think you're getting this in error, please copy the " .
-                        "entire URL out of the address bar at the top of your browser " .
-                        "window and email it to <109679\@bugzilla.org>";
-                    die "Internal error: $errstr" if $chart < 0;
-                    return &::DisplayError($errstr);
+                    ThrowCodeError("invalid_field_name", {field => $f});
                 }
 
                 # This is either from the internal chart (in which case we

CGI.pl

@@ -801,18 +801,6 @@ sub PutFooter {
 # ThrowUserError("some_tag", { bug_id => $bug_id, size => 127 });
 ###############################################################################
 
-# DisplayError is deprecated. Use ThrowCodeError, ThrowUserError or 
-# ThrowTemplateError instead.
-sub DisplayError {
-  ($vars->{'error'}, $vars->{'title'}) = (@_);
-
-  print "Content-type: text/html\n\n" if !$vars->{'header_done'};
-  $template->process("global/user-error.html.tmpl", $vars)
-    || ThrowTemplateError($template->error());   
-
-  return 1;
-}
-
 # For "this shouldn't happen"-type places in the code.
 # The contents of $extra_vars get printed out in the template - useful for
 # debugging info.

describecomponents.cgi

@@ -57,9 +57,7 @@ if (!defined $::FORM{'product'}) {
 
     my $prodsize = scalar(keys %products);
     if ($prodsize == 0) {
-        DisplayError("Either no products have been defined ".
-                     "or you have not been given access to any.\n");
-        exit;
+        ThrowUserError("no_products");
     }
     elsif ($prodsize > 1) {
         $::vars->{'proddesc'} = \%products;
@@ -93,8 +91,7 @@ if (!$product_id) {
 if (Param("usebuggroups") && GroupExists($product)) {
     confirm_login() unless $::userid;
     UserInGroup($product)
-      || DisplayError("You are not authorized to access that product.")
-        && exit;
+      || ThrowUserError("product_access_denied");
 }
 
 ######################################################################

globals.pl

@@ -680,24 +680,18 @@ sub CanSeeBug {
 
 sub ValidatePassword {
     # Determines whether or not a password is valid (i.e. meets Bugzilla's
-    # requirements for length and content).  If the password is valid, the
-    # function returns boolean false.  Otherwise it returns an error message
-    # (synonymous with boolean true) that can be displayed to the user.
-    
+    # requirements for length and content).    
     # If a second password is passed in, this function also verifies that
     # the two passwords match.
-
     my ($password, $matchpassword) = @_;
     
-    if ( length($password) < 3 ) {
-        return "The password is less than three characters long.  It must be at least three characters.";
-    } elsif ( length($password) > 16 ) {
-        return "The password is more than 16 characters long.  It must be no more than 16 characters.";
-    } elsif ( $matchpassword && $password ne $matchpassword ) { 
-        return "The two passwords do not match.";
+    if (length($password) < 3) {
+        ThrowUserError("password_too_short");
+    } elsif (length($password) > 16) {
+        ThrowUserError("password_too_long");
+    } elsif ($matchpassword && $password ne $matchpassword) { 
+        ThrowUserError("passwords_dont_match");
     }
-
-    return 0;
 }
 
 

reports.cgi

@@ -105,23 +105,21 @@ if (! defined $FORM{'product'}) {
     # Valid values are those products for which the user has permissions which appear
     # in the "product" drop-down menu on the report generation form.
     grep($_ eq $FORM{'product'}, @myproducts)
-      || DisplayError("You entered an invalid product name.") && exit;
+      || ThrowUserError("invalid_product_name", {product => $FORM{'product'}});
 
     # If usebuggroups is on, we don't want people to be able to view
     # reports for products they don't have permissions for...
     Param("usebuggroups") 
       && GroupExists($FORM{'product'}) 
       && !UserInGroup($FORM{'product'})
-      && DisplayError("You do not have the permissions necessary to view reports for this product.")
-      && exit;
+      && ThrowUserError("report_access_denied");
           
     # For security and correctness, validate the value of the "output" form variable.
     # Valid values are the keys from the %reports hash defined above which appear in
     # the "output" drop-down menu on the report generation form.
     $FORM{'output'} ||= "most_doomed"; # a reasonable default
     grep($_ eq $FORM{'output'}, keys %reports)
-      || DisplayError("You entered an invalid output type.") 
-      && exit;
+      || ThrowCodeError("invalid_output_type", {type => $FORM{'output'}});
 
     # We've checked that the product exists, and that the user can see it
     # This means that is OK to detaint

sanitycheck.cgi

@@ -64,9 +64,7 @@ confirm_login();
 # prevents users with a legitimate interest in Bugzilla integrity
 # from accessing the script).
 UserInGroup("editbugs")
-  || DisplayError("You are not authorized to access this script,
-                   which is reserved for users with the ability to edit bugs.")
-  && exit;
+  || ThrowUserError("sanity_check_access_denied");
 
 print "Content-type: text/html\n";
 print "\n";

showdependencygraph.cgi

@@ -70,8 +70,7 @@ sub AddLink {
 $::FORM{'rankdir'} = "LR" if !defined $::FORM{'rankdir'};
 
 if (!defined($::FORM{'id'}) && !defined($::FORM{'doall'})) {
-    DisplayError("No bug numbers given.");
-    exit;
+    ThrowCodeError("missing_bug_id");
 }    
 
 my $filename = "data/webdot/$$.dot";

sidebar.cgi

@@ -68,8 +68,7 @@ if ($useragent =~ m:Mozilla/([1-9][0-9]*):i && $1 >= 5 && $useragent !~ m/compat
     $template->process("sidebar.xul.tmpl", $vars)
       || ThrowTemplateError($template->error());
 } else {
-    DisplayError("sidebar.cgi currently only supports Mozilla based web browsers");
-    exit;
+    ThrowUserError("sidebar_supports_mozilla_only");
 }
 
 

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

@@ -125,6 +125,16 @@
     The target type was neither <em>bug</em> nor <em>attachment</em>
     but rather <em>[% variables.target_type FILTER html %]</em>.
   
+  [% ELSIF error == "invalid_field_name" %]
+    Can't use [% field FILTER html %] as a field name.
+    
+  [% ELSIF error == "invalid_output_type" %]
+    [% title = "Invalid Output Type" %]
+    Invalid output type [% type FILTER html %].
+    
+  [% ELSIF error == "missing_bug_id" %]
+    No bug ID was given.
+    
   [% ELSIF error == "no_y_axis_defined" %]
     No Y axis was defined when creating report. The X axis is optional,
     but the Y axis is compulsory.
@@ -138,6 +148,9 @@
   [% ELSIF error == "template_error" %]
     [% template_error_msg %]
 
+  [% ELSIF error == "unable_to_retrieve_password" %]
+     I was unable to retrieve your old password from the database.
+     
   [% ELSIF error == "undefined_field" %]
     [% field FILTER html %] was not defined; [% Param('browserbugmessage') %]
 

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

@@ -143,6 +143,10 @@
     really make sense to mark a bug as a duplicate of itself, 
     does it?  
     
+  [% ELSIF error == "email_change_in_progress" %]
+    [% title = "Email Change Already In Progress" %]
+     Email change already in progress; please check your email. 
+        
   [% ELSIF error == "email_confirmation_failed" %]
     [% title = "Email Address Email Address Confirmation Failed" %]
     Email address confirmation failed.
@@ -336,6 +340,10 @@
     [% title = "Quip Required" %]
     Please enter a quip in the text field.
     
+  [% ELSIF error == "new_password_missing" %]
+    [% title = "New Password Missing" %]
+    You must enter a new password.
+    
   [% ELSIF error == "no_bugs_chosen" %]
     [% title = "No Bugs Chosen" %]
     You apparently didn't choose any bugs to modify.
@@ -392,12 +400,38 @@
      Either no products have been defined to enter bugs against or you have not
      been given access to any.
      
+  [% ELSIF error == "old_password_incorrect" %]
+    [% title = "Incorrect Old Password" %]
+     You did not enter your old password correctly.
+     
+  [% ELSIF error == "old_password_required" %]
+    [% title = "Old Password Required" %]
+    You must enter your old password to change email address.
+         
+  [% ELSIF error == "passwords_dont_match" %]
+    [% title = "Passwords Don't Match" %]
+     The two passwords you entered did not match.
+     
+  [% ELSIF error == "password_too_long" %]
+    [% title = "Password Too Long" %]
+    The password is more than 16 characters long. It must be no more than 
+    16 characters.
+    
+  [% ELSIF error == "password_too_short" %]
+    [% title = "Password Too Short" %]
+    The password is less than three characters long. It must be at least 
+    three characters.
+      
   [% ELSIF error == "patch_too_large" %]
     [% title = "File Too Large" %]
     The file you are trying to attach is [% filesize %] kilobytes (KB) in size.
     Patches cannot be more than [% Param('maxpatchsize') %] KB in size.
     Try breaking your patch into several pieces.  
 
+  [% ELSIF error == "product_access_denied" %]
+    [% title = "Access Denied" %]
+     You do not have the permissions necessary to access that product.
+     
   [% ELSIF error == "query_name_missing" %]
     [% title = "No Query Name Specified" %]
     You must enter a name for your query.
@@ -408,6 +442,10 @@
     intentionally cleared out the "Reassign bug to" 
     field, [% Param("browserbugmessage") %]
 
+  [% ELSIF error == "report_access_denied" %]
+    [% 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 
@@ -433,6 +471,26 @@
      [% title = "Summary Needed" %]
      You must enter a summary for this bug.
 
+  [% ELSIF error == "sanity_check_access_denied" %]
+    [% title = "Access Denied" %]
+     You do not have the permissions necessary to run a sanity check.
+     
+  [% ELSIF error == "sidebar_supports_mozilla_only" %]
+     Sorry - sidebar.cgi currently only supports Mozilla based web browsers.
+     <a href="http://www.mozilla.org">Upgrade today</a>. :-)
+
+  [% ELSIF error == "too_many_votes_for_bug" %]
+     [% title = "Illegal Vote" %]
+     You may only use at most [% max %] votes for a single bug in the
+     <tt>[% prod FILTER html %]</tt> product, but you are trying to use 
+     [% votes %].
+     
+  [% ELSIF error == "too_many_votes_for_product" %]
+     [% title = "Illegal Vote" %]
+     You may only use at most [% max %] votes for bugs in the
+     <tt>[% prod FILTER html %]</tt> product, but you are trying to use 
+     [% votes %].
+     
   [% ELSIF error == "token_inexistent" %]
      [% title = "Token Does Not Exist" %]
      The token you submitted does not exist, has expired, or has
@@ -447,6 +505,10 @@
     [% title = "Unknown Tab" %]
     <code>[% current_tab_name FILTER html %]</code> is not a legal tab name.    
     
+  [% ELSIF error == "votes_must_be_nonnegative" %]
+    [% title = "Votes Must Be Non-negative" %]
+    Only use non-negative numbers for your bug votes.
+        
   [% ELSIF error == "wrong_token_for_cancelling_email_change" %]
     [% title = "Wrong Token" %]
     That token cannot be used to cancel an email address change.

token.cgi

@@ -113,11 +113,7 @@ if ( $::action eq 'chgpw' ) {
       && defined $::FORM{'matchpassword'}
       || ThrowUserError("require_new_password");
 
-     my $passworderror = ValidatePassword($::FORM{'password'}, $::FORM{'matchpassword'});
-     if ( $passworderror ) {
-         DisplayError($passworderror);
-         exit;
-     }
+    ValidatePassword($::FORM{'password'}, $::FORM{'matchpassword'});
 }
 
 ################################################################################

userprefs.cgi

@@ -92,29 +92,19 @@ sub SaveAccount {
         my $old = SqlQuote($::FORM{'Bugzilla_password'});
         SendSQL("SELECT cryptpassword FROM profiles WHERE userid = $userid");
         my $oldcryptedpwd = FetchOneColumn();
-        if (!$oldcryptedpwd) {
-            DisplayError("I was unable to retrieve your old password from the database.");
-            exit;
-        }
+        $oldcryptedpwd || ThrowCodeError("unable_to_retrieve_password");
+
         if (crypt($::FORM{'Bugzilla_password'}, $oldcryptedpwd) ne 
                   $oldcryptedpwd) 
         {
-            DisplayError("You did not enter your old password correctly.");
-            exit;
+            ThrowUserError("old_password_incorrect");
         }
 
         if ($pwd1 ne "" || $pwd2 ne "")
         {
-            if ($pwd1 ne $pwd2) {
-                DisplayError("The two passwords you entered did not match.");
-                exit;
-            }
-            if ($::FORM{'new_password1'} eq '') {
-                DisplayError("You must enter a new password.");
-                exit;
-            }
-            my $passworderror = ValidatePassword($pwd1);
-            (DisplayError($passworderror) && exit) if $passworderror;
+            ($pwd1 eq $pwd2) || ThrowUserError("passwords_dont_match");
+            $::FORM{'new_password1'} || ThrowUserError("new_password_missing");
+            ValidatePassword($pwd1);
         
             my $cryptedpassword = SqlQuote(Crypt($pwd1));
             SendSQL("UPDATE profiles 
@@ -130,27 +120,20 @@ sub SaveAccount {
         my $new_login_name = trim($::FORM{'new_login_name'});
 
         if($old_login_name ne $new_login_name) {
-            if( $::FORM{'Bugzilla_password'} eq "") {
-                DisplayError("You must enter your old password to 
-                                             change email address.");
-                exit;
-            }
+            $::FORM{'Bugzilla_password'} 
+              || ThrowCodeError("old_password_required");
 
             use Token;
             # Block multiple email changes for the same user.
             if (Token::HasEmailChangeToken($userid)) {
-                DisplayError("Email change already in progress; 
-                                          please check your email.");
-                exit;
+                ThrowUserError("email_change_in_progress");
             }
 
             # Before changing an email address, confirm one does not exist.
             CheckEmailSyntax($new_login_name);
             trick_taint($new_login_name);
-            if (!ValidateNewUser($new_login_name)) {
-                DisplayError("Account $new_login_name already exists");
-                exit;
-            }
+            ValidateNewUser($new_login_name)
+              || ThrowUserError("account_exists", {email => $new_login_name});
 
             Token::IssueEmailChangeToken($userid,$old_login_name,
                                                  $new_login_name);
@@ -325,7 +308,7 @@ sub SaveFooter {
                         "AND name = " . SqlQuote($name));
             }
         } else {
-            DisplayError("Hmm, the $name query seems to have gone away.");
+            ThrowUserError("missing_query", {queryname => $name});
         }
     }
     SendSQL("UPDATE profiles SET mybugslink = " . 

votes.cgi

@@ -79,7 +79,7 @@ elsif ($action eq "vote") {
     show_user();
 }
 else {
-    DisplayError("Unknown action: " . html_quote($action));
+    ThrowCodeError("unknown_action", {action => $action});
 }
 
 exit;
@@ -87,8 +87,8 @@ exit;
 # Display the names of all the people voting for this one bug.
 sub show_bug {
     my $bug_id = $::FORM{'bug_id'} 
-                  || DisplayError("Please give a bug ID to show the votes for.")
-                  && exit;
+      || ThrowCodeError("missing_bug_id");
+      
     my $total = 0;
     my @users;
     
@@ -126,10 +126,7 @@ sub show_user {
     # After DBNameToIdAndCheck is templatised and prints a Content-Type, 
     # the above should revert to a call to that function, and this 
     # special error handling should go away.
-    if (!$who) {
-        DisplayError(html_quote($name) . " is not a valid username.\n");
-        exit;
-    }
+    $who || ThrowUserError("invalid_username", {name => $name});
     
     my $canedit = 1 if ($name eq $::COOKIE{'Bugzilla_login'});
     
@@ -255,8 +252,7 @@ sub record_votes {
     foreach my $id (@buglist) {
       ValidateBugID($id);
       detaint_natural($::FORM{$id})
-        || DisplayError("Only use non-negative numbers for your bug votes.")
-        && exit;
+        || ThrowUserError("votes_must_be_nonnegative");
     }
 
     ############################################################################
@@ -283,28 +279,20 @@ sub record_votes {
             $prodcount{$prod} += $::FORM{$id};
             
             # Make sure we haven't broken the votes-per-bug limit
-            if ($::FORM{$id} > $max) {                
-                $prod = html_quote($prod);
-                my $votes = html_quote($::FORM{$id});
-                
-                DisplayError("You may only use at most $max votes for a single
-                              bug in the <tt>$prod</tt> product, but you are
-                              trying to use $votes.", "Illegal vote");
-                exit();
-            } 
+            ($::FORM{$id} <= $max)               
+              || ThrowUserError("too_many_votes_for_bug",
+                                {max => $max, 
+                                 product => $prod, 
+                                 votes => $::FORM{$id}});
         }
 
         # Make sure we haven't broken the votes-per-product limit
         foreach my $prod (keys(%prodcount)) {
-            if ($prodcount{$prod} > $::prodmaxvotes{$prod}) {
-                $prod = html_quote($prod);
-                
-                DisplayError("You may only use at most $::prodmaxvotes{$prod}
-                              votes for bugs in the <tt>$prod</tt> product, 
-                              but you are trying to use $prodcount{$prod}.",
-                              "Illegal vote");
-                exit();
-            }
+            ($prodcount{$prod} <= $::prodmaxvotes{$prod})
+              || ThrowUserError("too_many_votes_for_product",
+                                {max => $::prodmaxvotes{$prod}, 
+                                 product => $prod, 
+                                 votes => $prodcount{$prod}});
         }
     }