request.cgi should use $cgi-> instead of %::FORM

RESOLVED FIXED in Bugzilla 2.18

Status

()

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

People

(Reporter: Vlad Dascalu, Assigned: Vlad Dascalu)

Tracking

unspecified
Bugzilla 2.18
Bug Flags:
approval +

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

Description

14 years ago
request.cgi should use $cgi-> instead of $::FORM.
(Assignee)

Comment 1

14 years ago
I've got a diff in my tree. Need to check how accurate it is and prepate it for r?.
Blocks: 225818
No longer blocks: 234876
Status: NEW → ASSIGNED
(Assignee)

Comment 2

14 years ago
Created attachment 142591 [details] [diff] [review]
Version 1
(Assignee)

Updated

14 years ago
Target Milestone: --- → Bugzilla 2.18
(Assignee)

Comment 3

14 years ago
Created attachment 142854 [details] [diff] [review]
Version 2
(Assignee)

Updated

14 years ago
Attachment #142591 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 4

14 years ago
Comment on attachment 142854 [details] [diff] [review]
Version 2

>Index: request.cgi
> 
>+# initialize the $cgi variable
>+my $cgi = Bugzilla->cgi;

No need for the comment :)

>+    if (defined($cgi->param('requester')) && $cgi->param('requester') ne "") {

Don't need the parenthesis to defined here.. this happens in other parts of the
patch which would be nice to see removed.

>@@ -185,14 +190,14 @@
>         }
>         if (!$has_attachment_type) { push(@excluded_columns, 'attachment') }
>         
>-        push(@criteria, "flagtypes.name = " . SqlQuote($::FORM{'type'}));
>-        push(@excluded_columns, 'type') unless $::FORM{'do_union'};
>+        push(@criteria, "flagtypes.name = " . SqlQuote($form_type));
>+        push(@excluded_columns, 'type') unless $form_type;

Errr, unless do_union :-P

>+    my $form_group = $cgi->param('group');

[...]

>     $vars->{'excluded_columns'} = \@excluded_columns;
>-    $vars->{'group_field'} = $::FORM{'group'};
>+    $vars->{'group_field'} = $cgi->param('group');

Hmm, don't you want to use $form_group here?

Looks okay, but a hard read.
Attachment #142854 - Flags: review?(kiko) → review-
(Assignee)

Comment 5

14 years ago
Created attachment 142912 [details] [diff] [review]
Version 3

Thanks for your comments!

If you r+ it please a? it for me.
Attachment #142854 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 6

14 years ago
Comment on attachment 142912 [details] [diff] [review]
Version 3

Sorry I didn't catch this before: it's probably best to define $cgi in queue()
local's scope, and supply status and group as arguments to validate*.
Attachment #142912 - Flags: review?(kiko) → review-
(Assignee)

Comment 7

14 years ago
Created attachment 143006 [details] [diff] [review]
Version 4

Yeah, good catch!
(Assignee)

Updated

14 years ago
Attachment #142912 - Attachment is obsolete: true
(Assignee)

Updated

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

Updated

14 years ago
Attachment #143006 - Flags: review?(kiko) → review+
(Assignee)

Updated

14 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 8

14 years ago
Checking in request.cgi;
/cvsroot/mozilla/webtools/bugzilla/request.cgi,v  <--  request.cgi
new revision: 1.11; previous revision: 1.10
done
Checking in template/en/default/request/queue.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/request/queue.html.tmpl,v
 <--  queue.html.tmpl
new revision: 1.10; previous revision: 1.9
done
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Summary: request.cgi should use $cgi-> instead of $::FORM → request.cgi should use $cgi-> instead of %::FORM
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.