Closed Bug 234875 Opened 16 years ago Closed 16 years ago

Use $cgi->param in quips.cgi

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, 2 obsolete files)

Eliminate obsolete %FORM vars from quips.cgi.
Attached patch Version 1 (obsolete) — Splinter Review
Attachment #141736 - Flags: review?(kiko)
Status: NEW → ASSIGNED
Attachment #141736 - Flags: review?(jouni)
Comment on attachment 141736 [details] [diff] [review]
Version 1

r=jouni
Attachment #141736 - Flags: review?(jouni) → review+
Comment on attachment 141736 [details] [diff] [review]
Version 1

>@@ -104,7 +104,7 @@
>-       my $form = ($::FORM{'quipid_'.$quipid}) ? 1 : 0;
>+       my $form = ($cgi->param('quipid_'.$quipid)) ? 1 : 0;

This isn't going to do what you think it is.

Wrapping $cgi->param() in parens tells it to return a list of the values for
that param (since you can have a param mentioned more than once in your query
string).  You'll get an array returned here, and your ? comparison will then be
testing true/falseness based on the length of the array rather than the value
of the first item.

I suggest:

   my $form = scalar($cgi->param('quipid_'.$quipid)) ? 1 : 0;

which will guarantee that you get scalar results instead of a list.
Attachment #141736 - Flags: review?(kiko) → review-
Actually, in this situation, having it without the parens around it at all would
probably work fine, too.
Just for reference:

11:12 <@jth> justdave: It seems that |cgi->param('foo')| returns undef when the
param doesn't exist in both list and scalar context.
11:12 <@justdave> yes.
11:13 <@justdave> but a scalar undef gets passed as a param
11:13 <@justdave> an undef list doesn't get passed.
11:13 <@justdave> (because it's a list of zero params)
11:13 <@jth> Yeah. Anyway, that's why Vlad's code worked...

It still might be a good idea to clarify this by removing the extra parenthesis.
Comment on attachment 141736 [details] [diff] [review]
Version 1

ok, I'm going to recind my negative review...  jouni and I worked it out in
IRC...	what I said is accurate, but you can get away with it here only because
we're looking at checkboxes. :)  If it wasn't checked, it doesn't get passed,
and an empty list counts as false when doing a boolean comparison. :)

So whether you want to clean that up so it's not ambiguous is up to you, but
the code does actually work.
Attachment #141736 - Flags: review-
I'd rather have it without parenthesis if they are unnecessary.
Target Milestone: --- → Bugzilla 2.18
Attached patch Version 2Splinter Review
Removes the extra paranthesis.
Attachment #141736 - Attachment is obsolete: true
Attachment #142611 - Flags: review?(kiko)
Would that be written more clearly as "defined $cgi->..."?
I haven't checked the code, but there exists a possibility that the original
code makes a difference between 0 and undefined. Keeping it in the actual format
seems safe.
I'm worried that a quip with an id of zero would cause problems, and that's why
I'm asking. 
Attachment #142611 - Flags: review?(kiko)
Attached patch Version 3 (obsolete) — Splinter Review
True, sounds safe this way.
Attachment #142611 - Attachment is obsolete: true
Attachment #142630 - Flags: review?(kiko)
Flags: approval?
Comment on attachment 142611 [details] [diff] [review]
Version 2

I looked at quips.cgi to see how this works; I think this version is "more
correct" given that quipid_XXX is a checkbox, not something that equals an ID.
That hash there is kinda confusing :-)
Attachment #142611 - Attachment is obsolete: false
Attachment #142611 - Flags: review+
Comment on attachment 142630 [details] [diff] [review]
Version 3

Don't use this one. :)
Attachment #142630 - Attachment is obsolete: true
Attachment #142630 - Flags: review?(kiko)
Flags: approval? → approval+
Checking in quips.cgi;
/cvsroot/mozilla/webtools/bugzilla/quips.cgi,v  <--  quips.cgi
new revision: 1.22; previous revision: 1.21
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.