[FrameworkBundle] make RegisterKernelListenersPass reusable #6643

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@alexandresalome
Contributor

Bug fix: no
Feature addition: kinda
Backwards compatibility break: no

When I create a new event dispatcher for my applications, I would appreciate this code to be reusable.

@stof
Member
stof commented Jan 9, 2013

@alexandresalome quick question: why creating another dispatcher ?

@alexandresalome
Contributor

Sometimes you need to create a new dispatcher, out of Symfony kernel: Behat could be a good example.

Also in applications (like gitonomy) I tend to separate business events from kernel events. I do this for separation of concerns.

@bamarni bamarni commented on the diff Jan 21, 2013
...ncyInjection/Compiler/RegisterKernelListenersPass.php
@@ -14,17 +14,47 @@
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+/**
+ * Compiler pass to register tagged services for an event dispatcher.
+ */
class RegisterKernelListenersPass implements CompilerPassInterface
@bamarni
bamarni Jan 21, 2013 Contributor

then the class name isn't appropriate anymore

@alexandresalome
alexandresalome Jan 21, 2013 Contributor

See the last message in PR. It can be moved to two locations (HttpKernel or EventDispatcher).

@bamarni bamarni commented on the diff Jan 21, 2013
...ncyInjection/Compiler/RegisterKernelListenersPass.php
+ /**
+ * @var string
+ */
+ protected $listenerTag;
+
+ /**
+ * @var string
+ */
+ protected $subscriberTag;
+
+ /**
+ * @param string $dispatcherService Service name of the event dispatcher in processed container
+ * @param string $listenerTag Tag name used for listener
+ * @param string $listenerTag Tag name used for subscribers
+ */
+ public function __construct($dispatcherService = 'event_dispatcher', $listenerTag = 'kernel.event_listener', $subscriberTag = 'kernel.event_subscriber')
@bamarni
bamarni Jan 21, 2013 Contributor

I'd put $dispatcherService as third argument as it'd less likely be changed than the others.

@stof
stof Jan 21, 2013 Member

If you don't change the dispatcher service, registering a second compiler pass is totally useless. So it is likely to change

@bamarni
bamarni Jan 21, 2013 Contributor

Why useless? Then you'd have to tag services listening to your own events with kernel.event_listener, this doesn't make so much sense to me.

@stof
stof Jan 21, 2013 Member

why wouldn't it make sense ? You are using the event dispatcher used by the kernel, so why duplicating the way to register listeners in it ?

@bamarni
bamarni Jan 21, 2013 Contributor

Because the tag is prefixed with kernel, I have no guarantee tomorrow this compiler pass won't add any special behavior to my listeners or whatever because it assumes they are kernel events listeners.

If you tell me that it's not the case, it is meant to be generic and I can safely rely on it, I'll do so, but then I'm wondering why it has been prefixed like this? The "kernel" dispatcher service is called event_dispatcher, not kernel.event_dispatcher.

@stof
stof Jan 21, 2013 Member

I don't see any reason to add special behaviors on the registration of listeners (except improving the error reporting when someone tries to register a private service, which is currently silently skipped)

@bamarni
Contributor
bamarni commented Jan 21, 2013

👍 I usually duplicate this file for my application events.

@alexandresalome
Contributor

Regarding the question "should we create a new event dispatcher in our applications", the answer is clearly yes or no.

That being said, whatever this usage is correct or not, we still need this class for other cases.

An example: you want to create an application with a service container. Extensions in this application register services in your container. You want to allow registration on a dispatcher with tags on extension's services.

See the case?

Regarding the class, I think it should be moved to Symfony/Component/HttpKernel/DependencyInjection or Symfony/Component/EventDispatcher/DependencyInjection.

Waiting for advices,
Alexandre

@fabpot
Member
fabpot commented Mar 25, 2013

Looks good to me. I think putting it in HttpKernel is slightly better.

@fabpot
Member
fabpot commented Apr 25, 2013

I've finished this PR at #7848

@fabpot fabpot closed this Apr 25, 2013
@fabpot fabpot added a commit to fabpot/symfony that referenced this pull request Apr 25, 2013
@fabpot fabpot moved the kernel listener compiler pass to HttpKernel to make it reus…
…able (refs #6643)
5047227
@fabpot fabpot added a commit that referenced this pull request Apr 25, 2013
@fabpot fabpot merged branch fabpot/dispatcher-compiler-pass (PR #7848)
This PR was merged into the master branch.

Discussion
----------

[FrameworkBundle] make RegisterKernelListenersPass reusable

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #6643
| License       | MIT
| Doc PR        | n/a

see #6643

Commits
-------

5047227 moved the kernel listener compiler pass to HttpKernel to make it reusable (refs #6643)
45f1a16 make RegisterKernelListenersPass reusable
5d12b2b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment