Closed
Bug 328637
Opened 19 years ago
Closed 19 years ago
Remove all legal_* versioncache arrays
Categories
(Bugzilla :: Bugzilla-General, enhancement, P1)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.0
People
(Reporter: mkanat, Assigned: LpSolit)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
mkanat
:
review+
|
Details | Diff | Splinter Review |
I think I can take all the @::legal_* arrays out at once. Each one is only used in a few places.
Reporter | ||
Comment 2•19 years ago
|
||
Okay, okay. Dear bug--I love you. :-D I'll get on it when I have some time to work on upstream stuff.
Priority: -- → P1
Assignee | ||
Comment 3•19 years ago
|
||
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.
Assignee: mkanat → LpSolit
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•19 years ago
|
||
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.
Attachment #225906 -
Attachment is obsolete: true
Attachment #225952 -
Flags: review?(mkanat)
Reporter | ||
Comment 5•19 years ago
|
||
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?
Attachment #225952 -
Flags: review?(mkanat) → review-
Assignee | ||
Comment 6•19 years ago
|
||
(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? :(
Reporter | ||
Comment 7•19 years ago
|
||
(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.
Assignee | ||
Comment 8•19 years ago
|
||
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.
Assignee | ||
Comment 9•19 years ago
|
||
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.
Attachment #225952 -
Attachment is obsolete: true
Attachment #226101 -
Flags: review?(mkanat)
Reporter | ||
Comment 10•19 years ago
|
||
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.
Attachment #226101 -
Flags: review?(mkanat) → review+
Assignee | ||
Comment 11•19 years ago
|
||
Now checksetup.pl is running smoothly.
Attachment #226101 -
Attachment is obsolete: true
Attachment #226154 -
Flags: review?(mkanat)
Reporter | ||
Comment 12•19 years ago
|
||
Comment on attachment 226154 [details] [diff] [review] splitted patch, v3 (see bug 304601 for part 1) Okay, the interdiff looks good. :-)
Attachment #226154 -
Flags: review?(mkanat) → review+
Assignee | ||
Updated•19 years ago
|
Flags: approval?
Updated•19 years ago
|
Flags: approval? → approval+
Assignee | ||
Comment 13•19 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•