Bug 328602: Eliminate %::versions and @::legal_versions

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

10 files changed, 69 insertions, 97 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index e2222ccb7..f297ec681 100755
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -32,7 +32,7 @@ use strict;
 
 use vars qw($legal_keywords @legal_platform
             @legal_priority @legal_severity @legal_opsys @legal_bug_status
-            @settable_resolution %components %versions %target_milestone
+            @settable_resolution %components %target_milestone
             @enterable_products %milestoneurl %prodmaxvotes);
 
 use CGI::Carp qw(fatalsToBrowser);
@@ -46,6 +46,7 @@ use Bugzilla::FlagType;
 use Bugzilla::User;
 use Bugzilla::Util;
 use Bugzilla::Error;
+use Bugzilla::Product;
 
 use base qw(Exporter);
 @Bugzilla::Bug::EXPORT = qw(
@@ -641,6 +642,7 @@ sub choices {
     &::GetVersionTable();
 
     $self->{'choices'} = {};
+    $self->{prod_obj} ||= new Bugzilla::Product({name => $self->{product}});
 
     # Fiddle the product list.
     my $seen_curr_prod;
@@ -686,7 +688,7 @@ sub choices {
        'bug_status' => \@::legal_bug_status,
        'resolution' => \@res,
        'component' => $::components{$self->{product}},
-       'version' => $::versions{$self->{product}},
+       'version' => [map($_->name, @{$self->{prod_obj}->versions})],
        'target_milestone' => $::target_milestone{$self->{product}},
       };
 
diff --git a/Bugzilla/Version.pm b/Bugzilla/Version.pm
index 9492e8135..53bf9a678 100644
--- a/Bugzilla/Version.pm
+++ b/Bugzilla/Version.pm
@@ -109,6 +109,13 @@ sub check_version {
     return $version;
 }
 
+sub distinct_names {
+    my $dbh = Bugzilla->dbh;
+    my $names = $dbh->selectcol_arrayref(
+        'SELECT DISTINCT value FROM versions ORDER BY value');
+    return @$names;
+}
+
 1;
 
 __END__
@@ -172,6 +179,17 @@ Version.pm represents a Product Version object.
 
  Returns:     Bugzilla::Version object.
 
+=item C<distinct_names()>
+
+ Description: A utility function for getting all the
+              possible version values from the database,
+              regardless of what product they're in.
+              Returns a list with no duplicate versions.
+
+ Params:      none
+
+ Returns:     A list of strings (versions).
+
 =back
 
 =cut
diff --git a/buglist.cgi b/buglist.cgi
index ba105eab8..7b66f14e8 100755
--- a/buglist.cgi
+++ b/buglist.cgi
@@ -40,6 +40,7 @@ use Bugzilla::Search::Quicksearch;
 use Bugzilla::Constants;
 use Bugzilla::User;
 use Bugzilla::Bug;
+use Bugzilla::Product;
 
 # Include the Bugzilla CGI and general utility library.
 require "globals.pl";
@@ -51,8 +52,7 @@ use vars qw(@components
             @legal_product
             @legal_severity
             @settable_resolution
-            @target_milestone
-            @versions);
+            @target_milestone);
 
 my $cgi = Bugzilla->cgi;
 my $dbh = Bugzilla->dbh;
@@ -1062,10 +1062,12 @@ if ($dotweak) {
     # products), and a list of components for the product.
     $vars->{'bugproducts'} = [ keys %$bugproducts ];
     if (scalar(@{$vars->{'bugproducts'}}) == 1) {
-        my $product = $vars->{'bugproducts'}->[0];
-        $vars->{'versions'} = $::versions{$product};
-        $vars->{'components'} = $::components{$product};
-        $vars->{'targetmilestones'} = $::target_milestone{$product} if Param('usetargetmilestone');
+        my $product = new Bugzilla::Product(
+            {name => $vars->{'bugproducts'}->[0]});
+        $vars->{'versions'} = [map($_->name ,@{$product->versions})];
+        $vars->{'components'} = [map($_->name, @{$product->components})];
+        $vars->{'targetmilestones'} = [map($_->name, @{$product->milestones})]
+            if Param('usetargetmilestone');
     }
 }
 
diff --git a/config.cgi b/config.cgi
index bea4f6faa..c8c8ec887 100755
--- a/config.cgi
+++ b/config.cgi
@@ -45,7 +45,6 @@ use vars
 
     @legal_components 
     @legal_target_milestone 
-    @legal_versions 
     @legal_keywords 
   );
 
diff --git a/enter_bug.cgi b/enter_bug.cgi
index c5680fff6..3e87bf405 100755
--- a/enter_bug.cgi
+++ b/enter_bug.cgi
@@ -52,7 +52,6 @@ use vars qw(
   @legal_priority
   @legal_severity
   @legal_keywords
-  %versions
   %target_milestone
 );
 
