Add TableEventsTrait #18008
Add TableEventsTrait #18008
Conversation
| @@ -3204,16 +3205,6 @@ public function implementedEvents(): array | |||
| 'Model.beforeRules' => 'beforeRules', | |||
| 'Model.afterRules' => 'afterRules', | |||
| ]; | |||
| $events = []; | |||
|
|
|||
| foreach ($eventMap as $event => $method) { | |||
There was a problem hiding this comment.
Isnt this removing the opt in part of it?
There was a problem hiding this comment.
I don't understand your question but this code is unnecessary anymore because all events listed in $eventMap are implemented so $eventMap and $events at the bottom are the same.
There was a problem hiding this comment.
I was just kind of thinking that the trait could stay opt in
Not every table might need them anyway.
There was a problem hiding this comment.
With this implementation it is up to the user if she wants to implement the callbacks. If not implemented in the user code then empty callbacks are called. So you can say that they are still opt-in. The user is not forced to implement them.
There was a problem hiding this comment.
This foreach needs to stay since we want the trait usage to be opt-in. I already mentioned in the other PR that always having the methods prevents one from typing the $entity argument with a concrete entity class.
There was a problem hiding this comment.
Is phpstan also on board here?
With the overwrite?
There was a problem hiding this comment.
An example of the fatal error: https://onlinephp.io/c/f4e51
There was a problem hiding this comment.
It will throw an error if the signature is changed in the child class.
So if were to have this trait, it would have to be included in each table class separately and not Table (which can be done when baking).
Would have to check if static analyzers/IDEs complain if you override trait methods.
There was a problem hiding this comment.
Do you suggest to remove use TableEventsTrait; from the Table class?
|
I must say I like the interface approach more. The trait could still be there as fallback. But not how about it being present by default. |
| @@ -3204,16 +3205,6 @@ public function implementedEvents(): array | |||
| 'Model.beforeRules' => 'beforeRules', | |||
| 'Model.afterRules' => 'afterRules', | |||
| ]; | |||
| $events = []; | |||
|
|
|||
| foreach ($eventMap as $event => $method) { | |||
There was a problem hiding this comment.
Seems I am mistaken, PHP seems to be lenient when overriding trait methods.
shoud be replaced with this would allow to cleanly reuse this trait in muffin/webservices |
|
@asgraf This trait is under the |
|
Even with |
Explicitly define all model callbacks. See the discussion in #17990