Closed
Bug 283989
Opened 19 years ago
Closed 19 years ago
CGI.pl global init code should be moved to Bugzilla::CGI
Categories
(Bugzilla :: Bugzilla-General, defect, P2)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.22
People
(Reporter: mkanat, Assigned: mkanat)
References
Details
Attachments
(1 file, 2 obsolete files)
7.35 KB,
patch
|
Wurblzap
:
review+
|
Details | Diff | Splinter Review |
Right now CGI.pl does a few things at the top, like correctly set the TEMP environment variable for Win32, and check if shutdownhtml is turned on. If we remove CGI.pl from those scripts (as we have from a few), then those things won't happen. Instead, those things should happen during Bugzilla::CGI::new.
Assignee | ||
Comment 1•19 years ago
|
||
OK, moving shutdownhtml is almost impossible. When I try to do $template->process, it creates a new Bugzilla::CGI if one doesn't already exist. So I can't put the code into Bugzilla::CGI::new. And it uses Bugzilla->cgi to get that CGI, so I can't put the code in Bugzilla->cgi. Is there some other way that we can architect shutdownhtml so that we can remove it from a global include?
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•19 years ago
|
||
OK, this moves the global "init" code at the top of CGI.pl into the modules where it belongs. Note that part of this change makes it so that ALL of Bugzilla is now affected by shutdownhtml, even command-line scripts. checksetup.pl has a special exemption.
Attachment #176561 -
Flags: review?(wurblzap)
Assignee | ||
Updated•19 years ago
|
Assignee | ||
Updated•19 years ago
|
Priority: -- → P2
Assignee | ||
Updated•19 years ago
|
Attachment #176561 -
Flags: review?(wurblzap) → review?(LpSolit)
Comment 3•19 years ago
|
||
I poked around the patch a little... Here are my thoughts, not qualifying for a full review. I like that shutdownhtml affects all of Bugzilla. I don't like putting a SHUTDOWNHTML_EXEMPT constant into Constants.pm. It seems to me it has no use ever outside of the shutdownhtml code in Bugzilla.pm and should therefore stay there instead of cluttering Constants.pm. Similarly, I'm not too fond of the i_am_cgi sub. Do we need to be friendly to command line scripts? If we do, do we need to burden Util.pm with such a sub? (I'd have checked for GATEWAY_INTERFACE, too, but never mind that.) I like the way you're starting to introduce text/plain messages. I'd say a follow-up bug to address your message.txt.tmpl comment is in order :) The patch seems to work well for me. I think I'll be able to test it more comprehensively as soon as I know more about the script exemption handling and CGI detection.
Updated•19 years ago
|
Attachment #176561 -
Flags: review?(LpSolit)
Assignee | ||
Comment 4•19 years ago
|
||
Comment on attachment 176561 [details] [diff] [review] Move CGI globals around and re-work shutdownhtml OK, I agree with moving the constant, I suppose. i_am_cgi is useful and necessary, and will be used by other things in the future. Yes, we do need to be nice to command-line scripts. I didn't check for GATEWAY_INTERFACE because we improperly abuse that variable in Bugzilla. The script exemption handling is pretty simple... and the CGI detection is also fairly straightforward.
Attachment #176561 -
Flags: review?(wurblzap)
Comment 5•19 years ago
|
||
Comment on attachment 176561 [details] [diff] [review] Move CGI globals around and re-work shutdownhtml Tested; works. Please move the list of shutdown-exempted scripts out of Config.pm and into Bugzilla.pm as per comment 3 and comment 4. The remainder are nits. >+ print Bugzilla->cgi->header() if i_am_cgi(); Nit: it seems to me people tend to prefer the |i_am_cgi() && print Bugzilla->cgi->header()| notation. >+sub i_am_cgi () { Nit: I like invoked_by_cgi better... >+++ template/en/default/global/message.txt.tmpl 2005-03-07 03:28:25.000000000 -0800 >+[%# Yes, this may show some HTML. But it's the best we >+ # can do at the moment. %] Please file a follow-up bug for this.
Attachment #176561 -
Flags: review?(wurblzap) → review-
Comment 6•19 years ago
|
||
(In reply to comment #5) > >+ print Bugzilla->cgi->header() if i_am_cgi(); > > Nit: it seems to me people tend to prefer the |i_am_cgi() && print > Bugzilla->cgi->header()| notation. Not that it really matter, but I prefer |print $whatever if i_am_cgi();| as it seems to flow better and be a bit more obvious to my little mind
Assignee | ||
Comment 7•19 years ago
|
||
OK, maybe it's really v1.1 -- all I did was move the constants. :-)
Attachment #176561 -
Attachment is obsolete: true
Attachment #187868 -
Flags: review?(wurblzap)
Comment 8•19 years ago
|
||
Comment on attachment 187868 [details] [diff] [review] v2 The new message.txt.tmpl has gone MIA for some reason. >+# If Bugzilla is shut down, do not allow anything to run, just display a >+# message to the user about the downtime. Scripts listed in >+# SHUTDOWNHTML_EXPECT are exempt from this message. Typo -- SHUTDOWNHTML_EXEMPT. Good otherwise.
Attachment #187868 -
Flags: review?(wurblzap) → review-
Assignee | ||
Comment 9•19 years ago
|
||
OK, typo fixed and the template is now included. :-)
Attachment #187868 -
Attachment is obsolete: true
Attachment #189233 -
Flags: review?(wurblzap)
Comment 10•19 years ago
|
||
Comment on attachment 189233 [details] [diff] [review] v2.1 Tested; works. It turns out whineatnews.pl is not affected by the shutdownhtml param. This is current behaviour, though, too, so it is not a regression (but maybe another bug).
Attachment #189233 -
Flags: review?(wurblzap) → review+
Updated•19 years ago
|
Flags: approval2.20?
Comment 11•19 years ago
|
||
Comment on attachment 189233 [details] [diff] [review] v2.1 >+ # Contributor(s): Max Kanat-Alexander <mkanat@kerio.com> Didn't you change this to mkanat@bugzilla.org in other files?
Comment 12•19 years ago
|
||
Do we need this on the tip? This amounts to a feature change (developer feature) -- what benefit do we get by having it on the branch?
Comment 13•19 years ago
|
||
(In reply to comment #12) Sorry, this was me not having transitioned to post-branch time.
Flags: approval2.20? → approval?
Updated•19 years ago
|
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Updated•19 years ago
|
Flags: approval? → approval+
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
Assignee | ||
Comment 14•19 years ago
|
||
(In reply to comment #12) > Do we need this on the tip? This amounts to a feature change (developer > feature) -- what benefit do we get by having it on the branch? Scripts that don't "require CGI.pl" won't be affected by shutdownhtml. I think that's only editclassifications, though, so it's not a big deal.
Assignee | ||
Comment 15•19 years ago
|
||
Oh, forgot to fix my email address on initial checkin, so I fixed it with a second tiny checkin. :-) Thanks for pointing that out, Colin. Checking in Bugzilla.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v <-- Bugzilla.pm new revision: 1.18; previous revision: 1.17 done Checking in CGI.pl; /cvsroot/mozilla/webtools/bugzilla/CGI.pl,v <-- CGI.pl new revision: 1.245; previous revision: 1.244 done Checking in Bugzilla/CGI.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v <-- CGI.pm new revision: 1.17; previous revision: 1.16 done Checking in Bugzilla/Util.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v <-- Util.pm new revision: 1.30; previous revision: 1.29 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/message.txt.tmpl,v done Checking in template/en/default/global/message.txt.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/message.txt.tmpl,v <-- message.txt.tmpl initial revision: 1.1 done Email fix: Checking in template/en/default/global/message.txt.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/message.txt.tmpl,v <-- message.txt.tmpl new revision: 1.2; previous revision: 1.1 done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•19 years ago
|
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Assignee | ||
Comment 16•18 years ago
|
||
Added to the Bugzilla 2.22 Release Notes in bug 322960, including the data from comment 2.
Keywords: relnote
Whiteboard: [relnote comment 2]
You need to log in
before you can comment on or make changes to this bug.
Description
•