We should aim at getting rid of our custom CGI.pl and start using the standard CGI module. See bug 145030 for additional comments on the issue.

Regarding the issue mentioned in bug 145030 comment 12 (USE cgi without parameters hangs on Win32): I believe there is some TT-internal problem with using the CGI.pm. I wrote about this to the TT list and Randal Schwartz replied saying he had a similar problem. See his post: http://www.template-toolkit.org/pipermail/templates/2002-May/003161.html We should consider this when moving on to using CGI plugin from TT.

We should definately do this for parsing, but I don't think that this is wanted for output. TT gives us more control over output than CGI.pm.

Use CGI.pm for parsing. checksetup was updated to require a newer version of CGI, and since CGI includes CGI::Carp, I dropped that. Note that upgrading to CGI v2.86 (one release after what this patch requires) will mean that you lose informative error messages from TT. This is a perl core parser bug/misfeature - see http://www.xray.mpe.mpg.de/mailing-lists/perl5-porters/2002-09/msg01204.html. I've posted a patch to the TT list, but this this only affects error output, I don't think that its worth having checksetup.pl error out on this case. I've kept in code for backwards compatability (ie FORM/MFORM/COOKIE), but the file upload API was too different to convert easily, so I modified attachment.cgi to use that, instead The other difference is that because of the way the old code handled empty cgi params, the old code didn't taint them. CGI.pm does, which explains the small process_bug change I had to make. I also had to change Bugzilla/Search.pm and buglist.cgi. This can't use a tied hash, because FETCH is called in scalar cotext, and so returning the array would be impossible. As a result, its a bit ugly, but the cleanups from removing the %M stuff balance this out, right? :)

Additional changes from before: - update to get report.cgi changes - move CanonicaliseParams to CGI::Bugzilla, and rename - I also renamed the template var names. - move url_quote to Bugzilla::Util. I deliberatly left url_decode in CGI.pl, because I want to remove it in the next round of CGI stuff. url_encode may eventually be removed, but that will need edit* to be templatised first, so.... - document Bugzilla::CGI.pm (and change it to override new, not the undocumented init) - previously, $::COOKIE vals were quoted, and users had to manually unquote. Most of them didn't (although it didn't make a difference for those cases). CGI.pm does unescape them, so I fixed the few users to be correct

