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

RESOLVED FIXED in Bugzilla 2.18

Status

()

Bugzilla
Bugzilla-General
RESOLVED FIXED
14 years ago
5 years ago

People

(Reporter: wicked, Assigned: wicked)

Tracking

2.17.7
Bugzilla 2.18
Bug Flags:
approval +

Details

Attachments

(1 attachment, 1 obsolete attachment)

16.57 KB, patch
Christian Reis
: review+
justdave
: review+
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
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
(Assignee)

Comment 1

14 years ago
Created attachment 144033 [details] [diff] [review]
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.
(Assignee)

Updated

14 years ago
Attachment #144033 - Flags: review?(kiko)

Updated

14 years ago
Target Milestone: --- → Bugzilla 2.18

Comment 2

14 years ago
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.
Attachment #144033 - Flags: review?(kiko) → review-
(Assignee)

Comment 3

14 years ago
Created attachment 144274 [details] [diff] [review]
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.
Attachment #144033 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #144274 - Flags: review?(kiko)

Updated

14 years ago
Status: NEW → ASSIGNED

Comment 4

14 years ago
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.
Attachment #144274 - Flags: review?(kiko) → review+

Comment 5

14 years ago
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.
Flags: approval?
Flags: approval? → approval+

Comment 6

14 years ago
Checked in, fixed.

Patch for bug 237369: implement relatively simple changes from %FORM to $cgi-
>param variable; patch by Teemu Mannermaa <wicked@etlicon.fi>; 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   
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.