Call model methods using event handler with default priority

[mjauvin]

Replaces #145

This preserves current behavior: an event listener defined on the model with default priority (0) will get called first, then the model method will get called (through an event listener with default priority).

If an event listener uses a LOWER priority (e.g. -1), the model method will get called first (because its priority is 0 by default)

Note: there is still one thing I'd like to get clarified: an event listener defined in a plugin's boot method with default priority(0) will get called BEFORE the listener that calls the model method... so that means the Plugin's event listeners get registered FIRST, before the event listeners defined in the Model's bootNicerEvents() method... anyone care to explain why that is ?

My guess is that Plugin's boot() method gets called before any model are instanciated, which makes sense, right?

[LukeTowers]

@bennothommo @jaxwilko any final thoughts on this?

[mjauvin]

I still need to apply this to other places in the code base, but didn't want to invest the time until this is accepted as a solution.

[mjauvin]

I added halt:true to the fireEvent() call, but this might break BC if some existing listener returns a value since that will bypass the model method (which wasn't the case before).

So I think we shouldn't be using a halting event. But if we don't, then we will break things anyway because it wont't be possible anymore to cancel an action even with the model method.

Thoughts ?

[mjauvin]

Just to be clear: currently, you cannot prevent the action through an event hanlder. For example, if you return false from the beforeSave() method on a model, the save action will get cancelled... but if you return false from a listener on the model.beforeSave event, the action will still go through.

Now, it WOULD be nice if both methods would behave the same, but this will definitely break things in the most unexpected ways...

I checked all winter plugins and core, there are no cases where an event handler actually returns a value on the before{Create,Update,Save,Delete} events, and definitely nothing returns false.

[bennothommo]

I think there's always been an implicit knowledge that events, in general, will cancel any further functionality when false is returned, so the likelihood that people have accidentally returned a false when they shouldn't have is low.

The event docs for models explicitly state that you should return false in callback methods to cancel the action, and there's nothing else that I've seen that contradicts that, so I would say that both the callbacks and the events should work the same.

[mjauvin]

I think there's always been an implicit knowledge that events, in general, will cancel any further functionality when false is returned, so the likelihood that people have accidentally returned a false when they shouldn't have is low.

The event docs for models explicitly state that you should return false in callback methods to cancel the action, and there's nothing else that I've seen that contradicts that, so I would say that both the callbacks and the events should work the same.

I agree, this is the saner way to go and will set things straight.

[mjauvin]

@LukeTowers @bennothommo should I now go ahead and apply the above for the Validation & SoftDelete Traits and the Halcyon model ?

[mjauvin]

@bennothommo I don't understand the latest Code Analysis errors.

[LukeTowers]

@bennothommo could you take a look?

[bennothommo]

@mjauvin @LukeTowers for future reference, if you get an error like that from PHPStan ("Ignored error pattern..."), it means that an error we put in the baseline to be ignored originally is no longer occurring, so we just need to remove the error pattern that follows from phpstan-baseline.neon as I have. This is a good thing - the less we have being ignored in the baseline, the better.

[mjauvin]

Ok I'm going to add unit tests for this, any suggestion/preference ?