I copy and answer here @vboctor comments on the PR966
https://github.com/mantisbt/mantisbt/pull/966#pullrequestreview-14807058
Good change @cproensa - I've added some inline comments. But here are my high level comments:
It would be great to start the new extensibility model for these events where we have an object
per event. 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).
This will give have the following benefits:
I understand your proposal as having a dedicated class for each event
This object would be only used for passing the data to and back from the event callback?
From previous conversations, i guessed the idea to be passing a parameter as, for example, array defined as:
array( 'new_bug_data' => <new BugData object>, 'old_bug' => <old BugData object> )
instead of having dedicated parameters in the callback function.
The aim is to have the ability to add new parameter, or optional ones that may appear in some cases,
inside this array, and not break the callback definition for small changes like those.
If a property x (e.g. id) on BugData makes sense for Event 1 (create-post) but not Event 2 (create-pre), then it will only show up on Event 1.
If a property x is read-write on Event 1 but read-only on Event 2, then this can be achieved.
We can incorporate logic via property setters or methods on such event objects.
This expands the idea of a dedicated class, which would be populated with some/all of the Bugdata properties, in this example.
This would add a lot of overhead, to copy those fields, and to manage if they are updated by the plugin.
Further, in this example, the approach should be to actually send the raw BugData, which encapsulates the data being modified.
In a future where more object-mapping is used to manage all application entities, this may make sens, to have
a readonly instance, and an editable one, for each type of object (eg, BugData). But this may be managed at the entity classes,
not in the event. For simplicity, in that scenario, i'd go with embedding a read-only entity object in the event data object.
A change in internal BugData structure doesn't break plugins.
If we are worried about potential changes in BugData class, then the right move is to clean up that class. This class
was introduced some time ago, but has been left half-cooked. It's good as a starting point but if it's going to be used
to allocate all bug-related logic, maybe it needs a restructuration.
In your suggestion of keeping a stable interface, this could be rethought to separate a clean stable base class/interface,
façade, etc, and a lower level logic layer.
An internal property on BugData doesn't show up to plugins.
This may be out of scope. Currently plugins currently have access to all core functions, without restrictions.
I would rather if plugins don't register callbacks, but depend on PRE vs. POST events to do the logic without side-effects
in PRE and do the necessary action in POST.
If i understand correctly, that's how it's supposed to work. Maybe it has not been made clear enough in the documentation.
PRE-processing eventa can modify any data inside the current BugData object. They should not make changes outside of this class
for example, add a note, because the update action is not guaranteed to be finished.
POST-processing events should do all the external changes, becasue here is when the BugData changes has been already commited.
Furthermore, they should not stop the execution.
Ideally, and i have it planned for a later change, is to move some validation/changes into this workflow inside BugData.
For example, the change of status when a bug is assigned, should be made at the PRE-processing stage.
All the stuff about managing callbacks inside the BugData class is to adapt the current core actions to this requeriment,
where side effects are performed after preprocessing, after BugData changes are saved, and before notifying a post-processing
event.
We could also consider having an event type where if a plugin fail, we would stop the execution of the rest (for PRE events),
and another where we would continue with other plugins (for POST events).
Currently, the non-obviusly-documented method to stop a update execution is to throw an error. This is an all-or-nothing approach
but currently it's the only one availabe.
Ideally if a plugin, when hooked to a pre-processing event, must stop the modification, it can be flagged by raising an exception
instead of an error.
The exception can be catched from the outside code, for example: at bug action group page.
Your suggestion would mean that a true or false must be explicitly returned by each plugin, and the event manager then stops the
execution chain. This is a clean solution, however, think that a pugin would usually want to return a reason on why the execution
was stopped, very much like the error code, or exception payload, would express an invalid operation.
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. Some ORM models would have such functionality built-in.
The update methods provide parameters for both the modified BugData object, and the previous unchanged object. This was existing
functionality in these events.
The email notifications, with this PR has been kept the same as they were before.
BugData update already has some checks to compare old and new values and issue email notifiacionts based on this.
Old code that did changes directly (by updating bug table), has hardcoded notification logic. This must be reviewed
more coarefully, becasue some of them bypasses notifications on purpose, or trigger a different notification
than the one a raw change in BugData properties does.
I suggest targeting this to master branch to go into a future 2.x minor release.
In a totally personal note: i really need this functionality as soos as posible. This has been in work since 1.3.0.
I have some complex plugins that has some broken functionality because of this, and migrating to a 2.x version is not
feasible in the short term.
Answering some of the inlined comments:
Did you consider having "create_callbacks"? At least as it appears externally to the calls via add_create_callbacks()?
Yes, i did. However, at the moment i've tried to keep it simple.
As i told prevously, the callbacks implementation is to fill the void of not having object oriented entities, where
changes can be staged, validated, and comitted in a managed way.
Ideally, this situation should be transitional until in some future, the same strategy is used for all entities.
Should we use PHP anonymous functions here to define an anonymous function that calls file_move_bug_attachments
and pass in the required parameter, rather than getting a separate function name and array of parameters?
This applies to callback registration and all calls to it.
It's done somehow with other callbacks. For example:
$add_bugnote_func = function( array $p_bugnote_params ) { ... }
$t_updated_bug->add_update_callback( $add_bugnote_func, array( $t_bugnote_params ) );
Your proposal is more oriented at the typical javascript programming style, where callbacks are defined ad-hoc
as anonymous functions. Im't not sure if that is feasible here. At the time of execution of that code, the data
that is neede (for example, the bugnote text, etc) must be stored somewhere within the code scope.
I found the way to have it done is by storing it as the "parameters" which will be passed to the callback function.
Originally, the bugnote data would have been fetched from POST inputs, but i'd rather extract it from bug_update page
main body, and keep it reserved for later use, than accesing POST within the BugData update callbacks later. |