[DI][EventDispatcher] Add & wire closure-proxy argument type #20953

nicolas-grekas · 2016-12-16T11:13:37Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#12521, #12007, #20039
License	MIT
Doc PR	-

By resolving event subscribers at compile time, then wrapping listeners in a closure, we get laziness almost for free. This should solve/replaced all the above linked issues.

(WIP because tests are missing)

stof · 2016-12-16T11:16:29Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

+                $k = $type[1].' '.$k;
+            }
+            if ($type && $p->allowsNull()) {
+                $k = '?'.$k;


broken on PHP < 7.1

The ? is removed when not strictly required, which makes the code compatible with lower PHP versions.

stof · 2016-12-16T11:18:11Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

+        $signature = ($r->returnsReference() ? '&(' : '(').implode(', ', $signature).')';
+
+        if (method_exists($r, 'getReturnType') && $type = $r->getReturnType()) {
+            $signature .= ': '.($type->allowsNull() ? '?' : '').$this->generateTypeHint($type, $r);


broken on PHP 7.0 which has the return type but not nullable ones

same: ? removed when not required

the issue here is the call to $type->allowsNull()

the line above checks that there is a type. In PHP7, having a type XOR allowsNull, that's why it works there also

hmm, I forgot that PHP 7.0 has the method allowsNull even though return types cannot return null. But it is indeed logical as the same class is used for arguments too

stof · 2016-12-16T11:20:22Z

src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php

@@ -398,6 +399,12 @@ private function getArgumentsAsPhp(\DOMElement $node, $name, $lowercase = true)
                case 'expression':
                    $arguments[$key] = new Expression($arg->nodeValue);
                    break;
+                case 'closure-proxy':
+                    if (2 !== count($id = explode(':', $arg->getAttribute('id')))) {


would be better to use id and method IMO rather than exploding. It would be more consistent with other XML configuration settings

stof · 2016-12-16T11:21:35Z

src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php

@@ -108,7 +102,17 @@ public function process(ContainerBuilder $container)
                throw new InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $id, $interface));
            }

-            $definition->addMethodCall('addSubscriberService', array($id, $class));
+            foreach ($class::getSubscribedEvents() as $eventName => $params) {


this requires adding a ClassResource for the event subscriber class to invalidate the container when changing the class code. Otherwise, adding a new event in a subscriber would require clearing the cache manually in dev

added in a generic way in PhpDumper

stof · 2016-12-16T11:30:36Z

src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php

+                } else {
+                    foreach ($params as $listener) {
+                        $definition->addMethodCall('addListener', array($eventName, new ClosureProxyArgument(new Reference($id), $listener[0]), isset($listener[1]) ? $listener[1] : 0));
+                    }


is there a way to ensure that this logic stays in sync with the logic of the EventDispatcher to handle the return value of getSubscribedEvents ?

I don't think so...

I found a way! Don't tell me you don't like it, you asked for it ;)

stof · 2016-12-16T11:33:33Z

should this deprecate the ContainerAwareEventDispatcher ?

nicolas-grekas

I'll deprecate ContainerAwareEventDispatcher in #20937 so that it gets its own line in the changelog

nicolas-grekas · 2016-12-16T19:37:21Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

+                $k = $type[1].' '.$k;
+            }
+            if ($type && $p->allowsNull()) {
+                $k = '?'.$k;


The ? is removed when not strictly required, which makes the code compatible with lower PHP versions.

nicolas-grekas · 2016-12-16T19:37:39Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

+        $signature = ($r->returnsReference() ? '&(' : '(').implode(', ', $signature).')';
+
+        if (method_exists($r, 'getReturnType') && $type = $r->getReturnType()) {
+            $signature .= ': '.($type->allowsNull() ? '?' : '').$this->generateTypeHint($type, $r);


same: ? removed when not required

nicolas-grekas · 2016-12-16T19:37:48Z

src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php

@@ -398,6 +399,12 @@ private function getArgumentsAsPhp(\DOMElement $node, $name, $lowercase = true)
                case 'expression':
                    $arguments[$key] = new Expression($arg->nodeValue);
                    break;
+                case 'closure-proxy':
+                    if (2 !== count($id = explode(':', $arg->getAttribute('id')))) {


nicolas-grekas · 2016-12-16T19:38:05Z

src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php

@@ -108,7 +102,17 @@ public function process(ContainerBuilder $container)
                throw new InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $id, $interface));
            }

