234875 - Use $cgi->param in quips.cgi

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Vlad Dascalu]

Eliminate obsolete %FORM vars from quips.cgi.

Comment 1

[Vlad Dascalu]

Attachment: Version 1

Comment 2

[Jouni Heikniemi]

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

r=jouni

Comment 3

[Dave Miller [:justdave]]

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.

Comment 4

[Dave Miller [:justdave]]

Actually, in this situation, having it without the parens around it at all would
probably work fine, too.

Comment 5

[Jouni Heikniemi]

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 6

[Dave Miller [:justdave]]

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.

Comment 7

[Christian Reis]

I'd rather have it without parenthesis if they are unnecessary.

Comment 8

[Vlad Dascalu]

Attachment: Version 2

Removes the extra paranthesis.

Comment 9

[Christian Reis]

Would that be written more clearly as "defined $cgi->..."?

Comment 10

[Vlad Dascalu]

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.

Comment 11

[Christian Reis]

I'm worried that a quip with an id of zero would cause problems, and that's why
I'm asking. 

Comment 12

[Vlad Dascalu]

Attachment: Version 3

True, sounds safe this way.

Comment 13

[Christian Reis]

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 :-)

Comment 14

[Christian Reis]

Comment on attachment 142630 [details] [diff] [review]
Version 3

Don't use this one. :)

Comment 15

[Vlad Dascalu]

Checking in quips.cgi;
/cvsroot/mozilla/webtools/bugzilla/quips.cgi,v  <--  quips.cgi
new revision: 1.22; previous revision: 1.21
done