Bug 238878: Make hidden-fields template, User Matching and Flags use direct CGI instead of [% form.foo %] - Patch by Teemu Mannermaa <wicked@etlicon.fi> r=LpSolit a=justdave

9 files changed, 93 insertions, 89 deletions

diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index 961957278..0aca49c87 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -228,7 +228,7 @@ sub count {
 
 =over
 
-=item C<validate($data, $bug_id)>
+=item C<validate($cgi, $bug_id)>
 
 Validates fields containing flag modifications.
 
@@ -238,17 +238,17 @@ Validates fields containing flag modifications.
 
 sub validate {
     my $user = Bugzilla->user;
-    my ($data, $bug_id) = @_;
+    my ($cgi, $bug_id) = @_;
   
     # Get a list of flags to validate.  Uses the "map" function
     # to extract flag IDs from form field names by matching fields
     # whose name looks like "flag-nnn", where "nnn" is the ID,
     # and returning just the ID portion of matching field names.
-    my @ids = map(/^flag-(\d+)$/ ? $1 : (), keys %$data);
+    my @ids = map(/^flag-(\d+)$/ ? $1 : (), $cgi->param());
   
     foreach my $id (@ids)
     {
-        my $status = $data->{"flag-$id"};
+        my $status = $cgi->param("flag-$id");
         
         # Make sure the flag exists.
         my $flag = get($id);
@@ -277,9 +277,9 @@ sub validate {
         # feature and the attachment is marked private).
         if ($status eq '?'
             && $flag->{type}->{is_requesteeble}
-            && trim($data->{"requestee-$id"}))
+            && trim($cgi->param("requestee-$id")))
         {
-            my $requestee_email = trim($data->{"requestee-$id"});
+            my $requestee_email = trim($cgi->param("requestee-$id"));
             if ($requestee_email ne $flag->{'requestee'}->{'email'}) {
                 # We know the requestee exists because we ran
                 # Bugzilla::User::match_field before getting here.
@@ -299,7 +299,7 @@ sub validate {
                 # Throw an error if the target is a private attachment and
                 # the requestee isn't in the group of insiders who can see it.
                 if ($flag->{target}->{attachment}->{exists}
-                    && $data->{'isprivate'}
+                    && $cgi->param('isprivate')
                     && Param("insidergroup")
                     && !$requestee->in_group(Param("insidergroup")))
                 {
@@ -353,21 +353,21 @@ sub snapshot {
 
 =over
 
-=item C<process($target, $timestamp, $data)>
+=item C<process($target, $timestamp, $cgi)>
 
 Processes changes to flags.
 
 The target is the bug or attachment this flag is about, the timestamp
 is the date/time the bug was last touched (so that changes to the flag
-can be stamped with the same date/time), the data is the form data
-with flag fields that the user submitted.
+can be stamped with the same date/time), the cgi is the CGI object
+used to obtain the flag fields that the user submitted.
 
 =back
 
 =cut
 
 sub process {
-    my ($target, $timestamp, $data) = @_;
+    my ($target, $timestamp, $cgi) = @_;
 
     my $dbh = Bugzilla->dbh;
     my $bug_id = $target->{'bug'}->{'id'};
@@ -381,14 +381,14 @@ sub process {
     my @old_summaries = snapshot($bug_id, $attach_id);
     
     # Cancel pending requests if we are obsoleting an attachment.
-    if ($attach_id && $data->{'isobsolete'}) {
+    if ($attach_id && $cgi->param('isobsolete')) {
         CancelRequests($bug_id, $attach_id);
     }
 
     # Create new flags and update existing flags.
-    my $new_flags = FormToNewFlags($target, $data);
+    my $new_flags = FormToNewFlags($target, $cgi);
     foreach my $flag (@$new_flags) { create($flag, $timestamp) }
-    modify($data, $timestamp);
+    modify($cgi, $timestamp);
     
     # In case the bug's product/component has changed, clear flags that are
     # no longer valid.
@@ -521,7 +521,7 @@ sub migrate {
 
 =over
 
-=item C<modify($data, $timestamp)>
+=item C<modify($cgi, $timestamp)>
 
 Modifies flags in the database when a user changes them.
 Note that modified flags are always set active (is_active = 1) -
@@ -533,14 +533,14 @@ attachment.cgi midairs. See bug 223878 for details.
 =cut
 
 sub modify {
-    my ($data, $timestamp) = @_;
+    my ($cgi, $timestamp) = @_;
 
     # Use the date/time we were given if possible (allowing calling code
     # to synchronize the comment's timestamp with those of other records).
     $timestamp = ($timestamp ? &::SqlQuote($timestamp) : "NOW()");
     
     # Extract a list of flags from the form data.
-    my @ids = map(/^flag-(\d+)$/ ? $1 : (), keys %$data);
+    my @ids = map(/^flag-(\d+)$/ ? $1 : (), $cgi->param());
     
     # Loop over flags and update their record in the database if necessary.
     # Two kinds of changes can happen to a flag: it can be set to a different
@@ -550,8 +550,8 @@ sub modify {
     foreach my $id (@ids) {
         my $flag = get($id);
 
-        my $status = $data->{"flag-$id"};
-        my $requestee_email = trim($data->{"requestee-$id"});
+        my $status = $cgi->param("flag-$id");
+        my $requestee_email = trim($cgi->param("requestee-$id"));
 
         
         # Ignore flags the user didn't change. There are two components here:
@@ -672,7 +672,7 @@ sub clear {
 
 =over
 
-=item C<FormToNewFlags($target, $data)
+=item C<FormToNewFlags($target, $cgi)
 
 Someone pleasedocument this function.
 
@@ -681,20 +681,20 @@ Someone pleasedocument this function.
 =cut
 
 sub FormToNewFlags {
-    my ($target, $data) = @_;
+    my ($target, $cgi) = @_;
     
     # Get information about the setter to add to each flag.
     # Uses a conditional to suppress Perl's "used only once" warnings.
     my $setter = new Bugzilla::User($::userid);
 
     # Extract a list of flag type IDs from field names.
-    my @type_ids = map(/^flag_type-(\d+)$/ ? $1 : (), keys %$data);
-    @type_ids = grep($data->{"flag_type-$_"} ne 'X', @type_ids);
+    my @type_ids = map(/^flag_type-(\d+)$/ ? $1 : (), $cgi->param());
+    @type_ids = grep($cgi->param("flag_type-$_") ne 'X', @type_ids);
     
     # Process the form data and create an array of flag objects.
     my @flags;
     foreach my $type_id (@type_ids) {
-        my $status = $data->{"flag_type-$type_id"};
+        my $status = $cgi->param("flag_type-$type_id");
         &::trick_taint($status);
     
         # Create the flag record and populate it with data from the form.
@@ -706,7 +706,7 @@ sub FormToNewFlags {
         };
 
         if ($status eq "?") {
-            my $requestee = $data->{"requestee_type-$type_id"};
+            my $requestee = $cgi->param("requestee_type-$type_id");
             if ($requestee) {
                 my $requestee_id = login_to_id($requestee);
                 $flag->{'requestee'} = new Bugzilla::User($requestee_id);
diff --git a/Bugzilla/FlagType.pm b/Bugzilla/FlagType.pm
index 084777b29..da0c43281 100644
--- a/Bugzilla/FlagType.pm
+++ b/Bugzilla/FlagType.pm
@@ -303,7 +303,7 @@ sub count {
 
 =over
 
-=item C<validate($data, $bug_id, $attach_id)>
+=item C<validate($cgi, $bug_id, $attach_id)>
 
 Get a list of flag types to validate.  Uses the "map" function
 to extract flag type IDs from form field names by matching columns
@@ -316,13 +316,13 @@ and returning just the ID portion of matching field names.
 
 sub validate {
     my $user = Bugzilla->user;
-    my ($data, $bug_id, $attach_id) = @_;
+    my ($cgi, $bug_id, $attach_id) = @_;
   
-    my @ids = map(/^flag_type-(\d+)$/ ? $1 : (), keys %$data);
+    my @ids = map(/^flag_type-(\d+)$/ ? $1 : (), $cgi->param());
   
     foreach my $id (@ids)
     {
-        my $status = $data->{"flag_type-$id"};
+        my $status = $cgi->param("flag_type-$id");
         
         # Don't bother validating types the user didn't touch.
         next if $status eq "X";
@@ -348,9 +348,9 @@ sub validate {
         # feature and the attachment is marked private).
         if ($status eq '?'
             && $flag_type->{is_requesteeble}
-            && trim($data->{"requestee_type-$id"}))
+            && trim($cgi->param("requestee_type-$id")))
         {
-            my $requestee_email = trim($data->{"requestee_type-$id"});
+            my $requestee_email = trim($cgi->param("requestee_type-$id"));
 
             # We know the requestee exists because we ran
             # Bugzilla::User::match_field before getting here.
@@ -370,7 +370,7 @@ sub validate {
             # the requestee isn't in the group of insiders who can see it.
             if ($attach_id
                 && Param("insidergroup")
-                && $data->{'isprivate'}
+                && $cgi->param('isprivate')
                 && !$requestee->in_group(Param("insidergroup")))
             {
                 ThrowUserError("flag_requestee_unauthorized_attachment",
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 2096db407..8cb4d5f99 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -687,7 +687,8 @@ sub match {
 # Here's what it does:
 #
 # 1. Accepts a list of fields along with whether they may take multiple values
-# 2. Takes the values of those fields from $::FORM and passes them to match()
+# 2. Takes the values of those fields from the first parameter, a $cgi object 
+#    and passes them to match()
 # 3. Checks the results of the match and displays confirmation or failure
 #    messages as appropriate.
 #
@@ -710,7 +711,7 @@ sub match {
 
 # How to call it:
 #
-# Bugzilla::User::match_field ({
+# Bugzilla::User::match_field($cgi, {
 #   'field_name'    => { 'type' => fieldtype },
 #   'field_name2'   => { 'type' => fieldtype },
 #   [...]
@@ -720,8 +721,8 @@ sub match {
 #
 
 sub match_field {
-
-    my $fields         = shift;   # arguments as a hash
+    my $cgi          = shift;   # CGI object to look up fields in
+    my $fields       = shift;   # arguments as a hash
     my $matches      = {};      # the values sent to the template
     my $matchsuccess = 1;       # did the match fail?
     my $need_confirm = 0;       # whether to display confirmation screen
@@ -729,11 +730,9 @@ sub match_field {
     # prepare default form values
 
     my $vars = $::vars;
-    $vars->{'form'}  = \%::FORM;
-    $vars->{'mform'} = \%::MFORM;
 
     # What does a "--do_not_change--" field look like (if any)?
-    my $dontchange = $vars->{'form'}->{'dontchange'};
+    my $dontchange = $cgi->param('dontchange');
 
     # Fields can be regular expressions matching multiple form fields
     # (f.e. "requestee-(\d+)"), so expand each non-literal field
@@ -747,7 +746,7 @@ sub match_field {
             $expanded_fields->{$field_pattern} = $fields->{$field_pattern};
         }
         else {
-            my @field_names = grep(/$field_pattern/, keys %{$vars->{'form'}});
+            my @field_names = grep(/$field_pattern/, $cgi->param());
             foreach my $field_name (@field_names) {
                 $expanded_fields->{$field_name} = 
                   { type => $fields->{$field_pattern}->{'type'} };
@@ -774,23 +773,27 @@ sub match_field {
         # Tolerate fields that do not exist.
         #
         # This is so that fields like qa_contact can be specified in the code
-        # and it won't break if $::MFORM does not define them.
+        # and it won't break if the CGI object does not know about them.
         #
         # It has the side-effect that if a bad field name is passed it will be
         # quietly ignored rather than raising a code error.
 
-        next if !defined($vars->{'mform'}->{$field});
+        next if !defined $cgi->param($field);
 
         # Skip it if this is a --do_not_change-- field
-        next if $dontchange && $dontchange eq $vars->{'form'}->{$field};
+        next if $dontchange && $dontchange eq $cgi->param($field);
 
         # We need to move the query to $raw_field, where it will be split up,
-        # modified by the search, and put back into $::FORM and $::MFORM
+        # modified by the search, and put back into the CGI environment
         # incrementally.
 
-        my $raw_field = join(" ", @{$vars->{'mform'}->{$field}});
-        $vars->{'form'}->{$field}  = '';
-        $vars->{'mform'}->{$field} = [];
+        my $raw_field = join(" ", $cgi->param($field));
+
+        # When we add back in values later, it matters that we delete
+        # the param here, and not set it to '', so that we will add
+        # things to an empty list, and not to a list containing one
+        # empty string
+        $cgi->delete($field);
 
         my @queries = ();
 
@@ -835,12 +838,12 @@ sub match_field {
             if ((scalar(@{$users}) == 1)
                 && (@{$users}[0]->{'login'} eq $query))
             {
-                # delimit with spaces if necessary
-                if ($vars->{'form'}->{$field}) {
-                    $vars->{'form'}->{$field} .= " ";
-                }
-                $vars->{'form'}->{$field} .= @{$users}[0]->{'login'};
-                push @{$vars->{'mform'}->{$field}}, @{$users}[0]->{'login'};
+                $cgi->append(-name=>$field,
+                             -values=>[@{$users}[0]->{'login'}]);
+
+                # XXX FORM compatilibity code, will be removed in bug 225818
+                $::FORM{$field} = join(" ", $cgi->param($field));
+
                 next;
             }
 
@@ -850,12 +853,13 @@ sub match_field {
             # here is where it checks for multiple matches
 
             if (scalar(@{$users}) == 1) { # exactly one match
-                # delimit with spaces if necessary
-                if ($vars->{'form'}->{$field}) {
-                    $vars->{'form'}->{$field} .= " ";
-                }
-                $vars->{'form'}->{$field} .= @{$users}[0]->{'login'};
-                push @{$vars->{'mform'}->{$field}}, @{$users}[0]->{'login'};
+
+                $cgi->append(-name=>$field,
+                             -values=>[@{$users}[0]->{'login'}]);
+
+                # XXX FORM compatilibity code, will be removed in bug 225818
+                $::FORM{$field} = join(" ", $cgi->param($field));
+
                 $need_confirm = 1 if &::Param('confirmuniqueusermatch');
 
             }
diff --git a/attachment.cgi b/attachment.cgi
index df25b768d..9f4b603dc 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -126,10 +126,11 @@ elsif ($action eq "insert")
   # The order of these function calls is important, as both Flag::validate
   # and FlagType::validate assume User::match_field has ensured that the values
   # in the requestee fields are legitimate user email addresses.
-  Bugzilla::User::match_field({ '^requestee(_type)?-(\d+)$' => 
-                                    { 'type' => 'single' } });
-  Bugzilla::Flag::validate(\%::FORM, $bugid);
-  Bugzilla::FlagType::validate(\%::FORM, $bugid, $::FORM{'id'});
+  Bugzilla::User::match_field($cgi, {
+      '^requestee(_type)?-(\d+)$' => { 'type' => 'single' }
+  });
+  Bugzilla::Flag::validate($cgi, $bugid);
+  Bugzilla::FlagType::validate($cgi, $bugid, $::FORM{'id'});
   
   insert($data);
 }
@@ -155,10 +156,11 @@ elsif ($action eq "update")
   # The order of these function calls is important, as both Flag::validate
   # and FlagType::validate assume User::match_field has ensured that the values
   # in the requestee fields are legitimate user email addresses.
-  Bugzilla::User::match_field({ '^requestee(_type)?-(\d+)$' => 
-                                    { 'type' => 'single' } });
-  Bugzilla::Flag::validate(\%::FORM, $bugid);
-  Bugzilla::FlagType::validate(\%::FORM, $bugid, $::FORM{'id'});
+  Bugzilla::User::match_field($cgi, {
+      '^requestee(_type)?-(\d+)$' => { 'type' => 'single' }
+  });
+  Bugzilla::Flag::validate($cgi, $bugid);
+  Bugzilla::FlagType::validate($cgi, $bugid, $::FORM{'id'});
   
   update();
 }
@@ -1033,7 +1035,7 @@ sub insert
   
   # Create flags.
   my $target = Bugzilla::Flag::GetTarget(undef, $attachid);
-  Bugzilla::Flag::process($target, $timestamp, \%::FORM);
+  Bugzilla::Flag::process($target, $timestamp, $cgi);
    
   # Define the variables and functions that will be passed to the UI template.
   $vars->{'mailrecipients'} =  { 'changer' => Bugzilla->user->login,
@@ -1168,7 +1170,7 @@ sub update
   # is obsoleting this attachment without deleting any requests
   # the user submits at the same time.
   my $target = Bugzilla::Flag::GetTarget(undef, $::FORM{'id'});
-  Bugzilla::Flag::process($target, $timestamp, \%::FORM);
+  Bugzilla::Flag::process($target, $timestamp, $cgi);
 
   # Update the attachment record in the database.
   SendSQL("UPDATE  attachments 
diff --git a/editwhines.cgi b/editwhines.cgi
index 013ee0df4..e65a585d0 100755
--- a/editwhines.cgi
+++ b/editwhines.cgi
@@ -198,7 +198,7 @@ if ($cgi->param('update')) {
                 }
             }
             if (scalar %{$arglist}) {
-                &Bugzilla::User::match_field($arglist);
+                &Bugzilla::User::match_field($cgi, $arglist);
             }
 
             for my $sid (@scheduleids) {
diff --git a/post_bug.cgi b/post_bug.cgi
index 7f05dc7bc..84a9fd9df 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -60,7 +60,7 @@ my $dbh = Bugzilla->dbh;
 
 # do a match on the fields if applicable
 
-&Bugzilla::User::match_field ({
+&Bugzilla::User::match_field ($cgi, {
     'cc'            => { 'type' => 'multi'  },
     'assigned_to'   => { 'type' => 'single' },
     'qa_contact'    => { 'type' => 'single' },
diff --git a/process_bug.cgi b/process_bug.cgi
index 94d86c936..aff3698bd 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -44,6 +44,7 @@ use Bugzilla::Util;
 
 # Use the Flag module to modify flag data if the user set flags.
 use Bugzilla::Flag;
+use Bugzilla::FlagType;
 
 # Shut up misguided -w warnings about "used only once":
 
@@ -138,7 +139,7 @@ foreach my $field ("dependson", "blocked") {
 # The order of these function calls is important, as both Flag::validate
 # and FlagType::validate assume User::match_field has ensured that the values
 # in the requestee fields are legitimate user email addresses.
-&Bugzilla::User::match_field({
+&Bugzilla::User::match_field($cgi, {
     'qa_contact'                => { 'type' => 'single' },
     'newcc'                     => { 'type' => 'multi'  },
     'masscc'                    => { 'type' => 'multi'  },
@@ -148,8 +149,8 @@ foreach my $field ("dependson", "blocked") {
 # Validate flags, but only if the user is changing a single bug,
 # since the multi-change form doesn't include flag changes.
 if (defined $::FORM{'id'}) {
-    Bugzilla::Flag::validate(\%::FORM, $::FORM{'id'});
-    Bugzilla::FlagType::validate(\%::FORM, $::FORM{'id'});
+    Bugzilla::Flag::validate($cgi, $::FORM{'id'});
+    Bugzilla::FlagType::validate($cgi, $::FORM{'id'});
 }
 
 ######################################################################
@@ -1814,7 +1815,7 @@ foreach my $id (@idlist) {
     # Set and update flags.
     if ($UserInEditGroupSet) {
         my $target = Bugzilla::Flag::GetTarget($id);
-        Bugzilla::Flag::process($target, $timestamp, \%::FORM);
+        Bugzilla::Flag::process($target, $timestamp, $cgi);
     }
     if ($bug_changed) {
         SendSQL("UPDATE bugs SET delta_ts = $sql_timestamp WHERE bug_id = $id");
diff --git a/template/en/default/global/confirm-user-match.html.tmpl b/template/en/default/global/confirm-user-match.html.tmpl
index 456af1fc2..fd09f89a6 100644
--- a/template/en/default/global/confirm-user-match.html.tmpl
+++ b/template/en/default/global/confirm-user-match.html.tmpl
@@ -21,8 +21,6 @@
   #%]
 
 [%# INTERFACE:
-  # form: hash; the form values submitted to the script
-  # mform: hash; the form multi-values submitted to the script
   # fields: hash/record; the fields being matched, each of which has:
   #     type: single|multi: whether or not the user can select multiple matches
   #     flag_type: for flag requestee fields, the type of flag being requested
diff --git a/template/en/default/global/hidden-fields.html.tmpl b/template/en/default/global/hidden-fields.html.tmpl
index 2fa577b43..7327e43cf 100644
--- a/template/en/default/global/hidden-fields.html.tmpl
+++ b/template/en/default/global/hidden-fields.html.tmpl
@@ -20,23 +20,22 @@
   #%]
 
 [%# INTERFACE:
-  # form: hash; the form fields/values for which to generate hidden fields.
-  # mform: hash; the form fields/values with multiple values for which to
-  #   generate hidden fields.
   # exclude: string; a regular expression matching fields to exclude
   #   from the list of hidden fields generated by this template
   #%]
 
+[%# The global Bugzilla->cgi object is used to obtain form variable values. %]
+[% USE Bugzilla %]
+[% cgi = Bugzilla.cgi %]
+
 [%# Generate hidden form fields for non-excluded fields. %]
-[% FOREACH field = form %]
-  [% NEXT IF exclude && field.key.search(exclude) %]
-  [% IF mform.${field.key}.size > 1 %]
-    [% FOREACH mvalue = mform.${field.key} %]
-      <input type="hidden" name="[% field.key FILTER html %]"
+[% FOREACH field = cgi.param() %]
+  [% NEXT IF exclude && field.search(exclude) %]
+  [%# The '.slice(0)' bit is here to force the 'param(field)' to be evaluated
+      in a list context, so we can avoid extra code checking for single valued or
+      empty fields %]
+  [% FOREACH mvalue = cgi.param(field).slice(0) %]
+    <input type="hidden" name="[% field FILTER html %]"
              value="[% mvalue FILTER html FILTER html_linebreak %]">
     [% END %]
-  [% ELSE %]
-    <input type="hidden" name="[% field.key FILTER html %]"
-           value="[% field.value FILTER html FILTER html_linebreak %]">
-  [% END %]
 [% END %]