Bug 346483: Fix up param-updating code in checksetup.pl to all be in a module.

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> (module owner) a=myk

6 files changed, 143 insertions, 132 deletions

diff --git a/Bugzilla.pm b/Bugzilla.pm
index 0ffa1d1b9..3d8215937 100644
--- a/Bugzilla.pm
+++ b/Bugzilla.pm
@@ -25,6 +25,7 @@ package Bugzilla;
 
 use strict;
 
+use Bugzilla::Config;
 use Bugzilla::Constants;
 use Bugzilla::Auth;
 use Bugzilla::Auth::Persist::Cookie;
@@ -147,7 +148,7 @@ sub cgi {
 
 sub params {
     my $class = shift;
-    request_cache()->{params} ||= _load_param_values();
+    request_cache()->{params} ||= Bugzilla::Config::read_param_file();
     return request_cache()->{params};
 }
 
@@ -346,27 +347,6 @@ sub _cleanup {
     undef $_request_cache;
 }
 
-sub _load_param_values {
-    my %params;
-    my $datadir = bz_locations()->{'datadir'};
-    if (-e "$datadir/params") {
-        # Note that checksetup.pl sets file permissions on '$datadir/params'
-
-        # Using Safe mode is _not_ a guarantee of safety if someone does
-        # manage to write to the file. However, it won't hurt...
-        # See bug 165144 for not needing to eval this at all
-        my $s = new Safe;
-
-        $s->rdo("$datadir/params");
-        die "Error reading $datadir/params: $!" if $!;
-        die "Error evaluating $datadir/params: $@" if $@;
-
-        # Now read the param back out from the sandbox
-        %params = %{$s->varglob('param')};
-    }
-    return \%params;
-}
-
 sub END {
     _cleanup();
 }
diff --git a/Bugzilla/Config.pm b/Bugzilla/Config.pm
index 22bdeb1f5..7db033285 100644
--- a/Bugzilla/Config.pm
+++ b/Bugzilla/Config.pm
@@ -34,13 +34,15 @@ use strict;
 
 use base qw(Exporter);
 use Bugzilla::Constants;
+use Data::Dumper;
+use File::Temp;
 
 # Don't export localvars by default - people should have to explicitly
 # ask for it, as a (probably futile) attempt to stop code using it
 # when it shouldn't
 %Bugzilla::Config::EXPORT_TAGS =
   (
-   admin => [qw(UpdateParams SetParam WriteParams)],
+   admin => [qw(update_params SetParam write_params)],
    db => [qw($db_driver $db_host $db_port $db_name $db_user $db_pass $db_sock
              $db_check)],
    localconfig => [qw($cvsbin $interdiffbin $diffpath $webservergroup)],
@@ -49,16 +51,6 @@ Exporter::export_ok_tags('admin', 'db', 'localconfig');
 
 use vars qw(@param_list);
 
-# Data::Dumper is required as needed, below. The problem is that then when
-# the code locally sets $Data::Dumper::Foo, this triggers 'used only once'
-# warnings.
-# We can't predeclare another package's vars, though, so just use them
-{
-    local $Data::Dumper::Sortkeys;
-    local $Data::Dumper::Terse;
-    local $Data::Dumper::Indent;
-}
-
 # INITIALISATION CODE
 # Perl throws a warning if we use bz_locations() directly after do.
 our $localconfig = bz_locations()->{'localconfig'};
@@ -111,13 +103,16 @@ sub SetParam {
     Bugzilla->params->{$name} = $value;
 }
 
-sub UpdateParams {
-    # --- PARAM CONVERSION CODE ---
+sub update_params {
+    my ($params) = @_;
+    my $answer = $params->{answer} || {};
+
+    my $param = read_param_file();
+
+    # If we didn't return any param values, then this is a new installation.
+    my $new_install = !(keys %$param);
 
-    # Note that this isn't particularly 'clean' in terms of separating
-    # the backend code (ie this) from the actual params.
-    # We don't care about that, though
-    my $param = Bugzilla->params;
+    # --- UPDATE OLD PARAMS ---
 
     # Old Bugzilla versions stored the version number in the params file
     # We don't want it, so get rid of it
@@ -144,7 +139,9 @@ sub UpdateParams {
 
     # Change from old product groups to controls for group_control_map
     # 2002-10-14 bug 147275 bugreport@peshkin.net
-    if (exists $param->{'usebuggroups'} && !exists $param->{'makeproductgroups'}) {
+    if (exists $param->{'usebuggroups'} && 
+        !exists $param->{'makeproductgroups'}) 
+    {
         $param->{'makeproductgroups'} = $param->{'usebuggroups'};
     }
     if (exists $param->{'usebuggroupsentry'} 
@@ -158,7 +155,9 @@ sub UpdateParams {
     }
 
     # set verify method to whatever loginmethod was
-    if (exists $param->{'loginmethod'} && !exists $param->{'user_verify_class'}) {
+    if (exists $param->{'loginmethod'} 
+        && !exists $param->{'user_verify_class'}) 
+    {
         $param->{'user_verify_class'} = $param->{'loginmethod'};
         delete $param->{'loginmethod'};
     }
@@ -182,49 +181,96 @@ sub UpdateParams {
     _load_params unless %params;
     foreach my $item (@param_list) {
         my $name = $item->{'name'};
-        $param->{$name} = $item->{'default'} unless exists $param->{$name};
+        unless (exists $param->{$name}) {
+            print "New parameter: $name\n" unless $new_install;
+            $param->{$name} = $answer->{$name} || $item->{'default'};
+        }
     }
 
-    # --- REMOVE OLD PARAMS ---
+    $param->{'utf8'} = 1 if $new_install;
 
-    my @oldparams = ();
+    # --- REMOVE OLD PARAMS ---
 
-    # Remove any old params
+    my @oldparams;
+    # Remove any old params, put them in old-params.txt
     foreach my $item (keys %$param) {
         if (!grep($_ eq $item, map ($_->{'name'}, @param_list))) {
-            require Data::Dumper;
-
-            local $Data::Dumper::Terse = 1;
+            local $Data::Dumper::Terse  = 1;
             local $Data::Dumper::Indent = 0;
             push (@oldparams, [$item, Data::Dumper->Dump([$param->{$item}])]);
             delete $param->{$item};
         }
     }
 
-    return @oldparams;
+    if (@oldparams) {
+        my $op_file = new IO::File('old-params.txt', '>>', 0600)
+          || die "old-params.txt: $!";
+
+        print "The following parameters are no longer used in Bugzilla,",
+              " and so have been\nmoved from your parameters file into",
+              " old-params.txt:\n";
+
+        foreach my $p (@oldparams) {
+            my ($item, $value) = @$p;
+            print $op_file "\n\n$item:\n$value\n";
+            print $item;
+            print ", " unless $item eq $oldparams[$#oldparams]->[0];
+        }
+        print "\n";
+        $op_file->close;
+    }
+
+    if (ON_WINDOWS && !-e SENDMAIL_EXE
+        && $param->{'mail_delivery_method'} eq 'sendmail')
+    {
+        my $smtp = $answer->{'SMTP_SERVER'};
+        if (!$smtp) {
+            print "\nBugzilla requires an SMTP server to function on",
+                  " Windows.\nPlease enter your SMTP server's hostname: ";
+            $smtp = <STDIN>;
+            chomp $smtp;
+            if ($smtp) {
+                $param->{'smtpserver'} = $smtp;
+             }
+             else {
+                print "\nWarning: No SMTP Server provided, defaulting to",
+                      " localhost\n";
+            }
+        }
+
+        $param->{'mail_delivery_method'} = 'smtp';
+    }
+
+    write_params($param);
 }
 
-sub WriteParams {
-    require Data::Dumper;
-    my $datadir = bz_locations()->{'datadir'};
+sub write_params {
+    my ($param_data) = @_;
+    $param_data ||= Bugzilla->params;
+
+    my $datadir    = bz_locations()->{'datadir'};
+    my $param_file = "$datadir/params";
 
     # This only has an affect for Data::Dumper >= 2.12 (ie perl >= 5.8.0)
     # Its just cosmetic, though, so that doesn't matter
     local $Data::Dumper::Sortkeys = 1;
 
-    require File::Temp;
     my ($fh, $tmpname) = File::Temp::tempfile('params.XXXXX',
                                               DIR => $datadir );
 
-    print $fh (Data::Dumper->Dump([Bugzilla->params], ['*param']))
+    print $fh (Data::Dumper->Dump([$param_data], ['*param']))
       || die "Can't write param file: $!";
 
     close $fh;
 
-    rename $tmpname, "$datadir/params"
-      || die "Can't rename $tmpname to $datadir/params: $!";
+    rename $tmpname, $param_file
+      || die "Can't rename $tmpname to $param_file: $!";
+
+    ChmodDataFile($param_file, 0666);
 
-    ChmodDataFile("$datadir/params", 0666);
+    # And now we have to reset the params cache so that Bugzilla will re-read
+    # them.
+    delete Bugzilla->request_cache->{params};
 }
 
 # Some files in the data directory must be world readable if and only if
@@ -243,6 +289,27 @@ sub ChmodDataFile {
     chmod $perm,$file;
 }
 
+sub read_param_file {
+    my %params;
+    my $datadir = bz_locations()->{'datadir'};
+    if (-e "$datadir/params") {
+        # Note that checksetup.pl sets file permissions on '$datadir/params'
+
+        # Using Safe mode is _not_ a guarantee of safety if someone does
+        # manage to write to the file. However, it won't hurt...
+        # See bug 165144 for not needing to eval this at all
+        my $s = new Safe;
+
+        $s->rdo("$datadir/params");
+        die "Error reading $datadir/params: $!" if $!;
+        die "Error evaluating $datadir/params: $@" if $@;
+
+        # Now read the param back out from the sandbox
+        %params = %{$s->varglob('param')};
+    }
+    return \%params;
+}
+
 1;
 
 __END__
@@ -256,9 +323,9 @@ Bugzilla::Config - Configuration parameters for Bugzilla
   # Administration functions
   use Bugzilla::Config qw(:admin);
 
-  my @removed_params = UpgradeParams();
+  update_params();
   SetParam($param, $value);
-  WriteParams();
+  write_params();
 
   # Localconfig variables may also be imported
   use Bugzilla::Config qw(:db);
@@ -281,30 +348,35 @@ Parameters can be set, retrieved, and updated.
 Sets the param named $name to $value. Values are checked using the checker
 function for the given param if one exists.
 
-=item C<UpdateParams()>
+=item C<update_params()>
 
 Updates the parameters, by transitioning old params to new formats, setting
-defaults for new params, and removing obsolete ones.
+defaults for new params, and removing obsolete ones. Used by F<checksetup.pl>
+in the process of an installation or upgrade.
 
-Any removed params are returned in a list, with elements [$item, $oldvalue]
-where $item is the entry in the param list.
+Prints out information about what it's doing, if it makes any changes.
 
-=item C<WriteParams()>
+May prompt the user for input, if certain required parameters are not
+specified.
 
-Writes the parameters to disk.
+=item C<write_params($params)>
 
-=back
+Description: Writes the parameters to disk.
 
-=over
+Params:      C<$params> (optional) - A hashref to write to the disk
+               instead of C<Bugzilla->params>. Used only by
+               C<update_params>.
 
-=item *
+Returns:     nothing
 
-The new value for the parameter
+=item C<read_param_file()>
 
-=item *
+Description: Most callers should never need this. This is used
+             by C<Bugzilla->params> to directly read C<$datadir/params>
+             and load it into memory. Use C<Bugzilla->params> instead.
 
-A reference to the entry in the param list for this parameter
+Params:      none
 
-Functions should return error text, or the empty string if there was no error.
+Returns:     A hashref containing the current params in C<$datadir/params>.
 
 =back
diff --git a/checksetup.pl b/checksetup.pl
index 485173fb2..00230e5c5 100755
--- a/checksetup.pl
+++ b/checksetup.pl
@@ -187,7 +187,9 @@ The format of that file is as follows:
  $answer{'db_user'}   = 'mydbuser';
  $answer{'db_pass'}   = 'mydbpass';
 
- (Any localconfig variable can be specified as above.)
+ $answer{'urlbase'} = 'http://bugzilla.mydomain.com/';
+
+ (Any localconfig variable or parameter can be specified as above.)
 
  $answer{'ADMIN_OK'} = 'Y';
  $answer{'ADMIN_EMAIL'} = 'myadmin@mydomain.net';
@@ -203,6 +205,14 @@ function calls needs to be specified in this file.
 
 L<Bugzilla::Install::Requirements>
 
+L<Bugzilla::Install::Localconfig>
+
+L<Bugzilla::Install::Filesystem>
+
+L<Bugzilla::Config/update_params>
+
+L<Bugzilla::DB/CONNECTION>
+
 =cut
 
 ######################################################################
@@ -222,9 +232,7 @@ use lib ".";
 use Bugzilla::Constants;
 use Bugzilla::Install::Requirements;
 
-if ($^O =~ /MSWin32/i) {
-    require 5.008001; # for CGI 2.93 or higher
-}
+require 5.008001 if ON_WINDOWS; # for CGI 2.93 or higher
 
 ######################################################################
 # Subroutines
@@ -356,62 +364,13 @@ create_htaccess() if $lc_hash->{'create_htaccess'};
 my $datadir   = bz_locations()->{'datadir'};
 my $webdotdir = bz_locations()->{'webdotdir'};
 
-# Check for a new install
-my $newinstall = !-e "$datadir/params";
-
 # Remove parameters from the params file that no longer exist in Bugzilla,
 # and set the defaults for new ones
+update_params({ answer => \%answer});
 
-my @oldparams = UpdateParams();
-
-if (@oldparams) {
-    open(PARAMFILE, '>>', 'old-params.txt') 
-      || die "$0: Can't open old-params.txt for writing: $!\n";
-
-    print "The following parameters are no longer used in Bugzilla, " .
-          "and so have been\nmoved from your parameters file " .
-          "into old-params.txt:\n";
-
-    foreach my $p (@oldparams) {
-        my ($item, $value) = @{$p};
-
-        print PARAMFILE "\n\n$item:\n$value\n";
-
-        print $item;
-        print ", " unless $item eq $oldparams[$#oldparams]->[0];
-    }
-    print "\n";
-    close PARAMFILE;
-}
-
-# Set mail_delivery_method to SMTP and prompt for SMTP server
-# if running on Windows and no third party sendmail wrapper
-# is available
-if ($^O =~ /MSWin32/i
-    && Bugzilla->params->{'mail_delivery_method'} eq 'sendmail'
-    && !-e SENDMAIL_EXE)
-{
-    print "\nBugzilla requires an SMTP server to function on Windows.\n" .
-        "Please enter your SMTP server's hostname: ";
-    my $smtp = $answer{'SMTP_SERVER'} 
-        || ($silent && die("cant preload SMTP_SERVER")) 
-        || <STDIN>;
-    chomp $smtp;
-    if (!$smtp) {
-        print "\nWarning: No SMTP Server provided, defaulting to localhost\n";
-        $smtp = 'localhost';
-    }
-    SetParam('mail_delivery_method', 'smtp');
-    SetParam('smtpserver', $smtp);
-}
-
-# Enable UTF-8 on new installs
-if ($newinstall) {
-    SetParam('utf8', 1);
-}
-
-# WriteParams will only write out still-valid entries
-WriteParams();
+###########################################################################
+# Pre-compile --TEMPLATE-- code
+###########################################################################
 
 my $templatedir = bz_locations()->{'templatedir'};
 unless ($switch{'no-templates'}) {
diff --git a/editgroups.cgi b/editgroups.cgi
index 57708cd3e..a7a608694 100755
--- a/editgroups.cgi
+++ b/editgroups.cgi
@@ -639,7 +639,7 @@ sub doGroupChanges {
                     $update_params = 1;
                 }
             }
-            WriteParams() if $update_params;
+            write_params() if $update_params;
         }
         if ($desc ne $cgi->param('olddesc')) {
             $chgs = 1;
diff --git a/editparams.cgi b/editparams.cgi
index 5ae2bcc25..79063271a 100755
--- a/editparams.cgi
+++ b/editparams.cgi
@@ -124,7 +124,7 @@ if ($action eq 'save' && $current_module) {
     $vars->{'message'} = 'parameters_updated';
     $vars->{'param_changed'} = \@changes;
 
-    WriteParams();
+    write_params();
 }
 
 $template->process("admin/params/editparams.html.tmpl", $vars)
diff --git a/editvalues.cgi b/editvalues.cgi
index 109f8a29e..dc35c40e0 100755
--- a/editvalues.cgi
+++ b/editvalues.cgi
@@ -410,7 +410,7 @@ if ($action eq 'update') {
         && $valueold eq Bugzilla->params->{$defaults{$field}})
     {
         SetParam($defaults{$field}, $value);
-        WriteParams();
+        write_params();
         $vars->{'default_value_updated'} = 1;
     }