Bug 577800: Finish the cleanup of Search.pm's "init" function by removing

it and having its work be done by a new "sql" accessor instead. Also adds
some comments, moves functions around into sections, and creates a new
_user accessor.
r=mkanat, a=mkanat (module owner)

6 files changed, 250 insertions, 229 deletions

diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm
index ea2882440..e2c868dd8 100644
--- a/Bugzilla/Search.pm
+++ b/Bugzilla/Search.pm
@@ -58,6 +58,73 @@ use Date::Parse;
 use List::MoreUtils qw(all part uniq);
 use Storable qw(dclone);
 
+# Description Of Boolean Charts
+# -----------------------------
+#
+# A boolean chart is a way of representing the terms in a logical
+# expression.  Bugzilla builds SQL queries depending on how you enter
+# terms into the boolean chart. Boolean charts are represented in
+# urls as three-tuples of (chart id, row, column). The query form
+# (query.cgi) may contain an arbitrary number of boolean charts where
+# each chart represents a clause in a SQL query.
+#
+# The query form starts out with one boolean chart containing one
+# row and one column.  Extra rows can be created by pressing the
+# AND button at the bottom of the chart.  Extra columns are created
+# by pressing the OR button at the right end of the chart. Extra
+# charts are created by pressing "Add another boolean chart".
+#
+# Each chart consists of an arbitrary number of rows and columns.
+# The terms within a row are ORed together. The expressions represented
+# by each row are ANDed together. The expressions represented by each
+# chart are ANDed together.
+#
+#        ----------------------
+#        | col2 | col2 | col3 |
+# --------------|------|------|
+# | row1 |  a1  |  a2  |      |
+# |------|------|------|------|  => ((a1 OR a2) AND (b1 OR b2 OR b3) AND (c1))
+# | row2 |  b1  |  b2  |  b3  |
+# |------|------|------|------|
+# | row3 |  c1  |      |      |
+# -----------------------------
+#
+#        --------
+#        | col2 |
+# --------------|
+# | row1 |  d1  | => (d1)
+# ---------------
+#
+# Together, these two charts represent a SQL expression like this
+# SELECT blah FROM blah WHERE ( (a1 OR a2)AND(b1 OR b2 OR b3)AND(c1)) AND (d1)
+#
+# The terms within a single row of a boolean chart are all constraints
+# on a single piece of data.  If you're looking for a bug that has two
+# different people cc'd on it, then you need to use two boolean charts.
+# This will find bugs with one CC matching 'foo@blah.org' and and another
+# CC matching 'bar@blah.org'.
+#
+# --------------------------------------------------------------
+# CC    | equal to
+# foo@blah.org
+# --------------------------------------------------------------
+# CC    | equal to
+# bar@blah.org
+#
+# If you try to do this query by pressing the AND button in the
+# original boolean chart then what you'll get is an expression that
+# looks for a single CC where the login name is both "foo@blah.org",
+# and "bar@blah.org". This is impossible.
+#
+# --------------------------------------------------------------
+# CC    | equal to
+# foo@blah.org
+# AND
+# CC    | equal to
+# bar@blah.org
+# --------------------------------------------------------------
+
+
 #############
 # Constants #
 #############
