559452 - Improve the code and error checking from TinderboxPushlog Robot

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[(no longer active)]

From Gerv in bug 557445 comment 54:

> Ehsan: a quick code review of that code leads me to make the following
> comments:
> 
> - Please use a proper JSON escaping function. If your comment has a backslash
> in, for example, the JSON will not be well-formed and BzAPI will probably fail.
> 
> - If you just want a list of comments, don't get the whole bug with comments,
> just get the comments:
> https://wiki.mozilla.org/Bugzilla:REST_API:Methods#List_comments_for_bug_.28.2Fbug.2F.3Cid.3E.2Fcomment_GET.29
> 
> - Please check the return codes from the functions you call! This may well tell
> you if things are failing or not!
> 
> Looking at the logs on api-dev, the only errors in the last few days for
> /latest are people doing big searches which time out.
> 
> Gerv

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Gervase Markham [:gerv]]

Comment on attachment 439116 [details] [diff] [review]
Patch (v1)

>+            // server returned an error
>+            alert("Submitting the comment to bug " + id + " failed (" + result.error + ")");

result.error will always be "1" or a true value. For useful diagnostics, you want to print a serialization of the entire error object, with the possible exception of the html_page member:
https://wiki.mozilla.org/Bugzilla:REST_API:Objects#Error

Gerv

Comment 3

[(no longer active)]

(In reply to comment #2)
> (From update of attachment 439116 [details] [diff] [review])
> >+            // server returned an error
> >+            alert("Submitting the comment to bug " + id + " failed (" + result.error + ")");
> 
> result.error will always be "1" or a true value. For useful diagnostics, you
> want to print a serialization of the entire error object, with the possible
> exception of the html_page member:
> https://wiki.mozilla.org/Bugzilla:REST_API:Objects#Error

No, result.error is the result of curl_errno, which may be 0 for no error, or an error code.

Comment 4

[(no longer active)]

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/8a50d4854cdc

Comment 5

[(no longer active)]

I just solved the year's most stupid php bug!

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/af70100f64cd

This newline character caused output, and would subsequently generate errors like:

<br />
<b>Warning</b>:  Cannot modify header information - headers already sent by (output started at /www/htdocs/tmstests/tinderboxpushlog/summaries/JSON.php:808) in <b>/www/htdocs/tmstests/tinderboxpushlog/submitBugzillaComment.php</b> on line <b>19</b><br />

Which resulted in the "Marking 1 bug..." prompt never disappearing.

Markus, could you please deploy that changeset?  Thanks!

Comment 6

[Markus Stange [:mstange]]

Done.

One way to avoid these kinds of gotchas is to omit the ?> at the end. It's not required as far as I know.

Comment 7

[Gervase Markham [:gerv]]

Ehsan: do let me know if the new code reveals that the BzAPI is erroring out for you when it shouldn't be :-)

Gerv

Comment 8

[(no longer active)]

(In reply to comment #6)
> One way to avoid these kinds of gotchas is to omit the ?> at the end. It's not
> required as far as I know.

Hmm, good trick.  I don't know if it's allowed, but the PHP parser can handle it for sure, and it's not like PHP is a standardized language anyway!

(In reply to comment #7)
> Ehsan: do let me know if the new code reveals that the BzAPI is erroring out
> for you when it shouldn't be :-)

Sure, I will.  Thanks for your help!