235278 - Userprefs.cgi param conversion

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Vlad Dascalu]

Use the CGI methods in userprefs.cgi for cookie/param retrival, according to 
the guidelines in the dependency.

Comment 1

[Vlad Dascalu]

Attachment: Version 1

Comment 2

[Dave Miller [:justdave]]

Comment on attachment 142020 [details] [diff] [review]
Version 1

>+    my $pwd1 = Bugzilla->cgi->param('new_password1');
>+    my $pwd2 = Bugzilla->cgi->param('new_password2');

We're using this in far too many places.  It would look much cleaner (and
probably be more efficient) to assign Bugzilla->cgi to $cgi at the top of each
scope that's using it.

>@@ -227,14 +231,16 @@
>-    if (Param("supportwatchers") && exists $::FORM{'watchedusers'}) {
>+    if (Param("supportwatchers") && exists
>+            Bugzilla->cgi->param('watchedusers')) {

$cgi->param() is not a hash, so you can't use exists on it.  defined should
work okay here.

Comment 3

[Vlad Dascalu]

Attachment: Version 2

Thanks for the fast review!

Comment 4

[Christian Reis]

Comment on attachment 142050 [details] [diff] [review]
Version 2


r=kiko, with a few non-blocking comments below.

>Index: userprefs.cgi
ThrowUserError("passwords_dont_match");
>-            $::FORM{'new_password1'} || ThrowUserError("new_password_missing");
>+            $cgi->param('new_password1')
>+              || ThrowUserError("new_password_missing");

Does this *really* need a linebreak? :-)

FlagRequester)) {
>-        $updateString .= "~$flag~" . (defined($::FORM{$flag}) ? "on" : "");
>+        $updateString .= "~$flag~" .
>+            (defined($cgi->param($flag)) ? "on" : "");

I'm not used to seeing defined with parenthesis, but since this is the way it
was..

>@@ -227,14 +233,14 @@
>             
>             # If the form field for this preference is defined, then we
>             # know the checkbox was checked, so set the value to "on".
>-            $updateString .= "on" if defined $::FORM{"email$role$reason"};
>+            $updateString .= "on" if defined $cgi->param("email$role$reason");

Hmm. I wonder if this was a bug originally, since AFAIK you're supposed to
check for exists with hashes, right?

Comment 5

[Vlad Dascalu]

(In reply to comment #4)
> >+            $cgi->param('new_password1')
> >+              || ThrowUserError("new_password_missing");
> 
> Does this *really* need a linebreak? :-)

Yes, in this case. Without it it takes 83 chars.

> >-        $updateString .= "~$flag~" . (defined($::FORM{$flag}) ? "on" : "");
> >+        $updateString .= "~$flag~" .
> >+            (defined($cgi->param($flag)) ? "on" : "");
> 
> I'm not used to seeing defined with parenthesis, but since this is the way it
> was..

I removed the parenthesis. And in this case the extra line is not required,
indeed. :-)

> Hmm. I wonder if this was a bug originally, since AFAIK you're supposed to
> check for exists with hashes, right?

Yeap. Seems like it.

Comment 6

[Vlad Dascalu]

Attachment: Version 3

Comment 7

[Christian Reis]

(In reply to comment #5)
> > >+        $updateString .= "~$flag~" .
> > >+            (defined($cgi->param($flag)) ? "on" : "");
> > 
> > I'm not used to seeing defined with parenthesis, but since this is the way it
> > was..
> 
> I removed the parenthesis. And in this case the extra line is not required,
> indeed. :-)

Actually, I meant the parenthesis *to* defined:

  (defined($::FORM{$flag}) ? ... )

->

  (defined $::FORM($flag) ? ... )

As long as it works, r=kiko. Go seek approval :-)

Comment 8

[Vlad Dascalu]

Attachment: Version 4

Ok, you're right. The "." operator has a greater priority than the "?"
operator; in fact "?" has one of the lowest priorities, AFAIK.

Version 4. Tested. It works. :-)

Comment 9

[Dave Miller [:justdave]]

Comment on attachment 142627 [details] [diff] [review]
Version 4

>-$vars->{'login'} = $::COOKIE{'Bugzilla_login'};
>+$vars->{'login'} = $cgi->cookie('Bugzilla_login');

Don't use the Bugzilla_login cookie.  Nothing anywhere in Bugzilla (with the
exception of the Bugzilla::Auth::* modules) should care that it exists.  The
template can access it from the global user object as [% user.login %].

Comment 10

[Christian Reis]

Ho hum. Vlad, you might need to check other patches for that too :)

Comment 11

[Christian Reis]

Places apart from userprefs.cgi where $::COOKIE{'Bugzilla_login'} is still in use:

  attachment.cgi 
  buglist.cgi
  post_bug.cgi
  enter_bug.cgi
  process_bug.cgi
  move.pl

(and there's access to the global COOKIE there, in query.cgi and show_bug.cgi
too). Should bugs be open to clean that up?

As for Bugzilla_logincookie, I'm supposed to fix direct access to that in bug
226754.

Comment 12

[Dave Miller [:justdave]]

Comment on attachment 142627 [details] [diff] [review]
Version 4

ok, since that's covered by another bug, let's do this. :)

Comment 13

[Christian Reis]

Filed bug 236678 on the COOKIE cleanup. 

Comment 14

[Vlad Dascalu]

Attachment: Version 5

Comment 15

[Christian Reis]

Comment on attachment 143137 [details] [diff] [review]
Version 5

great! you just conflicted me with my patch to bug 226754!

Comment 16

[Vlad Dascalu]

Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.53; previous revision: 1.52
done
Checking in template/en/default/account/prefs/prefs.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/prefs.html.tmpl,v
 <--  prefs.html.tmpl
new revision: 1.13; previous revision: 1.12
done