-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DI] Allow for invokable event listeners #25275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
Sorry, something went wrong.
All reactions
I will use this one ! |
All reactions
Sorry, something went wrong.
@@ -68,6 +68,10 @@ public function process(ContainerBuilder $container) | |||
'/[^a-z0-9]/i', | |||
), function ($matches) { return strtoupper($matches[0]); }, $event['event']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know there is such magic default logic.
Sorry, something went wrong.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, yet convenient enough to keep IMHO, or at least didnt want to spoil this feat. along with a deprecation.
Sorry, something went wrong.
All reactions
public function testInvokableEventListener() | ||
{ | ||
$container = new ContainerBuilder(); | ||
$container->register('foo', 'stdClass')->addTag('kernel.event_listener', array('event' => 'foo.bar')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\stdClass::class
Sorry, something went wrong.
All reactions
@@ -68,6 +68,10 @@ public function process(ContainerBuilder $container) | |||
'/[^a-z0-9]/i', | |||
), function ($matches) { return strtoupper($matches[0]); }, $event['event']); | |||
$event['method'] = preg_replace('/[^a-z0-9]/i', '', $event['method']); | |||
|
|||
if (null !== ($class = $container->getDefinition($id)->getClass()) && !method_exists($class, $event['method']) && method_exists($class, '__invoke')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use $container->getReflectionClass() instead
this will fix the current logic that misses resource tracking
Sorry, something went wrong.
All reactions
I'm "-0": I don't see the benefit of this. Naming methods helps the reader, and is not a strong requirement. |
All reactions
Sorry, something went wrong.
naming classes too :) this allows me, the class author, to write IMHO pure event listeners and let users configure as usual. That is win-win. |
All reactions
Sorry, something went wrong.
Thank you @ro0NL. |
All reactions
Sorry, something went wrong.
This PR was merged into the 4.1-dev branch. Discussion ---------- [DI] Allow for invokable event listeners | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #... <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | symfony/symfony-docs#... <!--highly recommended for new features--> Inspired by #24637 / #25259. This adds invokable support for event listeners :) ```yaml Some\Foo: tags: [{ name: kernel.event_listener, event: kernel.request }] ``` ```php class Foo { public function __invoke(GetResponseEvent $event) { } } ``` Commits ------- fa5b7eb [DI] Allow for invokable event listeners
nicolas-grekas
Tobion
dunglas
Taluu
xabbuh
Simperfit
20uf
chalasr
Successfully merging this pull request may close these issues.
Inspired by #24637 / #25259. This adds invokable support for event listeners :)