Closed Bug 238876 Opened 16 years ago Closed 15 years ago

remove %FORM from process_bug.cgi

Categories

(Bugzilla :: Bugzilla-General, defect, P2)

2.17.7
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: justdave, Assigned: wicked)

References

Details

Attachments

(1 file, 2 obsolete files)

This is going to be nasty nasty difficult.  On par with attachment.cgi.

This is also blocked on the hidden-fields conversion (will add a dependency in a
few minutes) due to the mid-air collision handling. (that has to happen first)
Depends on: 238878
Blocks: 236678
No longer blocks: 225818
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
For the latest patch (and subsequent activity), see bug#238870, which contains a combined patch for 
process_bug, attachment, hidden fields and flags
Depends on: 251935
No longer blocks: 236678
Blocks: 225818
No longer depends on: 251935
Summary: remove %FORM and %COOKIE from process_bug.cgi → remove %FORM from process_bug.cgi
I'm doing COOKIE separately.
Hey Fred, you said that you knew process_bug pretty well, now... :-)
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.20
This bug has not been touched by its owner in over six months, even though it is
targeted to 2.20, for which the freeze is 10 days away. Unsetting the target
milestone, on the assumption that nobody is actually working on it or has any
plans to soon.

If you are the owner, and you plan to work on the bug, please give it a real
target milestone. If you are the owner, and you do *not* plan to work on it,
please reassign it to nobody@bugzilla.org or a .bugs component owner. If you are
*anybody*, and you get this comment, and *you* plan to work on the bug, please
reassign it to yourself if you have the ability.
Target Milestone: Bugzilla 2.20 → ---
Depends on: 287947
Attached patch Remove FORMs, V1 (obsolete) — Splinter Review
This patch changes FORM hash to CGI object in process_bug.cgi and related
templates. This is based on work by GavinS and is diffed against patch in bug
287947 (which is needed to apply cleanly).
Assignee: bugzilla → wicked
Attachment #179462 - Flags: review?(LpSolit)
Comment on attachment 179462 [details] [diff] [review]
Remove FORMs, V1

>@@ -147,9 +165,9 @@
>-if (defined $::FORM{'id'}) {
>-    Bugzilla::Flag::validate(\%::FORM, $::FORM{'id'});
>-    Bugzilla::FlagType::validate(\%::FORM, $::FORM{'id'});
>+if (defined $cgi->param('id')) {
>+    Bugzilla::Flag::validate(\%::FORM, $cgi->param('id'));
>+    Bugzilla::FlagType::validate(\%::FORM, $cgi->param('id'));

\%::FORM should be replaced by $cgi.


>@@ -162,12 +180,13 @@
>-    if (defined $::FORM{'delta_ts'} && $delta_ts && 
>-        $::FORM{'delta_ts'} ne $delta_ts) 
>+    if (defined $cgi->param('delta_ts') && $delta_ts &&
>+        $cgi->param('delta_ts') ne $delta_ts)

Nit: $delta_ts is defined when the bug is created and is taken from the DB
directly. Then we know it always exists and "&& $delta_ts" is useless. I know
this part of the code was already here, but this can be safely removed, IMO.


>@@ -487,19 +511,21 @@
>-    my $dupe = $::FORM{'id'};
>-    my $original = $::FORM{'dup_id'};
>+    # Remember that we validated both these ids earlier, so we know
>+    # they are both valid bug ids
>+    my $dupe = trim($cgi->param('id'));
>+    my $original = trim($cgi->param('dup_id'));

'id' and 'dup_id' have already been trimmed (and detainted) by ValidateBugID()
called earlier. Then trim() is useless.


>@@ -646,20 +668,20 @@
>-    if (!$::FORM{'dontchange'}
>-        || $str ne $::FORM{'dontchange'})
>+    if ($cgi->param('dontchange')
>+        || $str ne $cgi->param('dontchange'))

Warning: you removed the negation "!" here!!


>@@ -1004,14 +1035,14 @@
>-        ValidateBugID($::FORM{'dup_id'}, 'dup_id');
>+        ValidateBugID($cgi->param('dup_id'), 'dup_id');

ValidateBugID() will alter its first argument, so you should not give him
$cgi->param('dup_id') directly.


>-        $duplicate = $::FORM{'dup_id'};
>-        if (!defined($::FORM{'id'}) || $duplicate == $::FORM{'id'}) {
>+        $duplicate = trim($cgi->param('dup_id'));
>+        if (!defined $cgi->param('id') || $duplicate == $cgi->param('id')) {

$cgi->param('dup_id') is already trimmed by ValidateBugID above.


>@@ -1020,25 +1051,21 @@
>-if ($#idlist < 0) {
>-    ThrowUserError("no_bugs_chosen");
> }

Nice catch! This test is useless as it is already done at line 96! :)


>@@ -1394,23 +1421,23 @@
>-        $work_time = $::FORM{'work_time'};
>+        $work_time = $cgi->param('work_time');
>         if ($work_time) {
>             # AppendComment (called below) can in theory raise an error,
>             # but because we've already validated work_time here it's
>             # safe to log the entry before adding the comment.
>-            LogActivityEntry($id, "work_time", "", $::FORM{'work_time'},
>+            LogActivityEntry($id, "work_time", "", $cgi->param('work_time'),

Nit: use $work_time instead of $cgi->param('work_time') as a param as we have
just defined it.


>@@ -1462,9 +1489,10 @@
>     my $newproduct_id = $oldhash{'product_id'};
>-    if ((defined $::FORM{'product'})
>-        && ($::FORM{'product'} ne $::FORM{'dontchange'})) {
>-        my $newproduct_id = get_product_id($::FORM{'product'});
>+    if ((defined $cgi->param('product'))
>+        && ($cgi->param('product') ne $cgi->param('dontchange')))
>+    {
>+        my $newproduct_id = get_product_id($cgi->param('product'));
>     }

$newproduct_id is already declared above. Remove the second "my".


>@@ -1612,12 +1643,11 @@
>-    if ( 
>-      defined $::FORM{'product'} 
>-        && $::FORM{'product'} ne $::FORM{'dontchange'} 
>-          && $::FORM{'product'} ne $oldhash{'product'} 
>+    if (defined $cgi->param('product')
>+        && $cgi->param('product') ne $cgi->param('dontchange')
>+        && $cgi->param('product') ne $oldhash{'product'}
>     ) {
>-        my $newproduct_id = get_product_id($::FORM{'product'});
>+        my $newproduct_id = get_product_id($cgi->param('product'));

Remove this last line completely. $newproduct_id has already been defined
previously.
Attachment #179462 - Flags: review?(LpSolit) → review-
Attached patch Remove FORMs, V2 (obsolete) — Splinter Review
Most comments addressed. Only removed the my from the third $newproduct_id
declaration. Other changes to those vars need more eyes/opinions. Also this
patch is now diffed against both dependent patches.
Attachment #179462 - Attachment is obsolete: true
Attachment #179894 - Flags: review?(LpSolit)
Comment on attachment 179894 [details] [diff] [review]
Remove FORMs, V2

>--- process_bug.cgi-withdeps	2005-04-07 01:27:35.000000000 +0300
>+++ process_bug.cgi	2005-04-07 02:24:04.000000000 +0300

>@@ -1005,14 +1036,14 @@
>     /^duplicate$/ && CheckonComment( "duplicate" ) && do {
>         # Make sure we can change the original bug (issue A on bug 96085)
>         CheckFormFieldDefined($cgi, 'dup_id');
>-        ValidateBugID($::FORM{'dup_id'}, 'dup_id');
>+        $duplicate = $cgi->param('dup_id');
>+        ValidateBugID($duplicate, 'dup_id');
> 
>         # Also, let's see if the reporter has authorization to see
>         # the bug to which we are duping. If not we need to prompt.
>         DuplicateUserConfirm();

You have to update $cgi->param('dup_id') as DuplicateUserConfirm() uses it
later. Please update your patch by adding $cgi->param('dup_id', $duplicate);
after the validation. I can also do that on checkin, but I would prefer a new
patch so that I don't forget to fix it. ;)


This a very good job! Thanks! r=LpSolit
Attachment #179894 - Flags: review?(LpSolit) → review+
Flags: approval?
Target Milestone: --- → Bugzilla 2.20
Attachment #179894 - Attachment is obsolete: true
Attachment #179934 - Flags: review?(LpSolit)
Comment on attachment 179934 [details] [diff] [review]
Remove FORMs, V2.1

r=LpSolit
Attachment #179934 - Flags: review?(LpSolit) → review+
Flags: approval? → approval+
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.248; previous revision: 1.247
done
Checking in template/en/default/bug/process/confirm-duplicate.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/confirm-duplicate.html.tmpl,v
 <--  confirm-duplicate.html.tmpl
new revision: 1.9; previous revision: 1.8
done
Checking in template/en/default/bug/process/midair.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/midair.html.tmpl,v
 <--  midair.html.tmpl
new revision: 1.13; previous revision: 1.12
done
Checking in template/en/default/bug/process/verify-new-product.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/verify-new-product.html.tmpl,v
 <--  verify-new-product.html.tmpl
new revision: 1.15; previous revision: 1.14
done
Status: ASSIGNED → RESOLVED
Closed: 15 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.