Bug 301463: Move CheckFormField and CheckFormFieldDefined into Field.pm - Patch by Frédéric Buclin <LpSolit@gmail.com> r=mkanat a=justdave

4 files changed, 141 insertions, 79 deletions

diff --git a/Bugzilla/Field.pm b/Bugzilla/Field.pm
new file mode 100644
index 000000000..173ff5499
--- /dev/null
+++ b/Bugzilla/Field.pm
@@ -0,0 +1,106 @@
+# -*- 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.
+#
+# Contributor(s): Dan Mosedale <dmose@mozilla.org>
+#                 Frédéric Buclin <LpSolit@gmail.com>
+
+package Bugzilla::Field;
+
+use strict;
+
+use base qw(Exporter);
+@Bugzilla::Field::EXPORT = qw(check_form_field check_form_field_defined);
+
+use Bugzilla;
+use Bugzilla::Util;
+use Bugzilla::Error;
+
+
+sub check_form_field ($$;$) {
+    my ($cgi, $fieldname, $legalsRef) = @_;
+    my $dbh = Bugzilla->dbh;
+
+    if (!defined $cgi->param($fieldname)
+        || trim($cgi->param($fieldname)) eq ""
+        || (defined($legalsRef)
+            && lsearch($legalsRef, $cgi->param($fieldname)) < 0))
+    {
+        trick_taint($fieldname);
+        my ($result) = $dbh->selectrow_array("SELECT description FROM fielddefs
+                                              WHERE name = ?", undef, $fieldname);
+        
+        my $field = $result || $fieldname;
+        ThrowCodeError("illegal_field", { field => $field });
+    }
+}
+
+sub check_form_field_defined ($$) {
+    my ($cgi, $fieldname) = @_;
+
+    if (!defined $cgi->param($fieldname)) {
+        ThrowCodeError("undefined_field", { field => $fieldname });
+    }
+}
+
+=head1 NAME
+
+Bugzilla::Field - Useful routines for fields manipulation
+
+
+=head1 SYNOPSIS
+
+  use Bugzilla::Field;
+
+  # Validation Routines
+  check_form_field($cgi, $fieldname, \@legal_values);
+  check_form_field_defined($cgi, $fieldname);
+
+
+=head1 DESCRIPTION
+
+This package provides functions for dealing with CGI form fields.
+
+=head1 FUNCTIONS
+
+This package provides several types of routines:
+
+=head2 Validation
+
+=over
+
+=item C<check_form_field($cgi, $fieldname, \@legal_values)>
+
+Description: Makes sure the field $fieldname is defined and its value
+             is non empty. If @legal_values is defined, this routine
+             also checks whether its value is one of the legal values
+             associated with this field. If the test fails, an error
+             is thrown.
+
+Params:      $cgi          - a CGI object
+             $fieldname    - the field name to check
+             @legal_values - (optional) ref to a list of legal values
+
+Returns:     nothing
+
+=item C<check_form_field_defined($cgi, $fieldname)>
+
+Description: Makes sure the field $fieldname is defined and its value
+             is non empty. Else an error is thrown.
+
+Params:      $cgi       - a CGI object
+             $fieldname - the field name to check
+
+Returns:     nothing
+
+=back
diff --git a/CGI.pl b/CGI.pl
index 9c65efdf2..4b7269358 100644
--- a/CGI.pl
+++ b/CGI.pl
@@ -52,49 +52,6 @@ require 'globals.pl';
 
 use vars qw($template $vars);
 
-# Implementations of several of the below were blatently stolen from CGI.pm,
-# by Lincoln D. Stein.
-
-# check and see if a given field exists, is non-empty, and is set to a 
-# legal value.  assume a browser bug and abort appropriately if not.
-# if $legalsRef is not passed, just check to make sure the value exists and 
-# is non-NULL
-sub CheckFormField ($$;\@) {
-    my ($cgi,                    # a CGI object
-        $fieldname,              # the fieldname to check
-        $legalsRef               # (optional) ref to a list of legal values 
-       ) = @_;
-
-    if (!defined $cgi->param($fieldname)
-        || trim($cgi->param($fieldname)) eq ""
-        || (defined($legalsRef)
-            && lsearch($legalsRef, $cgi->param($fieldname))<0))
-    {
-        SendSQL("SELECT description FROM fielddefs WHERE name=" . SqlQuote($fieldname));
-        my $result = FetchOneColumn();
-        my $field;
-        if ($result) {
-            $field = $result;
-        }
-        else {
-            $field = $fieldname;
-        }
-        
-        ThrowCodeError("illegal_field", { field => $field });
-    }
-}
-
-# check and see if a given field is defined, and abort if not
-sub CheckFormFieldDefined ($$) {
-    my ($cgi,                    # a CGI object
-        $fieldname,              # the fieldname to check
-       ) = @_;
-
-    if (!defined $cgi->param($fieldname)) {
-        ThrowCodeError("undefined_field", { field => $fieldname });
-    }
-}
-
 sub PutHeader {
     ($vars->{'title'}, $vars->{'h1'}, $vars->{'h2'}) = (@_);
      
diff --git a/post_bug.cgi b/post_bug.cgi
index 576c4c2c3..f6db24943 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -26,13 +26,12 @@
 use strict;
 use lib qw(.);
 
+require "CGI.pl";
 use Bugzilla;
 use Bugzilla::Constants;
-require "CGI.pl";
-
 use Bugzilla::Bug;
-
 use Bugzilla::User;
+use Bugzilla::Field;
 
 # Shut up misguided -w warnings about "used only once". For some reason,
 # "use vars" chokes on me when I try it here.
@@ -196,18 +195,18 @@ if (!Param('letsubmitterchoosepriority')) {
 GetVersionTable();
 
 # Some more sanity checking
-CheckFormField($cgi, 'product',      \@::legal_product);
-CheckFormField($cgi, 'rep_platform', \@::legal_platform);
-CheckFormField($cgi, 'bug_severity', \@::legal_severity);
-CheckFormField($cgi, 'priority',     \@::legal_priority);
-CheckFormField($cgi, 'op_sys',       \@::legal_opsys);
-CheckFormField($cgi, 'bug_status',   ['UNCONFIRMED', 'NEW']);
-CheckFormField($cgi, 'version',          $::versions{$product});
-CheckFormField($cgi, 'component',        $::components{$product});
-CheckFormField($cgi, 'target_milestone', $::target_milestone{$product});
-CheckFormFieldDefined($cgi, 'assigned_to');
-CheckFormFieldDefined($cgi, 'bug_file_loc');
-CheckFormFieldDefined($cgi, 'comment');
+check_form_field($cgi, 'product',      \@::legal_product);
+check_form_field($cgi, 'rep_platform', \@::legal_platform);
+check_form_field($cgi, 'bug_severity', \@::legal_severity);
+check_form_field($cgi, 'priority',     \@::legal_priority);
+check_form_field($cgi, 'op_sys',       \@::legal_opsys);
+check_form_field($cgi, 'bug_status',   ['UNCONFIRMED', 'NEW']);
+check_form_field($cgi, 'version',          $::versions{$product});
+check_form_field($cgi, 'component',        $::components{$product});
+check_form_field($cgi, 'target_milestone', $::target_milestone{$product});
+check_form_field_defined($cgi, 'assigned_to');
+check_form_field_defined($cgi, 'bug_file_loc');
+check_form_field_defined($cgi, 'comment');
 
 my @used_fields;
 foreach my $field (@bug_fields) {
diff --git a/process_bug.cgi b/process_bug.cgi
index 10dc47d62..7b4b299c3 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -46,13 +46,13 @@ my $PrivilegesRequired = 0;
 
 use lib qw(.);
 
+require "CGI.pl";
 use Bugzilla;
 use Bugzilla::Constants;
-require "CGI.pl";
-
 use Bugzilla::Bug;
 use Bugzilla::User;
 use Bugzilla::Util;
+use Bugzilla::Field;
 
 # Use the Flag module to modify flag data if the user set flags.
 use Bugzilla::Flag;
@@ -201,9 +201,9 @@ if ($cgi->cookie("BUGLIST") && defined $cgi->param('id')) {
 
 GetVersionTable();
 
-CheckFormFieldDefined($cgi, 'product');
-CheckFormFieldDefined($cgi, 'version');
-CheckFormFieldDefined($cgi, 'component');
+check_form_field_defined($cgi, 'product');
+check_form_field_defined($cgi, 'version');
+check_form_field_defined($cgi, 'component');
 
 
 # This function checks if there is a comment required for a specific
@@ -292,7 +292,7 @@ if (((defined $cgi->param('id') && $cgi->param('product') ne $oldproduct)
 
     my $mok = 1;   # so it won't affect the 'if' statement if milestones aren't used
     if ( Param("usetargetmilestone") ) {
-       CheckFormFieldDefined($cgi, 'target_milestone');
+       check_form_field_defined($cgi, 'target_milestone');
        $mok = lsearch($::target_milestone{$prod},
                       $cgi->param('target_milestone')) >= 0;
     }
@@ -564,21 +564,21 @@ if (defined $cgi->param('id')) {
     # (XXX those error checks need to happen too, but implementing them 
     # is more work in the current architecture of this script...)
     #
-    CheckFormField($cgi, 'product', \@::legal_product);
-    CheckFormField($cgi, 'component', 
+    check_form_field($cgi, 'product', \@::legal_product);
+    check_form_field($cgi, 'component', 
                    \@{$::components{$cgi->param('product')}});
-    CheckFormField($cgi, 'version', \@{$::versions{$cgi->param('product')}});
+    check_form_field($cgi, 'version', \@{$::versions{$cgi->param('product')}});
     if ( Param("usetargetmilestone") ) {
-        CheckFormField($cgi, 'target_milestone', 
+        check_form_field($cgi, 'target_milestone', 
                        \@{$::target_milestone{$cgi->param('product')}});
     }
-    CheckFormField($cgi, 'rep_platform', \@::legal_platform);
-    CheckFormField($cgi, 'op_sys', \@::legal_opsys);
-    CheckFormField($cgi, 'priority', \@::legal_priority);
-    CheckFormField($cgi, 'bug_severity', \@::legal_severity);
-    CheckFormFieldDefined($cgi, 'bug_file_loc');
-    CheckFormFieldDefined($cgi, 'short_desc');
-    CheckFormFieldDefined($cgi, 'longdesclength');
+    check_form_field($cgi, 'rep_platform', \@::legal_platform);
+    check_form_field($cgi, 'op_sys', \@::legal_opsys);
+    check_form_field($cgi, 'priority', \@::legal_priority);
+    check_form_field($cgi, 'bug_severity', \@::legal_severity);
+    check_form_field_defined($cgi, 'bug_file_loc');
+    check_form_field_defined($cgi, 'short_desc');
+    check_form_field_defined($cgi, 'longdesclength');
 
     if (trim($cgi->param('short_desc')) eq "") {
         ThrowUserError("require_summary");
@@ -906,7 +906,7 @@ if (defined $cgi->param('qa_contact')
     }
 }
 
-CheckFormFieldDefined($cgi, 'knob');
+check_form_field_defined($cgi, 'knob');
 SWITCH: for ($cgi->param('knob')) {
     /^none$/ && do {
         last SWITCH;
@@ -930,7 +930,7 @@ SWITCH: for ($cgi->param('knob')) {
     };
     /^resolve$/ && CheckonComment( "resolve" ) && do {
         # Check here, because its the only place we require the resolution
-        CheckFormField($cgi, 'resolution', \@::settable_resolution);
+        check_form_field($cgi, 'resolution', \@::settable_resolution);
 
         # don't resolve as fixed while still unresolved blocking bugs
         if (Param("noresolveonopenblockers")
@@ -1014,7 +1014,7 @@ SWITCH: for ($cgi->param('knob')) {
     };
     /^duplicate$/ && CheckonComment( "duplicate" ) && do {
         # Make sure we can change the original bug (issue A on bug 96085)
-        CheckFormFieldDefined($cgi, 'dup_id');
+        check_form_field_defined($cgi, 'dup_id');
         $duplicate = $cgi->param('dup_id');
         ValidateBugID($duplicate, 'dup_id');
         $cgi->param('dup_id', $duplicate);
@@ -1769,7 +1769,7 @@ foreach my $id (@idlist) {
                       " has been marked as a duplicate of this bug. ***",
                       0, $timestamp);
 
-        CheckFormFieldDefined($cgi,'comment');
+        check_form_field_defined($cgi,'comment');
         SendSQL("INSERT INTO duplicates VALUES ($duplicate, " .
                 $cgi->param('id') . ")");
     }