If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Improve the code and error checking from TinderboxPushlog Robot

RESOLVED FIXED

Status

Tree Management Graveyard
TBPL
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

Trunk
x86
Mac OS X

Details

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
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
(Assignee)

Comment 1

8 years ago
Created attachment 439116 [details] [diff] [review]
Patch (v1)
Attachment #439116 - Flags: review?(mstange)
Attachment #439116 - Flags: review?(mstange) → review+
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
(Assignee)

Comment 3

8 years ago
(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.
(Assignee)

Comment 4

8 years ago
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/8a50d4854cdc
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 5

8 years ago
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!
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.
Ehsan: do let me know if the new code reveals that the BzAPI is erroring out for you when it shouldn't be :-)

Gerv
(Assignee)

Comment 8

8 years ago
(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!
Product: Webtools → Tree Management
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.