[2.3][EventDispatcher] Moved EventDispatcher awareness to separate baseclass #7582

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@ghost

ghost commented Apr 6, 2013

Q A
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets na
License MIT
Doc PR symfony/symfony-docs#2476

A feature was added to inject the EventDispatcher into every Event object on the off-chance that if an Event needed to access the EventDispatcher it could get it for free.

I don't believe this was a very good choice because Event object are now exceedingly difficult to debug - if you var_dump() one when using Symfony Fullstack Framework, you'' get an event, with the event dispatcher in it, which has the DIC injected into that (which has the event dispatcher as a property :) and it's just a mess to see anything relevant.

So I have opted to separate Events into an EventDispactherAware events which can be used as a separate base-class if necessary. I believe that is the best compromise.

...ny/Component/EventDispatcher/Tests/ContainerAwareEventDispatcherTest.php
@@ -164,7 +164,7 @@ public function testHasListenersOnLazyLoad()
$dispatcher = new ContainerAwareEventDispatcher($container);
$dispatcher->addListenerService('onEvent', array('service.listener', 'onEvent'));
- $event->setDispatcher($dispatcher);
+ //$event->setDispatcher($dispatcher);
@pborreli

pborreli Apr 6, 2013

Contributor

you forgot to remove this line

@ghost

ghost Apr 6, 2013

Thanks - fixed.

@ghost ghost referenced this pull request in symfony/symfony-docs Apr 6, 2013

Closed

[WCM] Update documentation for EventDispatcherAwareEvent documentation. #2476

@ghost

ghost commented Apr 6, 2013

Docs PR done.

Member

stof commented Apr 6, 2013

You have to change the existing event classes to avoid the BC break

