Closed Bug 225818 Opened 16 years ago Closed 15 years ago

%FORM, %MFORM, and %COOKIE need to go away, in favor of the CGI methods

Categories

(Bugzilla :: Bugzilla-General, defect)

2.17.5
defect
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: justdave, Assigned: wicked)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

We learned while implementing mod_perl for Zippy that it's easier to replace all
the occurrances of %FORM, %MFORM, and %COOKIE with the proper %cgi->param or
$cgi->cookie usage than it is to try to get %FORM, %MFORM, and %COOKIE exported
from some other namespace and still be visible the way we use it.  Since $cgi is
accessible from anywhere via Bugzilla->cgi already, it's just easier that way.

Things to watch out for:
  - In-memory modification of form fields and cookies:
    $cgi->param("foo") = "blah"  doesn't work.  Have to use:
    $cgi->param(-name=>"foo",-value=>"blah"); or better yet assign it to a local
    variable and change all future references to use the variable if the changed
    value is only being used in that scope.  If the changed value will be needed
    in some other module (such as Search or authentication stuff) then it'll be
    better to still modify the param list itself.
  - Use of $::FORM{foo} in a list context (such as in a parameter list to
    another sub) - $cgi->param("foo") needs to be wrapped in scalar() in this
    case to guarantee we get an undef instead of an empty list if the value
    hadn't been passed at all

\%::FORM or \%::MFORM can be replaced with $cgi->Vars.

Some useful regexps:

$::FORM{} ||= foo :
Find: \$::FORM{([^}]*)}\s*\|\|=\s*([^;]*)
Replace: $cgi->param(-name => \1, -value => $cgi->param(\1) || \2)

$::FORM{} = foo :
Find: \$::FORM{([^}]*)}\s*=\s*([^;]*)
Replace: $cgi->param(-name => \1, -value => \2)

foo = $::FORM{} :
Find: \$::FORM{([^}]*)}
Replace: $cgi->param(\1)

The above work in the search/replace dialog in BBEdit.  If you take on part of
this, make sure you replace one at a time, then find next and replace, and
verify them as you go.  There's not a 100% guarantee that those will Do The
Right Thing in all cases and you'd need to catch the edge cases.
Depends on: 234171
Depends on: 234875
Depends on: 234876
Depends on: 234879
Depends on: 234896
Depends on: 234898
Depends on: 235175
additional information I've learned since this was originally posted....

>   - In-memory modification of form fields and cookies:
>     $cgi->param("foo") = "blah"  doesn't work.  Have to use:
>     $cgi->param(-name=>"foo",-value=>"blah");

This actually works as $cgi->param("foo","blah");  (the named parameters to
param() are optional)

> \%::FORM or \%::MFORM can be replaced with $cgi->Vars.

This actually isn't a good match.

Most of the places that are using \%::FORM or \%::MFORM just need to be
rewritten.  $cgi->params  will return an array of all of the field names passed.
 Looping through that array and using $cgi->param($name) in list context will
return an array of the values that were returned (like $::MFORM{$name}), and
using it in scalar context will return them concatenated (like $::FORM{$name}).
Depends on: 235222
Depends on: 235268
Depends on: 235278
Depends on: 236019
No longer depends on: 235222
Depends on: 236678
Depends on: 226754
No longer blocks: 183992
Depends on: 237369
No longer blocks: 123193
AFTER all the dependencies are solved, the "backward compatibility code" in
CGI.pl can be removed as a patch to this bug.

CGI.pl:
410:    $::FORM{$name} = join('', @val);
411:    $::MFORM{$name} = \@val;
No longer blocks: 210663
No longer depends on: 234171
No longer depends on: 238862
No longer depends on: 238864
No longer depends on: 238866
No longer depends on: 238868
No longer depends on: 238870
No longer depends on: 238873
No longer depends on: 238874
No longer depends on: 238875
No longer depends on: 238876
Depends on: 238876
Depends on: 238875
Depends on: 238873
Depends on: 238866
Depends on: 238870
So the cookie code died; however, we need to proceed to clean up the four
hairiest files in the tree. Any ideas on how to do this in a planned way? Should
we work on refactoring those files as we go?
This was going pretty gungho for a while, and we're *so* close to being done...
 I don't suppose there's any chance of this making it for 2.20?  It'd be good to
be able to say goodbye to these awful hacks in our next stable version.  With
the amount of upheaval we've had in the tree recently, now would be the perfect
opportunity to make a mess if needed while cleaning these up. :)
Target Milestone: --- → Bugzilla 2.20
Depends on: 284811
Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.
Assignee: justdave → general
QA Contact: mattyt-bugzilla → default-qa
OK, I'll take care of this.
Assignee: general → wicked
Depends on: 289372
Depends on: 289373
(In reply to comment #2)
> AFTER all the dependencies are solved, the "backward compatibility code" in
> CGI.pl can be removed as a patch to this bug.
> 
> CGI.pl:
> 410:    $::FORM{$name} = join('', @val);
> 411:    $::MFORM{$name} = \@val;
> 

Yeah... I already smell this patch coming; maybe this week... I just finished
reviewing 6 deFORMication patches (152 Kb total, thanks a lot)!
Status: NEW → ASSIGNED
Last things to remove in the final patch:

# XXX FORM compatilibity code, will be removed in bug 225818
$::FORM{$field} = join(" ", $cgi->param($field));

from User.pm (appearing twice)


# Set up stuff for compatibility with the old CGI.pl code
# 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

foreach my $name ($::cgi->param()) {
    my @val = $::cgi->param($name);
    $::FORM{$name} = join('', @val);
    $::MFORM{$name} = \@val;
}

from CGI.pl


'bug/create/comment.txt.tmpl' => [
  'form.comment', 
],

from filterexceptions.pl


and finally convert

[% IF form.comment.length > 0 %]
------- Additional Comments from [% user.identity %]
[%+ form.comment %]

in request/email.txt.tmpl


And %FORM will definitely die! :)
Blocks: bz-majorarch
Farewell %FORM. You served us well but now it's time for you to move on. We
will remember you always. Enjoy your retirement and new abundance of free time!
/me sobs

So, this is the final deFORMication patch that removes last uses of FORM hash.
It also fixes few mistakes and nits left in post_bug and process_bug.
Attachment #180179 - Flags: review?
Comment on attachment 180179 [details] [diff] [review]
Final patch to finally remove FORM hash, V1

I reviewed 9 patches about deFORMication already... one more, one less. ;)
Attachment #180179 - Flags: review?(LpSolit)
Comment on attachment 180179 [details] [diff] [review]
Final patch to finally remove FORM hash, V1

my last tests are all successful. %FORM is dead! :)

r=LpSolit
Attachment #180179 - Flags: review?(LpSolit)
Attachment #180179 - Flags: review?
Attachment #180179 - Flags: review+
Flags: approval?
Flags: approval? → approval+
Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.238; previous revision: 1.237
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.113; previous revision: 1.112
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.250; previous revision: 1.249
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.53; previous revision: 1.52
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.41; previous revision: 1.40
done
Checking in template/en/default/request/email.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/request/email.txt.tmpl,v
 <--  email.txt.tmpl
new revision: 1.9; previous revision: 1.8
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.