237369 - Relatively simple changes from %FORM to $cgi->param variable

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Teemu Mannermaa (:wicked)]

Deprecated %FORM variables will be changed to use $cgi->param variables in:
 - config.cgi
 - globals.pl
 - report.cgi
 - reports.cgi
 - duplicates.cgi
 - showdependencygraph.cgi
 - showdependencytree.cgi
 - doeditparams.cgi
 - describecomponents.cgi
 - long_list.cgi

Comment 1

[Teemu Mannermaa (:wicked)]

Attachment: Changes V1

This changes the deprecated FORM/COOKIE vars. Also changes $::buffer global var
to $cgi->query_string() in duplicates.cgi.

All tests pass and changed scripts seem to be still working.

Comment 2

[Christian Reis]

Comment on attachment 144033 [details] [diff] [review]
Changes V1

>Index: config.cgi
>Index: globals.pl
>Index: report.cgi

These are OK (wanna split these and the other ones that are trivially okay --
see end of review -- into a separate bug)?

>Index: reports.cgi
>@@ -232,15 +234,17 @@
>     # Instead, just require that each field name consists only of letters
>     # and number
> 
>-    if ($FORM{'datasets'} !~ m/[A-Za-z0-9:]/) {
>-        die "Invalid datasets $FORM{'datasets'}";
>+    my $datasets = join('', $cgi->param('datasets'));

It might be nice to define this right after the check done for undefined
datasets, at the beginning of show_chart. Then you could..

> sub generate_chart {
>-    my ($data_file, $image_file, $type) = @_;
>+    my ($data_file, $image_file, $type, $product) = @_;
>     
>     if (! open FILE, $data_file) {
>         ThrowCodeError("chart_data_not_generated");
>@@ -261,7 +265,7 @@
> 
>     my @fields;
>     my @labels = qw(DATE);
>-    my %datasets = map { $_ => 1 } split /:/, $FORM{datasets};
>+    my %datasets = map { $_ => 1 } split /:/, join('', $cgi->param('datasets'));

.. pass datasets into generate_chart and avoid another join.

>     # Restrict to product if requested
>-    if ($::FORM{'product'}) {
>+    if (defined $cgi->param('product')) {
>         $params->param('product', join(',', @query_products));

Hmm, I think this check isn't for defined, but if it's actually set to
something that evaluates as true (otherwise doing "?product=&component=foo"
will trigger here)

>Index: showdependencygraph.cgi
>-$::FORM{'rankdir'} = "LR" if !defined $::FORM{'rankdir'};
>+$cgi->param('rankdir', "LR") if !defined $cgi->param('rankdir');

I would rather you used

  my $rankdir = $cgi->param('rankdir') || "LR";

And replaced the calls to cgi->param("rankdir") below..

>-if (!defined($::FORM{'id'}) && !defined($::FORM{'doall'})) {
>+if (!defined($cgi->param('id')) && !defined($cgi->param('doall'))) {
>     ThrowCodeError("missing_bug_id");
>-}    
>+}

nit: I don't like to see parenthesis used to call "define", but whatever.

>-if ($::FORM{'doall'}) {
>+if (defined $cgi->param('doall')) {
>     SendSQL("SELECT blocked, dependson FROM dependencies");

again, is it really defined here?

>Index: showdependencytree.cgi

OK too.

>Index: doeditparams.cgi
>-            $value = \@{$::MFORM{$name}};
>+            $value = [ $cgi->param($name) ];

Hmm, double-check to see if this does what we expect it does, and if so, ok.

>Index: describecomponents.cgi
>-    $::FORM{'product'} = (keys %products)[0];
>+    $cgi->param('product', (keys %products)[0]);
> }
> 
>-my $product = $::FORM{'product'};
>+my $product = $cgi->param('product');

This bit would probably be better written as

  my $product = (keys %products)[0];
} else {
  my $product = $cgi->param('product');
}

since the upper bit is for the if !defined bits.

>Index: long_list.cgi

OK too.

Comment 3

[Teemu Mannermaa (:wicked)]

Attachment: Changes V2

Revised patch as per review comments.

>>Index: doeditparams.cgi
>>-	       $value = \@{$::MFORM{$name}};
>>+	       $value = [ $cgi->param($name) ];
> Hmm, double-check to see if this does what we expect it does, and if so, ok.

I assume it does because justdave recommended to use it. :) Nevertheless, I did
some researching and it really does seem that both the old and new code makes
$value to be a reference to an unnamed array. I was going to test that bit of
code but it seems there is no type m parameters and thus that code path is
never executed.

Comment 4

[Christian Reis]

Comment on attachment 144274 [details] [diff] [review]
Changes V2

>Index: describecomponents.cgi
>-if (!defined $::FORM{'product'}) {
>+if (!$product) {

This should probably be 

  if (!defined $product) 

but that can be fixed when checking in.

Comment 5

[Christian Reis]

Significant cleanups to FORM and MFORM over the tree; would be nice to see it on
the trunk to catch any regressions sooner than later.

Comment 6

[Vlad Dascalu]

Checked in, fixed.

Patch for bug 237369: implement relatively simple changes from %FORM to $cgi-
>param variable; patch by Teemu Mannermaa <[e-mail]>; r=kiko, 
justdave; a=justdave.  

2004-03-26 13:02 jocuri%softhome.net  mozilla/ webtools/ bugzilla/ 
describecomponents.cgi  1.25  1/1
2004-03-26 13:00 jocuri%softhome.net  mozilla/ webtools/ bugzilla/ 
describecomponents.cgi  1.24  3/5   
2004-03-26 13:00 jocuri%softhome.net  mozilla/ webtools/ bugzilla/ 
doeditparams.cgi  1.30  3/5   
2004-03-26 13:00 jocuri%softhome.net  mozilla/ webtools/ bugzilla/ 
duplicates.cgi  1.42  8/10   
2004-03-26 13:00 jocuri%softhome.net  mozilla/ webtools/ bugzilla/ globals.pl  
1.260  2/1   
2004-03-26 13:00 jocuri%softhome.net  mozilla/ webtools/ bugzilla/ 
long_list.cgi  1.37  4/4   
2004-03-26 13:00 jocuri%softhome.net  mozilla/ webtools/ bugzilla/ report.cgi  
1.21  4/4   
2004-03-26 13:00 jocuri%softhome.net  mozilla/ webtools/ bugzilla/ reports.cgi  
1.70  24/20   
2004-03-26 13:00 jocuri%softhome.net  mozilla/ webtools/ bugzilla/ 
showdependencygraph.cgi  1.33  13/13   
2004-03-26 13:00 jocuri%softhome.net  mozilla/ webtools/ bugzilla/ 
showdependencytree.cgi  1.28  4/6   
2004-03-26 13:00 jocuri%softhome.net  mozilla/ webtools/ bugzilla/ config.cgi  
1.4  3/3