> Index: CGI.pl > =================================================================== I note you've left the shutdownhtml stuff in. Where's that going to live eventually? > +# Set up stuff for compatability with the old CGI.pl code Compatibility :-) > +# This code will be removed as soon as possible, in favour of > +# using the CGI.pm stuff directly > + > +# XXX - mod_perl - reset these between runs I _really_ wish you wouldn't do this :-) Can we not have a "change these things for mod_perl" document or bug somewhere? This is really ugly.. > +foreach my $name ($::cgi->param()) { > + my @val = $::cgi->param($name); > + $::FORM{$name} = join('', @val); I'm not convinced this is right. Surely, for multi-valued form fields, $::FORM{$name} contains just one of them? > Index: attachment.cgi > =================================================================== > use lib qw(.); > > use vars qw( > + $cgi Ironic that in our quest to eliminate global vars, we are creating more :-) > -# Win32 specific hack to avoid a hang when creating/showing an attachment > -if ($^O eq 'MSWin32') { > - binmode(STDIN); > - binmode(STDOUT); > -} > - Why has this gone? > + # We could get away with reading only as much as required, except that then > + # we wouldn't have a size to print to the error handler below. > + while (<$fh>) { > + $data .= $_; > } I'm sure there's a way to slurp an entire filehandle into a string using array context... > Index: buglist.cgi > =================================================================== > +# The params object to use for the actual query itsself > +# This will be modified, so make a copy > +my $params = new Bugzilla::CGI($cgi); Do you mean "the CGI object"? And what is "the actual query itself"? Having read below, calling this object $params is highly confusing. What's wrong with $form or $request? People understand that :-) > Index: report.cgi > =================================================================== > +# Clone the params, so that Bugzilla::Search can modify them > +my $params = new Bugzilla::CGI($cgi); Now that's a better comment :-) Now I know what's going on. "params" is still a very strange name for an object representing an HTTP request, though. > -SendSQL($query, $::userid); > +SendSQL($query); Is this because $::userid is assumed to be the current user if it's not given, so the second arg is not needed? > -$::buffer =~ s/format=[^&]*&?//g; > +$cgi->delete('format'); Ooh, I like this :-) > # Calculate the base query URL for the hyperlinked numbers > -$vars->{'buglistbase'} = CanonicaliseParams($::buffer, > - ["x_axis_field", "y_axis_field", "z_axis_field", @axis_fields]); > -$vars->{'buffer'} = $::buffer; > +$vars->{'querybase'} = $cgi->canonicalise_query("x_axis_field", > + "y_axis_field", > + "z_axis_field", > + @axis_fields); There's no evidence on the interface that this is an exclude list. I know it's long winded, but canonicalise_query_and_exclude()? > Index: Bugzilla/CGI.pm > =================================================================== > +# CGI.pm does this, and its AUTOLOAD doesn't handle it, so we need to, > +# otherwise AUTOLOAD is called for our DESTROY, and we fail > +sub DESTROY {}; Does _what_? :-) Please be a bit more explanatory; not everyone has your deep knowledge of Perl module internals :-) > +sub new { > + my ($invocant, @args) = @_; > + my $class = ref($invocant) || $invocant; > + > + my $self = $class->SUPER::new(@args); > + > + # Check for errors > + # All of the Bugzilla code wants to do this, so do it here instead of > + # in each script > + > + # XXX - once we have working exceptions, rewrite this to use them We're _writing_ a bug tracking system, yet our requests for enhancement are comments in the code? :-) (Yes, I am going to keep banging on about this. :-) > + > + my $err = $self->cgi_error; > + > + if ($err) { > + # XXX - under mod_perl we can use the request object to > + # enable the apache ErrorDocument stuff, which is localisable > + # (and localised by default under apache2) > + # This doesn't appear to be possible under mod_cgi > + # Under mod_perl v2, though, this happens automatically, and the > + # message body is ignored. > + > + # Note that this error block is only triggered by CGI.pm for malformed > + # multipart requests, and so should never happen unless there is a > + # browser bug More full stops? :-) > + > + # Using CGI.pm to do this means that ThrowCodeError prints the > + # content-type again... Not if you set $vars->{'header_done'} to 1 :-) > +# We want this sorted, apparently, plus the ability to exclude > +# certain params "apparently"? :-) That's going to confuse someone in six months time. "We want this sorted so the resulting strings can be compared for equality." > + Bugzilla::CGI - CGI handling for bugzilla _B_ugzilla :-) > +After creating the CGI object, C<Bugzilla::CGI> automatically checks > +I<cgi_error>, and throws an CodeError if a problem is detected. a CodeError. > Index: Bugzilla/Util.pm > =================================================================== > +=item C<url_quote($val)> > + > +Quotes characters so that they may be included as part of a url. > + > +=cut > + > +# This orignally came from CGI.pm, by Lincoln D. Stein If it did, why aren't we using his version now? :-) Gerv

