Closed Bug 912540 Opened 7 years ago Closed 7 years ago

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

Categories

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

Production
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: gerv, Assigned: glob)

References

Details

Attachments

(1 file)

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
oops -- it's there to perform server-side validation of the "reviewer required" (normally this is caught by javascript).

thanks for noticing that! :)
Assignee: nobody → glob
Summary: flag_end_of_update hook uses non-existent parameter "changes" → "mandatory reviewer" requirement isn't validated server-side, due to flag_end_of_update hook using a non-existent parameter "changes"
Blocks: 751862
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.
Component: Extensions: Review → Extensions: Push
Summary: "mandatory reviewer" requirement isn't validated server-side, due to flag_end_of_update hook using a non-existent parameter "changes" → the push extension alters the flag_end_of_update hook's args for all extensions not just itself
Attached patch 912540_1.patch β€” β€” Splinter Review
instead of cloning $args as discussed, i just leave the old variables in place, and remove $args->{changes} before the hook returns.
Attachment #800666 - Flags: review?(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
Attachment #800666 - Flags: review?(dkl) → review+
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bmo/4.2/
modified extensions/Push/Extension.pm
modified extensions/Review/Extension.pm
Committed revision 8989.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: Extensions: Push → Extensions
You need to log in before you can comment on or make changes to this bug.