Bug 328638: Remove @::legal_keywords and %::keywordsbyname

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=LpSolit, a=justdave

15 files changed, 259 insertions, 63 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 43f5a1285..c314b8ee1 100755
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -30,7 +30,7 @@ package Bugzilla::Bug;
 
 use strict;
 
-use vars qw($legal_keywords @legal_platform
+use vars qw(@legal_platform
             @legal_priority @legal_severity @legal_opsys @legal_bug_status
             @settable_resolution %components %target_milestone
             @enterable_products %milestoneurl %prodmaxvotes);
@@ -523,11 +523,6 @@ sub show_attachment_flags {
     return $self->{'show_attachment_flags'};
 }
 
-
-sub use_keywords {
-    return @::legal_keywords;
-}
-
 sub use_votes {
     my ($self) = @_;
     return 0 if $self->{'error'};
@@ -1337,7 +1332,7 @@ sub _validate_attribute {
            longdescs milestoneurl attachments
            isopened isunconfirmed
            flag_types num_attachment_flag_types
-           show_attachment_flags use_keywords any_flags_requesteeble),
+           show_attachment_flags any_flags_requesteeble),
 
         # Bug fields.
         Bugzilla::Bug->fields
diff --git a/Bugzilla/Keyword.pm b/Bugzilla/Keyword.pm
new file mode 100644
index 000000000..b35827288
--- /dev/null
+++ b/Bugzilla/Keyword.pm
@@ -0,0 +1,213 @@
+# -*- 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): Max Kanat-Alexander <mkanat@bugzilla.org>
+
+use strict;
+
+package Bugzilla::Keyword;
+
+use Bugzilla::Util;
+use Bugzilla::Error;
+
+###############################
+####    Initialization     ####
+###############################
+
+use constant DB_COLUMNS => qw(
+   keyworddefs.id
+   keyworddefs.name
+   keyworddefs.description
+);
+
+my $columns = join(", ", DB_COLUMNS);
+
+sub new {
+    my $invocant = shift;
+    my $class = ref($invocant) || $invocant;
+    my $self = {};
+    bless($self, $class);
+    return $self->_init(@_);
+}
+
+sub _init {
+    my $self = shift;
+    my ($param) = @_;
+    my $dbh = Bugzilla->dbh;
+
+    my $id = $param unless (ref $param eq 'HASH');
+    my $keyword;
+
+    if (defined $id) {
+        detaint_natural($id)
+          || ThrowCodeError('param_must_be_numeric',
+                            {function => 'Bugzilla::Keyword::_init'});
+
+        $keyword = $dbh->selectrow_hashref(qq{
+            SELECT $columns FROM keyworddefs
+             WHERE id = ?}, undef, $id);
+    } elsif (defined $param->{'name'}) {
+        trick_taint($param->{'name'});
+        $keyword = $dbh->selectrow_hashref(qq{
+            SELECT $columns FROM keyworddefs
+             WHERE name = ?}, undef, $param->{'name'});
+    } else {
+        ThrowCodeError('bad_arg',
+            {argument => 'param',
+             function => 'Bugzilla::Keyword::_init'});
+    }
+
+    return undef unless (defined $keyword);
+
+    foreach my $field (keys %$keyword) {
+        $self->{$field} = $keyword->{$field};
+    }
+    return $self;
+}
+
+sub new_from_list {
+    my $class = shift;
+    my ($id_list) = @_;
+    my $dbh = Bugzilla->dbh;
+
+    my $keywords;
+    if ($id_list) {
+        my @detainted_ids;
+        foreach my $id (@$id_list) {
+            detaint_natural($id) ||
+                ThrowCodeError('param_must_be_numeric',
+                              {function => 'Bugzilla::Keyword::new_from_list'});
+            push(@detainted_ids, $id);
+        }
+        $keywords = $dbh->selectall_arrayref(
+            "SELECT $columns FROM keyworddefs WHERE id IN (" 
+            . join(',', @detainted_ids) . ")", {Slice=>{}}) || [];
+    } else {
+        ThrowCodeError('bad_arg',
+            {argument => 'id_list',
+             function => 'Bugzilla::Keyword::new_from_list'});
+    }
+
+    foreach my $keyword (@$keywords) {
+        bless($keyword, $class);
+    }
+    return $keywords;
+}
+
+###############################
+####      Accessors      ######
+###############################
+
+sub id                { return $_[0]->{'id'};          }
+sub name              { return $_[0]->{'name'};        }
+sub description       { return $_[0]->{'description'}; }
+
+###############################
+####      Subroutines    ######
+###############################
+
+sub get_all_keywords {
+    my $dbh = Bugzilla->dbh;
+
+    my $ids = $dbh->selectcol_arrayref(q{
+        SELECT id FROM keyworddefs ORDER BY name});
+
+    my $keywords = Bugzilla::Keyword->new_from_list($ids);
+    return @$keywords;
+}
+
+sub keyword_count {
+    my ($count) = 
+        Bugzilla->dbh->selectrow_array('SELECT COUNT(*) FROM keyworddefs');
+    return $count;
+}
+
+1;
+
+__END__
+
+=head1 NAME
+
+Bugzilla::Keyword - A Keyword that can be added to a bug.
+
+=head1 SYNOPSIS
+
+ use Bugzilla::Keyword;
+
+ my $keyword = new Bugzilla::Keyword(1);
+ my $keyword = new Bugzilla::Keyword({name => 'perf'});
+
+ my $id          = $keyword->id;
+ my $name        = $keyword->name;
+ my $description = $keyword->description;
+
+=head1 DESCRIPTION
+
+Bugzilla::Keyword represents a keyword that can be added to a bug.
+
+=head1 METHODS
+
+=over
+
+=item C<new($param)>
+
+ Description: The constructor is used to load an existing keyword
+              by passing a keyword id or a hash.
+
+ Params:      $param - If you pass an integer, the integer is the
+                       keyword id from the database that we want to
+                       read in. If you pass in a hash with 'name' key,
+                       then the value of the name key is the name of a
+                       keyword from the DB.
+
+ Returns:     A Bugzilla::Keyword object.
+
+=item C<new_from_list(\@id_list)>
+
+ Description: Creates an array of Keyword objects, given an
+              array of ids.
+
+ Params:      \@id_list - A reference to an array of numbers, keyword ids.
+                          If any of these are not numeric, the function
+                          will throw an error. If any of these are not
+                          valid keyword ids, they will simply be skipped.
+
+ Returns:     A reference to an array of C<Bugzilla::Keyword> objects.
+
+=back
+
+=head1 SUBROUTINES
+
+=over
+
+=item C<get_all_keywords()>
+
+ Description: Returns all keywords from the database.
+
+ Params:      none.
+
+ Returns:     A list of C<Bugzilla::Keyword> objects,
+              or an empty list if there are none.
+
+=item C<keyword_count()> 
+
+ Description: A utility function to get the total number
+              of keywords defined. Mostly used to see
+              if there are any keywords defined at all.
+ Params:      none
+ Returns:     An integer, the count of keywords.
+
+=back
+
+=cut
diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm
index 0e1f7057d..181f49b19 100644
--- a/Bugzilla/Search.pm
+++ b/Bugzilla/Search.pm
@@ -42,6 +42,7 @@ use Bugzilla::Group;
 use Bugzilla::User;
 use Bugzilla::Field;
 use Bugzilla::Bug;
+use Bugzilla::Keyword;
 
 use Date::Format;
 use Date::Parse;
@@ -933,9 +934,9 @@ sub init {
                  if ($value eq '') {
                      next;
                  }
-                 my $id = &::GetKeywordIdFromName($value);
-                 if ($id) {
-                     push(@list, "$table.keywordid = $id");
+                 my $keyword = new Bugzilla::Keyword({name => $value});
+                 if ($keyword) {
+                     push(@list, "$table.keywordid = " . $keyword->id);
                  }
                  else {
                      ThrowUserError("unknown_keyword",
diff --git a/Bugzilla/Search/Quicksearch.pm b/Bugzilla/Search/Quicksearch.pm
index 4d9958c63..e4224b959 100644
--- a/Bugzilla/Search/Quicksearch.pm
+++ b/Bugzilla/Search/Quicksearch.pm
@@ -27,6 +27,7 @@ use Bugzilla;
 use Bugzilla::Config;
 use Bugzilla::Error;
 use Bugzilla::Constants;
+use Bugzilla::Keyword;
 use Bugzilla::Bug;
 
 use base qw(Exporter);
@@ -325,7 +326,7 @@ sub quicksearch {
                                              $word, $negate);
                                 }
                                 if (grep({lc($word) eq $_}
-                                         @::legal_keywords)) {
+                                         map($_->name, Bugzilla::Keyword::get_all_keywords()))) {
                                     addChart('keywords', 'substring',
                                              $word, $negate);
                                     if (length($word)>2) {
diff --git a/buglist.cgi b/buglist.cgi
index c778c5f10..0e6fb2ba5 100755
--- a/buglist.cgi
+++ b/buglist.cgi
@@ -41,12 +41,12 @@ use Bugzilla::Constants;
 use Bugzilla::User;
 use Bugzilla::Bug;
 use Bugzilla::Product;
+use Bugzilla::Keyword;
 
 # Include the Bugzilla CGI and general utility library.
 require "globals.pl";
 
 use vars qw(@components
-            @legal_keywords
             @legal_platform
             @legal_priority
             @legal_product
@@ -1042,7 +1042,7 @@ $vars->{'currenttime'} = time();
 # The following variables are used when the user is making changes to multiple bugs.
 if ($dotweak) {
     $vars->{'dotweak'} = 1;
-    $vars->{'use_keywords'} = 1 if @::legal_keywords;
+    $vars->{'use_keywords'} = 1 if Bugzilla::Keyword::keyword_count();
 
     $vars->{'products'} = Bugzilla->user->get_enterable_products;
     $vars->{'platforms'} = \@::legal_platform;
diff --git a/colchange.cgi b/colchange.cgi
index 43bfd86ee..72ce01f44 100755
--- a/colchange.cgi
+++ b/colchange.cgi
@@ -25,13 +25,10 @@ use strict;
 
 use lib qw(.);
 
-use vars qw(
-  @legal_keywords
-);
-
 use Bugzilla;
 use Bugzilla::Constants;
 use Bugzilla::User;
+use Bugzilla::Keyword;
 require "globals.pl";
 
 Bugzilla->login();
@@ -71,7 +68,7 @@ if (Param("useqacontact")) {
 if (Param("usestatuswhiteboard")) {
     push(@masterlist, "status_whiteboard");
 }
-if (@::legal_keywords) {
+if (Bugzilla::Keyword::keyword_count()) {
     push(@masterlist, "keywords");
 }
 
diff --git a/config.cgi b/config.cgi
index 371383e1c..f3471e14f 100755
--- a/config.cgi
+++ b/config.cgi
@@ -33,6 +33,7 @@ use lib qw(.);
 require "globals.pl";
 use Bugzilla;
 use Bugzilla::Constants;
+use Bugzilla::Keyword;
 use Bugzilla::Bug;
 
 # Suppress "used only once" warnings.
@@ -46,7 +47,6 @@ use vars
 
     @legal_components 
     @legal_target_milestone 
-    @legal_keywords 
   );
 
 # Use the global template variables defined in globals.pl 
@@ -69,7 +69,7 @@ $vars->{'priority'}  = \@::legal_priority;
 $vars->{'severity'}  = \@::legal_severity;
 $vars->{'platform'}   = \@::legal_platform;
 $vars->{'op_sys'}    = \@::legal_opsys;
-$vars->{'keyword'}    = \@::legal_keywords;
+$vars->{'keyword'}    = [map($_->name, Bugzilla::Keyword::get_all_keywords())];
 $vars->{'resolution'} = \@::legal_resolution;
 $vars->{'status'}    = \@::legal_bug_status;
 
diff --git a/enter_bug.cgi b/enter_bug.cgi
index 3e87bf405..d1d070148 100755
--- a/enter_bug.cgi
+++ b/enter_bug.cgi
@@ -43,6 +43,7 @@ use Bugzilla::Bug;
 use Bugzilla::User;
 use Bugzilla::Hook;
 use Bugzilla::Product;
+use Bugzilla::Keyword;
 require "globals.pl";
 
 use vars qw(
@@ -51,7 +52,6 @@ use vars qw(
   @legal_platform
   @legal_priority
   @legal_severity
-  @legal_keywords
   %target_milestone
 );
 
@@ -375,7 +375,7 @@ $vars->{'bug_severity'}          = \@legal_severity;
 $vars->{'rep_platform'}          = \@legal_platform;
 $vars->{'op_sys'}                = \@legal_opsys; 
 
-$vars->{'use_keywords'}          = 1 if (@::legal_keywords);
+$vars->{'use_keywords'}          = 1 if Bugzilla::Keyword::keyword_count();
 
 $vars->{'assigned_to'}           = formvalue('assigned_to');
 $vars->{'assigned_to_disabled'}  = !UserInGroup('editbugs');
diff --git a/globals.pl b/globals.pl
index be3df7d75..556ea3f3e 100644
--- a/globals.pl
+++ b/globals.pl
@@ -45,10 +45,8 @@ use Bugzilla::Error;
 sub globals_pl_sillyness {
     my $zz;
     $zz = @main::enterable_products;
-    $zz = %main::keywordsbyname;
     $zz = @main::legal_bug_status;
     $zz = @main::legal_components;
-    $zz = @main::legal_keywords;
     $zz = @main::legal_opsys;
     $zz = @main::legal_platform;
     $zz = @main::legal_priority;
@@ -258,17 +256,6 @@ sub GenerateVersionTable {
                                        '*::milestoneurl']));
     }
 
-    SendSQL("SELECT id, name FROM keyworddefs ORDER BY name");
-    while (MoreSQLData()) {
-        my ($id, $name) = FetchSQLData();
-        push(@::legal_keywords, $name);
-        $name = lc($name);
-        $::keywordsbyname{$name} = $id;
-    }
-
-    print $fh (Data::Dumper->Dump([\@::legal_keywords, \%::keywordsbyname],
-                                  ['*::legal_keywords', '*::keywordsbyname']));
-
     print $fh "1;\n";
     close $fh;
 
@@ -278,13 +265,6 @@ sub GenerateVersionTable {
 }
 
 
-sub GetKeywordIdFromName {
-    my ($name) = (@_);
-    $name = lc($name);
-    return $::keywordsbyname{$name};
-}
-
-
 $::VersionTableLoaded = 0;
 sub GetVersionTable {
     return if $::VersionTableLoaded;
diff --git a/importxml.pl b/importxml.pl
index 7b96cb59e..494a00eca 100755
--- a/importxml.pl
+++ b/importxml.pl
@@ -84,6 +84,7 @@ use Bugzilla::BugMail;
 use Bugzilla::User;
 use Bugzilla::Util;
 use Bugzilla::Constants;
+use Bugzilla::Keyword;
 
 use MIME::Base64;
 use MIME::Parser;
@@ -1070,14 +1071,14 @@ sub process_bug {
         );
         foreach my $keyword ( split( /[\s,]+/, $bug_fields{'keywords'} )) {
             next unless $keyword;
-            my $i = GetKeywordIdFromName($keyword);
-            if ( !$i ) {
+            my $keyword_obj = new Bugzilla::Keyword({name => $keyword});
+            if (!$keyword_obj) {
                 $err .= "Skipping unknown keyword: $keyword.\n";
                 next;
             }
-            if ( !$keywordseen{$i} ) {
-                $key_sth->execute( $id, $i );
-                $keywordseen{$i} = 1;
+            if (!$keywordseen{$keyword_obj->id}) {
+                $key_sth->execute($id, $keyword_obj->id);
+                $keywordseen{$keyword_obj->id} = 1;
             }
         }
         my ($keywordarray) = $dbh->selectcol_arrayref(
diff --git a/post_bug.cgi b/post_bug.cgi
index 3e589ebe8..de8063543 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -34,6 +34,7 @@ use Bugzilla::Bug;
 use Bugzilla::User;
 use Bugzilla::Field;
 use Bugzilla::Product;
+use Bugzilla::Keyword;
 
 # Shut up misguided -w warnings about "used only once". For some reason,
 # "use vars" chokes on me when I try it here.
@@ -273,14 +274,14 @@ if ($cgi->param('keywords') && UserInGroup("editbugs")) {
         if ($keyword eq '') {
            next;
         }
-        my $i = GetKeywordIdFromName($keyword);
-        if (!$i) {
+        my $keyword_obj = new Bugzilla::Keyword({name => $keyword});
+        if (!$keyword_obj) {
             ThrowUserError("unknown_keyword",
                            { keyword => $keyword });
         }
-        if (!$keywordseen{$i}) {
-            push(@keywordlist, $i);
-            $keywordseen{$i} = 1;
+        if (!$keywordseen{$keyword_obj->id}) {
+            push(@keywordlist, $keyword_obj->id);
+            $keywordseen{$keyword_obj->id} = 1;
         }
     }
 }
@@ -518,6 +519,7 @@ if ($cgi->cookie("BUGLIST")) {
     @bug_list = split(/:/, $cgi->cookie("BUGLIST"));
 }
 $vars->{'bug_list'} = \@bug_list;
+$vars->{'use_keywords'} = 1 if Bugzilla::Keyword::keyword_count();
 
 print $cgi->header();
 $template->process("bug/create/created.html.tmpl", $vars)
diff --git a/process_bug.cgi b/process_bug.cgi
index c2605afc2..d1efd3825 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -57,6 +57,7 @@ use Bugzilla::User;
 use Bugzilla::Util;
 use Bugzilla::Field;
 use Bugzilla::Product;
+use Bugzilla::Keyword;
 
 # Use the Flag module to modify flag data if the user set flags.
 use Bugzilla::Flag;
@@ -82,6 +83,7 @@ my $cgi = Bugzilla->cgi;
 my $dbh = Bugzilla->dbh;
 my $template = Bugzilla->template;
 my $vars = {};
+$vars->{'use_keywords'} = 1 if Bugzilla::Keyword::keyword_count();
 
 my $requiremilestone = 0;
 
@@ -1278,14 +1280,14 @@ if (defined $cgi->param('keywords')) {
         if ($keyword eq '') {
             next;
         }
-        my $i = GetKeywordIdFromName($keyword);
-        if (!$i) {
+        my $keyword_obj = new Bugzilla::Keyword({name => $keyword});
+        if (!$keyword_obj) {
             ThrowUserError("unknown_keyword",
                            { keyword => $keyword });
         }
-        if (!$keywordseen{$i}) {
-            push(@keywordlist, $i);
-            $keywordseen{$i} = 1;
+        if (!$keywordseen{$keyword_obj->id}) {
+            push(@keywordlist, $keyword_obj->id);
+            $keywordseen{$keyword_obj->id} = 1;
         }
     }
 }
@@ -1297,7 +1299,8 @@ if (!grep($keywordaction eq $_, qw(add delete makeexact))) {
 
 if ($::comma eq ""
     && (! @groupAdd) && (! @groupDel)
-    && (! @::legal_keywords || (0 == @keywordlist && $keywordaction ne "makeexact"))
+    && (!Bugzilla::Keyword::keyword_count() 
+        || (0 == @keywordlist && $keywordaction ne "makeexact"))
     && defined $cgi->param('masscc') && ! $cgi->param('masscc')
     ) {
     if (!defined $cgi->param('comment') || $cgi->param('comment') =~ /^\s*$/) {
@@ -1651,7 +1654,9 @@ foreach my $id (@idlist) {
         $bug_changed = 1;
     }
 
-    if (@::legal_keywords && defined $cgi->param('keywords')) {
+    if (Bugzilla::Keyword::keyword_count() 
+        && defined $cgi->param('keywords')) 
+    {
         # There are three kinds of "keywordsaction": makeexact, add, delete.
         # For makeexact, we delete everything, and then add our things.
         # For add, we delete things we're adding (to make sure we don't
diff --git a/query.cgi b/query.cgi
index 33f2d19fa..9bb90e782 100755
--- a/query.cgi
+++ b/query.cgi
@@ -36,12 +36,12 @@ use Bugzilla::User;
 use Bugzilla::Util;
 use Bugzilla::Product;
 use Bugzilla::Version;
+use Bugzilla::Keyword;
 
 use vars qw(
     @legal_resolution
     @legal_bug_status
     @legal_components
-    @legal_keywords
     @legal_opsys
     @legal_platform
     @legal_priority
@@ -275,7 +275,7 @@ if (Param('usetargetmilestone')) {
     $vars->{'target_milestone'} = \@milestones;
 }
 
-$vars->{'have_keywords'} = scalar(@::legal_keywords);
+$vars->{'have_keywords'} = Bugzilla::Keyword::keyword_count();
 
 push @::legal_resolution, "---"; # Oy, what a hack.
 shift @::legal_resolution; 
diff --git a/show_bug.cgi b/show_bug.cgi
index 19aa88573..7f846efea 100755
--- a/show_bug.cgi
+++ b/show_bug.cgi
@@ -27,6 +27,7 @@ use lib qw(.);
 use Bugzilla;
 use Bugzilla::Constants;
 use Bugzilla::User;
+use Bugzilla::Keyword;
 
 require "globals.pl";
 
@@ -94,7 +95,7 @@ eval {
 
 $vars->{'bugs'} = \@bugs;
 $vars->{'marks'} = \%marks;
-$vars->{'use_keywords'} = 1 if (@::legal_keywords);
+$vars->{'use_keywords'} = 1 if Bugzilla::Keyword::keyword_count();
 
 # Next bug in list (if there is one)
 my @bug_list;
diff --git a/template/en/default/bug/edit.html.tmpl b/template/en/default/bug/edit.html.tmpl
index 02fb20d61..b08ce6432 100644
--- a/template/en/default/bug/edit.html.tmpl
+++ b/template/en/default/bug/edit.html.tmpl
@@ -394,7 +394,7 @@
             </tr>
           [% END %]
 
-          [% IF bug.use_keywords %]
+          [% IF use_keywords %]
             <tr>
               <td align="right">
                 <b>