147833 - Start using CGI.pm

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P3)

Description

[Jouni Heikniemi]

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.

Comment 1

[Jouni Heikniemi]

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.

Comment 2

[Bradley Baetz (:bbaetz)]

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.

Comment 3

[Bradley Baetz (:bbaetz)]

Attachment: v1

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

Comment 4

[Bradley Baetz (:bbaetz)]

Attachment: v2

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

Comment 5

[Gervase Markham [:gerv]]

> 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

Comment 6

[Bradley Baetz (:bbaetz)]

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

Comment 7

[Bradley Baetz (:bbaetz)]

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?

Comment 8

[Bradley Baetz (:bbaetz)]

Attachment: v3

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.

Comment 9

[Gervase Markham [:gerv]]

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

Comment 10

[Bradley Baetz (:bbaetz)]

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

[Gervase Markham [:gerv]]

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

Comment 12

[Bradley Baetz (:bbaetz)]

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.

Comment 13

[Bradley Baetz (:bbaetz)]

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

Comment 14

[Dave Miller [:justdave]]

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

Comment 15

[Bradley Baetz (:bbaetz)]

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.

Comment 16

[Bradley Baetz (:bbaetz)]

Attachment: fix bitrot

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

Comment 17

[Dave Miller [:justdave]]

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.

Comment 18

[Bradley Baetz (:bbaetz)]

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

Comment 19

[Dave Miller [:justdave]]

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.

Comment 20

[Dave Miller [:justdave]]

Attachment: fix bitrot, take 2

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

Comment 21

[Bradley Baetz (:bbaetz)]

Attachment: v5

Bah, must have uplodaed an old version.

This also fixes the Search.pm thing described earlier

Comment 22

[Dave Miller [:justdave]]

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.

Comment 23

[Bradley Baetz (:bbaetz)]

Attachment: v5.1

Fixes CGI.pm's broken version fu.

Comment 24

[Bradley Baetz (:bbaetz)]

Comment on attachment 103434 [details] [diff] [review]
v5.1

carrying forward r=justdave

Comment 25

[Gervase Markham [:gerv]]

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

Comment 26

[Bradley Baetz (:bbaetz)]

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.

Comment 27

[Bradley Baetz (:bbaetz)]

Marking as fixed, too....