143743 - Warning generated in dependency tree

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Jouni Heikniemi]

The dependency tree template generates a warning because not all dependencies
have dependencies themselves, and there is one condition in display_tree block
which doesn't check for this. The warning message:

Argument "" isn't numeric in numeric gt (>) at 
data/template/en/default/bug/dependency-tree.html.tmpl line 284 (#1) (W 
numeric) The indicated string was fed as an argument to an operator that 
expected a numeric value instead. If you're fortunate the message will identify 
which operator was so unfortunate. [Sat May 11 19:00:28 2002] 

Patch follows. Ccing Burnus and Myk (template authors) for comments/review.

Comment 1

[Jouni Heikniemi]

Attachment: Patch

Adds one condition, killing the warning.

Comment 2

[Jouni Heikniemi]

The fix should be trivial enough, so proposing milestone 2.16 to get this into
the branch as well.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

I wonder if it would be better for every bug record to have a dependencies field.

Comment 4

[Dave Miller [:justdave]]

uh, is that anything like the "keyword cache"?  I'm not particularly fond of
that idea...

Comment 5

[Myk Melez [:myk] [@mykmelez]]

No, I mean that when generating the Perl data structure representing the
dependency tree, we should initialize each Perl bug record with an empty list of
dependencies.  Then we wouldn't have to check whether or not the list of
dependencies exists, we could just check its size (like we currently do).

Comment 6

[Dave Miller [:justdave]]

Oh, for the values passed to the template you mean, not the DB schema.  Yeah,
that makes perfect sense.  Bug.pm would fit well for feeding bugs to templates
ya know...  are we using it?  (just an off-topic aside :)

Comment 7

[Jouni Heikniemi]

Before implementing any solution, the true meaning of these undef'd fields in
template data structures should be considered.

This is not the first bug (I think it's the fourth for 2.16) I've filed with
exactly the same characteristics. We should consider whether "nulls" should be
passed as empty lists or undef'd variables and then make it a standard coding
practice. The check "foo && foo.size > 0" is actually redundant in most of the
cases, since either one of the conditions would suffice if the code was designed
uniformly.

So, there should be a design document either stating that lists with empty
meaning should be passed as undef'd or a document stating that there should
always be a list, empty if appropriate. This way all template authors could
trust on certain convention being used.


I still think this bug should be fixed using my patch, since it's quick and
easy. If something is decided on the more generic issue presented above, a new
bug should be filed to convert all the existing empty list code to use whichever
method is chosen. 

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 83210 [details] [diff] [review]
Patch

As it turns out, the code in showdependencytree.cgi already initializes 
the Perl bug records with empty arrays (line 170), but Perl returns 
the undefined value when evaluating an empty array in a scalar context.

I think the Template Toolkit should be handling this situation 
by returning numeric zero instead of the undefined value in this situation.
I'll report this bug to the Template Toolkit folks.  In the meantime,
Jouni's patch is the right workaround, so 2xr=myk.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: fixes problem in showdependencytree.cgi

Ok, the problem turns out to be that I'm initializing the array reference with
an empty list instead of an array reference.  This patch fixes the problem.

Comment 10

[Bradley Baetz (:bbaetz)]

Comment on attachment 83603 [details] [diff] [review]
patch v2: fixes problem in showdependencytree.cgi

r=bbaetz, per discussion on TT list.

Comment 11

[Dave Miller [:justdave]]

Comment on attachment 83603 [details] [diff] [review]
patch v2: fixes problem in showdependencytree.cgi

r= justdave

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Checking in bztip/showdependencytree.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencytree.cgi,v  <-- 
showdependencytree.cgi
new revision: 1.18; previous revision: 1.17
done
Checking in bz216/showdependencytree.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencytree.cgi,v  <-- 
showdependencytree.cgi
new revision: 1.17.2.1; previous revision: 1.17
done