283989 - CGI.pl global init code should be moved to Bugzilla::CGI

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P2)

Description

[Max Kanat-Alexander]

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.

Comment 1

[Max Kanat-Alexander]

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?

Comment 2

[Max Kanat-Alexander]

Attachment: Move CGI globals around and re-work shutdownhtml

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.

Comment 3

[Marc Schumann [:Wurblzap]]

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.

Comment 4

[Max Kanat-Alexander]

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.

Comment 5

[Marc Schumann [: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.

Comment 6

[Jacob Steenhagen]

(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

Comment 7

[Max Kanat-Alexander]

Attachment: v2

OK, maybe it's really v1.1 -- all I did was move the constants. :-)

Comment 8

[Marc Schumann [: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.

Comment 9

[Max Kanat-Alexander]

Attachment: v2.1

OK, typo fixed and the template is now included. :-)

Comment 10

[Marc Schumann [: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).

Comment 11

[Colin Ogilvie [:cso]]

Comment on attachment 189233 [details] [diff] [review]
v2.1

>+  # Contributor(s): Max Kanat-Alexander <[e-mail]>

Didn't you change this to [e-mail] in other files?

Comment 12

[Dave Miller [:justdave]]

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

[Marc Schumann [:Wurblzap]]

(In reply to comment #12)

Sorry, this was me not having transitioned to post-branch time.

Comment 14

[Max Kanat-Alexander]

(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.

Comment 15

[Max Kanat-Alexander]

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

Comment 16

[Max Kanat-Alexander]

Added to the Bugzilla 2.22 Release Notes in bug 322960, including the data from comment 2.