Closed
Bug 904467
Opened 11 years ago
Closed 10 years ago
error when %recipients is empty (e.g. after bugmail_recipients empties it)
Categories
(Bugzilla :: Email Notifications, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 5.0
People
(Reporter: dnozay, Assigned: dnozay)
References
Details
Attachments
(1 file, 1 obsolete file)
1.81 KB,
patch
|
glob
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.71 Safari/537.36 Steps to reproduce: wrote an extension that clears the list of bugmail recipients for bulk updates; the gist is: sub bugmail_recipients { my ($self, $args) = @_; my $recipients = @$args{qw(recipients)}; # clear recipients %$recipients = (); } Actual results: (note line numbers are from 4.2 not trunk) Software error: DBD::mysql::db selectall_arrayref failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')' at line 2 [for Statement "SELECT watcher, watched FROM watch WHERE watched IN ()"] at Bugzilla/BugMail.pm line 200 Bugzilla::BugMail::Send(1, 'HASH(0x3e45648)', 'HASH(0x4d004c8)') called at Bugzilla/Bug.pm line 1224 Bugzilla::Bug::_send_bugmail('HASH(0x4d004c8)', 'HASH(0x488ec58)') called at Bugzilla/Bug.pm line 1163 Bugzilla::Bug::send_changes('Bugzilla::Bug=HASH(0x4c923e8)', 'HASH(0x492add8)', 'HASH(0x488ec58)') called at /var/www/bugzilla.proximdata.com/process_bug.cgi line 385 For help, please send mail to the webmaster ([no address given]), giving this error message and the time and date of the error. ----- code doesn't check for elements before using the empty %recipients in the db query. please note that this code is fixed in BMO 4.2 Expected results: " email sent to: no one "
Comment 2•11 years ago
|
||
Comment on attachment 789419 [details] [diff] [review] patch.diff >+ if (scalar keys %recipients) { This is a hack. No developer will understand why this check is here when you know that %recipients always has at least two entries: the reporter and the bug assignee. Moreoever, if your goal is to clear %recipients, then you will miss global watchers who will still get bugmails.
Updated•11 years ago
|
Severity: normal → minor
Comment 3•11 years ago
|
||
(In reply to Frédéric Buclin from comment #2) > This is a hack. No developer will understand why this check is here when you > know that %recipients always has at least two entries Said differently: you must add a comment right above this line explaining why it's there.
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #789600 -
Flags: review?(LpSolit)
Assignee | ||
Comment 5•11 years ago
|
||
thanks for the review (In reply to Frédéric Buclin from comment #2) > Comment on attachment 789419 [details] [diff] [review] > patch.diff > > >+ if (scalar keys %recipients) { > > This is a hack. No developer will understand why this check is here when you > know that %recipients always has at least two entries: the reporter and the > bug assignee. Bugzilla::Hook::process('bugmail_recipients', { bug => $bug, recipients => \%recipients, users => \%user_cache, diffs => \@diffs }); any bugmail_recipients hook may do anything to the parameters passed by reference. > > Moreoever, if your goal is to clear %recipients, then you will miss global > watchers who will still get bugmails. I fully understand that the global watchers are processed late. If "global watching" was a vanilla extension, it could inspect and augment %recipients in a bugmail_recipients hook. The only potential problem I see is the extension system which AFAIK doesn't let you specify an order to run hooks other than the one derived from the extension filenames lexical order. Rather than addressing that, this is a "simple" patch to fix a broken assumption.
Updated•11 years ago
|
Attachment #789600 -
Flags: review?(LpSolit) → review?(glob)
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #789419 -
Attachment is obsolete: true
Attachment #789419 -
Flags: review?(justdave)
Comment on attachment 789600 [details] [diff] [review] v2 r=glob i'm happy with that comment; extension developers are expected to be able to understand both perl and bugzilla.
Attachment #789600 -
Flags: review?(glob) → review+
Assignee | ||
Comment 7•10 years ago
|
||
Byron / Frédéric, would you mind committing on my behalf? thank you.
Comment 8•10 years ago
|
||
Thanks for the patch! Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk modified Bugzilla/BugMail.pm Committed revision 8861.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•