-            $definition->addMethodCall('addSubscriberService', array($id, $class));
+            foreach ($class::getSubscribedEvents() as $eventName => $params) {


added in a generic way in PhpDumper

nicolas-grekas · 2016-12-16T19:38:14Z

src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php

+                } else {
+                    foreach ($params as $listener) {
+                        $definition->addMethodCall('addListener', array($eventName, new ClosureProxyArgument(new Reference($id), $listener[0]), isset($listener[1]) ? $listener[1] : 0));
+                    }


I don't think so...

nicolas-grekas · 2016-12-17T08:24:13Z

fancy use case: <argument type="closure-proxy" id="service_container" method="get" />
ping @unkind

Koc · 2016-12-17T09:25:06Z

Referenced PR with Drupal event dispatcher also reduces addListener calls by injecting map with listeners in constructor

nicolas-grekas · 2016-12-17T09:55:17Z

@Koc, true, but I really doubt there is anything to save here. I think most of the perf gain came from turning subscribers into listeners at compile time instead, which this PR does. Would you like to confirm?

unkind · 2016-12-17T12:51:05Z

fancy use case: <argument type="closure-proxy" id="service_container" method="get" />

If I get your right, it allows to access private services. But they can be inlined (and most likely they would).

nicolas-grekas · 2016-12-18T08:47:37Z

it allows to access private service

of course closure proxies work with private services, but the example you quoted (<argument type="closure-proxy" id="service_container" method="get" /> doesn't, since it gets through the public interface of the container).

unkind · 2016-12-18T09:13:16Z

@nicolas-grekas well, I don't understand the purpose on this code then :)

nicolas-grekas · 2016-12-27T12:48:19Z

Tests added. PR embeds #20907 for now, waiting for it to be merged beforehand.
Status: needs review

nicolas-grekas · 2017-01-06T08:11:47Z

PR rebased & ready.
The yaml loader expects =closure_proxy to define the proxies.
Eg.

    foo:
        class: FooClass
        arguments:
            - =closure_proxy: ['@bar', getBuz]

xml is:

    <service id="foo" class="FooClass">
      <argument type="closure-proxy" id="bar" method="getBuz"/>
    </service>

Status: needs review

fabpot · 2017-01-07T14:01:42Z

src/Symfony/Component/EventDispatcher/Debug/WrappedListener.php

@@ -46,7 +46,13 @@ public function __construct($listener, $name, Stopwatch $stopwatch, EventDispatc
            $this->name = is_object($listener[0]) ? get_class($listener[0]) : $listener[0];
            $this->pretty = $this->name.'::'.$listener[1];
        } elseif ($listener instanceof \Closure) {
-            $this->pretty = $this->name = 'closure';
+            $r = new \ReflectionFunction($listener);
+            if (preg_match('#^/\*\* @closure-proxy ([^: ]++)::([^: ]++) \*/$#', $r->getDocComment(), $m)) {


hehe, I was wondering why you had this phpdoc comment in the container generation :) Note that this trick does only work with a dumped container. Not a big deal though.

fabpot · 2017-01-07T14:02:03Z

src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php

@@ -59,10 +61,6 @@ public function process(ContainerBuilder $container)

        foreach ($container->findTaggedServiceIds($this->listenerTag) as $id => $events) {
            $def = $container->getDefinition($id);
-            if (!$def->isPublic()) {
-                throw new InvalidArgumentException(sprintf('The service "%s" must be public as event listeners are lazy-loaded.', $id));
-            }


fabpot · 2017-01-07T14:07:40Z

src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php

@@ -108,7 +104,26 @@ public function process(ContainerBuilder $container)
                throw new InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $id, $interface));
            }

-            $definition->addMethodCall('addSubscriberService', array($id, $class));
+            $r = new \ReflectionClass($class);
+            $extractingDispatcher->addSubscriber($r->newInstanceWithoutConstructor());


Won't work if the getSubscribedEvents is somewhat configured via "static" properties or a global state. We probably don't care though.

fabpot · 2017-01-07T14:13:39Z

Thank you @nicolas-grekas.

…t type (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI][EventDispatcher] Add & wire closure-proxy argument type | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #12521, #12007, #20039 | License | MIT | Doc PR | - By resolving event subscribers at compile time, then wrapping listeners in a closure, we get laziness almost for free. This should solve/replaced all the above linked issues. (WIP because tests are missing) Commits ------- ecdf857 [DI][EventDispatcher] Add & wire closure-proxy argument type

This PR was merged into the 3.3 branch. Discussion ---------- [Di] Remove closure-proxy arguments | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - With #23008, we don't need this in core anymore. Let's drop it now. Technically that's a BC break, but for a feature that is very new, and still quite hidden. Doing this now would save us from maintaining this code, and help reduce the overall complexity. Basically reverts #20953 Commits ------- 57daadb [Di] Remove closure-proxy arguments

nicolas-grekas · 2017-06-02T09:13:23Z

Reverted in #23022

carsonbot added Status: Needs Review EventDispatcher Feature labels Dec 16, 2016

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Dec 16, 2016

stof reviewed Dec 16, 2016

View reviewed changes

nicolas-grekas mentioned this pull request Dec 16, 2016

[DoctrineBridge][EventDispatcher] Add LazyEventManager and LazyEventDispatcher #20039

Closed

nicolas-grekas commented Dec 16, 2016

View reviewed changes

nicolas-grekas force-pushed the di-method-dump branch from 47ab06d to 793cada Compare December 16, 2016 19:39

nicolas-grekas mentioned this pull request Dec 16, 2016

[EventDispatcher] Deprecate ContainerAwareEventDispatcher #20937

Merged

nicolas-grekas force-pushed the di-method-dump branch 12 times, most recently from 6016ff1 to 463e5f2 Compare December 17, 2016 08:16

nicolas-grekas force-pushed the di-method-dump branch from 463e5f2 to ddea5de Compare December 17, 2016 08:33

carsonbot added Status: Needs Review and removed Status: Needs Work labels Dec 27, 2016

nicolas-grekas force-pushed the di-method-dump branch 4 times, most recently from 2cb1ba9 to 5a0ec76 Compare January 2, 2017 16:16

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Jan 3, 2017

nicolas-grekas force-pushed the di-method-dump branch 2 times, most recently from 1f87716 to 8c104a0 Compare January 5, 2017 20:38

[DI][EventDispatcher] Add & wire closure-proxy argument type

ecdf857

nicolas-grekas force-pushed the di-method-dump branch from 8c104a0 to ecdf857 Compare January 6, 2017 08:05

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jan 6, 2017

fabpot approved these changes Jan 7, 2017

View reviewed changes

fabpot merged commit ecdf857 into symfony:master Jan 7, 2017

nicolas-grekas deleted the di-method-dump branch January 7, 2017 16:32

xabbuh mentioned this pull request Apr 5, 2017

Add a new Link component #22273

Closed

TomasVotruba mentioned this pull request Apr 28, 2017

[DefaultAutoconfigure] init + apply Symfony 3.3 deprecated-packages/symplify#155

Merged

7 tasks

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

dmaicher mentioned this pull request May 31, 2017

[EventDispatcher] After upgrading to 3.3.0 removeSubscriber() does not work any more. #22970

Closed

nicolas-grekas mentioned this pull request Jun 1, 2017

[Di] Remove closure-proxy arguments #23022

Merged

bestform mentioned this pull request May 5, 2020

[DI][EventDispatcher] Undocumented BC Break in RegisterListenersPass #36708

Closed

nicolas-grekas mentioned this pull request Dec 21, 2022

Problems decorating the "event_dispatcher" service #48130

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI][EventDispatcher] Add & wire closure-proxy argument type #20953

