Closed
Bug 201816
Opened 22 years ago
Closed 21 years ago
use CGI.pm for header output
Categories
(Bugzilla :: Bugzilla-General, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: bbaetz, Assigned: bbaetz)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
bugreport
:
review+
|
Details | Diff | Splinter Review |
We should use CGI.pm for all our output. Patch coming after a bit more testing.
Important note - this does not work in a utf8 locale under perl 5.8, because of
perl core bug 21951, http://bugs6.perl.org/rt2/Ticket/Display.html?id=21951
Since the webserver tends to run in the "C" locale, that shouldn't be an issue,
although it is when doing cmdline testing if you have utf8 set up (which I was
doing, which is how I noticed this). Once theres a workarround we can use it, or
require a fixed version of CGI. Do we need to do that before this is checked in?
If so, I can probably make a workarround which would work for bugzilla. I'd
rather not, though.
CGI.pm version bump is so that we can have no charset sent, rather than a blank
charset (ie |Content-Type: text/html; charset=|)
The content-type conversion semi-automated via perl -pi, although I went through
everything manually and did cookies entirely manually.
I got rid of the headers_done test in the ThrowError* code by using
:unique_headers in CGI.pm - when trying to print the headers, it will just
return if they're already been printed. This obviously only works if everywhere
uses CGI.pm, and noone prints manually.
Assignee | ||
Comment 1•22 years ago
|
||
OK, here we go. This is streightforward, really. Only issue is wher eI used
Bugzilla->cgi vs a local $cgi var:
edit* and friends got Bugzilla->cgi, since they need rewrites to be any use for
mod_perl anyway.
Subroutines in cgi scripts can't use the script-local var from within a
subroutine because of how mod_perl wraps the script - they end up as closures.
See mod_perl docs for details. (We do get a warning about this under mod_perl,
so its not a hidden bug). So if printing the header was the only use the sub
had for a cgi object, I just used Bugzilla->cgi.
I did add |use Bugzilla| lines for all but edit*. If I missed some, it doesn't
matter for now since CGI.pl brings it in.
I also changed the cookie expiration dates to have the correct day, and changed
them while I was at it.
Assignee | ||
Updated•22 years ago
|
Attachment #120352 -
Flags: review?(myk)
Assignee | ||
Updated•22 years ago
|
Target Milestone: --- → Bugzilla 2.18
Comment 2•22 years ago
|
||
One shouldn't forget to update
http://www.bugzilla.org/docs/html/extraconfig.html#content-type
if this gets in.
(Setting $self->charset(''); in Bugzilla/CGI.pm is less clumpsy than the
previous perl -pi -e -- and I paves the way for UTF-8/bug 126266 :-)
Assignee | ||
Comment 3•22 years ago
|
||
Oh, and I moved the contenttypes stuff out of localconfig while I was there.
Comment 4•22 years ago
|
||
I'd rather not break utf8 personally... what's the workaround?
Assignee | ||
Comment 5•22 years ago
|
||
Its not UTF8 which breaks per se; its running under the utf8 locale which does.
I haven't had a response to my bug report. I'll try to work out a work arround
this weekend.
Comment 6•22 years ago
|
||
Comment on attachment 120352 [details] [diff] [review]
patch
Well, this reads OK, but I tried it out and it broke my logincookes. I have to
log in after every page. Cookiepath is "/" for me which always worked in the
past.
Comment 7•22 years ago
|
||
Incidentally, the cookie it tried to set on my machine is....
"<param>cookiepath</param>"
:-)
Comment 8•22 years ago
|
||
This is still pretty toxic. In prder to get the correct cookie set, CGI.pm
needs to refer to Bugzilla::Config::Param() instead of merely Param(). Even so,
and even after clearing cookies, this patch forces me to log in for every page.
If I back the patch out, that does not happen.
Traces show the logincooke being sent to the browser and then sent back to the
server when the next page is requested, but the cookie does not seem to be
accepted with the patch in place.
Did you try this using perl 5.6.1 ??
Checking perl modules ...
Checking for AppConfig (v1.52) ok: found v1.52
Checking for CGI (v2.88) ok: found v2.91
Checking for Data::Dumper (any) ok: found v2.102
Checking for Date::Format (v2.21) ok: found v2.21
Checking for DBI (v1.32) ok: found v1.32
Checking for DBD::mysql (v2.1010) ok: found v2.1017
Checking for File::Spec (v0.82) ok: found v0.82
Checking for File::Temp (any) ok: found v0.12
Checking for Template (v2.08) ok: found v2.08
Checking for Text::Wrap (v2001.0131) ok: found v2001.0131
The following Perl modules are optional:
Checking for GD (v1.20) ok: found v1.38
Checking for Chart::Base (v0.99) ok: found v0.99
Checking for XML::Parser (any) ok: found v2.31
Checking for GD::Graph (any) ok: found v1.35
Checking for GD::Text::Align (any) ok: found v1
Checking user setup ...
Comment 9•22 years ago
|
||
... and just to make this a bit more interesting, the correct cookie values for
Bugzilla_Login and Bugzilla_Loginccokie are 1 and 133 respectively. (with the
latter incrementing each time I do a new login) I see these values being sent
to the browser and back to the server.
I added
print STDERR qq[login "$login" "$login_cookie"\n];
to Authenticate()
When I run without this patch, I get...
login "1" "133"
When I apply this patch, I get...
login "2" "2"
login "2" "2"
I have no idea where these "2"s are coming from.
Assignee | ||
Comment 10•22 years ago
|
||
I really don't like overloading routines for fetch+get, especially when its not
easy to tell which is which from the arguments directly.
Anyway, this works, subject to the caveat in the comments.
Oh, and I sent a patch to the CGI.pm author with a workarround for that utf8
bug.
Attachment #120352 -
Attachment is obsolete: true
Comment 11•22 years ago
|
||
Comment on attachment 121058 [details] [diff] [review]
fix cookie fetching
r=joel
Works much better. Do elaborate (to developers??) on what the patch you
submitted to the CGI.pm author is.
Attachment #121058 -
Flags: review+
Assignee | ||
Comment 12•22 years ago
|
||
Nah, I'll just wait for upstream to apply it and then bump the required version.
If it takes a while, then I'll note it. Bascically, I replaces \s with |
\r\n\t|, and everything works. This skips out the unicode space chars, but thats
ok (It still works if I manually add those in, so its something else odd going
on here)
Assignee | ||
Comment 13•22 years ago
|
||
CGI 2.92 was released today, so require that to fix the bug
Attachment #121058 -
Attachment is obsolete: true
Assignee | ||
Comment 14•22 years ago
|
||
Comment on attachment 121897 [details] [diff] [review]
require CGI-2.92
Actually, we need 2.93 instead
Assignee | ||
Comment 15•22 years ago
|
||
Comment on attachment 121897 [details] [diff] [review]
require CGI-2.92
Actually, we need 2.93 instead because 2.92 breaks all mod_perl-1 apps using
CGI.pm. I've changed that locally.
joel, can you rubberstamp this, please? Its basically identical to the version
you already r='d.
Attachment #121897 -
Flags: review?(bugreport)
Comment 16•22 years ago
|
||
Comment on attachment 121897 [details] [diff] [review]
require CGI-2.92
r=joel
Attachment #121897 -
Flags: review?(bugreport) → review+
Assignee | ||
Updated•22 years ago
|
Flags: approval?
Updated•22 years ago
|
Flags: approval?
Comment 17•22 years ago
|
||
Comment on attachment 121897 [details] [diff] [review]
require CGI-2.92
because of the size of this patch I'd like a second review on it before it goes
in. Might be me if I get time, but I'll leave it blank in case someone else
beats me to it, since I probably won't get to it right away.
Attachment #121897 -
Flags: review?
Assignee | ||
Updated•22 years ago
|
Attachment #121897 -
Flags: review? → review?(justdave)
Comment 18•21 years ago
|
||
Comment on attachment 121897 [details] [diff] [review]
require CGI-2.92
I'm obviously not going to get time to touch this... we just did a release, so
we have time to get stable again. Joel's usually decent at this stuff anyway.
If it has problems let's file new bugs.
Attachment #121897 -
Flags: review?(justdave)
Updated•21 years ago
|
Flags: approval+
Assignee | ||
Comment 19•21 years ago
|
||
Checked in. Have fun!
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•21 years ago
|
Attachment #120352 -
Flags: review?(myk)
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•