271048 - Classification list is blank on query.cgi

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Jeff Lawson]

I've enabled the new Classifications feature on my bugzilla installation made
from a current head checkout (Nov 21 06:37:30 UTC 2004) and the Classification
selection box is always empty.

I have traced the bug to being in that sort() is interpreting its argument as
the sorting function, rather than the list to be sorted.

Comment 1

[Jeff Lawson]

Attachment: use a temporary list to workaround sort issue

Comment 2

[Jeff Lawson]

My bugzilla is using Perl 5.6.1 on Redhat ES 2.1 x86, if that's of any interest.

Comment 3

[Vlad Dascalu]

Comment on attachment 166644 [details] [diff] [review]
use a temporary list to workaround sort issue

Sort doesn't interpret its argument as
the sorting function, that would be illogical.

This is a hack because it provides a solution without understanding the root
cause.

By taking a look at the sort perldoc documentation ('perldoc -f sort' in the
command line) I could see that:

>> In scalar context, the behaviour of "sort()" is undefined. <<

So I wonder if GetSelectableClassifications() doesn't return a scalar context
by any chance. This would also explain why your hack works.

So I took a look in globals.pl and the return statement for the "sub
GetSelectableClassifications()" is:

    return (@selectable_classes);

I wonder if removing those () around the @selectable_classes would solve the
issue for you.

Jeff, can you test that (reverse your change, remove the () in globals.pl,
retest, and if it works, re-diff, attach a new patch and request review again)?
Thanks!

Comment 4

[Jeff Lawson]

vladd, I just tried it.  Reverting my change to query.cgi and changing the
globals.pl as you suggest does not work.

In fact prior to creating my patch, I had tried adding both a "die" and a
debugging "print STDERR" just inside the sub GetSelectableClassifications and my
message was not printed at all.

That is how I came to the conclusion that sort() was getting confused and
interpreting its argument as a sort function with an empty array.

Comment 5

[Vlad Dascalu]

I found this which might be useful:

http://london.pm.org/pipermail/london.pm/Week-of-Mon-20011210/007882.html

Quote:

The indirect object slot is tricky. What's happening is that "sort $scalar"
is theoretically ambiguous - it could be "sort the list consisting of this
scalar with the default sort routine" or "sort, with the routine whose name
is in $scalar (or to which $scalar contains a reference, a list that I'm
going to give you in a second", and the tokeniser(?) has to find out which
it is.

perldoc -f sort (5.6.1) says

    "SUBNAME may be a scalar variable name (unsubscripted), in
    which case the value provides the name of (or a reference to)
    the actual subroutine to use."

Note the "unsubscripted" thing. AFAIK, that's the criterion the tokeniser
uses to determine whether it's a data item or a SUBNAME; it doesn't
lookahead very far.

Comment 6

[Vlad Dascalu]

Ok, so 5.6 sucks.

We could use "sort {$a cmp $b} Get..."; it's less intrusive, doesn't require an
additional variable and it doesn't change the code that the interpreter will try
to execute. Jeff, could we try that instead?

Comment 7

[Jeff Lawson]

Attachment: eliminate sort ambiguity by using cmp

That works as well.

Comment 8

[Jeff Lawson]

i don't have commit privs, so someone else will eventually have to commit it
once approved

Comment 9

[Albert Ting]

Attachment: suggested patch

Jeff, can you try this patch instead?  GetSelectableClassifications ought to
always return a sorted list, similar to GetSelectableProducts.	This way, any
future code won't have to explicitly sort (and potential create another sort
bug).

Comment 10

[Jeff Lawson]

Albert, that patch works ok too.

Comment 11

[Albert Ting]

Thanks Jeff.  Vlad, I don't have review permissions, but would like to request
we use the newer patch to fix this bug. 

Comment 12

[Vlad Dascalu]

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.276; previous revision: 1.275
done
Checking in query.cgi;
/cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
new revision: 1.133; previous revision: 1.132
done