Move bug events into BugData by cproensa · Pull Request #1098 · mantisbt/mantisbt

cproensa · 2017-04-21T23:24:09Z

This replaces PR #966
rebased to current master

cproensa · 2017-04-21T23:55:08Z

It adds on suggestions from #966

Better documentation for deprecated events
Events parameters are now a single array, which contains different elements, that previously were separated parameters. This i think is @vboctor suggestion for event parameters. Having named indexes helps to have the data better self-explained. It would also allow to add more parameters if needed without breaking the method specification.
add_create_callback, as a wrapper for add_update_callback, as suggested by vboctor

Other victor suggestions (#966 (review))

It would be great to start the new extensibility model for these events where we have an object per event.

my interpretation is the array parameter, for example:

For the event objects it is tempting to re-use BugData as a field on these events, but I would rather have the raw properties or event objects that we want to expose (normal or calculated)

If we ever have newer interfaces/classes, these events should be updated. Ideally, I think a scenario where BugData is a readonly class, and we have a class for Bug-Modification, which is oriented to being aware of the delta changes, providing more control for notifications, and work in coordination with other entities (bugnotes, attachments, etc). That's another story, though. At the moment, out only interface is BugData

We could also consider having an event type where if a plugin fail, we would stop the execution
of the rest

ideally, execution could be stopped with an error. In a scenario where we supports raising error as exceptions, then that could be catched and managed.

It would be great for the update method to know the before/after state. This enables better email notifications and other changes that depend on the delta rather than just the final state. Currently the code disables email notification from within update method and triggers it by the caller

in this PR, notifications are kept as they were, triggered in the callers. BugData already have some logic that triggers notifications, so that could be used. However that was not changed in order to keep this PR more simple.

vboctor

I added some inline comments. Didn't test or review in full details that all actions that need to run continue to run. Though with exception of one case (from memory) it looked good, the one case could be right though.

High level comments:

The way we call APIs by name and array of args is not IDE friendly. They won't be detected anymore by the IDE as usages.
The lack of event specific objects makes event handling more complex and complete to detect the delta. The plugin has to do a diff to reverse engineer the change type. The plugins will also be blind to changes that happen only as part of the callbacks. We should really try not to have bug modifying logic in the callbacks. For example: it is OK to send an email, update history, but we shouldn't set custom fields, add notes, add attachments, etc.
I'm not sure that changes by plugins will trigger the appropriate history changes. I suspect that we only track changes based on the original change.

Imagine the approach of having an object for change status. Such change can include all information that about the change including old status, new status, any extra fields set, and note added. This provides the plugin with the ability to validate and handle the event as a bundle with full visibility.

vboctor · 2017-04-24T01:13:14Z


 		$t_old_data = bug_get( $this->id, true );

+		event_signal( 'EVENT_BUG_UPDATE_PRE', array( array( 'modified_bug' => $this, 'original_bug' => $t_old_data ) ) );


I would rather use old and new and avoid the term bug.

vboctor · 2017-04-24T01:14:48Z

+			call_user_func_array( $t_callback['func'], $t_callback['params'] );
 		}

+		event_signal( 'EVENT_BUG_UPDATE_POST', array( array( 'modified_bug' => $this, 'original_bug' => $t_old_data ) ) );


Same here old and new.

vboctor · 2017-04-24T01:24:11Z

-	$t_bugnote_id = bugnote_add( $p_bug_id, $p_bugnote_text, $p_time_tracking, $p_bugnote_private, 0, '', null, false );
-	bugnote_process_mentions( $p_bug_id, $t_bugnote_id, $p_bugnote_text );
+	# @TODO
+	# If time tracking is enabled, this code is executed outside of BugData validation.


I don't recall the reason why we do this special behavior for time tracking notes? Do you know why?

Btw, @ TODOs are picked up by some IDEs like PhpStorm, but they show the first line next to them, so it is generally good practice to try the have the todo on the same line and having enough info on first line to provide an idea of what needs to be done. There are multiple occurrences of such usage of @ TODOs.

vboctor · 2017-04-24T01:26:58Z

