Added missing kernel.event_subscriber tag (closes #2012) #2021
Conversation
|
I'm not sure the test is good enough so feel free to add some more. |
| */ | ||
| public function addSubscriberService($serviceId, $class) | ||
| { | ||
| // We don't check again if $class implements EventSubscriberInterface as it was checked at compile-time |
There was a problem hiding this comment.
I realize the compiler pass checks this (for the kernel.event_subscriber case), but I think it'd still be a good idea to check $class and throw an InvalidArgumentException if it does not implement the interface? This is a public method, so it is possible that someone could invoke it on their own.
Also, there's no reason this method cannot accept the class or object instance as its second parameter. The getSubscribedEvents() invocation would follow the same syntax already used, so it'd just be a matter of changing the doc comment and perhaps rename the $class parameter (PHP functions tend to use mixed $object in places where an object or class are accepted.
There was a problem hiding this comment.
@jmikola For your first comment I agree but @schmittjoh thought it would add unnecessary overhead. I'm waiting for more inputs before changing it again (maybe from @stof or @fabpot?).
For your second comment I don't really see how an object could be passed as we are here dealing with a service not yet loaded. If we want to add an actual object as a subscriber, we can already use addSubscriber(). Or maybe I'm missing something?
There was a problem hiding this comment.
Ah, you're absolutely right. I didn't catch that you were working with $listenerIds and got confused. I've been jumping between too many EventDispatcher classes recently :)
There was a problem hiding this comment.
In the case, would it be easier if the compiler pass simply dealt with the EventSubscriberInterface class itself and make calls to addListenerService()?
Either way, you have to add functionality (and tests) to RegisterKernelListenersPass, but it wouldn't require any new functionality in ContainerAwareEventDispatcher.
There was a problem hiding this comment.
The problem if we implement this inside the compiler pass is that we would have to add the class file as a DIC resource and the DIC would have to be recompiled for each modification inside the class. With my implementation, we just check for events to register at run-time.
|
I re-implemented the check for EventSubscriberInterface in ContainerAwareEventDispatcher because I think the overhead is minimum and it allows to use this method even without the tag (at run-time). |
|
@jalliot Your branch conflicts with the current master. could you rebase it ? |
|
Rebased |
|
@fabpot What do you think about this PR? At the moment, the subscribers are not really usable in Symfony2 because of the lack of this tag. |
|
I don't like subscribers. There are other PRs on adding more support for them, but the reality is that they are complex for no added benefit. I'm wondering if it wouldn't be better to just remove them altogether. |
|
@fabpot Well I prefer listeners too but I think that if Symfony2 does support subscribers (which it does at the moment), it should do it properly and completely, thus allowing to register subscriber services like here or to register several methods for one same event like in #2148. |
Commits ------- 5146a1f [EventDispatcher] Added possibility for subscribers to subscribe several times for same event Discussion ---------- [EventDispatcher] Added possibility for subscribers to subscribe several times for same event [EventDispatcher] Added possibility for subscribers to subscribe several times for same event closes #2146 And it is of course fully BC :) --------------------------------------------------------------------------- by jalliot at 2011/09/09 17:34:07 -0700 If merged, #2021 will have to reflect the change too
| if (!$refClass->implementsInterface($interface)) { | ||
| throw new \InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $serviceId, $interface)); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think the code that checks the interface here is unneeded as most of the time, the check will already have been done by the compiler pass.
There was a problem hiding this comment.
Johannes thought that too but I wasn't sure it was worth not checking... I'll remove the check by tomorrow then.
|
Sure thing. Will do it as well as removing the check for the interface tonight or tomorrow :) |
|
Tests do not pass. |
|
@fabpot Fixed |
Commits ------- 3223c5a Removed now useless test 21cf0ac Backported new behaviour from PR #2148 and removed check for interface at run-time 8b240d4 Implementation of kernel.event_subscriber tag for services. Discussion ---------- Added missing kernel.event_subscriber tag (closes #2012) This PR adds a ``kernel.event_subscriber`` tag which allows to register services as event subscribers in the same way ``kernel.event_listener`` allows to register them as event listeners. The service is still lazy loaded and the DIC does not need to be recompiled for every modification in the service's code. There is one important thing to remember: If the service is created by a factory, the class parameter **MUST** reflect the real class of the service, although it is not needed at the moment for the DIC. For that issue, we could either forbid services created by factories or add a note to the documentation. This PR closes #2012. --------------------------------------------------------------------------- by jalliot at 2011/08/24 06:42:18 -0700 I'm not sure the test is good enough so feel free to add some more. --------------------------------------------------------------------------- by jalliot at 2011/08/25 03:46:20 -0700 I re-implemented the check for EventSubscriberInterface in ContainerAwareEventDispatcher because I think the overhead is minimum and it allows to use this method even without the tag (at run-time). I also added some tests for RegisterKernelListenersPass. --------------------------------------------------------------------------- by stof at 2011/09/04 02:42:00 -0700 @jalliot Your branch conflicts with the current master. could you rebase it ? --------------------------------------------------------------------------- by jalliot at 2011/09/04 02:57:03 -0700 Rebased --------------------------------------------------------------------------- by jalliot at 2011/09/13 02:19:46 -0700 @fabpot What do you think about this PR? At the moment, the subscribers are not really usable in Symfony2 because of the lack of this tag. --------------------------------------------------------------------------- by fabpot at 2011/09/13 04:17:46 -0700 I don't like subscribers. There are other PRs on adding more support for them, but the reality is that they are complex for no added benefit. I'm wondering if it wouldn't be better to just remove them altogether. --------------------------------------------------------------------------- by jalliot at 2011/09/13 04:38:20 -0700 @fabpot Well I prefer listeners too but I think that if Symfony2 does support subscribers (which it does at the moment), it should do it properly and completely, thus allowing to register subscriber services like here or to register several methods for one same event like in #2148. But I guess that if you merged those 2 PRs (well actually this one would have to be modified first if #2148 is merged but I'll do it then), many use cases would be covered and people should stop asking for more support :) (except maybe for removing the static modifier but this would be wrong IMO and prevent entirely this PR). --------------------------------------------------------------------------- by fabpot at 2011/09/28 11:47:10 -0700 @jalliot: #2148 has been merged. Can you update this PR accordingly? thanks. --------------------------------------------------------------------------- by jalliot at 2011/09/28 12:00:44 -0700 Sure thing. Will do it as well as removing the check for the interface tonight or tomorrow :) --------------------------------------------------------------------------- by jalliot at 2011/09/29 08:53:17 -0700 @fabpot Check for interface removed and #2148 merged. Also rebased on latest master. --------------------------------------------------------------------------- by fabpot at 2011/09/29 09:09:11 -0700 Tests do not pass. --------------------------------------------------------------------------- by jalliot at 2011/09/29 09:18:48 -0700 @fabpot Fixed
This PR adds a
kernel.event_subscribertag which allows to register services as event subscribers in the same waykernel.event_listenerallows to register them as event listeners.The service is still lazy loaded and the DIC does not need to be recompiled for every modification in the service's code.
There is one important thing to remember:
If the service is created by a factory, the class parameter MUST reflect the real class of the service, although it is not needed at the moment for the DIC. For that issue, we could either forbid services created by factories or add a note to the documentation.
This PR closes #2012.