[DI][EventDispatcher] Add & wire closure-proxy argument type #20953

nicolas-grekas commented Dec 16, 2016

stof Dec 16, 2016

nicolas-grekas Dec 16, 2016

stof Dec 16, 2016

nicolas-grekas Dec 16, 2016

stof Dec 16, 2016

nicolas-grekas Dec 16, 2016

stof Dec 19, 2016

stof Dec 16, 2016

nicolas-grekas Dec 16, 2016

stof Dec 16, 2016

nicolas-grekas Dec 16, 2016 •

edited

Loading

stof Dec 16, 2016

nicolas-grekas Dec 16, 2016

nicolas-grekas Dec 16, 2016

stof commented Dec 16, 2016

nicolas-grekas left a comment

nicolas-grekas Dec 16, 2016

nicolas-grekas Dec 16, 2016

nicolas-grekas Dec 16, 2016

nicolas-grekas Dec 16, 2016 •

edited

Loading

nicolas-grekas Dec 16, 2016

nicolas-grekas commented Dec 17, 2016 •

edited

Loading

Koc commented Dec 17, 2016

nicolas-grekas commented Dec 17, 2016 •

edited

Loading

unkind commented Dec 17, 2016

nicolas-grekas commented Dec 18, 2016

unkind commented Dec 18, 2016

nicolas-grekas commented Dec 27, 2016

nicolas-grekas commented Jan 6, 2017

fabpot Jan 7, 2017

fabpot Jan 7, 2017

fabpot Jan 7, 2017

fabpot commented Jan 7, 2017

nicolas-grekas commented Jun 2, 2017

[DI][EventDispatcher] Add & wire closure-proxy argument type #20953

[DI][EventDispatcher] Add & wire closure-proxy argument type #20953

Conversation

nicolas-grekas commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Dec 16, 2016

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Dec 17, 2016 • edited Loading

Koc commented Dec 17, 2016

nicolas-grekas commented Dec 17, 2016 • edited Loading

unkind commented Dec 17, 2016

nicolas-grekas commented Dec 18, 2016

unkind commented Dec 18, 2016

nicolas-grekas commented Dec 27, 2016

nicolas-grekas commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jan 7, 2017

nicolas-grekas commented Jun 2, 2017

nicolas-grekas Dec 16, 2016 •

edited

Loading

nicolas-grekas Dec 16, 2016 •

edited

Loading

nicolas-grekas commented Dec 17, 2016 •

edited

Loading

nicolas-grekas commented Dec 17, 2016 •

edited

Loading