912540 - the push extension alters the flag_end_of_update hook's args for all extensions not just itself

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[Gervase Markham [:gerv]]

Review/Extension.pm contains this code:

sub flag_end_of_update {
    my ($self, $args) = @_;
    my ($attachment, $changes) = @$args{qw(object changes)};
    if (exists $changes->{'flag.review'}
...

The flag_end_of_update hook does not have a "changes" argument. Flag.pm, line 554:

Bugzilla::Hook::process('flag_end_of_update', { object    => $self,
                                                timestamp => $timestamp,
                                                old_flags => \@old_summaries,
                                                new_flags => \@new_summaries,
                                              });

Also: http://www.bugzilla.org/docs/4.2/en/html/api/Bugzilla/Hook.html#flag_end_of_update

So this code will never do anything.

What was it supposed to do?

Gerv

Comment 1

[:glob ✱]

oops -- it's there to perform server-side validation of the "reviewer required" (normally this is caught by javascript).

thanks for noticing that! :)

Comment 2

[:glob ✱]

ahhhh.

so, the changes arg _is_ being passed to that function, but via extreme and invalid trickiness.

the push extension likes all its data in a consistent format, so it converts from old_flags/new_flags into a changes object.  it alters $args for any extension called after it, so the review extension _does_ get passes a 'changes' parameter.

the push ext needs to make a copy of $args before it starts butchering it, and the review extension needs to play with new_flags.

Comment 3

[:glob ✱]

Attachment: 912540_1.patch

instead of cloning $args as discussed, i just leave the old variables in place, and remove $args->{changes} before the hook returns.

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 800666 [details] [diff] [review]
912540_1.patch

Review of attachment 800666 [details] [diff] [review]:
-----------------------------------------------------------------

Works well in my testing. r=dkl

Comment 5

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
modified extensions/Push/Extension.pm
modified extensions/Review/Extension.pm
Committed revision 8989.