Implement object_end_of_set_all for Bugzilla::Bug

RESOLVED WORKSFORME

Status

()

--
enhancement
RESOLVED WORKSFORME
9 years ago
7 years ago

People

(Reporter: mockodin, Assigned: mkanat)

Tracking

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
Allow hooking immediately before update. The idea being to allow for custom field validation between fields, such as a custom start and end date.

I am using a custom hook for this exact process. Server side validation before committing changes for a project start and end date. What I did:

Perhaps Add:
Bugzilla::Hook::process('object_before_update',
                        { object => $self, old_object => $old_self });

Immediately before:
foreach my $column ($self->UPDATE_COLUMNS) {

Than create a extension to make use of it.
(Assignee)

Comment 1

9 years ago
You should definitely not ever be doing any validation inside of update().

You should be hooking object_end_of_set_all (which I believe exists, and should if it doesn't) instead. Then set_all should be being used everywhere in Bugzilla, or the hook should be being inserted in appropriate places for the cases when set_all isn't being used.
(Reporter)

Comment 2

9 years ago
I did the following in a working extension, updated a bug and saw in the log... nothing. So either it doesn't work or is not implemented. That was the first thing I tried.

sub object_end_of_set_all {
    my ($self, $args) = @_;
    
    my $object = $args->{'class'};
    my $object_params = $args->{'params'};
    
    warn "ping";
}

I do wonder what is the objection over 'object_before_update' vs 'object_end_of_update' which also has the potential to force a transaction roll back.

the advantage of hooking here is we have already gone through the standard validations and this acts as a final chance to hook and kill.
(Assignee)

Comment 3

9 years ago
I think the thing to do here instead of object_before_update is to implement object_end_of_set_all for Bug objects, which is a two-line change to process_bug.cgi.
Severity: normal → enhancement
Summary: Add Hook 'object_before_update' to Object.pm → Implement object_end_of_set_all for Bugzilla::Bug
Target Milestone: --- → Bugzilla 3.6
(Assignee)

Comment 4

9 years ago
To answer your question, update() is never ever supposed to do validation. The way that Bugzilla works is that an array of objects could all have various set_() functions called on them, and then after that they are all updated. So all validation is done before any database updates are done.

Rollbacks should never be a normal process. Particularly under MySQL, they are INCREDIBLY slow.
(Reporter)

Comment 5

9 years ago
(In reply to comment #3)
> I think the thing to do here instead of object_before_update is to implement
> object_end_of_set_all for Bug objects, which is a two-line change to
> process_bug.cgi.

Reasonable.

(In reply to comment #4)
> Rollbacks should never be a normal process. Particularly under MySQL, they are
> INCREDIBLY slow.
More of an example than anything else, my proposal was killing before any changes were made so there shouldn't be any rollback to speak of other than simply rolling back a transaction that had done nothing anyhow. The only reason for the hook being after the start transaction was that we look up the existing bug data and pass would then pass it on to the hook.

*chuckles* They are very fast in mssql :-)

I'll take a look at comment 3.
(Reporter)

Comment 6

9 years ago
Hmm it would appear that the the call to the set_all function no longer exists in process_bug.cgi and also in Bugzilla::Bug in the BUGZILLA-3_6-BRANCH.
(Reporter)

Comment 7

9 years ago
This works:
# Add to == process_bug.cgi

# Hook for Server Side Validation Extensions
foreach my $b (@bug_objects) {
    Bugzilla::Hook::process('bug_before_update', { bug_object => $b });
}

Immediately before the $b->_check_strict_isolation(); loop

I don't think we need to pass input_params do we?; As that could be gathered by calling Bugzilla->input_params to the extension as needed. Often probably not needed at all since we have already run set for everything except for when moving or setting status, dup_of or resolve... in any of those cases calling Bugzilla->input_params should work.
(Reporter)

Comment 8

9 years ago
Hmm, another modification would be to pass the whole bug array in and loop extension side... save the trouble to looking for the existence of a extension hundreds of times on a mass update..
(Reporter)

Comment 9

9 years ago
So this would end up as easy as:
Bugzilla::Hook::process('bug_before_update', { bug_objects => @bug_objects });
(Assignee)

Comment 10

9 years ago
I'm not asking you to implement before_update, I'm asking you to implement before_set_all.
(Assignee)

Comment 11

9 years ago
Don't pass an array to the hook.
(Assignee)

Comment 12

9 years ago
Created attachment 426150 [details] [diff] [review]
v1

Something like this.
Assignee: extensions → mkanat
Status: NEW → ASSIGNED

Updated

8 years ago
Target Milestone: Bugzilla 3.6 → Bugzilla 4.0

Comment 13

7 years ago
As process_bug.cgi now calls $bug->set_all() which itself calls Bugzilla::Object->set_all() which contains a hook for object_end_of_set_all, nothing else is required to make this work. So WFM.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WORKSFORME
Target Milestone: Bugzilla 4.0 → ---
You need to log in before you can comment on or make changes to this bug.