328637 - Remove all legal_* versioncache arrays

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P1)

Description

[Max Kanat-Alexander]

I think I can take all the @::legal_* arrays out at once. Each one is only used in a few places.

Comment 1

[Frédéric Buclin]

mkanat, this bug needs some love. ;)

Comment 2

[Max Kanat-Alexander]

Okay, okay. Dear bug--I love you. :-D

I'll get on it when I have some time to work on upstream stuff.

Comment 3

[Frédéric Buclin]

Attachment: work in progress, v0.8

Do not apply this patch it won't work... for two reasons:
1) Bugzilla::Config::BugFields requires Bugzilla::Field to get valid OSes, platforms, severities and priorities, but $dbh is not defined yet and so the system crashes (including checksetup.pl and runtests.pl);

2) This "cvs diff" mixes several patches about globals.pl (because some other patches are already applied on my system, but these patches haven't been reviewed yet). I tried to edit this diff as best as possible, but there are some parts I cannot fix correctly (due to conflicts). So this patch won't apply correctly anyway. ;)

This gives you a good idea of how my final patch will look like though.

Comment 4

[Frédéric Buclin]

Attachment: patch, v1

Ok, this patch works. I had to remove some dependency loops to get it working correctly. Among others, I created Bugzilla->locations which returns an array for all location variables defined in Config.pm. To avoid a too big patch, these variables are now duplicated in Config.pm for all the scripts using Bugzilla::Config qw(:location) and in Constants.pm where Bugzilla->locations is pointing to. See bug 304601 about this solution.

Now thanks to Bugzilla->locations, Error.pm, Config/Common.pm and Config/L10n.pm no longer depends on Config.pm, which removes all these big dependencies involving the Config::* modules. And my patch is now working just fine. :)

Oh, and get_legal_field_values() is now in Field.pm and can be called from any module as Field.pm isn't involved in any loop.

I also had to hack Config.pm a bit to get Bugzilla->dbh defined before Bugzilla::Field::get_legal_field_values() is called (this function uses Bugzilla->dbh). This mainly means moving some stuff in a routine and calling this routine only when we really need it, i.e. when we call Param(). This hack will be removed later in a separate bug (unless you really love huge unreviewable patches).

Also, GetVersionTable() has been completely deleted from globals.pl. versioncache is dead!!! \o/

Note that this patch requires bug 338796 and bug 338793 (in this order) to land first, due to conflicts.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 225952 [details] [diff] [review]
patch, v1

This looks pretty good, but I'd like to split it into three patches to make it a bit easier to review.

That is, the "locations" thing should be a separate patch (I think we have a bug for it), and the other changes to Bugzilla::Config should probably be a separate patch. It would just be a lot easier for me to review it all that way.

I wish that we could just make all of the locations variables into constants themselves, without having to use Bugzilla->locations.

Also, do we have to put that sub inside of Bugzilla::Constants? If we're going to use Bugzilla->locations, can't the code live inside Bugzilla.pm?

Comment 6

[Frédéric Buclin]

