Bug 398308: Make Search.pm take a hashref for its "params" argument

Bugzilla/CGI.pm

@@ -110,7 +110,6 @@ sub new {
 sub canonicalise_query {
     my ($self, @exclude) = @_;
 
-    $self->convert_old_params();
     # Reconstruct the URL by concatenating the sorted param=value pairs
     my @parameters;
     foreach my $key (sort($self->param())) {
@@ -135,17 +134,6 @@ sub canonicalise_query {
     return join("&", @parameters);
 }
 
-sub convert_old_params {
-    my $self = shift;
-
-    # bugidtype is now bug_id_type.
-    if ($self->param('bugidtype')) {
-        my $value = $self->param('bugidtype') eq 'exclude' ? 'nowords' : 'anyexact';
-        $self->param('bug_id_type', $value);
-        $self->delete('bugidtype');
-    }
-}
-
 sub clean_search_url {
     my $self = shift;
     # Delete any empty URL parameter.

Bugzilla/Search.pm

@@ -332,8 +332,10 @@ use constant SPECIAL_PARSING => {
     delta_ts    => \&_timestamp_translate,
 };
 
-# Backwards compatibility for times that we changed the names of fields.
+# Backwards compatibility for times that we changed the names of fields
+# or URL parameters.
 use constant FIELD_MAP => {
+    bugidtype => 'bug_id_type',
     changedin => 'days_elapsed',
     long_desc => 'longdesc',
 };
@@ -680,6 +682,23 @@ sub _multi_select_fields {
     return $self->{multi_select_fields};
 }
 
+# $self->{params} contains values that could be undef, could be a string,
+# or could be an arrayref. Sometimes we want that value as an array,
+# always.
+sub _param_array {
+    my ($self, $name) = @_;
+    my $value = $self->_params->{$name};
+    if (!defined $value) {
+        return ();
+    }
+    if (ref($value) eq 'ARRAY') {
+        return @$value;
+    }
+    return ($value);
+}
+
+sub _params { $_[0]->{params} }
+
 sub _user { return $_[0]->{user} }
 
 ##############################
@@ -1068,7 +1087,24 @@ sub _skip_group_by {
 # Internal Accessors: Special Params Parsing #
 ##############################################
 
-sub _params { $_[0]->{params} }
+# Backwards compatibility for old field names.
+sub _convert_old_params {
+    my ($self) = @_;
+    my $params = $self->_params;
+    
+    # bugidtype has different values in modern Search.pm.
+    if (defined $params->{'bugidtype'}) {
+        my $value = $params->{'bugidtype'};
+        $params->{'bugidtype'} = $value eq 'exclude' ? 'nowords' : 'anyexact';
+    }
+    
+    foreach my $old_name (keys %{ FIELD_MAP() }) {
+        if (defined $params->{$old_name}) {
+            my $new_name = FIELD_MAP->{$old_name};
+            $params->{$new_name} = delete $params->{$old_name};
+        }
+    }
+}
 
 sub _convert_special_params_to_chart_params {
     my ($self) = @_;
@@ -1078,9 +1114,9 @@ sub _convert_special_params_to_chart_params {
     
     # first we delete any sign of "Chart #-1" from the HTML form hash
     # since we want to guarantee the user didn't hide something here
-    my @badcharts = grep { /^(field|type|value)-1-/ } $params->param();
+    my @badcharts = grep { /^(field|type|value)-1-/ } keys %$params;
     foreach my $field (@badcharts) {
-        $params->delete($field);
+        delete $params->{$field};
     }
 
     # now we take our special chart and stuff it into the form hash
@@ -1088,10 +1124,11 @@ sub _convert_special_params_to_chart_params {
     my $and = 0;
     foreach my $or_array (@special_charts) {
         my $or = 0;
+        my $identifier = "$chart-$and-$or";
         while (@$or_array) {
-            $params->param("field$chart-$and-$or", shift @$or_array);
-            $params->param("type$chart-$and-$or", shift @$or_array);
-            $params->param("value$chart-$and-$or", shift @$or_array);
+            $params->{"field$identifier"} = shift @$or_array;
+            $params->{"type$identifier"}  = shift @$or_array;
+            $params->{"value$identifier"} = shift @$or_array;
             $or++;
         }
         $and++;
@@ -1102,6 +1139,7 @@ sub _convert_special_params_to_chart_params {
 # charts.
 sub _special_charts {
     my ($self) = @_;
+    $self->_convert_old_params();
     $self->_special_parse_bug_status();
     $self->_special_parse_resolution();
     my @charts = $self->_parse_basic_fields();
@@ -1116,27 +1154,17 @@ sub _parse_basic_fields {
     my $params = $self->_params;
     my $chart_fields = $self->_chart_fields;
     
-    foreach my $old_name (keys %{ FIELD_MAP() }) {
-        if (defined $params->param($old_name)) {
-            my @value = $params->param($old_name);
-            $params->delete($old_name);
-            my $new_name = FIELD_MAP->{$old_name};
-            $params->param($new_name, @value);
-        }
-    }
-    
     my @charts;
     foreach my $field_name (keys %$chart_fields) {
         # CGI params shouldn't have periods in them, so we only accept
         # period-separated fields with underscores where the periods go.
         my $param_name = $field_name;
         $param_name =~ s/\./_/g;
-        next if !defined $params->param($param_name);
-        my $operator = $params->param("${param_name}_type");
-        $operator = 'anyexact' if !$operator;
+        my @values = $self->_param_array($param_name);
+        next if !@values;
+        my $operator = $params->{"${param_name}_type"} || 'anyexact';
         $operator = 'matches' if $operator eq 'content';
-        my $string_value = join(',', $params->param($param_name));
-        push(@charts, [$field_name, $operator, $string_value]);
+        push(@charts, [$field_name, $operator, \@values]);
     }
     return @charts;
 }
@@ -1144,9 +1172,9 @@ sub _parse_basic_fields {
 sub _special_parse_bug_status {
     my ($self) = @_;
     my $params = $self->_params;
-    return if !defined $params->param('bug_status');
+    return if !defined $params->{'bug_status'};
 
-    my @bug_status = $params->param('bug_status');
+    my @bug_status = $self->_param_array('bug_status');
     # Also include inactive bug statuses, as you can query them.
     my $legal_statuses = $self->_chart_fields->{'bug_status'}->legal_values;
 
@@ -1172,10 +1200,10 @@ sub _special_parse_bug_status {
     # If the user has selected every status, change to selecting none.
     # This is functionally equivalent, but quite a lot faster.    
     if ($all or scalar(@bug_status) == scalar(@$legal_statuses)) {
-        $params->delete('bug_status');
+        delete $params->{'bug_status'};
     }
     else {
-        $params->param('bug_status', @bug_status);
+        $params->{'bug_status'} = \@bug_status;
     }
 }
 
@@ -1183,14 +1211,12 @@ sub _special_parse_chfield {
     my ($self) = @_;
     my $params = $self->_params;
     
-    my $date_from = trim(lc($params->param('chfieldfrom')));
-    $date_from = '' if !defined $date_from;
-    my $date_to = trim(lc($params->param('chfieldto')));
-    $date_to = '' if !defined $date_to;
+    my $date_from = trim(lc($params->{'chfieldfrom'} || ''));
+    my $date_to = trim(lc($params->{'chfieldto'} || ''));
     $date_from = '' if $date_from eq 'now';
     $date_to = '' if $date_to eq 'now';
-    my @fields = $params->param('chfield');
-    my $value_to = trim($params->param('chfieldvalue'));
+    my @fields = $self->_param_array('chfield');
+    my $value_to = $params->{'chfieldvalue'};
     $value_to = '' if !defined $value_to;
 
     my @charts;
@@ -1260,38 +1286,38 @@ sub _special_parse_deadline {
     my $params = $self->_params;
     
     my @charts;
-    if (my $from = $params->param('deadlinefrom')) {
+    if (my $from = $params->{'deadlinefrom'}) {
         push(@charts, ['deadline', 'greaterthaneq', $from]);
     }
-    if (my $to = $params->param('deadlineto')) {
+    if (my $to = $params->{'deadlineto'}) {
         push(@charts, ['deadline', 'lessthaneq', $to]);
     }
-      
-    return @charts;    
+    
+    return @charts;
 }
 
 sub _special_parse_email {
     my ($self) = @_;
     my $params = $self->_params;
     
-    my @email_params = grep { $_ =~ /^email\d+$/ } $params->param();
+    my @email_params = grep { $_ =~ /^email\d+$/ } keys %$params;
     
     my @charts;
     foreach my $param (@email_params) {
         $param =~ /(\d+)$/;
         my $id = $1;
-        my $email = trim($params->param("email$id"));
-        next if $email eq "";
-        my $type = $params->param("emailtype$id");
+        my $email = trim($params->{"email$id"});
+        next if !$email;
+        my $type = $params->{"emailtype$id"} || 'anyexact';
         $type = "anyexact" if $type eq "exact";
 
         my @or_charts;
         foreach my $field qw(assigned_to reporter cc qa_contact) {
-            if ($params->param("email$field$id")) {
+            if ($params->{"email$field$id"}) {
                 push(@or_charts, $field, $type, $email);
             }
         }
-        if ($params->param("emaillongdesc$id")) {
+        if ($params->{"emaillongdesc$id"}) {
             push(@or_charts, "commenter", $type, $email);
         }
 
@@ -1304,17 +1330,16 @@ sub _special_parse_email {
 sub _special_parse_resolution {
     my ($self) = @_;
     my $params = $self->_params;
-    return if !defined $params->param('resolution');
+    return if !defined $params->{'resolution'};
 
-    my @resolution = $params->param('resolution');
+    my @resolution = $self->_param_array('resolution');
     my $legal_resolutions = $self->_chart_fields->{resolution}->legal_values;
     @resolution = _valid_values(\@resolution, $legal_resolutions, '---');
     if (scalar(@resolution) == scalar(@$legal_resolutions)) {
-        $params->delete('resolution');
+        delete $params->{'resolution'};
     }
 }
 
-
 sub _valid_values {
     my ($input, $valid, $extra_value) = @_;
     my @result;
@@ -1380,11 +1405,8 @@ sub _charts_to_conditions {
 sub _params_to_charts {
     my ($self) = @_;
     my $params = $self->_params;
-    # XXX This should probably just be moved into using FIELD_MAP here
-    #     in Search.pm.
-    $params->convert_old_params();
     $self->_convert_special_params_to_chart_params();
-    my @param_list = $params->param();
+    my @param_list = keys %$params;
     
     my @all_field_params = grep { /^field-?\d+/ } @param_list;
     my @chart_ids = map { /^field(-?\d+)/; $1 } @all_field_params;
@@ -1410,7 +1432,7 @@ sub _params_to_charts {
                 # meaning that the field, value, or operator were empty.
                 push(@or_charts, $info) if defined $info;
             }
-            if ($params->param("negate$chart_id")) {
+            if ($params->{"negate$chart_id"}) {
                 push(@and_charts, NEGATE);
             }
             push(@and_charts, \@or_charts);
@@ -1433,12 +1455,12 @@ sub _handle_chart {
     
     my $identifier = "$chart_id-$and_id-$or_id";
     
-    my $field = $params->param("field$identifier");
-    my $operator = $params->param("type$identifier");
-    my $value = $params->param("value$identifier");
+    my $field = $params->{"field$identifier"};
+    my $operator = $params->{"type$identifier"};
+    my @values = $self->_param_array("value$identifier");
 
-    return if (!defined $field or !defined $operator or !defined $value);
-    $value = trim($value);
+    return if (!defined $field or !defined $operator or !@values);
+    my $value = trim(join(',', @values));
     return if $value eq '';
     $self->_chart_fields->{$field}
         or ThrowCodeError("invalid_field_name", { field => $field });
@@ -1474,7 +1496,7 @@ sub _handle_chart {
     
     return \%search_args;
 }
-    
+   
 ##################################
 # do_search_function And Helpers #
 ##################################

buglist.cgi

@@ -849,7 +849,7 @@ my @orderstrings = split(/,\s*/, $order);
 
 # Generate the basic SQL query that will be used to generate the bug list.
 my $search = new Bugzilla::Search('fields' => \@selectcolumns, 
-                                  'params' => $params,
+                                  'params' => scalar $params->Vars,
                                   'order' => \@orderstrings);
 my $query = $search->sql;
 $vars->{'search_description'} = $search->search_description;

collectstats.pl

@@ -509,7 +509,7 @@ sub CollectSeriesData {
         # Do not die if Search->new() detects invalid data, such as an obsolete
         # login name or a renamed product or component, etc.
         eval {
-            my $search = new Bugzilla::Search('params' => $cgi,
+            my $search = new Bugzilla::Search('params' => scalar $cgi->Vars,
                                               'fields' => ["bug_id"],
                                               'user'   => $user);
             my $sql = $search->sql;

report.cgi

@@ -127,7 +127,7 @@ my @axis_fields = ($row_field || EMPTY_COLUMN,
 # Clone the params, so that Bugzilla::Search can modify them
 my $params = new Bugzilla::CGI($cgi);
 my $search = new Bugzilla::Search('fields' => \@axis_fields, 
-                                  'params' => $params);
+                                  'params' => scalar $params->Vars);
 my $query = $search->sql;
 
 $::SIG{TERM} = 'DEFAULT';

whine.pl

@@ -446,7 +446,7 @@ sub run_queries {
         my $searchparams = new Bugzilla::CGI($savedquery);
         my $search = new Bugzilla::Search(
             'fields' => \@searchfields,
-            'params' => $searchparams,
+            'params' => scalar $searchparams->Vars,
             'user'   => $args->{'recipient'}, # the search runs as the recipient
         );
         my $sqlquery = $search->sql;

xt/lib/Bugzilla/Test/Search/AndTest.pm

@@ -25,7 +25,6 @@ package Bugzilla::Test::Search::AndTest;
 use base qw(Bugzilla::Test::Search::OrTest);
 
 use Bugzilla::Test::Search::Constants;
-use Bugzilla::Test::Search::FakeCGI;
 use List::MoreUtils qw(all);
 
 use constant type => 'AND';
@@ -55,15 +54,15 @@ sub _join_broken_constant { {} }
 sub search_params {
     my ($self) = @_;
     my @all_params = map { $_->search_params } $self->field_tests;
-    my $params = new Bugzilla::Test::Search::FakeCGI;
+    my %params;
     my $chart = 0;
     foreach my $item (@all_params) {
-        $params->param("field0-$chart-0", $item->param('field0-0-0'));
-        $params->param("type0-$chart-0", $item->param('type0-0-0'));
-        $params->param("value0-$chart-0", $item->param('value0-0-0'));
+        $params{"field0-$chart-0"} = $item->{'field0-0-0'};
+        $params{"type0-$chart-0"}  = $item->{'type0-0-0'};
+        $params{"value0-$chart-0"} = $item->{'value0-0-0'};
         $chart++;
     }
-    return $params;
+    return \%params;
 }
 
 1;
\ No newline at end of file

xt/lib/Bugzilla/Test/Search/FakeCGI.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 Everything Solved, Inc.
-# Portions created by the Initial Developer are Copyright (C) 2010 the
-# Initial Developer. All Rights Reserved.
-#
-# Contributor(s):
-#   Max Kanat-Alexander <mkanat@bugzilla.org>
-
-# Calling CGI::param over and over turned out to be one of the slowest
-# parts of search.t. So we create a simpler thing here that just supports
-# "param" in a fast way.
-package Bugzilla::Test::Search::FakeCGI;
-
-sub new {
-    my ($class) = @_;
-    return bless {}, $class;
-}
-
-sub param {
-    my ($self, $name, @values) = @_;
-    if (!defined $name) {
-        return keys %$self;
-    }
-
-    if (@values) {
-        if (ref $values[0] eq 'ARRAY') {
-            $self->{$name} = $values[0];
-        }
-        else {
-            $self->{$name} = \@values;
-        }
-    }
-    
-    return () if !exists $self->{$name};
-    
-    my $item = $self->{$name};
-    return wantarray ? @{ $item || [] } : $item->[0];
-}
-
-sub delete {
-    my ($self, $name) = @_;
-    delete $self->{$name};
-}
-
-# We don't need to do this, because we don't use old params in search.t.
-sub convert_old_params {}
-
-1;
\ No newline at end of file

xt/lib/Bugzilla/Test/Search/FieldTest.pm

@@ -26,7 +26,6 @@ package Bugzilla::Test::Search::FieldTest;
 
 use strict;
 use warnings;
-use Bugzilla::Test::Search::FakeCGI;
 use Bugzilla::Search;
 use Bugzilla::Test::Search::Constants;
 
@@ -278,19 +277,16 @@ sub join_broken {
 
 # The CGI object that will get passed to Bugzilla::Search as its arguments.
 sub search_params {
-    my $self = shift;
+    my ($self) = @_;
     return $self->{search_params} if $self->{search_params};
 
-    my $field = $self->field;
-    my $operator = $self->operator;
-    my $value = $self->translated_value;
-    
-    my $cgi = new Bugzilla::Test::Search::FakeCGI;
-    $cgi->param("field0-0-0", $field);
-    $cgi->param('type0-0-0', $operator);
-    $cgi->param('value0-0-0', $value);
+    my %params = (
+        "field0-0-0" => $self->field,
+        "type0-0-0"  => $self->operator,
+        "value0-0-0"  => $self->translated_value,
+    );
     
-    $self->{search_params} = $cgi;
+    $self->{search_params} = \%params;
     return $self->{search_params};
 }
 

xt/lib/Bugzilla/Test/Search/OrTest.pm

@@ -25,7 +25,6 @@ package Bugzilla::Test::Search::OrTest;
 use base qw(Bugzilla::Test::Search::FieldTest);
 
 use Bugzilla::Test::Search::Constants;
-use Bugzilla::Test::Search::FakeCGI;
 use List::MoreUtils qw(any uniq);
 
 use constant type => 'OR';
@@ -172,15 +171,15 @@ sub search_columns {
 sub search_params {
     my ($self) = @_;
     my @all_params = map { $_->search_params } $self->field_tests;
-    my $params = new Bugzilla::Test::Search::FakeCGI;
+    my %params;
     my $chart = 0;
     foreach my $item (@all_params) {
-        $params->param("field0-0-$chart", $item->param('field0-0-0'));
-        $params->param("type0-0-$chart", $item->param('type0-0-0'));
-        $params->param("value0-0-$chart", $item->param('value0-0-0'));
+        $params{"field0-0-$chart"} = $item->{'field0-0-0'};
+        $params{"type0-0-$chart"}  = $item->{'type0-0-0'};
+        $params{"value0-0-$chart"} = $item->{'value0-0-0'};
         $chart++;
     }
-    return $params;
+    return \%params;
 }
 
 1;
\ No newline at end of file