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.

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.

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 :-)

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

Incidentally, the cookie it tried to set on my machine is.... "<param>cookiepath</param>" :-)

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

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

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.

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.

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)

CGI 2.92 was released today, so require that to fix the bug

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 CGI.pm. I've changed that locally. joel, can you rubberstamp this, please? Its basically identical to the version you already r='d.

Comment on attachment 121897 [details] [diff] [review] require CGI-2.92 r=joel

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.

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.

Checked in. Have fun!