[RFC][2.3][EventDispatcher] toward cleaner events #7627
Conversation
| @@ -158,10 +159,10 @@ public function removeSubscriber(EventSubscriberInterface $subscriber) | |||
| * @param string $eventName The name of the event to dispatch. | |||
| * @param Event $event The event object to pass to the event handlers/listeners. | |||
| */ | |||
| protected function doDispatch($listeners, $eventName, Event $event) | |||
| protected function doDispatch($listeners, $eventName, Event $event, EventDispatcherInterface $dispatcher = null) | |||
|
tests should be updated for this |
|
This is not ready for merge. I'd like feedback on the approach before spending the time to flesh it out. |
| $event->setName($eventName); | ||
|
|
||
| if (!isset($this->listeners[$eventName])) { | ||
| return $event; | ||
| } | ||
|
|
||
| $this->doDispatch($this->getListeners($eventName), $eventName, $event); | ||
| $this->doDispatch($this->getListeners($eventName), $eventName, $event, $dispatcher); |
|
@kriswallsmith - if you don't have time I can finish this PR up and add the necessary tests, but I would be sorry to see getName() removed from the base |
|
The one thing I don't like about this is that the new third parameter for dispatch() is also exposed to "userland", i. e. client code wanting to dispatch an event. It is optional, but in times of IDEs and code suggestion one might wonder what to put in there. In fact it is relevant only internally in event dispatcher compositions. As the title is "towards cleaner events" I would also like to add the following reasoning. Event objects are supposed to carry additional (domain) information about what happened. Their name is given to a dispatcher along with the object. As long as the API is like that, the name is not a property of the Event in the first place. If it were, the dispatcher's client could as well set the name on the Event itself (instead of passing it to the dispatcher) or the Event would even provide the name (hard-coded). The same applies to the dispatcher: If an Event describes what happened, it should be agnostic of how that information is distributed to interested parties (listeners). The Event class currently mixes up what happend with how that is distributed. In fact, the base class does not even care about what and only provides methods for the how. This PR is in line with that reasoning and starts to clean this up, but it does not go the whole way. If you take responsibilities for the name and dispatcher from this base class, you leave it with the propagation-stopping stuff alone. IMO that belongs to the other two and could easily be implemented another way as listeners get the dispatcher already (even more explicitely in this PR). So what we could do in the long run is to separate the "Event" (an arbitrary, userland, "what"-object simply passed around; no base class needed; dispatcher-agnostic) from an "EventPropagation" class. The first dispatcher involved creates it and it is passed to all listeners, similar to what is suggested above. Only that the EventPropagation is more valuable than the dispatcher alone as it could take more methods in the future without changing listeners' signature, keep track of all dispatchers involved etc. _Update_: I made #7792 for this. |
|
@kriswallsmith it cannot be moved to the dispatcher, only to a separate class. The propagation is a state related to the event. The dispatcher must stay stateless, otherwise it will become really hard to dispatch several events (and even worse when dispatching an event inside a listener for another event) |
|
I've updated my PR. Propagation can be stopped by throwing an exception. I know some of you are going to balk about this use of an exception, but again, it's the simplest and cleanest solution to the problem. Other ideas are welcome, of course. |
|
I created #7792 to show an idea how the Event Dispatcher could be made less obtrusive in 3.0 based on this work. |
|
I'm (obviously) -1 to this change. I agree with @Drak that doing this just to fix a problem in the way we collect data is wrong. Then, we decided a long time ago to use the current approach (which was really the Doctrine approach), so, that's not to change it now just before the first LTS. We might revisit this for 3.0, but as this is in a very long time, I'm closing this PR for now. |
|
I'm not clear on why you oppose this PR (it's not obvious). Would you mind explaining? Thanks! |
| trigger_error( | ||
| 'Calling stopPropagation() on the event object is deprecated. '. | ||
| 'Throw a StopPropagationException instead.', | ||
| E_USER_DEPRECATED |
There was a problem hiding this comment.
This seems like a bad idea of using exceptions to control application flow.
I've quickly implemented my proposed fixes for both #6686 and #7582. We can't remove
getName()andgetDispatcher()fromEventbecause those methods are marked@api, but we can deprecate them and remove them in 3.0.