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)

2.19.2
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.22

People

(Reporter: mkanat, Assigned: mkanat)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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
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)
Keywords: relnote
Whiteboard: [relnote comment 2]
Target Milestone: --- → Bugzilla 2.20
Priority: -- → P2
Attachment #176561 - Flags: review?(wurblzap) → review?(LpSolit)
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.
Attachment #176561 - Flags: review?(LpSolit)
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 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-
(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
Blocks: 299287
Attached patch v2 (obsolete) — Splinter Review
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 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-
Attached patch v2.1Splinter Review
OK, typo fixed and the template is now included. :-)
Attachment #187868 - Attachment is obsolete: true
Attachment #189233 - Flags: review?(wurblzap)
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+
Flags: approval2.20?
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?
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?
(In reply to comment #12)

Sorry, this was me not having transitioned to post-branch time.
Flags: approval2.20? → approval?
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Flags: approval? → approval+
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
(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.
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
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Blocks: 300978
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.

Attachment

General

Created:
Updated:
Size: