Such changes should target 5.next directly (minimum).

Such changes should target 5.next directly.

This is not a backward compatible change and would have to wait till 6.0, if approved.

I do like the idea of enforcing the signature for the callback methods, but not too sure about this implementation. If someone adds the beforeMarshal() method but forgets to implement the interface the callback won't be executed. One would imagine such an oversight is easy to debug, but based on my experience in the support channel such silent failures can often lead to a lot of hair pulling, especially for new users.

@ADmad an alternative approach could be to add empty methods like public function beforeMarshal(EventInterface $event, ArrayObject $data, ArrayObject $options): void {/* To be implemented in children classes if necessarry */} directly to Table class like the existing initialize method.

Yes, adding empty methods is an option too, we already have that for controller events.

But adding the empty methods in 5.2 can also if problematic. Since a signature mismatch with existing methods will create a PHP error.

The main issue with actual methods and inheritance is that you cannot use the concrete objects as types, only EntityInterface etc
Then you rely on the docblock and IDE to "typehint" the correct one to be able to see available properties and methods and let static analyzers do their job.
Right now people are free to use the concrete one if they want to directly.

The main issue with actual methods and inheritance is that you cannot use the concrete objects as types, only EntityInterface

Yes, hence the chance of signature mismatch, I too often use type the argument using the concrete entity class.

If someone adds the beforeMarshal() method but forgets to implement the interface the callback won't be executed.

@ADmad I adjusted my changes so that if a model has a callback method but does not implement a corresponding interface then an exception will be thrown. What do you think about this approach?

..if a model has a callback method but does not implement a corresponding interface then an exception will be thrown

That could be considered for the next major.

But I am not in favor of using similar interfaces for callbacks which take an entity as an argument. Typing the argument using EntityInterface in the interface would prevent specifying a concrete entity class as the argument type in the method as that's not be contravariant.

But I am not in favor of using similar interfaces for callbacks which take an entity as an argument. Typing the argument using EntityInterface in the interface would prevent specifying a concrete entity class as the argument type in the method as that's not be contravariant.

That I do not understand. Do you suggest to remove "EntityInterface" from the method signature like
public function afterSave(EventInterface $event, $entity, ArrayObject $options): void;?

Would a different approach #18008 be better than this?

Do you suggest to remove "EntityInterface" from the method signature like

Not specifying the type for the $entity argument would be an option then we are back to specifying @var annotation to make the type known to static analyzers and IDEs. I don't really have a solution to this problem right now.

Users could chose.
If you add these interfaces to your app/plugin table classes it would autocomplete via IDE for you.
Everything else still works normally.

We could in the future also check on the interface for existence of callbacks.
Of course this would be opt-in, the classic way still works as before.
So the code still mainly checks on the method existence of course.

So basically this is just a way for users to more quickly add their event callbacks with pre-defined signatures if they want to.

I think the framework should provide them as part of the contract. And then people can use to use them.

I like the TableEventsTrait approach of #18008. Users can opt-in to add it to their table classes to get method autocompletion.

@ADmad I've finished #18008 and all tests pass there.

With the other PR advancing, closing this for now.