> <shutdownhtml> DBI.pm, I think. Maybe. I need to thik about that before doing anything with it. Separate bug :) > <mod_perl annotations> No, I think this is useful. Its trivial to find them all witha grep, and its then really clear where the bug is. If we have it somewehere else, then line numbers get out of date, and so on. > I'm not convinced this is right. Surely, for multi-valued form fields, > $::FORM{$name} contains just one of them? No, that would be the sensible thinkg to do. Look at what the old code does... Yes, this sucks, and I can't see anyone who relies on it, not to mention that relying on it probablly wouldn't do anything, but I decided to keep bug compatability for now. > <global $::cgi> The problem is that I have to do this globally, for the compat code. However, creating a new CGI object isn't something which can be done twice (once in CGI.pl, and once in th .cgi), because for a POST request, we have to read from stdin, and that can only happen once - the sceond time you try, you wouldn't get the data. > <binmode> CGI.pm does this (and the $| = 1, which I also removed from CGI.pl) > <naming of cgi object> Well, I could use $query, but that conflicts with too many other scripts, who use that to assemble sql stuff. Its not only the form - cookie stuff and so on is there. Its not truly a 'request' either, since we will eventually use it for output. When I called it params, in that context its only being used to access ->params, and the fact that you could call other methods oin it is just an optional extra. This allows us to eventually have a Bugzilla::CmdLine (or Bugzilla:MailFoo, or whatever) with ->params as an option. > <set $vars->{header-done} to 1> That doesn't work, because that then the header.html.tmpl tempalte isn't processed, so we don't get the banner in the error. header_done will be going away eventually (the cgi object knows if we've printed a content type....) > "We want this sorted so the resulting strings can be compared for equality." Yeah, but since you tack stuff onto the end in the template, thats not stictly true. I still don't know why we want to compare them for equality, and in any event, getting the query string from the CGI object gets them out in hash key order, which is stable across perl version. We could then use ->delete.... > <url_quote from CGI.pm> If it did, why aren't we using his version now? :-) Because CGI::Util is labeled as having no user-accessible parts inside. As I said in my comments, this will go eventually, and maybe it ssafe to use it from inside Bugzilla::CGI. I'd have to check, but theres no point while edit* still use it.

Oops, forgot the sendsql comment: > > -SendSQL($query, $::userid); > > +SendSQL($query); > Is this because $::userid is assumed to be the current user if it's not given, > so the second arg is not needed? No, its because the second argument to sendsql is not a userid, but rather a flag - queries with that flag aren't manually replicated to the shadowdb. Thats not an issue for queries... Did you mean something else here?

Note that CGI.pm-2.87 will die with a taint error on uploaded files. I've mailed the author, who is going to fix this.

>> <mod_perl annotations> > > No, I think this is useful. Its trivial to find them all witha grep, and its > then really clear where the bug is. If we have it somewehere else, then line > numbers get out of date, and so on. Not if you say things like "We need to convert to using local variables in the conversion section of foo.cgi". The same criticism could be applied to any technical RFE. > No, that would be the sensible thinkg to do. Look at what the old code does... > Yes, this sucks, and I can't see anyone who relies on it, not to mention that > relying on it probablly wouldn't do anything, but I decided to keep bug > compatability for now. OK, if you add a comment so it doesn't confuse other people. >> <global $::cgi> > > The problem is that I have to do this globally, for the compat code. However, I was only teasing :-) >> <naming of cgi object> > > Well, I could use $query, but that conflicts with too many other scripts, who > use that to assemble sql stuff. Its not only the form - cookie stuff and so on > is there. Its not truly a 'request' either, since we will eventually use it for > output. Then how is it "params"? :-) And using it for output also is a really weird idea. How does that work? > When I called it params, in that context its only being used to access ->params, > and the fact that you could call other methods oin it is just an optional extra. > This allows us to eventually have a Bugzilla::CmdLine (or Bugzilla:MailFoo, or > whatever) with ->params as an option. Call it $request. If we start using it for output, we can do: var $response = $request; I'm serious. $params isn't good. :-) >> "We want this sorted so the resulting strings can be compared for equality." > > Yeah, but since you tack stuff onto the end in the template, thats not stictly > true. It is certainly true that the outputs of this function can be string-compared for equality. What you do to them after you get them back is your business. > I still don't know why we want to compare them for equality, and in any In order to avoid running the same query for graphing more than once. Yes, graphing doesn't exist yet :-) If you can think of a better way of comparing two Bugzilla queries, I'd love to hear about it. Gerv

> Not if you say things like "We need to convert to using local variables in the > conversion section of foo.cgi". The same criticism could be applied to any > technical RFE. Sure. Thats not where the stuff I'm commenting comes from though (and there already is a bug on converting those sorts of things). This isn't an RFE - its a specific indication of something which may be a bug. I've already filed a bug on cleaning up all thse comments (bug 173897), and its likely that they're all going to be done in one go (maybe two, but certainly not one-by-one) > Then how is it "params"? :-) Because from within Bugzilla/Search.pm, thats all it is. Bugzilla::Search shouldn't be using CGI stuff, and we could easily pass in any other object which got the 'params' somehow (and we do, in the case of stored queries - that fact that that object happens to hte same class is a temporary impl detail which people must not rely on. > And using it for output also is a really weird > idea. How does that work? my $cookie = $cgi->cookie(-name='BZ_FOO', -value=[a,b], # yes, you can 'store' arrays. ); $cgi->header(-type => text/rdf, -cookie => $cookie); or, in the simple case: $cgi->header('text/plain'); or even: $cgi->header; # defaults to text/html We can also do rediections/serverside-push/etc this way Note that I dislike that cookie api, and will probably use an 'add_cookie' function, so that we don't need to pass arround the cookie object, and thus have all our callers of the login functions having to keep that. |header| will then automatically send any cookies added in that way. |perldoc CGI| is your friend. We really do need to do this for mod_perl. Its not essential, but its easier, and almost certainly more efficient, too (otherwise apache has to parse the output, just like it does for the mod_cgi case) I don't want to use CGI.pm for outputing the html, although it can do that - I think our TT stuff is more flexible. > Call it $request. If we start using it for output, we can do: > var $response = $request; No, that would be ugly. The two objects arne't the same, so why treat them as if they are? Its a cgi object, I don't see whats wrong with calling it $cgi.