@@ -442,6 +509,9 @@ sub COLUMNS {
 
     foreach my $col (@email_fields) {
         my $sql = "map_${col}.login_name";
+        # XXX This needs to be generated inside an accessor instead,
+        #     probably, because it should use $self->_user to determine
+        #     this, not Bugzilla->user.
         if (!Bugzilla->user->id) {
              $sql = $dbh->sql_string_until($sql, $dbh->quote('@'));
         }
@@ -517,6 +587,67 @@ use constant GROUP_BY_SKIP => EMPTY_COLUMN, qw(
     percentage_complete
 );
 
+###############
+# Constructor #
+###############
+
+# Note that the params argument may be modified by Bugzilla::Search
+sub new {
+    my $invocant = shift;
+    my $class = ref($invocant) || $invocant;
+  
+    my $self = { @_ };
+    bless($self, $class);
+    $self->{'user'} ||= Bugzilla->user;
+
+    return $self;
+}
+
+
+####################
+# Public Accessors #
+####################
+
+sub sql {
+    my ($self) = @_;
+    return $self->{sql} if $self->{sql};
+    my $dbh = Bugzilla->dbh;
+    
+    my ($joins, $having_terms, $where_terms) = $self->_charts_to_conditions();
+
+    my $select = join(', ', $self->_sql_select);
+    my $from = $self->_sql_from($joins);
+    my $where = $self->_sql_where($where_terms);
+    my $group_by = $dbh->sql_group_by($self->_sql_group_by);
+    my $having = @$having_terms
+                 ? "\nHAVING " . join(' AND ', @$having_terms) : '';
+    my $order_by = $self->_sql_order_by
+                   ? "\nORDER BY " . join(', ', $self->_sql_order_by) : '';
+    
+    my $query = <<END;
+SELECT $select
+  FROM $from
+ WHERE $where
+$group_by$having$order_by
+END
+    $self->{sql} = $query;
+    return $self->{sql};
+}
+
+sub search_description {
+    my ($self, $params) = @_;
+    my $desc = $self->{'search_description'} ||= [];
+    if ($params) {
+        push(@$desc, $params);
+    }
+    # Make sure that the description has actually been generated if
+    # people are asking for the whole thing.
+    else {
+        $self->sql;
+    }
+    return $self->{'search_description'};
+}
+
 ######################
 # Internal Accessors #
 ######################
@@ -528,7 +659,7 @@ sub _chart_fields {
     if (!$self->{chart_fields}) {
         my $chart_fields = Bugzilla->fields({ by_name => 1 });
 
-        if (!Bugzilla->user->is_timetracker) {
+        if (!$self->_user->is_timetracker) {
             foreach my $tt_field (TIMETRACKING_FIELDS) {
                 delete $chart_fields->{$tt_field};
             }
@@ -538,6 +669,9 @@ sub _chart_fields {
     return $self->{chart_fields};
 }
 
+# There are various places in Search.pm that we need to know the list of
+# valid multi-select fields--or really, fields that are stored like
+# multi-selects, which includes BUG_URLS fields.
 sub _multi_select_fields {
     my ($self) = @_;
     $self->{multi_select_fields} ||= Bugzilla->fields({
@@ -546,6 +680,8 @@ sub _multi_select_fields {
     return $self->{multi_select_fields};
 }
 
+sub _user { return $_[0]->{user} }
+
 ##############################
 # Internal Accessors: SELECT #
 ##############################
@@ -653,6 +789,23 @@ sub _sql_order_by {
     return @{ $self->{sql_order_by} };
 }
 
+sub _translate_order_by_column {
+    my ($self, $order_by_item) = @_;
+
+    my ($field, $direction) = split_order_term($order_by_item);
+    
+    $direction = '' if lc($direction) eq 'asc';
+    my $special_order = $self->_special_order->{$field}->{order};
+    # Standard fields have underscores in their SELECT alias instead
+    # of a period (because aliases can't have periods).
+    $field =~ s/\./_/g;
+    my @items = $special_order ? @$special_order : $field;
+    if (lc($direction) eq 'desc') {
+        @items = map { "$_ DESC" } @items;
+    }
+    return @items;
+}
+
 ############################
 # Internal Accessors: FROM #
 ############################
@@ -756,7 +909,7 @@ sub _select_order_joins {
 # These are the joins that are *always* in the FROM clause.
 sub _standard_joins {
     my ($self) = @_;
-    my $user = $self->{'user'};
+    my $user = $self->_user;
     my @joins;
 
     my $security_join = {
@@ -790,6 +943,7 @@ sub _sql_from {
     return "bugs\n" . join("\n", @join_sql);
 }
 
+# This takes a join data structure and turns it into actual JOIN SQL.
 sub _translate_join {
     my ($self, $join_info) = @_;
     
@@ -822,6 +976,51 @@ sub _translate_join {
     return @join_sql;
 }
 
+#############################
+# Internal Accessors: WHERE #
+#############################
+
+# Note: There's also quite a bit of stuff that affects the WHERE clause
+# in the "Internal Accessors: Boolean Charts" section.
+
+# The terms that are always in the WHERE clause. These implement bug
+# group security.
+sub _standard_where {
+    my ($self) = @_;
+    # If replication lags badly between the shadow db and the main DB,
+    # it's possible for bugs to show up in searches before their group
+    # controls are properly set. To prevent this, when initially creating
+    # bugs we set their creation_ts to NULL, and don't give them a creation_ts
+    # until their group controls are set. So if a bug has a NULL creation_ts,
+    # it shouldn't show up in searches at all.
+    my @where = ('bugs.creation_ts IS NOT NULL');
+    
+    my $security_term = 'security_map.group_id IS NULL';
+
+    my $user = $self->_user;
+    if ($user->id) {
+        my $userid = $user->id;
+        $security_term .= <<END;
+ OR (bugs.reporter_accessible = 1 AND bugs.reporter = $userid)
+ OR (bugs.cclist_accessible = 1 AND security_cc.who IS NOT NULL)
+ OR bugs.assigned_to = $userid
+END
+        if (Bugzilla->params->{'useqacontact'}) {
+            $security_term.= " OR bugs.qa_contact = $userid";
+        }
+        $security_term = "($security_term)";
+    }
+
+    push(@where, $security_term);
+
+    return @where;
+}
+
+sub _sql_where {
+    my ($self, $where_terms) = @_;
+    return join(' AND ', $self->_standard_where, @$where_terms);
+}
+
 ################################
 # Internal Accessors: GROUP BY #
 ################################
@@ -1057,7 +1256,7 @@ sub _special_parse_chfield {
 
 sub _special_parse_deadline {
     my ($self) = @_;
-    return if !Bugzilla->user->is_timetracker;
+    return if !$self->_user->is_timetracker;
     my $params = $self->_params;
     
     my @charts;
@@ -1115,6 +1314,21 @@ sub _special_parse_resolution {
     }
 }
 
+
+sub _valid_values {
+    my ($input, $valid, $extra_value) = @_;
+    my @result;
+    foreach my $item (@$input) {
+        if (defined $extra_value and $item eq $extra_value) {
+            push(@result, $item);
+        }
+        elsif (grep { $_->name eq $item } @$valid) {
+            push(@result, $item);
+        }
+    }
+    return @result;
+}
+
 ######################################
 # Internal Accessors: Boolean Charts #
 ######################################
@@ -1166,6 +1380,9 @@ 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();
     
@@ -1259,195 +1476,9 @@ sub _handle_chart {
 }
     
 ##################################
-# Helpers for Internal Accessors #
+# do_search_function And Helpers #
 ##################################
 
-sub _valid_values {
-    my ($input, $valid, $extra_value) = @_;
-    my @result;
-    foreach my $item (@$input) {
-        if (defined $extra_value and $item eq $extra_value) {
-            push(@result, $item);
-        }
-        elsif (grep { $_->name eq $item } @$valid) {
-            push(@result, $item);
-        }
-    }
-    return @result;
-}
-
-sub _translate_order_by_column {
-    my ($self, $order_by_item) = @_;
-
-    my ($field, $direction) = split_order_term($order_by_item);
-    
-    $direction = '' if lc($direction) eq 'asc';
-    my $special_order = $self->_special_order->{$field}->{order};
-    # Standard fields have underscores in their SELECT alias instead
-    # of a period (because aliases can't have periods).
-    $field =~ s/\./_/g;
-    my @items = $special_order ? @$special_order : $field;
-    if (lc($direction) eq 'desc') {
-        @items = map { "$_ DESC" } @items;
-    }
-    return @items;
-}
-
-###############
-# Constructor #
-###############
-
-# Create a new Search
-# Note that the param argument may be modified by Bugzilla::Search
-sub new {
-    my $invocant = shift;
-    my $class = ref($invocant) || $invocant;
-  
-    my $self = { @_ };
-    bless($self, $class);
-
-    $self->init();
- 
-    return $self;
-}
-
-sub init {
-    my $self = shift;
-    my $params = $self->_params;
-    $params->convert_old_params();
-    $self->{'user'} ||= Bugzilla->user;
-    my $user = $self->{'user'};
-
-    my $dbh = Bugzilla->dbh;
-
-   
-
-
-# A boolean chart is a way of representing the terms in a logical
-# expression.  Bugzilla builds SQL queries depending on how you enter
-# terms into the boolean chart. Boolean charts are represented in
-# urls as tree-tuples of (chart id, row, column). The query form
-# (query.cgi) may contain an arbitrary number of boolean charts where
-# each chart represents a clause in a SQL query.
-#
-# The query form starts out with one boolean chart containing one
-# row and one column.  Extra rows can be created by pressing the
-# AND button at the bottom of the chart.  Extra columns are created
-# by pressing the OR button at the right end of the chart. Extra
-# charts are created by pressing "Add another boolean chart".
-#
-# Each chart consists of an arbitrary number of rows and columns.
-# The terms within a row are ORed together. The expressions represented
-# by each row are ANDed together. The expressions represented by each
-# chart are ANDed together.
-#
-#        ----------------------
-#        | col2 | col2 | col3 |
-# --------------|------|------|
-# | row1 |  a1  |  a2  |      |
-# |------|------|------|------|  => ((a1 OR a2) AND (b1 OR b2 OR b3) AND (c1))
-# | row2 |  b1  |  b2  |  b3  |
-# |------|------|------|------|
-# | row3 |  c1  |      |      |
-# -----------------------------
-#
-#        --------
-#        | col2 |
-# --------------|
-# | row1 |  d1  | => (d1)
-# ---------------
-#
-# Together, these two charts represent a SQL expression like this
-# SELECT blah FROM blah WHERE ( (a1 OR a2)AND(b1 OR b2 OR b3)AND(c1)) AND (d1)
-#
-# The terms within a single row of a boolean chart are all constraints
-# on a single piece of data.  If you're looking for a bug that has two
-# different people cc'd on it, then you need to use two boolean charts.
-# This will find bugs with one CC matching 'foo@blah.org' and and another
-# CC matching 'bar@blah.org'.
-#
-# --------------------------------------------------------------
-# CC    | equal to
-# foo@blah.org
-# --------------------------------------------------------------
-# CC    | equal to
-# bar@blah.org
-#
-# If you try to do this query by pressing the AND button in the
-# original boolean chart then what you'll get is an expression that
-# looks for a single CC where the login name is both "foo@blah.org",
-# and "bar@blah.org". This is impossible.
-#
-# --------------------------------------------------------------
-# CC    | equal to
-# foo@blah.org
-# AND
-# CC    | equal to
-# bar@blah.org
-# --------------------------------------------------------------
-
-# $chartid is the number of the current chart whose SQL we're constructing
-# $row is the current row of the current chart
-
-# names for table aliases are constructed using $chartid and $row
-#   SELECT blah  FROM $table "$table_$chartid_$row" WHERE ....
-
-# $f  = field of table in bug db (e.g. bug_id, reporter, etc)
-# $ff = qualified field name (field name prefixed by table)
-#       e.g. bugs_activity.bug_id
-# $t  = type of query. e.g. "equal to", "changed after", case sensitive substr"
-# $v  = value - value the user typed in to the form
-# $q  = sanitized version of user input trick_taint(($dbh->quote($v)))
-# @supptables = Tables and/or table aliases used in query
-# %suppseen   = A hash used to store all the tables in supptables to weed
-#               out duplicates.
-# @supplist   = A list used to accumulate all the JOIN clauses for each
-#               chart to merge the ON sections of each.
-# $suppstring = String which is pasted into query containing all table names
-
-    my ($joins, $having, $where_terms) = $self->_charts_to_conditions();
-
-    my $query = "SELECT " . join(', ', $self->_sql_select) .
-                "\n  FROM " . $self->_sql_from($joins);
-
-    push(@$where_terms, 'bugs.creation_ts IS NOT NULL');
-
-    my $security_term = '(security_map.group_id IS NULL';
-
-    if ($user->id) {
-        my $userid = $user->id;
-        $security_term .= <<END;
- OR (bugs.reporter_accessible = 1 AND bugs.reporter = $userid)
- OR (bugs.cclist_accessible = 1 AND security_cc.who IS NOT NULL)
- OR bugs.assigned_to = $userid
-END
-        if (Bugzilla->params->{'useqacontact'}) {
-            $security_term.= " OR bugs.qa_contact = $userid";
-        }
-    }
-
-    $security_term .= ")";
-    push(@$where_terms, $security_term);
-    
-    $query .= "\n WHERE " . join(' AND ', @$where_terms) . ' '
-              . $dbh->sql_group_by($self->_sql_group_by);
-
-
-    if (@$having) {
-        $query .= " HAVING " . join(" AND ", @$having);
-    }
-
-    if ($self->_sql_order_by) {
-        $query .= " ORDER BY " . join(',', $self->_sql_order_by);
-    }
-
-    $self->{'sql'} = $query;
-}
-
-###############################################################################
-# Helper functions for the init() method.
-###############################################################################
-
 # This takes information about the current boolean chart and translates
 # it into SQL, using the constants at the top of this file.
 sub do_search_function {
@@ -1539,6 +1570,9 @@ sub _pick_override_function {
     return $search_func;
 }
 
+###########################
+# Search Function Helpers #
+###########################
 
 sub SqlifyDate {
     my ($str) = @_;
@@ -1633,20 +1667,6 @@ sub GetByWordListSubstr {
     return \@list;
 }
 
-sub getSQL {
-    my $self = shift;
-    return $self->{'sql'};
-}
-
-sub search_description {
-    my ($self, $params) = @_;
-    my $desc = $self->{'search_description'} ||= [];
-    if ($params) {
-        push(@$desc, $params);
-    }
-    return $self->{'search_description'};
-}
-
 sub pronoun {
     my ($noun, $user) = (@_);
     if ($noun eq "%user%") {
@@ -1668,6 +1688,10 @@ sub pronoun {
     return 0;
 }
 
+######################
+# Public Subroutines #
+######################
+
 # Validate that the query type is one we can deal with
 sub IsValidQueryType
 {
@@ -1725,7 +1749,7 @@ sub _invalid_combination {
 sub _contact_pronoun {
     my ($self, $args) = @_;
     my ($value, $quoted) = @$args{qw(value quoted)};
-    my $user = $self->{'user'};
+    my $user = $self->_user;
     
     if ($value =~ /^\%group/) {
         $self->_contact_exact_group($args);
@@ -1785,7 +1809,7 @@ sub _qa_contact_nonchanged {
 sub _cc_pronoun {
     my ($self, $args) = @_;
     my ($full_field, $value) = @$args{qw(full_field value)};
-    my $user = $self->{'user'};
+    my $user = $self->_user;
 
     if ($value =~ /\%group/) {
         return $self->_cc_exact_group($args);
@@ -1801,7 +1825,7 @@ sub _cc_exact_group {
     my ($self, $args) = @_;
     my ($chart_id, $sequence, $joins, $operator, $value) =
         @$args{qw(chart_id sequence joins operator value)};
-    my $user = $self->{'user'};
+    my $user = $self->_user;
     my $dbh = Bugzilla->dbh;
     
     $value =~ m/%group\.([^%]+)%/;
@@ -1912,7 +1936,7 @@ sub _content_matches {
     # Add the fulltext table to the query so we can search on it.
     my $table = "bugs_fulltext_$chart_id";
     my $comments_col = "comments";
-    $comments_col = "comments_noprivate" unless $self->{'user'}->is_insider;
+    $comments_col = "comments_noprivate" unless $self->_user->is_insider;
     push(@$joins, { table => 'bugs_fulltext', as => $table });
     
     # Create search terms to add to the SELECT and WHERE clauses.
@@ -1959,7 +1983,7 @@ sub _timestamp_translate {
 sub _commenter_pronoun {
     my ($self, $args) = @_;
     my $value = $args->{value};
-    my $user = $self->{'user'};
+    my $user = $self->_user;
 
     if ($value =~ /^(%\w+%)$/) {
         $args->{value} = pronoun($1, $user);
@@ -1979,7 +2003,7 @@ sub _commenter {
     }
     my $table = "longdescs_$chart_id";
     
-    my $extra = $self->{'user'}->is_insider ? "" : "AND $table.isprivate = 0";
+    my $extra = $self->_user->is_insider ? "" : "AND $table.isprivate = 0";
     # commenter_pronoun could have changed $full_field to something else,
     # so we only set this if commenter_pronoun hasn't set it.
     if ($full_field eq 'bugs.commenter') {
@@ -2001,7 +2025,7 @@ sub _long_desc {
     my ($chart_id, $joins) = @$args{qw(chart_id joins)};
     
     my $table = "longdescs_$chart_id";
-    my $extra = $self->{'user'}->is_insider ? [] : ["$table.isprivate = 0"];
+    my $extra = $self->_user->is_insider ? [] : ["$table.isprivate = 0"];
     my $join = {
         table => 'longdescs',
         as    => $table,
@@ -2016,7 +2040,7 @@ sub _longdescs_isprivate {
     my ($chart_id, $joins) = @$args{qw(chart_id joins)};
     
     my $table = "longdescs_$chart_id";
-    my $extra = $self->{'user'}->is_insider ? [] : ["$table.isprivate = 0"];
+    my $extra = $self->_user->is_insider ? [] : ["$table.isprivate = 0"];
     my $join = {
         table => 'longdescs',
         as    => $table,
@@ -2123,7 +2147,7 @@ sub _attach_data_thedata {
     
     my $attach_table = "attachments_$chart_id";
     my $data_table = "attachdata_$chart_id";
-    my $extra = $self->{'user'}->is_insider
+    my $extra = $self->_user->is_insider
                 ? [] : ["$attach_table.isprivate = 0"];
     my $attachments_join = {
         table => 'attachments',
@@ -2146,7 +2170,7 @@ sub _attachments_submitter {
     
     my $attach_table = "attachments_$chart_id";
     my $profiles_table = "map_attachment_submitter_$chart_id";    
-    my $extra = $self->{'user'}->is_insider
+    my $extra = $self->_user->is_insider
                 ? [] : ["$attach_table.isprivate = 0"];
     my $attachments_join = {
         table => 'attachments',
@@ -2171,7 +2195,7 @@ sub _attachments {
     my $dbh = Bugzilla->dbh;
     
     my $table = "attachments_$chart_id";
-    my $extra = $self->{'user'}->is_insider? [] : ["$table.isprivate = 0"];
+    my $extra = $self->_user->is_insider ? [] : ["$table.isprivate = 0"];
     my $join = {
         table => 'attachments',
         as    => $table,
@@ -2190,7 +2214,7 @@ sub _join_flag_tables {
     
     my $attach_table = "attachments_$chart_id";
     my $flags_table = "flags_$chart_id";
-    my $extra = $self->{'user'}->is_insider
+    my $extra = $self->_user->is_insider
                 ? [] : ["$attach_table.isprivate = 0"];
     my $attachments_join = {
         table => 'attachments',
diff --git a/buglist.cgi b/buglist.cgi
index 1972dd5b3..878d13d71 100755
--- a/buglist.cgi
+++ b/buglist.cgi
@@ -851,7 +851,7 @@ my @orderstrings = split(/,\s*/, $order);
 my $search = new Bugzilla::Search('fields' => \@selectcolumns, 
                                   'params' => $params,
                                   'order' => \@orderstrings);
-my $query = $search->getSQL();
+my $query = $search->sql;
 $vars->{'search_description'} = $search->search_description;
 
 if (defined $cgi->param('limit')) {
diff --git a/collectstats.pl b/collectstats.pl
index af055ab32..f090ba5fc 100755
--- a/collectstats.pl
+++ b/collectstats.pl
@@ -512,7 +512,7 @@ sub CollectSeriesData {
             my $search = new Bugzilla::Search('params' => $cgi,
                                               'fields' => ["bug_id"],
                                               'user'   => $user);
-            my $sql = $search->getSQL();
+            my $sql = $search->sql;
             $data = $shadow_dbh->selectall_arrayref($sql);
         };
 
diff --git a/report.cgi b/report.cgi
index 5d2679e1e..89f5ff674 100755
--- a/report.cgi
+++ b/report.cgi
@@ -128,7 +128,7 @@ my @axis_fields = ($row_field || EMPTY_COLUMN,
 my $params = new Bugzilla::CGI($cgi);
 my $search = new Bugzilla::Search('fields' => \@axis_fields, 
                                   'params' => $params);
-my $query = $search->getSQL();
+my $query = $search->sql;
 
 $::SIG{TERM} = 'DEFAULT';
 $::SIG{PIPE} = 'DEFAULT';
diff --git a/whine.pl b/whine.pl
index 1f22b65fc..789cea79e 100755
--- a/whine.pl
+++ b/whine.pl
@@ -449,7 +449,7 @@ sub run_queries {
             'params' => $searchparams,
             'user'   => $args->{'recipient'}, # the search runs as the recipient
         );
-        my $sqlquery = $search->getSQL();
+        my $sqlquery = $search->sql;
         $sth = $dbh->prepare($sqlquery);
         $sth->execute;
 
diff --git a/xt/lib/Bugzilla/Test/Search/FieldTest.pm b/xt/lib/Bugzilla/Test/Search/FieldTest.pm
index 7ebf760d1..4d24c5bd3 100644
--- a/xt/lib/Bugzilla/Test/Search/FieldTest.pm
+++ b/xt/lib/Bugzilla/Test/Search/FieldTest.pm
@@ -497,20 +497,17 @@ sub do_tests {
 
     my $search_broken = $self->search_known_broken;
     
-    my $search;
+    my $search = $self->_test_search_object_creation();
+    
+    my $sql;
     TODO: {
         local $TODO = $search_broken if $search_broken;
-        $search = $self->_test_search_object_creation();
+        lives_ok { $sql = $search->sql } "$name: generate SQL";
     }
     
-    my ($results, $sql);
+    my $results;
     SKIP: {
-        skip "Can't run SQL without Search object", 2 if !$search;
-        lives_ok { $sql = $search->getSQL() } "$name: get SQL";
-    
-        # This prevents warnings from DBD::mysql if we pass undef $sql,
-        # which happens if "new Bugzilla::Search" fails.
-        $sql ||= '';
+        skip "Can't run SQL without any SQL", 1 if !defined $sql;
         $results = $self->_test_sql($sql);
     }