@@ -467,7 +466,7 @@ else {
 #
 # Eventually maybe each product should have a "current version"
 # parameter.
-$vars->{'version'} = $::versions{$product} || [];
+$vars->{'version'} = [map($_->name, @{$prod_obj->versions})];
 
 if ( ($cloned_bug_id) &&
      ("$product" eq "$cloned_bug->{'product'}" ) ) {
diff --git a/globals.pl b/globals.pl
index 7242415e4..d346afba4 100644
--- a/globals.pl
+++ b/globals.pl
@@ -55,7 +55,6 @@ sub globals_pl_sillyness {
     $zz = @main::legal_product;
     $zz = @main::legal_severity;
     $zz = @main::legal_target_milestone;
-    $zz = @main::legal_versions;
     $zz = @main::milestoneurl;
     $zz = @main::prodmaxvotes;
 }
@@ -107,21 +106,7 @@ $::SIG{PIPE} = 'IGNORE';
 sub GenerateVersionTable {
     my $dbh = Bugzilla->dbh;
 
-    SendSQL("SELECT versions.value, products.name " .
-            "FROM versions, products " .
-            "WHERE products.id = versions.product_id " .
-            "ORDER BY versions.value");
-    my @line;
-    my %varray;
-    my %carray;
-    while (@line = FetchSQLData()) {
-        my ($v,$p1) = (@line);
-        if (!defined $::versions{$p1}) {
-            $::versions{$p1} = [];
-        }
-        push @{$::versions{$p1}}, $v;
-        $varray{$v} = 1;
-    }
+    my (@line, %carray);
     SendSQL("SELECT components.name, products.name " .
             "FROM components, products " .
             "WHERE products.id = components.product_id " .
@@ -202,8 +187,7 @@ sub GenerateVersionTable {
         splice(@::settable_resolution, $z, 1);
     }
 
-    my @list = sort { uc($a) cmp uc($b)} keys(%::versions);
-    @::legal_product = @list;
+    @::legal_product = map($_->name, Bugzilla::Product::get_all_products());
 
     require File::Temp;
     my ($fh, $tmpname) = File::Temp::tempfile("versioncache.XXXXX",
@@ -217,17 +201,16 @@ sub GenerateVersionTable {
     print $fh "#\n";
 
     require Data::Dumper;
-    print $fh (Data::Dumper->Dump([\@::log_columns, \%::versions],
-                                  ['*::log_columns', '*::versions']));
+    print $fh (Data::Dumper->Dump([\@::log_columns],
+                                  ['*::log_columns']));
 
-    foreach my $i (@list) {
+    foreach my $i (@::legal_product) {
         if (!defined $::components{$i}) {
             $::components{$i} = [];
         }
     }
-    @::legal_versions = sort {uc($a) cmp uc($b)} keys(%varray);
-    print $fh (Data::Dumper->Dump([\@::legal_versions, \%::components],
-                                  ['*::legal_versions', '*::components']));
+    print $fh (Data::Dumper->Dump([\%::components],
+                                  ['*::components']));
     @::legal_components = sort {uc($a) cmp uc($b)} keys(%carray);
 
     print $fh (Data::Dumper->Dump([\@::legal_components, \@::legal_product,
@@ -311,11 +294,11 @@ sub GetVersionTable {
         $file_generated = 1;
     }
     require "$datadir/versioncache";
-    if (!defined %::versions && !$file_generated) {
+    if (!defined @::legal_keywords && !$file_generated) {
         GenerateVersionTable();
         do "$datadir/versioncache";
     }
-    if (!defined %::versions) {
+    if (!defined @::legal_keywords) {
         die "Can't generate file $datadir/versioncache";
     }
     $::VersionTableLoaded = 1;
diff --git a/post_bug.cgi b/post_bug.cgi
index 50ef6ecef..3e589ebe8 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -33,13 +33,13 @@ use Bugzilla::Util;
 use Bugzilla::Bug;
 use Bugzilla::User;
 use Bugzilla::Field;
+use Bugzilla::Product;
 
 # Shut up misguided -w warnings about "used only once". For some reason,
 # "use vars" chokes on me when I try it here.
 sub sillyness {
     my $zz;
     $zz = %::components;
-    $zz = %::versions;
     $zz = @::legal_opsys;
     $zz = @::legal_platform;
     $zz = @::legal_priority;
@@ -100,7 +100,8 @@ ValidateComment($comment);
 my $product = $cgi->param('product');
 $user->can_enter_product($product, 1);
 
-my $product_id = get_product_id($product);
+my $prod_obj = new Bugzilla::Product({name => $product});
+my $product_id = $prod_obj->id;
 
 # Set cookies
 if (defined $cgi->param('product')) {
@@ -223,7 +224,8 @@ check_field('bug_severity', scalar $cgi->param('bug_severity'), \@::legal_severi
 check_field('priority',     scalar $cgi->param('priority'),     \@::legal_priority);
 check_field('op_sys',       scalar $cgi->param('op_sys'),       \@::legal_opsys);
 check_field('bug_status',   scalar $cgi->param('bug_status'),   ['UNCONFIRMED', 'NEW']);
-check_field('version',      scalar $cgi->param('version'),      $::versions{$product});
+check_field('version',      scalar $cgi->param('version'),
+            [map($_->name, @{$prod_obj->versions})]);
 check_field('component',    scalar $cgi->param('component'),    $::components{$product});
 check_field('target_milestone', scalar $cgi->param('target_milestone'),
             $::target_milestone{$product});
diff --git a/process_bug.cgi b/process_bug.cgi
index d37ec5231..0e5ec9427 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -56,6 +56,7 @@ use Bugzilla::BugMail;
 use Bugzilla::User;
 use Bugzilla::Util;
 use Bugzilla::Field;
+use Bugzilla::Product;
 
 # Use the Flag module to modify flag data if the user set flags.
 use Bugzilla::Flag;
@@ -64,7 +65,6 @@ use Bugzilla::FlagType;
 # Shut up misguided -w warnings about "used only once":
 
 use vars qw(@legal_product
-          %versions
           %components
           %legal_opsys
           %legal_platform
@@ -308,6 +308,7 @@ if (((defined $cgi->param('id') && $cgi->param('product') ne $oldproduct)
     }
 
     my $prod = $cgi->param('product');
+    my $prod_obj = new Bugzilla::Product({name => $prod});
     trick_taint($prod);
 
     # If at least one bug does not belong to the product we are
@@ -335,7 +336,8 @@ if (((defined $cgi->param('id') && $cgi->param('product') ne $oldproduct)
     # pretty weird case, and not terribly unreasonable behavior, but 
     # worthy of a comment, perhaps.
     #
-    my $vok = lsearch($::versions{$prod}, $cgi->param('version')) >= 0;
+    my @version_names = map($_->name, @{$prod_obj->versions});
+    my $vok = lsearch(\@version_names, $cgi->param('version')) >= 0;
     my $cok = lsearch($::components{$prod}, $cgi->param('component')) >= 0;
 
     my $mok = 1;   # so it won't affect the 'if' statement if milestones aren't used
@@ -359,7 +361,7 @@ if (((defined $cgi->param('id') && $cgi->param('product') ne $oldproduct)
             # We set the defaults to these fields to the old value,
             # if its a valid option, otherwise we use the default where
             # that's appropriate
-            $vars->{'versions'} = $::versions{$prod};
+            $vars->{'versions'} = \@version_names;
             if ($vok) {
                 $defaults{'version'} = $cgi->param('version');
             }
@@ -612,11 +614,11 @@ if (defined $cgi->param('id')) {
     # values that have been changed instead of submitting all the new values.
     # (XXX those error checks need to happen too, but implementing them 
     # is more work in the current architecture of this script...)
-    check_field('product', scalar $cgi->param('product'), \@::legal_product);
+    my $prod_obj = Bugzilla::Product::check_product($cgi->param('product'));
     check_field('component', scalar $cgi->param('component'), 
                 \@{$::components{$cgi->param('product')}});
     check_field('version', scalar $cgi->param('version'),
-                \@{$::versions{$cgi->param('product')}});
+                [map($_->name, @{$prod_obj->versions})]);
     if ( Param("usetargetmilestone") ) {
         check_field('target_milestone', scalar $cgi->param('target_milestone'), 
                     \@{$::target_milestone{$cgi->param('product')}});
diff --git a/query.cgi b/query.cgi
index 9576966e1..33f2d19fa 100755
--- a/query.cgi
+++ b/query.cgi
@@ -34,6 +34,8 @@ use Bugzilla::Constants;
 use Bugzilla::Search;
 use Bugzilla::User;
 use Bugzilla::Util;
+use Bugzilla::Product;
+use Bugzilla::Version;
 
 use vars qw(
     @legal_resolution
@@ -46,9 +48,7 @@ use vars qw(
     @legal_product
     @legal_severity
     @legal_target_milestone
-    @legal_versions
     @log_columns
-    %versions
     %components
 );
 
@@ -220,34 +220,24 @@ GetVersionTable();
 # if using groups for entry, then we don't want people to see products they 
 # don't have access to. Remove them from the list.
 
-my @selectable_product_objects = @{$user->get_selectable_products};
+my @selectable_products = sort {lc($a->name) cmp lc($b->name)} 
+                               @{$user->get_selectable_products};
 
 my %component_set;
 my %version_set;
 my %milestone_set;
-# extract product names
-my @products = map { $_->name } @selectable_product_objects;
 
-foreach my $prod_name (@products) {
+foreach my $prod_obj (@selectable_products) {
     # We build up boolean hashes in the "-set" hashes for each of these things 
     # before making a list because there may be duplicates names across products.
-    if ($::components{$prod_name}) {
-        foreach my $c (@{$::components{$prod_name}}) {
-            $component_set{$c} = 1;
-        }
-    }
-    foreach my $v (@{$::versions{$prod_name}}) {
-        $version_set{$v} = 1;
-    }
-    foreach my $m (@{$::target_milestone{$prod_name}}) {
-        $milestone_set{$m} = 1;
-    }
+    my @component_names = map($_->name, @{$prod_obj->components});
+    my @version_names   = map($_->name, @{$prod_obj->versions});
+    my @milestone_names = map($_->name, @{$prod_obj->milestones});
+    $component_set{$_} = 1 foreach (@component_names);
+    $version_set{$_}   = 1 foreach (@version_names);
+    $milestone_set{$_} = 1 foreach (@milestone_names);
 }
 
-# @products is now all the products we are ever concerned with, as a list
-# %x_set is now a unique "list" of the relevant components/versions/tms
-@products = sort { lc($a) cmp lc($b) } @products;
-
 # Create the component, version and milestone lists.
 my @components = ();
 my @versions = ();
@@ -257,7 +247,8 @@ foreach my $c (@::legal_components) {
         push @components, $c;
     }
 }
-foreach my $v (@::legal_versions) {
+my @all_versions = Bugzilla::Version::distinct_names();
+foreach my $v (@all_versions) {
     if ($version_set{$v}) {
         push @versions, $v;
     }
@@ -268,33 +259,7 @@ foreach my $m (@::legal_target_milestone) {
     }
 }
 
-# Create data structures representing each product.
-for (my $i = 0; $i < @products; ++$i) {
-    my $p = $products[$i];
-    
-    # Bug 190611: band-aid to avoid crashing with no versions defined
-    if (!defined ($::components{$p})) {
-        $::components{$p} = [];
-    }
-    
-    # Create hash to hold attributes for each product.
-    my %product = (
-        'name'       => $p,
-        'components' => [ sort { lc($a) cmp lc($b) } @{$::components{$p}} ],
-        'versions'   => [ sort { lc($a) cmp lc($b) } @{$::versions{$p}}   ]
-    );
-    
-    if (Param('usetargetmilestone')) {
-        # Sorting here is required for ordering multiple selections 
-        # correctly; see bug 97736 for discussion on how to fix this
-        $product{'milestones'} =  
-                      [ sort { lc($a) cmp lc($b) } @{$::target_milestone{$p}} ];
-    }
-    
-    # Assign hash back to product array.
-    $products[$i] = \%product;
-}
-$vars->{'product'} = \@products;
+$vars->{'product'} = \@selectable_products;
 
 # Create data structures representing each classification
 if (Param('useclassification')) {
diff --git a/template/en/default/search/form.html.tmpl b/template/en/default/search/form.html.tmpl
index daef7e58d..6b0a5a2c8 100644
--- a/template/en/default/search/form.html.tmpl
+++ b/template/en/default/search/form.html.tmpl
@@ -62,12 +62,12 @@ var tms = new Array();
   prods['[% p.name FILTER js %]'] = [% n %]
   [% END %]
   cpts[[% n %]] = [
-    [%- FOREACH item = p.components %]'[% item FILTER js %]'[% ", " UNLESS loop.last %] [%- END -%] ];
+    [%- FOREACH item = p.components %]'[% item.name FILTER js %]'[% ", " UNLESS loop.last %] [%- END -%] ];
   vers[[% n %]] = [
-    [%- FOREACH item = p.versions -%]'[%  item FILTER js %]'[% ", " UNLESS loop.last %] [%- END -%] ];
+    [%- FOREACH item = p.versions -%]'[%  item.name FILTER js %]'[% ", " UNLESS loop.last %] [%- END -%] ];
   [% IF Param('usetargetmilestone') %]
   tms[[% n %]]  = [
-     [%- FOREACH item = p.milestones %]'[% item FILTER js %]'[% ", " UNLESS loop.last %] [%- END -%] ];
+     [%- FOREACH item = p.milestones %]'[% item.name FILTER js %]'[% ", " UNLESS loop.last %] [%- END -%] ];
   [% END %]
   [% n = n+1 %]
 [% END %]