Make whining trap errors thrown by Search.pm

RESOLVED FIXED in Bugzilla 4.0

Status

()

Bugzilla
Whining
RESOLVED FIXED
13 years ago
6 years ago

People

(Reporter: Joel Peshkin, Assigned: Frédéric Buclin)

Tracking

2.19.1
Bugzilla 4.0
Bug Flags:
approval4.0 +
blocking4.0.3 +

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

13 years ago
Now that bug 257344 has landed, take advantage of it.

Comment 1

12 years ago
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

Updated

12 years ago
No longer blocks: 304989
Depends on: 304989

Comment 2

12 years ago
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.

Comment 3

12 years ago
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).

Comment 4

12 years ago
I'm pulling off the dependency on bug 304989 so that maybe this can be fixed sometime soon.
No longer depends on: 304989

Comment 5

12 years ago
Created attachment 204083 [details] [diff] [review]
Patch v1

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 6

12 years ago
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.

Updated

12 years ago
Attachment #204083 - Flags: review?(erik)

Comment 7

12 years ago
Created attachment 205407 [details] [diff] [review]
Patch v1.1

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

Updated

12 years ago
Attachment #205407 - Flags: review?(erik)

Comment 8

12 years ago
erik: Ping?
QA Contact: mattyt-bugzilla → default-qa
Whiteboard: [patch awaiting review]
Target Milestone: --- → Bugzilla 2.22
(Assignee)

Updated

11 years ago
Severity: normal → enhancement
Target Milestone: Bugzilla 2.22 → Bugzilla 2.24

Updated

11 years ago
Whiteboard: [patch awaiting review]

Comment 9

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

Comment 10

11 years ago
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

Updated

9 years ago
Assignee: karl.kornel → whining
Status: ASSIGNED → NEW
Target Milestone: Bugzilla 3.2 → ---
(Assignee)

Comment 11

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

Updated

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

Comment 12

6 years ago
Created attachment 577030 [details] [diff] [review]
patch, v2

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?
(Assignee)

Comment 14

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

Comment 15

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.