-		if( user_exists( $t_old_reporter_id ) ) {
-			bug_monitor( $p_duplicate_id, $t_old_reporter_id );
+		if( user_exists( $t_bugdata->reporter_id ) ) {
+			$t_bugdata->add_update_callback( 'bug_monitor', array( $p_duplicate_id, $t_bugdata->reporter_id ) );


This is a general comment about the approach for registering callbacks. Since this is no longer a normal call to APIs like bug_monitor(), IDEs like PhpStorm won't pickup such usage as a call to the function. Should we instead have a callback function that would call such APIs normally?

vboctor · 2017-04-24T01:31:32Z

+				</para>
+
+				<para>
+					Note: As the create action may be altered or stopped by other plugins,


How should plugins trigger the stopping in a way that works well with web UI and APIs? Is it via trigger_error? It is worth testing this case and clarifying guidance in docs as needed.

cproensa · 2017-04-24T08:17:52Z

I'm not sure that changes by plugins will trigger the appropriate history changes. I suspect that we only track changes based on the original change.

BugData::update() always calls history_log_event_direct for its properties. Any change should be logged to history.

This is a general comment about the approach for registering callbacks. Since this is no longer a normal call to APIs like bug_monitor(), IDEs like PhpStorm won't pickup such usage as a call to the function. Should we instead have a callback function that would call such APIs normally?

I see that would mean use anonymous functions, like they are used in some places in this PR by assigning a variable:
$callback = function () { ... };

How should plugins trigger the stopping in a way that works well with web UI and APIs? Is it via trigger_error? It is worth testing this case and clarifying guidance in docs as needed.

yes, trigger_error is currently the only way to stop an invalid bug change. It's definitely not oriented to "works well with web UI and APIs", but we don't have any other way at the moment.
My suggestion for this case is using exceptions, but not in the scope of this PR.

The lack of event specific objects makes event handling more complex and complete to detect the delta. The plugin has to do a diff to reverse engineer the change type. The plugins will also be blind to changes that happen only as part of the callbacks.

That's the same behaviour we have now (for BugData properties).
Ideally, we should have all bug related data objectified: notes, attachments, custom fields, etc. so we can manage all the bug context as a whole, and manage proper deltas. That's a lot redesign which is out of scope of this PR. That would probably relate to a redesigned notification system too.

We should really try not to have bug modifying logic in the callbacks. For example: it is OK to send an email, update history, but we shouldn't set custom fields, add notes, add attachments, etc.

modifying in the callbacks was precisely the motivation to add them.
We want to modify custom fields, notes, attachments, only:

After the pre-processing events are cleared. Otherwise we would have committed modifications even if the bug update failed due to an error, which may be intended to stop an invalid bug change.
Before the post-update event is signalled. At that point the receivers of the event must have available the final state of the bug change and all the related entities.

Additionally.
I'm still not sure if the way of passing the data to the event as one single array:
array( 'old' => BugData object, 'new' => BugData object )
is in line of your suggestions instead of having both objects as raw parameters (like is done at the moment with events)

vboctor · 2017-04-24T15:07:39Z

BugData::update() always calls history_log_event_direct for its properties. Any change should be logged to history.

Good.

I see that would mean use anonymous functions, like they are used in some places in this PR by assigning a variable: $callback = function () { ... };

Yes

yes, trigger_error is currently the only way to stop an invalid bug change. It's definitely not oriented to "works well with web UI and APIs", but we don't have any other way at the moment. My suggestion for this case is using exceptions, but not in the scope of this PR.

Would it be too hard to support exceptions thrown from event handlers? i.e. for event X, plugins can throw exception Y and it will be caught by the event_signal() caller? That will provide a way that is friendly to web ui/api. Similar to our current APIs, the code used in such passes should avoid calls like ensure_condition1() which internally triggers error, but call is_condition1() and throw the exception if needed. I'm OK with being part of a different PR, but don't think it needs to be associated with an exceptions everywhere scope of work.

Ideally, we should have all bug related data objectified: notes, attachments, custom fields, etc. so we can manage all the bug context as a whole, and manage proper deltas. That's a lot redesign which is out of scope of this PR. That would probably relate to a redesigned notification system too.

To summarize my position here:

I agree that we need to some ORM model with proper model objects and support for new/modified data that can be processed and then committed to db. Active Record kind of stuff. In fact I was going to open an issue for discussion this - "Modern Model".
In the case where we have the "Modern Model" described above, I suspect we still need Event Objects.
I agree that this is too much work for this PR.
I dislike the fact that we are churning the code a lot, deprecating events and adding new ones, but we only solved part of the problem.

I'm still not sure if the way of passing the data to the event as one single array:

I was just suggesting changing the naming of the old and new objects rather than changing how you send them to the events.

modifying in the callbacks was precisely the motivation to add them.

I understand. However, we still have the gap where plugins don't see a note change. And have to manually stitch a status change + related note. For example, there is no way for a plugin to stop changing a status without a note explaining the work or making sure that a custom field is populated with some value. If we use event objects, then they can look at the whole change, approve it or modify it, then it goes through.

cproensa · 2017-04-24T18:23:18Z

Would it be too hard to support exceptions thrown from event handlers? i.e. for event X, plugins can throw exception Y and it will be caught by the event_signal() caller? That will provide a way that is friendly to web ui/api. Similar to our current APIs, the code used in such passes should avoid calls like ensure_condition1() which internally triggers error, but call is_condition1() and throw the exception if needed. I'm OK with being part of a different PR, but don't think it needs to be associated with an exceptions everywhere scope of work.

In some other place i suggested introducing an alternate error handler that packages the captured error in an exception, and then throws it. The context was to manage bug-action-group where one error would not stop the whole batch. This would be another application of the same concept.
The error->exception mapper should be used as a switchable mode, very like we now use a modifier to not populate error pages (ajax requests, etc.). One OldStyleMantisErrorException is enough for this, and contains info for actual error code that were captured.
This allows to achieve that behaviour withouth refactoring all the code to use exceptions, which can also be introduced progressively.
Nonetheless, a exception can be created to manage the specific case of "invalid bugdata change".
Previous scenario means that both the dedicated exception, and any arbitrary error, can be managed equally.

I dislike the fact that we are churning the code a lot, deprecating events and adding new ones, but we only solved part of the problem.

I dont know. For sure: we are lacking full coverage with the current events. Almost any action that doesn't come from bug-update-page or bug-create-page is not managed by those.
The callback stuff is intended to add a little of consistency in the update workflow. We can leave it out and it won't be worse than we were before.

I was just suggesting changing the naming of the old and new objects rather than changing how you send them to the events.

ok, but i'd rather be sure that this is a desired trait for future events too. This array thing makes more sense when an event would have a bigger number of parameters. So the plugin callee should not define:
function event_handler( $p_event, $dontcare1, $dontcare2, $dontcare3, $relevant_param ) { ... }
In this PR, two of the events only issues one parameter, so the array is overkill. However, I'm aiming for consistency here.

If you have other plans, of which there are many possibilities, there's the option to leave the array parameter out, to keep consistency with the old event convention, until a better defined methodology is defined.

vboctor · 2017-04-26T15:17:13Z

ok, but i'd rather be sure that this is a desired trait for future events too.

I think it is a common pattern for event handlers to get an event type + event args. And the args is either a strongly typed class or a property bag with keys and values. Events can start with single or two parameters, but then in the future, more may be added to provide more context or whatever, and we need a way to do them. When referring to an array/property bag, keys are much better than array offsets.

Here is an example for triggering AuthPlugin event:

	$t_event_arguments = array(
		'user_id' => $t_user_id,
		'username' => $t_username,
		'email' => $t_email,
	);

	$t_flags = event_signal( 'EVENT_AUTH_USER_FLAGS', array( $t_event_arguments ) );

removing callbacks

It is unclear to me if we are benefiting from having callbacks vs. running such follow up tasks after the call to ->update() or ->create()? It seems that we update db, email, then call callbacks anyway. So does it make a different if it is inside the update/create calls vs. called after from the calling code? I get the value though in the case where the code needs to run after saving to validating via event, validating via core, saving to db and before email notifications.

So my feel is that they can be useful when we need such callbacks to be called in the middle of the create/update functions. And maybe we overtime just reduce reliance on follow up bug updates to BugData in such functions.

If code can just run after update()/create() is done, then it shouldn't be a callback. If code needs to run in the middle of these methods, then it should be but we should make it IDE friendly.

cproensa · 2017-08-19T00:11:07Z

Updated with some of the requested changes

Rewrite callbacks to call directly to api functions (IDE friendly)
Rename event parameter indexex as "new', "old", etc
Clean up the timetracking special cases for bugnotes

cproensa · 2017-09-28T19:23:44Z

@vboctor reminder!

A callback queue in BugData object. The registered callbacks are performed in the update() function, after the data are saved, and before the EVENT_UPDATE_BUG event is called. These callbacks are needed becasuse with some bug actions, some auxiliary operations must be performed between the events EVENT_UPDATE_BUG_DATA and EVENT_UPDATE_BUG. For example: bug notes, must be inserted only if the action is performed, which can be stopped if the validation fails.

EVENT_UPDATE_BUG_DATA and EVENT_UPDATE_BUG are now triggered in the BugData update function. Rewrite bug_update.php to reflect this change.

Use BugData class for bug modifications

Avoid early returns in update method to make sure all update steps are performed.

Use the returned BugData object from the event call to comply with the definition of EVENT_UPDATE_BUG_DATA as EVENT_TYPE_EXECUTE The argumentation, as explained in the pull request: >>> Currently we have EVENT_UPDATE_BUG_DATA defined as "chain". This means that the returned value by the event call is the object expected to be used. In the case of modifying a BugData object, this is not needed, as the BugData object passed as parameter to the event is a reference, and changes made through this reference are made into the actual (original) object. Let's see the BugData::update() function after this PR: inside update(), we call: event_signal( 'EVENT_UPDATE_BUG_DATA', $this, $t_old_data ); and later, we update the bug with the values from $this. If the plugin makes changes to the supplied BugData reference, there is no problem, they are saved in the actual object. But, being a chain event, the returned reference may be altered, to another object not being the original one. This doesnt make much sense, but is possible though. For example: function plugin_bug_data_hook( $event, $updated_bug, $old_bug ){ $another_data = new BugData(); ... return $another_data; } back to the BugData::update(), we cannot update "self" with another reference, from inside the object. <<< The described exceptions are corner cases that probably should not be supported, but as current event functionality allows it, the changes in this commit are made to account for it.

The call to validate() is now placed after the event call, which may return a modified object. The validation is now placed immediatly before database update.

EVENT_REPORT_BUG_DATA and EVENT_REPORT_BUG are now triggered in the BugData create function.

Move the code that is executed after bug creation into callbacks, to be campleted before EVENT_REPORT_BUG is event is triggered

Create new events for changes triggered from BugData internal functions. 'EVENT_API_REPORT_BUG_PRE' => EVENT_TYPE_EXECUTE, 'EVENT_API_REPORT_BUG_POST' => EVENT_TYPE_EXECUTE, 'EVENT_API_UPDATE_BUG_PRE' => EVENT_TYPE_EXECUTE, 'EVENT_API_UPDATE_BUG_POST' => EVENT_TYPE_EXECUTE, Revert the changes from: 3c7f573 As with the new events they are not needed.

As new events have been created, keep the old ones to keep old behaviour for compatibility.

At this point we dont know the bug id, so use the call without it.

Rename the new events after suggestions from Pull Request

Make both events: EVENT_BUG_UPDATE_PRE, EVENT_BUG_UPDATE_POST have the same parameter definition.

Function add_create_callback() as wrapper for add_update_callback(), named in context of bug creation.

Replace callbacks tha use call_user_func_array with anonymous functions taht make the actual function call. This way IDE tools can detenct function usages.

Use shorter terms 'new' and 'old', omitting the term 'bug'.

If timetracking is enabled, the configuration may be set to not allow a timetracking note with empty text. Do a previous check before doing the actual bug update. If the bugnote is not valid, an error would stop the execution before actual changes are made.

cproensa · 2017-12-12T09:01:46Z

@vboctor
What are the blocking issues in this PR, at this point?
This was started in Aug-2016...

vboctor · 2017-12-13T07:50:21Z

@cproensa will have a look again. Would be good to get others to have a look too.

cproensa · 2018-02-17T17:18:50Z

@vboctor
With the introduction of commands this PR is becoming unmergeable.
I am tired of keeping it alive, so will close it shortly

Could you at this point of the command refactoring, provide a solution for processing events consistently?
For me it's a real need right now, after 1-2 years of messing with it. And regarding any plugin development, it's also a priority.

vboctor · 2018-02-18T04:19:38Z

@cproensa I'm working on more commands like IssueAddCommand and I think events will make a lot more sense in context of such commands. The events will also use the REST API format for its args which will be more powerful and portable compared to what we had in the past.

cproensa · 2018-02-18T09:43:09Z

I'm working on more commands like IssueAddCommand and I think events will make a lot more sense in context of such commands

How is that? You can introduce new events, or trigger the existing ones, for each "command", but that won't solve the same issue that i try to address here.

cproensa · 2019-02-21T23:24:09Z

outdated

cproensa mentioned this pull request Apr 21, 2017

Move bug events into BugData #966

Closed

vboctor assigned cproensa Apr 23, 2017

vboctor requested changes Apr 24, 2017

View reviewed changes

cproensa force-pushed the m2_bugdata_events branch from 6de40fd to f330874 Compare August 19, 2017 00:08

cproensa mentioned this pull request Sep 28, 2017

Create event EVENT_BUG_ACTIONGROUP_FORM #1199

Merged

cproensa added 18 commits October 21, 2017 13:19

Move bug update events into BugData

b5110bc

EVENT_UPDATE_BUG_DATA and EVENT_UPDATE_BUG are now triggered in the BugData update function. Rewrite bug_update.php to reflect this change.

Use BugData for bug_move

93b20d4

Use BugData class for bug modifications

Use BugData for bug_assign

84b7a47

Use BugData class for bug modifications

Use BugData for bug_close

b093319

Use BugData class for bug modifications

Use BugData for bug_resolve

9b5f4b4

Use BugData class for bug modifications

Use BugData for bug_reopen

df6c132

Use BugData class for bug modifications

Use BugData for bug_stick.php

03148ac

Use BugData class for bug modifications

Use BugData for sponsorship_api.php

308f50e

Use BugData class for bug modifications

Use BugData for bugnote_add.php

4bfd3db

Use BugData class for bug modifications

Use BugData for bug_actiongroup actions

ee4f7a9

Use BugData class for bug modifications

Clean up code, add comments

3b7551e

Fix BugData update early return

23058b2

Avoid early returns in update method to make sure all update steps are performed.

validate BugData object after plugin events

e15f4c9

The call to validate() is now placed after the event call, which may return a modified object. The validation is now placed immediatly before database update.

Move bug update events into BugData

b3835bd

EVENT_REPORT_BUG_DATA and EVENT_REPORT_BUG are now triggered in the BugData create function.

Move code into callbacks

737e69c

Move the code that is executed after bug creation into callbacks, to be campleted before EVENT_REPORT_BUG is event is triggered

cproensa added 13 commits October 21, 2017 13:28

Add documentation for new events

a06faf6

Restore original bug events

e57080b

As new events have been created, keep the old ones to keep old behaviour for compatibility.

Fix check for file upload

53c11f8

At this point we dont know the bug id, so use the call without it.

Fix docbook ids

74ce687

Throw a warning for deprecated events

71df117

Rename events

3891884

Rename the new events after suggestions from Pull Request

Reorder parameters in bug update events

260d6ef

Make both events: EVENT_BUG_UPDATE_PRE, EVENT_BUG_UPDATE_POST have the same parameter definition.

Use an indexed array for events parameter

c7f203f

Better indication of deprecated event types

096ce37

Function add_create_callback() as wrapper

803c877

Function add_create_callback() as wrapper for add_update_callback(), named in context of bug creation.

Replace callbacks for IDE friendly calls

11f991c

Replace callbacks tha use call_user_func_array with anonymous functions taht make the actual function call. This way IDE tools can detenct function usages.

Rename event parameter indexes

bc130ea

Use shorter terms 'new' and 'old', omitting the term 'bug'.

cproensa force-pushed the m2_bugdata_events branch from f330874 to e9caf0d Compare October 21, 2017 11:30

cproensa mentioned this pull request Nov 23, 2017

Support exceptions and re-use of business logic for UI and APIs #1240

Merged

cproensa closed this Feb 21, 2019


		$t_old_data = bug_get( $this->id, true );

		event_signal( 'EVENT_BUG_UPDATE_PRE', array( array( 'modified_bug' => $this, 'original_bug' => $t_old_data ) ) );

Conversation

cproensa commented Apr 21, 2017

Uh oh!

cproensa commented Apr 21, 2017

Uh oh!

vboctor left a comment

Choose a reason for hiding this comment

Uh oh!

vboctor Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

vboctor Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

vboctor Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

vboctor Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

vboctor Apr 24, 2017

Choose a reason for hiding this comment

Uh oh!

cproensa commented Apr 24, 2017

Uh oh!

vboctor commented Apr 24, 2017

Uh oh!

cproensa commented Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vboctor commented Apr 26, 2017

Uh oh!

cproensa commented Aug 19, 2017

Uh oh!

cproensa commented Sep 28, 2017

Uh oh!

cproensa commented Dec 12, 2017

Uh oh!

vboctor commented Dec 13, 2017

Uh oh!

cproensa commented Feb 17, 2018

Uh oh!

vboctor commented Feb 18, 2018

Uh oh!

cproensa commented Feb 18, 2018

Uh oh!

cproensa commented Feb 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cproensa commented Apr 24, 2017 •

edited

Loading