86300 - Multiple usage of potentially uninitialized variables in globals.pl/GetBugLink

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P2)

Description

[Thomas Sandford]

I am getting the following warnings in my web server (Apache) log file when 
running bugzilla locally:

Use of uninitialized value at globals.pl line 795 (#1)
Use of uninitialized value at globals.pl line 796 (#1)
Use of uninitialized value at globals.pl line 797 (#1)
Use of uninitialized value at globals.pl line 799 (#1)
Use of uninitialized value at globals.pl line 800 (#1)

This appears to be due to NULLs being returned by the SQL query a few lines 
back.

Comment 1

[Thomas Sandford]

Also occurs on earlier lines 791 and 792 (and causes similar problems within 
the value_quote() calls on the next lines.

Further investigation reveals that the reason I am seeing this error is that I 
have the text ... see bug <number> ... in my bug description, where <number> is 
not a bugnumber existing in the bugzilla database (I was referring back to bug 
numbers in a previous database I had migrated from into bugzilla).

GetBugLink should check for this case, and not produce a link if it occurs.

Comment 2

[Jacob Steenhagen]

One of things I intend to do sometime soon is start looking at my webserver logs
and trying to solve some various warnings (There currently aren't any compile
time warings, but there are a few run-time).

Also, there's a bug somewhere about not turing bug ### into a link if it's
doesn't exist which would also solve this particular problem.

Comment 3

[Thomas Sandford]

Other bugs relating to bug numbers in links: bug 31168, bug 85709.

I now have a patch that fixes this bug in my local installation - I'll attach 
it as soon as I've worked out how to create it against current (2.13) sources 
and how to attach it!!

Comment 4

[Jacob Steenhagen]

See also bug 86923 for more similar error messages.

Comment 5

[Thomas Sandford]

Attachment: Patch (against 1.12) to fix this problem

Comment 6

[Thomas Sandford]

Sorry. Patch is (obviously) against 2.12 not 1.12!

Comment 7

[Matthew Tuck [:CodeMachine]]

Comment on attachment 39479 [details] [diff] [review]
Patch (against 1.12) to fix this problem

Needs work.  Would be cleaner if the code, including the my declaration, was inside if (MoreSQLData()).

Comment 8

[Jacob Steenhagen]

Attachment: patch

Comment 9

[Jacob Steenhagen]

The patch I just uploaded uses if (MoreSQLData()), is against current CVS (to
incorporate recent changes in that routine) and also impliments a cache so if
the same bug number is mentioned twice in one report (which is fairly common) it
only has to run the SQL query once.

Comment 10

[Jacob Steenhagen]

Attachment: patch -- diff -u, check for defined

Comment 11

[Matthew Tuck [:CodeMachine]]

This looks good, but I think the cache should be on bug number and link text,
since it relies on the latter.

Comment 12

[Matthew Tuck [:CodeMachine]]

Given the fact the number is numeric, there should be no problems in you key on
"$bugnum-$linktext".

Comment 13

[Matthew Tuck [:CodeMachine]]

Actually, that would fail to cache the SQL results between the same bug but with
different link text (eg bug XXXXX and bug #XXXXX).

Since we're really interested in caching the SQL results (I assume), maybe we
should just cache a reference to the record list.

Comment 14

[Jacob Steenhagen]

Attachment: Cache values instead of the entire link

Comment 15

[Jacob Steenhagen]

That's a very good point.  This latest patch caches the values that can be used
to build the link, not the link itslef.

Comment 16

[Matthew Tuck [:CodeMachine]]

Looks good, except:

if ($bug_state eq $::unconfirmedstate) {
...
if (! IsOpenedState($bug_state)) {

This should probably be else.  It doesn't matter but I think it makes it more
obvious.

Also, please quote the resolution and status.  ATM people can hack their
installations, soon they will have custom resolutions and hopefully statuses for
2.18.

Comment 17

[Jacob Steenhagen]

Attachment: value_quote() the entire $title

Comment 18

[Jacob Steenhagen]

I had a dream last night about using elsif there... :)

Anyway, I've now run $title through value_quote() right before dumping it out to
the %::buglink hash instead of doing each part individually (not that it matters
much, it just meant I didn't have to type value_quote() as much :)

Comment 19

[Matthew Tuck [:CodeMachine]]

Comment on attachment 55548 [details] [diff] [review]
value_quote() the entire $title

I'm a big fan of your work.

Comment 20

[Matthew Tuck [:CodeMachine]]

This also fixes bug #85709, I believe.

I'm thinking we should change the presentational HTML to CSS classes on the
status, or perhaps templatise the HTML that gets output, but that is not
necessary for this bug.

Comment 21

[Gervase Markham [:gerv]]

Comment on attachment 55548 [details] [diff] [review]
value_quote() the entire $title

If Matty tested the patch, r=gerv. If he didn't, someone will have to, so
!r=gerv :-)

Gerv

Comment 22

[Jacob Steenhagen]

mpt: I saw in #mozwebtools this evening where you mentioned that you didn't want
Bugzilla to link to non-existant bugs... that's covered in the patch to this bug.

Comment 23

[Matthew Tuck [:CodeMachine]]

I tested it, but I think both reviewers are supposed to test.

Comment 24

[Gervase Markham [:gerv]]

Oh, check it in :-)

Gerv

Comment 25

[Jacob Steenhagen]

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.126; previous revision: 1.125
done

Comment 26

[Jacob Steenhagen]

must... click... radio... button...

Comment 27

[Matthew Tuck [:CodeMachine]]

*** Bug 85709 has been marked as a duplicate of this bug. ***

Comment 28

[Matthew Tuck [:CodeMachine]]

*** Bug 136723 has been marked as a duplicate of this bug. ***

Comment 29

[Matthew Tuck [:CodeMachine]]

*** Bug 119890 has been marked as a duplicate of this bug. ***

Comment 30

[Andreas Franke (gone)]

*** Bug 144173 has been marked as a duplicate of this bug. ***