Closed Bug 201816 Opened 21 years ago Closed 21 years ago

use for header output


(Bugzilla :: Bugzilla-General, defect)

Not set



Bugzilla 2.18


(Reporter: bbaetz, Assigned: bbaetz)




(1 file, 2 obsolete files)

We should use 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,

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. 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 - when trying to print the headers, it will just
return if they're already been printed. This obviously only works if everywhere
uses, and noone prints manually.
Attached patch patch (obsolete) — Splinter Review
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 brings it in.

I also changed the cookie expiration dates to have the correct day, and changed
them while I was at it.
Attachment #120352 - Flags: review?(myk)
Target Milestone: --- → Bugzilla 2.18
One shouldn't forget to update
if this gets in.
(Setting   $self->charset('');  in Bugzilla/ is less clumpsy than the
previous perl -pi -e  -- and I paves the way for UTF-8/bug 126266 :-)
Blocks: 201955
Oh, and I moved the contenttypes stuff out of localconfig while I was there.
I'd rather not break utf8 personally... what's the workaround?
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 on attachment 120352 [details] [diff] [review]

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

Incidentally, the cookie it tried to set on my machine is....
This is still pretty toxic.  In prder to get the correct cookie set,
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 ...  
... 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.

Attached patch fix cookie fetching (obsolete) — Splinter Review
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 author with a workarround for that utf8
Attachment #120352 - Attachment is obsolete: true
Comment on attachment 121058 [details] [diff] [review]
fix cookie fetching

Works much better.  Do elaborate (to developers??) on what the patch you
submitted to the author is.
Attachment #121058 - Flags: review+
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)
Attached patch require CGI-2.92Splinter Review
CGI 2.92 was released today, so require that to fix the bug
Attachment #121058 - Attachment is obsolete: true
Comment on attachment 121897 [details] [diff] [review]
require CGI-2.92

Actually, we need 2.93 instead
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 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 on attachment 121897 [details] [diff] [review]
require CGI-2.92

Attachment #121897 - Flags: review?(bugreport) → review+
Flags: approval?
Flags: approval?
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?
Attachment #121897 - Flags: review? → review?(justdave)
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)
Flags: approval+
Checked in. Have fun!
Closed: 21 years ago
Resolution: --- → FIXED
Attachment #120352 - Flags: review?(myk)
Depends on: 204964
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.