304601 - Bugzilla::Config's :locations exports need to be in their own module

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

One of the major dependency-loop creators in Bugzilla is the fact that a LOT of
things need Bugzilla::Config's :locations variables, like $datadir.

These are totally independent of the rest of Bugzilla::Config, and can go in
their own library. I'd call it Bugzilla::Config::Locations.

This should also be the one-stop-shop for people who want to modify Bugzilla to
run with a different directory structure.

The library should have *zero* dependencies on other Bugzilla libraries.

Comment 1

[Frédéric Buclin]

Why not having Bugzilla->local_config('datadir') instead? Same for all other parameters defined in localconfig: Bugzilla->local_config('foo'). This would remove a lot of dependencies on Config.pm, especially now that we have Bugzilla->param().

Comment 2

[Max Kanat-Alexander]

(In reply to comment #1)
> Why not having Bugzilla->local_config('datadir') instead? Same for all other
> parameters defined in localconfig: Bugzilla->local_config('foo'). This would
> remove a lot of dependencies on Config.pm, especially now that we have
> Bugzilla->param().

  Yeah, that's a good idea. Either that or make them all constants in Bugzilla/Constants.pm. I like the convenience now of being able to do $datadir, though, so that I can put it in strings and so on. Can you think of any clever way we could keep that convenience?

Comment 3

[Frédéric Buclin]

Attachment: splitted patch, v1 (see bug 328637)

OK, I tried to split my patch from bug 328637 into pieces as well as possible. This patch should (maybe, I didn't test) work independently of "part 2" attached in bug 328637, but I strongly suggest to apply both patches before testing anything.

This patch introduces Bugzilla->locations and removes dependency loops involving Bugzilla::Config::*. In my opinion, I cannot split further.

This patch is required by bug 328637 and must be committed *simultaneously*.

Comment 4

[Frédéric Buclin]

FYI, no, it's not a mistake: I really wanted to attach the part about Field.pm here, as it's required by some of the Bugzilla::Config::* modules.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 226098 [details] [diff] [review]
splitted patch, v1 (see bug 328637)

Okay, make it *only* Bugzilla::Constants::locations(), and get rid of Bugzilla->locations. Bugzilla::Constant can always be used by everybody everywhere.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 226098 [details] [diff] [review]
splitted patch, v1 (see bug 328637)

>+sub get_legal_field_values {

Oh, also, you don't *have* to do this, but I really wanted to rename get_legal_field_values to legal_field_values. Or make it a method of a Field object and call it "legal_values."

>Index: Bugzilla/Config/BugFields.pm
>+
>+  # If called by Config.pm, do not pass it data which requires
>+  # a query to the DB, as $dbh is not set yet. These data are
>+  # only useful to editparams.cgi anyway.
>+  my @add_items;
>+  if ($get_all_data) {
>+      my @legal_priorities = @{get_legal_field_values('priority')};
>+      my @legal_severities = @{get_legal_field_values('bug_severity')};
>+      my @legal_platforms  = @{get_legal_field_values('rep_platform')};
>+      my @legal_OS         = @{get_legal_field_values('op_sys')};
>+
>+      @add_items = (
>+        {

  I *really*, *really* don't like having this hash listed twice, for just a small change.

  It worked fine before, when globals.pl was doing the function calls. So what's wrong?? It should still work fine now. I mean, remember that globals.pl used to have to generate all this stuff, just like we're doing now, at the same time in the code.

 And you made the get_param_list call happen at runtime instead of compile-time now, right? (Since you moved around that %param stuff in Bugzilla::Config.) So this should work fine without all of this $get_all_data stuff.

Comment 7

[Frédéric Buclin]

Attachment: splitted patch, v2 (see bug 328637)

Here we go. The hack in BugFields.pm has been removed.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 226155 [details] [diff] [review]
splitted patch, v2 (see bug 328637)

Okay, the interdiff looks good. :-)

Comment 9

[Frédéric Buclin]

Attachment: fix CGI.pm

We have to replace Param() by Bugzilla->params in CGI.pm, else "perl -cwT buglist.cgi" fails when the 'localconfig' file is missing. Go figure!

Comment 10

[Frédéric Buclin]

Attachment: fix CGI.pm

err, sorry for the previous patch. It contained a local change.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 226186 [details] [diff] [review]
fix CGI.pm

>Index: Bugzilla/CGI.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v
>retrieving revision 1.22
>diff -u -r1.22 CGI.pm
>--- Bugzilla/CGI.pm     14 Jun 2006 00:26:27 -0000      1.22
>+++ Bugzilla/CGI.pm     19 Jun 2006 18:53:20 -0000
>@@ -40,7 +40,6 @@
> 
> use Bugzilla::Error;
> use Bugzilla::Util;
>-use Bugzilla::Config;
> 
> # We need to disable output buffering - see bug 179174
> $| = 1;
>@@ -79,11 +78,13 @@
>     $self->{Bugzilla_cookie_list} = [];
> 
>     # Send appropriate charset
>-    $self->charset(Param('utf8') ? 'UTF-8' : '');
>+    $self->charset(Bugzilla->params->{'utf8'} ? 'UTF-8' : '');
> 
>     # Redirect to SSL if required
>-    if (Param('sslbase') ne '' and Param('ssl') eq 'always' and i_am_cgi()) {
>-        $self->require_https(Param('sslbase'));
>+    if (Bugzilla->params->{'sslbase'} ne ''
>+        && Bugzilla->params->{'ssl'} eq 'always'
>+        &&i_am_cgi()) {
>+        $self->require_https(Bugzilla->params->{'sslbase'});

Nit: && i_am_cgi()


>+    $paramhash{'-domain'} = Bugzilla->params->{'cookiedomain'} if Bugzilla->params->{'cookiedomain'};

Nit:

    $paramhash{'-domain'} = Bugzilla->params->{'cookiedomain'}
      if Bugzilla->params->{'cookiedomain'};

Otherwise this looks good and seems a worthy change, even if things weren't breaking without it.  I'd be more comfortable if we knew exactly why things are horked, but I'm ok with taking this.  Nits can be fixed upon checkin, but in that case please attach the patch you eventually checked in to this bug.

Comment 12

[Frédéric Buclin]

Attachment: fix CGI.pm, v1.1

The patch I'm going to check in.

Comment 13

[Frédéric Buclin]

I committed bug 304601 and bug 328637 simultaneously (3 patches at once). Here is the mixed CVS log:

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.36; previous revision: 1.35
done
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.333; previous revision: 1.332
done
Checking in colchange.cgi;
/cvsroot/mozilla/webtools/bugzilla/colchange.cgi,v  <--  colchange.cgi
new revision: 1.53; previous revision: 1.52
done
Checking in collectstats.pl;
/cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v  <--  collectstats.pl
new revision: 1.51; previous revision: 1.50
done
Checking in config.cgi;
/cvsroot/mozilla/webtools/bugzilla/config.cgi,v  <--  config.cgi
new revision: 1.20; previous revision: 1.19
done
Checking in duplicates.cgi;
/cvsroot/mozilla/webtools/bugzilla/duplicates.cgi,v  <--  duplicates.cgi
new revision: 1.55; previous revision: 1.54
done
Checking in editclassifications.cgi;
/cvsroot/mozilla/webtools/bugzilla/editclassifications.cgi,v  <--  editclassifications.cgi
new revision: 1.19; previous revision: 1.18
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.72; previous revision: 1.71
done
Checking in editkeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/editkeywords.cgi,v  <--  editkeywords.cgi
new revision: 1.37; previous revision: 1.36
done
Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi
new revision: 1.53; previous revision: 1.52
done
Checking in editparams.cgi;
/cvsroot/mozilla/webtools/bugzilla/editparams.cgi,v  <--  editparams.cgi
new revision: 1.34; previous revision: 1.33
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.124; previous revision: 1.123
done
Checking in editvalues.cgi;
/cvsroot/mozilla/webtools/bugzilla/editvalues.cgi,v  <--  editvalues.cgi
new revision: 1.10; previous revision: 1.9
done
Checking in editversions.cgi;
/cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v  <--  editversions.cgi
new revision: 1.48; previous revision: 1.47
done
Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.136; previous revision: 1.135
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.372; previous revision: 1.371
done
Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.57; previous revision: 1.56
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.149; previous revision: 1.148
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.328; previous revision: 1.327
done
Checking in query.cgi;
/cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
new revision: 1.163; previous revision: 1.162
done
Checking in report.cgi;
/cvsroot/mozilla/webtools/bugzilla/report.cgi,v  <--  report.cgi
new revision: 1.38; previous revision: 1.37
done
Checking in reports.cgi;
/cvsroot/mozilla/webtools/bugzilla/reports.cgi,v  <--  reports.cgi
new revision: 1.83; previous revision: 1.82
done
Checking in show_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/show_bug.cgi,v  <--  show_bug.cgi
new revision: 1.43; previous revision: 1.42
done
Checking in summarize_time.cgi;
/cvsroot/mozilla/webtools/bugzilla/summarize_time.cgi,v  <--  summarize_time.cgi
new revision: 1.17; previous revision: 1.16
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.98; previous revision: 1.97
done
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.39; previous revision: 1.38
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.121; previous revision: 1.120
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.80; previous revision: 1.79
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.24; previous revision: 1.23
done
Checking in Bugzilla/Config.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config.pm,v  <--  Config.pm
new revision: 1.58; previous revision: 1.57
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.40; previous revision: 1.39
done
Checking in Bugzilla/Error.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Error.pm,v  <--  Error.pm
new revision: 1.16; previous revision: 1.15
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.12; previous revision: 1.11
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.129; previous revision: 1.128
done
Checking in Bugzilla/Config/BugFields.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/BugFields.pm,v  <--  BugFields.pm
new revision: 1.4; previous revision: 1.3
done
Checking in Bugzilla/Config/Common.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Common.pm,v  <--  Common.pm
new revision: 1.8; previous revision: 1.7
done
Checking in Bugzilla/Config/L10n.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/L10n.pm,v  <--  L10n.pm
new revision: 1.2; previous revision: 1.1
done
Checking in Bugzilla/Search/Quicksearch.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search/Quicksearch.pm,v  <--  Quicksearch.pm
new revision: 1.8; previous revision: 1.7
done
Checking in docs/xml/customization.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v  <--  customization.xml
new revision: 1.34; previous revision: 1.33
done