Comment on attachment 102578 [details] [diff] [review] v3 > I've already filed a bug on cleaning up all thse comments (bug 173897), That's a cop-out, though. "Bug: please fix all the bugs I commented about in the code rather than filing one or multiple bugs about them." If they are too small to merit a bug each, file a bug about a lot of the issues at once. But I can see I'm not going to win this one, so I'll register a continuing objection to its general ugliness, an exhortation to restrict its use to mod_perl related things, and let it go at that. > > Then how is it "params"? :-) > > Because from within Bugzilla/Search.pm, thats all it is. In which case, you should have no objection to calling it $request, because your objection to that name was that "Its not truly a 'request' either, since we will eventually use it for output." +my $request = new Bugzilla::CGI($cgi); in buglist.cgi, (and perhaps a similar name-change in Search.pm too; but it's buglist.cgi where +my $params = new Bugzilla::CGI($cgi); causes me great confusion.) > No, that would be ugly. The two objects arne't the same, so why treat them as if > they are? Its a cgi object, I don't see whats wrong with calling it $cgi. There's nothing wrong with $cgi. It's $params I object to. :-) But again, although I do wish you'd change it, at least in buglist.cgi, I can't force you to, and it's not worth withholding a review over. This does need a 2r=, though. Gerv

Its a global var (for the moment) - it has to be $::cgi in all the scripts. In buglist.cgi/report.cgi, I make a copy. I can't call it $query, because thats used lower down for the sql. Theres no reason that it has to be a Bugzilla::CGI object, it just happens that thats the easiest/only way of parsing stored queries. I'd prefer it to be a semi-opaque type, and if perl was strongly typed, I'd cast it to anther type immediatly after creation to make that obvious. Anyone trying to use it to store cookies is going to have problems.

-> me Note that the latest CGI.pm version (2.87) doesn't work with uploads if taint mode is on. This is fixed in a devel-2.88 version teh author sent to me after I reported this bug. IOW, if you need to upgrade for this, don't upgrade to teh latest version just yet. Theres no need to document this, since 2.88 should be out RSN (probably before this is ready to go in, in fact)

That means we need to change checksetup.pl to require version 2.88 of CGI then, right?

No, we don't require that. The prpoblem was that people woudl jsut use CPAN to update to the latest version, and it would fail. However, since 2.88 includes tainting for POST'd files, we probably do want that.

Just fixes bitrot, and requires CGI.pm-2.88, as discussed.

Comment on attachment 103110 [details] [diff] [review] fix bitrot >Index: Bugzilla/Search.pm >=================================================================== >@@ -96,8 +93,8 @@ > } > > my $minvotes; >- if (defined $F{'votes'}) { >- my $c = trim($F{'votes'}); >+ if ($params->param('votes')) { >+ my $c = trim($params->param('votes')); > if ($c ne "") { > if ($c !~ /^[0-9]*$/) { > $::vars->{'value'} = $c; Shouldn't this keep the "defined"? If the field is passed, but has a 0 in it, this will fail if you don't test if it's defined first. That's the only thing that jumps out at me after visual inspection of the patch. I haven't had a chance to test it yet.

Well, if I want votes > 0, then we should check for them :) I guess not doing that is an optimisation, though.

Comment on attachment 103110 [details] [diff] [review] fix bitrot This is now live (with conflicts resolved) at http://landfill.bugzilla.org/bz147833/ when trying to upload an attachment, got the following: Software error: Can't find param named maxsize at Bugzilla/Config.pm line 124.

This is the patch that's applied on landfill currently, for inspection purposes in case I resolved a conflict incorrectly or something

Bah, must have uplodaed an old version. This also fixes the Search.pm thing described earlier

Comment on attachment 103425 [details] [diff] [review] v5 OK, this works, with the exception of the problem we discovered on Landfill with how the CGI author decided to number his versions... (2.752 is not < 2.88 because the period is a separator, not a decimal, so the version check said we were OK when we weren't) r=justdave with that hack to the version check we just discussed on IRC requesting a second review since, although the patch is relatively small, it has a major potential impact if anything goes wrong.

Fixes CGI.pm's broken version fu.

Comment on attachment 103434 [details] [diff] [review] v5.1 carrying forward r=justdave

Comment on attachment 103434 [details] [diff] [review] v5.1 r=gerv. Please check this in ASAP, as I want to merge it with other patches :-) Gerv

Checked in, w/o the sidebar.cgi bit (a=justdave for that) I didn't realise that the login stuff set that to a conanical (unquoted) value. bleh.

Marking as fixed, too....