@@ -43,7 +43,9 @@ public function dispatch($eventName, Event $event = null)
$event = new Event();
}
- $event->setDispatcher($this);
+ if ($event instanceof EventDispatcherAwareEvent) {
@stof

stof Apr 6, 2013

Member

I would do the check based on an interface, so that you can extend another event class and add the method

@kriswallsmith

kriswallsmith Apr 9, 2013

Contributor

I agree with @stof

@ghost

ghost commented Apr 6, 2013

@stof what existing event classes? Nothing in Sf is using events to get the dispatcher.

Member

stof commented Apr 6, 2013

@Drak Core listeners are not. but this does not mean that userland listeners are not using it.

+ *
+ * @api
+ */
+class EventDispatcherAwareEvent extends Event
@staabm

staabm Apr 7, 2013

Contributor

Maybe abstract?

@ghost

ghost Apr 7, 2013

No, not here. It's the same principal as Event which can be used as is (and is also not Abstract nor implements an Interface).

+ public function testEventDoesNotReceiveTheDispatcherInstance()
+ {
+ $this->dispatcher->addListener('test', function ($event) use (&$dispatcher) {
+ $dispatcher = method_exists($event, 'getDispatcher') ? $event->getDispatcher() : null;
@staabm

staabm Apr 7, 2013

Contributor

Define the dispatcher in the outer scope, defaulting to null?

@ghost

ghost Apr 7, 2013

six and two threes. It simply follows the form of the testEventReceivesTheDispatcherInstance() test-case.

Owner

fabpot commented Apr 9, 2013

IIRC, the Drupal guys don't like the fact that we set the dispatcher and the event each time for performance reasons. So, this PR is going into the right direction (at least for them), but what about also moving the event name?

ping @Crell

@ghost

ghost commented Apr 9, 2013

@fabpot - from my perspective it's not performance, but just pure inconvenience to inject the dispatcher.

The logic for having the event dispatcher is sound, but it becomes very unweildly and impractical to have it by default as stated in the PR description:

if you var_dump($event) when using Symfony Fullstack Framework, you get an event with the event dispatcher in it, which has the DIC injected into that - which has the event dispatcher as a property and it's just a mess to see anything relevant.

The the event name however is essential metadata. You remember that was also part of the original symfony 1 event dispatcher. Listeners should be able to know the name. The same listener can be attached via different event names so you cannot assume a listener knows the name by which it was called.

So to be clear, I actually like the dispatcher as part of the event, but it's not very practical by default. The event name however, is completely unobtrusive and essential.

Contributor

Crell commented Apr 9, 2013

I wasn't even aware that Events got the dispatcher injected into them. I agree with Drak that sounds weird, and wold make debugging more difficult. I don't think it would be a performance concern, just a DX (Developer eXperience) issue.

The thing that bugs us on the Drupal side (or me at least) is as Fabien says the cost of setting up the dispatcher (calling every single subscriber's static registration method on every request), and then the many layers of stack call to get through in an ContainerAwareDispatcher to fire even a single event. I don't think that's really related to this issue, though.

What Drak's doing here makes sense to me, although I agree with Stof that it needs to work based on an interface, even if a base class is provided, too. If I want to define an event type that extends an existing event type, but mine needs to be EventDispatcherAware, bam, I can't, because that's 2 base classes. Interface with default base class works much better.

(I so look forward to a future with PHP 5.4 where those default base classes can just become traits...)

Contributor

kriswallsmith commented Apr 9, 2013

What if we change the signature of the listener callable to something like this?

function($event, $eventName, $dispatcher);

That way we avoid the dispatcher calling any setters on the event object, which has always smelled wrong to me.

@ghost

ghost commented Apr 9, 2013

I see it like this. The Symfony Event Dispatcher pattern is very simple. There is no need for an EventInterface because you need at the very minimum an Event so there is no particular value in an interface for it. You need a concrete Event, period.

A certain amount of meta-data is already available for automatically when you dispatch an event, namely the event name and the event dispatcher. Currently that information is available in the base Event class which does not need to be an interface as shown. So in splitting the Event into two parts is just splitting what is already available. It's not like we're injecting something foreign into the event object (like a DIC), this is stuff that is just there anyway. So there is no need for an interface, because you either extend from Event or EventDispatcherAwareEvent. An interface or trait is just unnecessary cruft.

@kriswallsmith The problem with this is your listener would not know if a dispatcher is supposed to be there or not, so it would mean an internal test in the event object if $this->hasDispatcher(), as opposed to being a subclass of EventDispatcherAwareEvent which knows by virtue of itself.

So while this might seem like a case for an interface, it's actually not, because the we're talking about the stuff that's already available internally to the event, and not something injected from outside, like the DIC or something. It's not the same as being ContainerAware.

See the point?

Contributor

Crell commented Apr 10, 2013

"So there is no need for an interface, because you either extend from Event or EventDispatcherAwareEvent. An interface or trait is just unnecessary cruft."

Or you extend from something that extends one of those, but want the other one. That's why you need an interface.

Contributor

kriswallsmith commented Apr 10, 2013

@Drak I don't think I was clear in my suggestion. We can keep the Event class as it is for BC but trigger deprecation errors when either getName() or getDispatcher() is called. The new way to access the event name and dispatcher would be as the second and third arguments passed to the listener callable.

This line in doDispatch()

call_user_func($listener, $event);

…would change to…

call_user_func($listener, $event, $eventName, $this);

No need for more event classes or interfaces. No need for the dispatcher to modify the event object (once we phase out the BC layer). Much cleaner.

Member

stof commented Apr 14, 2013

@mpdude your dispatcher stack does not fix the issue with composition of dispatcher. The call to propagate will never be done on the outter dispatcher as it is called from dispatch of the inner dispatcher. And propagate has nothing to do in the EventDispatcherInterface IMO.

Member

stof commented Apr 15, 2013

@mpdude If you use composition, it means you will call the same method on the wrapped instance. This is how composition works. Now, you would be requiring the EventDispatcher to implement 2 methods instead of 1 to dispatch the event, with a difference being hard to understand (imagine how you would document the difference between them for people starting to use the component). And it is a BC break as it adds a new method in the interface, which cannot be done in 2.3 (the EventDispatcher component is already marked as being API-stable)

Contributor

mpdude commented Apr 15, 2013

(Updated: @stof's previous comments relate to my #7792 and have nothing to do with the OP's suggestion. I removed my comments already that are irrelevant here.)

Owner

fabpot commented Apr 22, 2013

Breaking BC for the event dispatcher in 2.3 is out of question.

Contributor

mpdude commented Apr 26, 2013

The problem with this approach is that Events decide (via an interface or extended baseclass) whether or not the Dispatcher shall be passed to listeners. But most of the time, it depends on the listener if the dispatcher is needed. The Event class cannot make a sensible decision IMO.

fabpot added a commit that referenced this pull request Sep 13, 2013

merged branch drak/edaware2 (PR #7852)
This PR was squashed before being merged into the master branch (closes #7852).

Discussion
----------

[2.3][EventDispatcher] Make events lighter

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | na
| License       | MIT
| Doc PR        | symfony/symfony-docs#2476

I've taken the previous discussions and taking into consideration @fabpot does not want to break BC. This PR now provides the `EventDispatcher` as an argument to listeners. I've made a second separate commit which also passes the event name.

This PR is alternative to #7582

Commits
-------

e2bff32 [2.3][EventDispatcher] Make events lighter
Owner

fabpot commented Sep 13, 2013

Closing in favor of #7852

@fabpot fabpot closed this Sep 13, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment