Fire model events after event methods to facilitate extensibility.

[mjauvin]

This is a proposal to fire the model events (before/after create/delete/save/update) after the corresponding model event method has been called in order to allow overriding the behavior of the model event methods using events.

TODO:

• apply to Halcyon/Model
• should this apply to SoftDelete trait ?
• should this apply to Validation trait ?

[mjauvin]

Any feedback on this one guys?

[LukeTowers]

@mjauvin could you provide a usage example?

[mjauvin]

@LukeTowers one example is in your EasyForms plugin, for the Confirmation model, you have the beforeSave() method like this:

    public function beforeSave()
    {
        // Prevent default confirmations from being disabled or renamed
        if ($this->is_default) {
            $this->is_active = true;
            $this->name = 'Default Confirmation';
        }
    }

If for some reason you want to extend this model dynamically by using the model.beforeSave event, you cannot change is_active for example since the model method runs AFTER the event gets called.

This is just an example, but the idea is that currently, the default model functionality cannot be changed dynamically with model events if the original model has a corresponding method defined which overrides the changes you want to make.

[LukeTowers]

If we're going to make this change it should be applied to everything in the core that fires the model events using this pattern.

[mjauvin]

If we're going to make this change it should be applied to everything in the core that fires the model events using this pattern.

Done!

[LukeTowers]

@mjauvin after thinking about this more I think we're still stuck with the same problem. Now if someone wants to prevent something happening in beforeSave or beforeValidate for example they aren't able to do so.

I think the only solution would be to force the model methods to fire as listeners with a default priority on the events, i.e.

if ($model->methodExists('beforeSave')) {
    $model->bindEvent('model.beforeSave', fn => $model->beforeSave()); // or whatever the syntax is
}

Any thoughts on this @bennothommo or @jaxwilko?

[bennothommo]

I still have the same thoughts as before to be honest - I've always felt that the internal "developer intended" model methods should have the last say on anything, over any externally driven functionality through events.

I know it goes (somewhat) against our implied total extensibility, but it gives some agency to the plugin (or core feature) to have at least some control over its own functionality.

EDIT: However... I think it would be palatable to copy DOM events, in that an external event listener can prevent the default functionality, so perhaps we can keep the original order of the calls (prior to @mjauvin's PR) but maybe allow some way to "prevent default" - even if it's just allowing false to be returned and using that to skip the model's callback method.

I realise this goes against my first thoughts 😂. I guess I'm just trying to find a middle ground.

[mjauvin]

Ok, I see good points on each side, not sure what to think anymore...

[jaxwilko]

I'm kinda with @bennothommo's edit, overriding the default would be nice, but could also introduce a load of different problems. What I see being an issue could be replacing a 3rd party plugin's beforeSave to manipulate data but then the 3rd party pushing an update with extra logic in beforeSave than now causes issues when the method is being overwritten.

Is it worth writing the test cases for what we would expect as plugin devs and then working out how it should work from that?

[LukeTowers]

What I see being an issue could be replacing a 3rd party plugin's beforeSave to manipulate data but then the 3rd party pushing an update with extra logic in beforeSave than now causes issues when the method is being overwritten.

My expectations as both the author of the original plugin and the author of the extending plugin is that it is solely the responsibility of the extending plugin to maintain compatibility with the original and to appropriately lock down its own requirements if such a concern arises.

At this point in time I've personally not had a need for any changes to be made to the way the events / methods play with each other currently, but I think placing them on the same level and giving the control to the listener via the event priority system to attach either before or after the methods run would give the most flexibility.

However, as always, I do not like making changes for changes sake and I agree with @jaxwilko that we'd need to see some more real world examples of the need to change what we already have or just test cases for the desired functionality in general.

[mjauvin]

I would go with @LukeTowers' proposed solution with the events, I thinks it gives maximum flexibility without breaking anything like changing the order might...

[LukeTowers]

@bennothommo @jaxwilko any final thoughts?

[jaxwilko]

Nope, I did encounter something that might be relevant, trying to prevent file deletion in a afterDelete (File model), but it's not that important for my use case atm

[mjauvin]

Nope, I did encounter something that might be relevant, trying to prevent file deletion in a afterDelete (File model), but it's not that important for my use case atm

@jaxwilko You can use the deleting() method on the model to do this:

YourModel::deleting(function ($model) {
    return false;
}); 

[mjauvin]

See #150 for the solution proposed by @LukeTowers

I tested it and it works very well, preserving default behavior.

[LukeTowers]

Closing in favour of #150