Closed Bug 235278 Opened 16 years ago Closed 16 years ago

Userprefs.cgi param conversion

Categories

(Bugzilla :: Bugzilla-General, defect)

2.17.6
defect
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: goobix, Assigned: goobix)

References

Details

Attachments

(1 file, 4 obsolete files)

Use the CGI methods in userprefs.cgi for cookie/param retrival, according to 
the guidelines in the dependency.
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.18
Attached patch Version 1 (obsolete) — Splinter Review
Assignee: justdave → vlad
Attachment #142020 - Flags: review?(preed)
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.
Attachment #142020 - Flags: review?(preed) → review-
Attached patch Version 2 (obsolete) — Splinter Review
Thanks for the fast review!
Attachment #142020 - Attachment is obsolete: true
Attachment #142050 - Flags: review?(timeless)
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?
Attachment #142050 - Flags: review?(timeless) → review+
(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.
Attached patch Version 3 (obsolete) — Splinter Review
Attachment #142050 - Attachment is obsolete: true
Attachment #142615 - Flags: review?(kiko)
(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 :-)
Flags: approval?
Attachment #142615 - Flags: review?(kiko)
Attached patch Version 4 (obsolete) — Splinter Review
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. :-)
Attachment #142615 - Attachment is obsolete: true
Attachment #142627 - Flags: review?(kiko)
Attachment #142627 - Flags: review?(kiko) → review+
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 %].
Attachment #142627 - Flags: review-
Flags: approval?
Ho hum. Vlad, you might need to check other patches for that too :)
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 on attachment 142627 [details] [diff] [review]
Version 4

ok, since that's covered by another bug, let's do this. :)
Attachment #142627 - Flags: review-
Flags: approval+
Filed bug 236678 on the COOKIE cleanup. 
Attached patch Version 5Splinter Review
Attachment #142627 - Attachment is obsolete: true
Attachment #143137 - Flags: review?(kiko)
Comment on attachment 143137 [details] [diff] [review]
Version 5

great! you just conflicted me with my patch to bug 226754!
Attachment #143137 - Flags: review?(kiko) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 16 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.