Closed Bug 277073 Opened 16 years ago Closed 9 years ago

Make whining trap errors thrown by Search.pm

Categories

(Bugzilla :: Whining, defect)

2.19.1
defect
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: bugreport, Assigned: LpSolit)

References

Details

(Whiteboard: [4.0.3 only; 4.2 and 5.0 fixed by bug 255606])

Attachments

(1 file, 2 obsolete files)

Now that bug 257344 has landed, take advantage of it.
Well, look at this...

I was looking for a way to trap Search.pm errors, and I find this!  I can take this as soon as bug 304989 
lands.
Blocks: 304989
No longer blocks: 304989
Depends on: 304989
If you'd like.  I can still finish it if your queue is too full, though.

I'm working my way back through all of the neglected whining bugs.
I'd like to take this, as (In reply to comment #2)
> If you'd like.  I can still finish it if your queue is too full, though.

Oh, that's not a problem!  I don't want to do anything with this right now because it will probably bitrot 
my patches to bug 304989 (which is why I marked bug 304989 as blocking it).
I'm pulling off the dependency on bug 304989 so that maybe this can be fixed sometime soon.
No longer depends on: 304989
Attached patch Patch v1 (obsolete) — Splinter Review
Well, since there are going to be no major whine.pl changes any time soon, we might as well fix this for now.

This patch wraps the Bugzilla::CGI creation, Bugzilla::Search creation, and search SQL execution inside an eval block.  A death inside the block causes a message to be printed, including the the title of the query, the login of the author, and the error message.  Execution then continues with the next query.
Attachment #204083 - Flags: review?(erik)
Comment on attachment 204083 [details] [diff] [review]
Patch v1

>Index: whine.pl
>+    # Get, run, and process one query.  We may have trouble here, so mark the
>+    # loop for easy `next`ing.
>+    QUERY:
>     for my $query_id (keys %{$queries}) {
>         my $thisquery = $queries->{$query_id};
>         next unless $thisquery->{'name'};   # named query is blank
          ~~~~

This is kind of a style nit, but since we have the block marked now, I'd like to use the label in all of its next statements to avoid confusion by people reading it in the future.

>+        if ($@) {
>+            print 'WARNING: Error constructing and/or executing query "', 
>+                  $thisquery->{'title'}, '" of user ',
>+                  $args->{'author'}->login, ": $@\n";
>+            next QUERY;
>+        }

Should that go to STDERR?  Willing to listen to reason if it shouldn't.
Attachment #204083 - Flags: review?(erik)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Modification of attachment 204083 [details] [diff] [review] with respect to comment 6:

><<<snip>>>
> This is kind of a style nit, but since we have the block marked now, I'd like
> to use the label in all of its next statements to avoid confusion by people
> reading it in the future.

Fine with me.

><<<snip>>>
> 
> Should that go to STDERR?  Willing to listen to reason if it shouldn't.

Also fine with me.
Assignee: erik → karl
Attachment #204083 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #205407 - Flags: review?(erik)
erik: Ping?
QA Contact: mattyt-bugzilla → default-qa
Whiteboard: [patch awaiting review]
Target Milestone: --- → Bugzilla 2.22
Severity: normal → enhancement
Target Milestone: Bugzilla 2.22 → Bugzilla 2.24
Whiteboard: [patch awaiting review]
Comment on attachment 205407 [details] [diff] [review]
Patch v1.1

Sorry for the delay.

I tried to take a look at this today, but the patch no longer applies cleanly to the trunk. Nowadays the queries are in an array as opposed to a hash.

So the patch could be updated with minimal effort to apply again to the trunk.
Attachment #205407 - Flags: review?(erik) → review-
This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2
Assignee: karl.kornel → whining
Status: ASSIGNED → NEW
Target Milestone: Bugzilla 3.2 → ---
This problem has been fixed in 4.2 and 5.0 by bug 255606. It needs to be backported to 4.0.3.
Assignee: whining → LpSolit
Severity: enhancement → normal
Status: NEW → ASSIGNED
Flags: blocking4.0.3+
Summary: Make whining trap errors by keeping Search.pm from using exit → Make whining trap errors thrown by Search.pm
Target Milestone: --- → Bugzilla 4.0
Whiteboard: [4.0.3 only; 4.2 and 5.0 fixed by bug 255606]
Attached patch patch, v2Splinter Review
Backported from 4.2/trunk, see bug 255606. Tested, works.
Attachment #205407 - Attachment is obsolete: true
Attachment #577030 - Flags: review?(glob)
Comment on attachment 577030 [details] [diff] [review]
patch, v2

>=== modified file 'whine.pl'
>@@ -444,11 +444,17 @@
>+            print get_text('whine_query_failed', { query_name => $thisquery->{'name'},

Nit: This probably should have gone to STDERR like the other error message from the script.
Attachment #577030 - Flags: review?(glob) → review+
Flags: approval4.0?
(In reply to Teemu Mannermaa (:wicked) from comment #13)
> Nit: This probably should have gone to STDERR like the other error message
> from the script.

Ah, good idea. I will fix that on checkin. Thanks! :)
Flags: approval4.0? → approval4.0+
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified whine.pl
modified template/en/default/global/messages.html.tmpl
Committed revision 7658.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.