(In reply to comment #5)
> This looks pretty good, but I'd like to split it into three patches to make it
> a bit easier to review.

Bah, this patch is less than 100 Kb! :-D


> That is, the "locations" thing should be a separate patch (I think we have a
> bug for it)

Yup, that's bug 304601. But read my next comment for the reason I wanted to do it here.


>, and the other changes to Bugzilla::Config should probably be a
> separate patch. It would just be a lot easier for me to review it all that way.

The reason I put everything in the same patch is that fixing :locations separately would require to fix all these 'unlink "$datadir/versioncache";', but they are going to be removed in this patch. This sounded like extraneous work to me (i.e. fixing something which I'm going to remove). :-/


> I wish that we could just make all of the locations variables into constants
> themselves, without having to use Bugzilla->locations.

Can we have $ENV{'FOO'} into constants? Because some variables use it. If not, we haven't choice.


> Also, do we have to put that sub inside of Bugzilla::Constants? If we're going
> to use Bugzilla->locations, can't the code live inside Bugzilla.pm?

Bad idea. Calling Bugzilla from Config::L10n doesn't seem to work. I tried it already.


Max, do you still really want to see this patch splitted into pieces? :(

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> The reason I put everything in the same patch is that fixing :locations
> separately would require to fix all these 'unlink "$datadir/versioncache";',
> but they are going to be removed in this patch. This sounded like extraneous
> work to me (i.e. fixing something which I'm going to remove). :-/

  That makes sense to me. Any chance that you could just split it out onto the other bug and we'll check them in simultaneously? It would just make the review clearer to me.

> Can we have $ENV{'FOO'} into constants? Because some variables use it. If not,
> we haven't choice.

  Yeah, we should be able to. $ENV exists at compile time.

> > Also, do we have to put that sub inside of Bugzilla::Constants? If we're going
> > to use Bugzilla->locations, can't the code live inside Bugzilla.pm?
> 
> Bad idea. Calling Bugzilla from Config::L10n doesn't seem to work. I tried it
> already.

  Doesn't work? It should work, even in checksetup.pl. Bugzilla.pm is loaded before Bugzilla::Config.

  Well, anyhow, I don't really want them living in two places, that's pretty confusing. Better to just have them only in Bugzilla::Constants.

> Max, do you still really want to see this patch splitted into pieces? :(

  If it's going to delay it a lot, you don't have to. But it is hard to review this way, because it's multiple changes. No matter how large a patch is, if it's just one change it all makes sense and I don't have to keep switching my mind around to figure out what's part of what. But when it's multiple things in one patch, it's more difficult to review. It's not that I'm stupid, it's just that it's easier for *anybody* to review just a single change.

Comment 8

[Frédéric Buclin]

Note: http://www.perl.com/doc/manual/html/lib/constant.html

"As with all use directives, defining a constant happens at compile time. Thus, it's probably not correct to put a constant declaration inside of a conditional statement (like if ($foo) { use constant ... })."

As we have:

    if ($ENV{'PROJECT'} && $ENV{'PROJECT'} =~ /^(\w+)$/) {
        $project = $1;
        $localconfig = "$libpath/localconfig.$project";
        $datadir = "$libpath/data/$project";
    } else {
        $localconfig = "$libpath/localconfig";
        $datadir = "$libpath/data";
    }

we cannot use constants here. I will stick with Bugzilla->locations pointing to Bugzilla::Constants::locations(). The reason I put this code in Constants.pm is that they are pseudo-constants. I'm going to put this part of the patch in bug 304601 as requested by mkanat in his previous comments.

Comment 9

[Frédéric Buclin]

Attachment: splitted patch, v2 (see bug 304601 for part 1)

versioncache removal specific patch. The part 1 from bug 304601 must be applied first. I strongly suggest you to apply both patches simultaneously before running checksetup.pl and testing them (same when committing them to CVS).

If you are looking for Field.pm, it's now in part 1. ;)

Note that compared to my previous patch, I now leave "do $localconfig;" in globals.pl alone. One of the reasons is described in bug 341862, i.e. we have a :db for DB related variables, but we have nothing to export $cvsbin, $diffpath, $webservergroup, etc.. and so removing "do $localconfig;" from globals.pl makes these variables to be unavailable from outside Config.pm. We will fix that in a separate bug.

Also, you will now notice that editparams.cgi calls Bugzilla::Config::Foo->get_param_list(1), i.e. with "1" as argument, which means "give me everything you know about parameters". That's because Bugzilla/Config/BugFields.pm needs to query the DB in order to get available priorities, platforms, etc..., but we cannot do that while $dbh is unknown, which is the case among others in checksetup.pl at line 509. Only BugFields.pm will care about this parameter, see the "part 1" patch in bug 304601.

Comment 10

[Max Kanat-Alexander]

Comment on attachment 226101 [details] [diff] [review]
splitted patch, v2 (see bug 304601 for part 1)

Okay, thinking about this, how about we eliminate settable_resolutions in a different patch, by just adding an "is_settable" column to the resolution table in the DB? Or we could just do that in a separate patch after this bug, provided that you promise to write the patch.

Also, I'm not sure I like the get_param_list(1). At least make that a string with some readable value that explains what it's doing, instead of "1".

Otherwise this looks fine.

Comment 11

[Frédéric Buclin]

Attachment: splitted patch, v3 (see bug 304601 for part 1)

Now checksetup.pl is running smoothly.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 226154 [details] [diff] [review]
splitted patch, v3 (see bug 304601 for part 1)

Okay, the interdiff looks good. :-)

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