bug #39799 [DoctrineBridge] Fix circular loop with EntityManager (jderusse)

This PR was merged into the 4.4 branch. Discussion ---------- [DoctrineBridge] Fix circular loop with EntityManager | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | fix #39619 | License | MIT | Doc PR | - This PR fix a segfault in EntityManager by making the LazyEventManager handle EventSubscriber in a lazy way. Maybe #34200 too Commits ------- 23d692107c Fix circular loop with EntityManager
2021-01-14 20:19:38 +01:00 · 2021-01-14 20:19:38 +01:00 · 2c9a837ac7
commit 2c9a837ac7
parent 1d7c3f6d6e 23d692107c
4 changed files with 175 additions and 31 deletions
--- a/src/Symfony/Bridge/Doctrine/ContainerAwareEventManager.php
+++ b/src/Symfony/Bridge/Doctrine/ContainerAwareEventManager.php
@ -16,7 +16,7 @@ use Doctrine\Common\EventManager;
 use Psr\Container\ContainerInterface;

 /**
- * Allows lazy loading of listener services.
+ * Allows lazy loading of listener and subscriber services.
 *
 * @author Johannes M. Schmitt <schmittjoh@gmail.com>
 */
@ -28,13 +28,16 @@ class ContainerAwareEventManager extends EventManager
     * <event> => <listeners>
     */
    private $listeners = [];
+    private $subscribers;
    private $initialized = [];
+    private $initializedSubscribers = false;
    private $methods = [];
    private $container;

-    public function __construct(ContainerInterface $container)
+    public function __construct(ContainerInterface $container, array $subscriberIds = [])
    {
        $this->container = $container;
+        $this->subscribers = $subscriberIds;
    }

    /**
@ -44,6 +47,9 @@ class ContainerAwareEventManager extends EventManager
     */
    public function dispatchEvent($eventName, EventArgs $eventArgs = null)
    {
+        if (!$this->initializedSubscribers) {
+            $this->initializeSubscribers();
+        }
        if (!isset($this->listeners[$eventName])) {
            return;
        }
@ -66,6 +72,9 @@ class ContainerAwareEventManager extends EventManager
     */
    public function getListeners($event = null)
    {
+        if (!$this->initializedSubscribers) {
+            $this->initializeSubscribers();
+        }
        if (null !== $event) {
            if (!isset($this->initialized[$event])) {
                $this->initializeListeners($event);
@ -90,6 +99,10 @@ class ContainerAwareEventManager extends EventManager
     */
    public function hasListeners($event)
    {
+        if (!$this->initializedSubscribers) {
+            $this->initializeSubscribers();
+        }
+
        return isset($this->listeners[$event]) && $this->listeners[$event];
    }

@ -138,6 +151,7 @@ class ContainerAwareEventManager extends EventManager

    private function initializeListeners(string $eventName)
    {
+        $this->initialized[$eventName] = true;
        foreach ($this->listeners[$eventName] as $hash => $listener) {
            if (\is_string($listener)) {
                $this->listeners[$eventName][$hash] = $listener = $this->container->get($listener);
@ -145,7 +159,16 @@ class ContainerAwareEventManager extends EventManager
                $this->methods[$eventName][$hash] = $this->getMethod($listener, $eventName);
            }
        }
-        $this->initialized[$eventName] = true;
+    }
+
+    private function initializeSubscribers()
+    {
+        $this->initializedSubscribers = true;
+        foreach ($this->subscribers as $id => $subscriber) {
+            if (\is_string($subscriber)) {
+                parent::addEventSubscriber($this->subscribers[$id] = $this->container->get($subscriber));
+            }
+        }
    }

    /**
--- a/src/Symfony/Bridge/Doctrine/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPass.php
+++ b/src/Symfony/Bridge/Doctrine/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPass.php
@ -11,6 +11,8 @@

 namespace Symfony\Bridge\Doctrine\DependencyInjection\CompilerPass;

+use Symfony\Bridge\Doctrine\ContainerAwareEventManager;
+use Symfony\Component\DependencyInjection\ChildDefinition;
 use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
 use Symfony\Component\DependencyInjection\Compiler\ServiceLocatorTagPass;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
@ -55,15 +57,24 @@ class RegisterEventListenersAndSubscribersPass implements CompilerPassInterface
        }

        $this->connections = $container->getParameter($this->connections);
-        $this->addTaggedSubscribers($container);
-        $this->addTaggedListeners($container);
+        $listenerRefs = [];
+        $this->addTaggedSubscribers($container, $listenerRefs);
+        $this->addTaggedListeners($container, $listenerRefs);
+
+        // replace service container argument of event managers with smaller service locator
+        // so services can even remain private
+        foreach ($listenerRefs as $connection => $refs) {
+            $this->getEventManagerDef($container, $connection)
+                ->replaceArgument(0, ServiceLocatorTagPass::register($container, $refs));
+        }
    }

-    private function addTaggedSubscribers(ContainerBuilder $container)
+    private function addTaggedSubscribers(ContainerBuilder $container, array &$listenerRefs)
    {
        $subscriberTag = $this->tagPrefix.'.event_subscriber';
        $taggedSubscribers = $this->findAndSortTags($subscriberTag, $container);

+        $managerDefs = [];
        foreach ($taggedSubscribers as $taggedSubscriber) {
            [$id, $tag] = $taggedSubscriber;
            $connections = isset($tag['connection']) ? [$tag['connection']] : array_keys($this->connections);
@ -72,16 +83,33 @@ class RegisterEventListenersAndSubscribersPass implements CompilerPassInterface
                    throw new RuntimeException(sprintf('The Doctrine connection "%s" referenced in service "%s" does not exist. Available connections names: "%s".', $con, $id, implode('", "', array_keys($this->connections))));
                }

-                $this->getEventManagerDef($container, $con)->addMethodCall('addEventSubscriber', [new Reference($id)]);
+                if (!isset($managerDefs[$con])) {
+                    $managerDef = $parentDef = $this->getEventManagerDef($container, $con);
+                    while ($parentDef instanceof ChildDefinition) {
+                        $parentDef = $container->findDefinition($parentDef->getParent());
+                    }
+                    $managerClass = $container->getParameterBag()->resolveValue($parentDef->getClass());
+                    $managerDefs[$con] = [$managerDef, $managerClass];
+                } else {
+                    [$managerDef, $managerClass] = $managerDefs[$con];
+                }
+
+                if (ContainerAwareEventManager::class === $managerClass) {
+                    $listenerRefs[$con][$id] = new Reference($id);
+                    $refs = $managerDef->getArguments()[1] ?? [];
+                    $refs[] = $id;
+                    $managerDef->setArgument(1, $refs);
+                } else {
+                    $managerDef->addMethodCall('addEventSubscriber', [new Reference($id)]);
+                }
            }
        }
    }

-    private function addTaggedListeners(ContainerBuilder $container)
+    private function addTaggedListeners(ContainerBuilder $container, array &$listenerRefs)
    {
        $listenerTag = $this->tagPrefix.'.event_listener';
        $taggedListeners = $this->findAndSortTags($listenerTag, $container);
-        $listenerRefs = [];

        foreach ($taggedListeners as $taggedListener) {
            [$id, $tag] = $taggedListener;
@ -100,13 +128,6 @@ class RegisterEventListenersAndSubscribersPass implements CompilerPassInterface
                $this->getEventManagerDef($container, $con)->addMethodCall('addEventListener', [[$tag['event']], $id]);
            }
        }
-
-        // replace service container argument of event managers with smaller service locator
-        // so services can even remain private
-        foreach ($listenerRefs as $connection => $refs) {
-            $this->getEventManagerDef($container, $connection)
-                ->replaceArgument(0, ServiceLocatorTagPass::register($container, $refs));
-        }
    }

    private function getEventManagerDef(ContainerBuilder $container, string $name)
--- a/src/Symfony/Bridge/Doctrine/Tests/ContainerAwareEventManagerTest.php
+++ b/src/Symfony/Bridge/Doctrine/Tests/ContainerAwareEventManagerTest.php
@ -11,6 +11,7 @@

 namespace Symfony\Bridge\Doctrine\Tests;

+use Doctrine\Common\EventSubscriber;
 use PHPUnit\Framework\TestCase;
 use Symfony\Bridge\Doctrine\ContainerAwareEventManager;
 use Symfony\Component\DependencyInjection\Container;
@ -28,6 +29,8 @@ class ContainerAwareEventManagerTest extends TestCase

    public function testDispatchEvent()
    {
+        $this->evm = new ContainerAwareEventManager($this->container, ['lazy4']);
+
        $this->container->set('lazy1', $listener1 = new MyListener());
        $this->evm->addEventListener('foo', 'lazy1');
        $this->evm->addEventListener('foo', $listener2 = new MyListener());
@ -37,10 +40,18 @@ class ContainerAwareEventManagerTest extends TestCase
        $this->container->set('lazy3', $listener5 = new MyListener());
        $this->evm->addEventListener('foo', $listener5 = new MyListener());
        $this->evm->addEventListener('bar', $listener5);
+        $this->container->set('lazy4', $subscriber1 = new MySubscriber(['foo']));
+        $this->evm->addEventSubscriber($subscriber2 = new MySubscriber(['bar']));
+
+        $this->assertSame(0, $subscriber1->calledSubscribedEventsCount);
+        $this->assertSame(1, $subscriber2->calledSubscribedEventsCount);

        $this->evm->dispatchEvent('foo');
        $this->evm->dispatchEvent('bar');

+        $this->assertSame(1, $subscriber1->calledSubscribedEventsCount);
+        $this->assertSame(1, $subscriber2->calledSubscribedEventsCount);
+
        $this->assertSame(0, $listener1->calledByInvokeCount);
        $this->assertSame(1, $listener1->calledByEventNameCount);
        $this->assertSame(0, $listener2->calledByInvokeCount);
@ -51,10 +62,16 @@ class ContainerAwareEventManagerTest extends TestCase
        $this->assertSame(0, $listener4->calledByEventNameCount);
        $this->assertSame(1, $listener5->calledByInvokeCount);
        $this->assertSame(1, $listener5->calledByEventNameCount);
+        $this->assertSame(0, $subscriber1->calledByInvokeCount);
+        $this->assertSame(1, $subscriber1->calledByEventNameCount);
+        $this->assertSame(1, $subscriber2->calledByInvokeCount);
+        $this->assertSame(0, $subscriber2->calledByEventNameCount);
    }

-    public function testAddEventListenerAfterDispatchEvent()
+    public function testAddEventListenerAndSubscriberAfterDispatchEvent()
    {
+        $this->evm = new ContainerAwareEventManager($this->container, ['lazy7']);
+
        $this->container->set('lazy1', $listener1 = new MyListener());
        $this->evm->addEventListener('foo', 'lazy1');
        $this->evm->addEventListener('foo', $listener2 = new MyListener());
@ -64,10 +81,18 @@ class ContainerAwareEventManagerTest extends TestCase
        $this->container->set('lazy3', $listener5 = new MyListener());
        $this->evm->addEventListener('foo', $listener5 = new MyListener());
        $this->evm->addEventListener('bar', $listener5);
+        $this->container->set('lazy7', $subscriber1 = new MySubscriber(['foo']));
+        $this->evm->addEventSubscriber($subscriber2 = new MySubscriber(['bar']));
+
+        $this->assertSame(0, $subscriber1->calledSubscribedEventsCount);
+        $this->assertSame(1, $subscriber2->calledSubscribedEventsCount);

        $this->evm->dispatchEvent('foo');
        $this->evm->dispatchEvent('bar');

+        $this->assertSame(1, $subscriber1->calledSubscribedEventsCount);
+        $this->assertSame(1, $subscriber2->calledSubscribedEventsCount);
+
        $this->container->set('lazy4', $listener6 = new MyListener());
        $this->evm->addEventListener('foo', 'lazy4');
        $this->evm->addEventListener('foo', $listener7 = new MyListener());
@ -77,10 +102,19 @@ class ContainerAwareEventManagerTest extends TestCase
        $this->container->set('lazy6', $listener10 = new MyListener());
        $this->evm->addEventListener('foo', $listener10 = new MyListener());
        $this->evm->addEventListener('bar', $listener10);
+        $this->evm->addEventSubscriber($subscriber3 = new MySubscriber(['bar']));
+
+        $this->assertSame(1, $subscriber1->calledSubscribedEventsCount);
+        $this->assertSame(1, $subscriber2->calledSubscribedEventsCount);
+        $this->assertSame(1, $subscriber3->calledSubscribedEventsCount);

        $this->evm->dispatchEvent('foo');
        $this->evm->dispatchEvent('bar');

+        $this->assertSame(1, $subscriber1->calledSubscribedEventsCount);
+        $this->assertSame(1, $subscriber2->calledSubscribedEventsCount);
+        $this->assertSame(1, $subscriber3->calledSubscribedEventsCount);
+
        $this->assertSame(0, $listener1->calledByInvokeCount);
        $this->assertSame(2, $listener1->calledByEventNameCount);
        $this->assertSame(0, $listener2->calledByInvokeCount);
@ -91,6 +125,10 @@ class ContainerAwareEventManagerTest extends TestCase
        $this->assertSame(0, $listener4->calledByEventNameCount);
        $this->assertSame(2, $listener5->calledByInvokeCount);
        $this->assertSame(2, $listener5->calledByEventNameCount);
+        $this->assertSame(0, $subscriber1->calledByInvokeCount);
+        $this->assertSame(2, $subscriber1->calledByEventNameCount);
+        $this->assertSame(2, $subscriber2->calledByInvokeCount);
+        $this->assertSame(0, $subscriber2->calledByEventNameCount);

        $this->assertSame(0, $listener6->calledByInvokeCount);
        $this->assertSame(1, $listener6->calledByEventNameCount);
@ -102,6 +140,8 @@ class ContainerAwareEventManagerTest extends TestCase
        $this->assertSame(0, $listener9->calledByEventNameCount);
        $this->assertSame(1, $listener10->calledByInvokeCount);
        $this->assertSame(1, $listener10->calledByEventNameCount);
+        $this->assertSame(1, $subscriber3->calledByInvokeCount);
+        $this->assertSame(0, $subscriber3->calledByEventNameCount);
    }

    public function testGetListenersForEvent()
@ -166,3 +206,21 @@ class MyListener
        ++$this->calledByEventNameCount;
    }
 }
+
+class MySubscriber extends MyListener implements EventSubscriber
+{
+    public $calledSubscribedEventsCount = 0;
+    private $listenedEvents;
+
+    public function __construct(array $listenedEvents)
+    {
+        $this->listenedEvents = $listenedEvents;
+    }
+
+    public function getSubscribedEvents(): array
+    {
+        ++$this->calledSubscribedEventsCount;
+
+        return $this->listenedEvents;
+    }
+}
--- a/src/Symfony/Bridge/Doctrine/Tests/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPassTest.php
+++ b/src/Symfony/Bridge/Doctrine/Tests/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPassTest.php
@ -12,6 +12,7 @@
 namespace Symfony\Bridge\Doctrine\Tests\DependencyInjection\CompilerPass;

 use PHPUnit\Framework\TestCase;
+use Symfony\Bridge\Doctrine\ContainerAwareEventManager;
 use Symfony\Bridge\Doctrine\DependencyInjection\CompilerPass\RegisterEventListenersAndSubscribersPass;
 use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
@ -209,20 +210,46 @@ class RegisterEventListenersAndSubscribersPassTest extends TestCase

        $this->process($container);

+        $eventManagerDef = $container->getDefinition('doctrine.dbal.default_connection.event_manager');
+
+        // first connection
        $this->assertEquals(
            [
-                ['addEventSubscriber', [new Reference('a')]],
-                ['addEventSubscriber', [new Reference('b')]],
+                'a',
+                'b',
            ],
-            $container->getDefinition('doctrine.dbal.default_connection.event_manager')->getMethodCalls()
+            $eventManagerDef->getArgument(1)
        );

+        $serviceLocatorDef = $container->getDefinition((string) $eventManagerDef->getArgument(0));
+        $this->assertSame(ServiceLocator::class, $serviceLocatorDef->getClass());
        $this->assertEquals(
            [
-                ['addEventSubscriber', [new Reference('a')]],
-                ['addEventSubscriber', [new Reference('c')]],
+                'a' => new ServiceClosureArgument(new Reference('a')),
+                'b' => new ServiceClosureArgument(new Reference('b')),
            ],
-            $container->getDefinition('doctrine.dbal.second_connection.event_manager')->getMethodCalls()
+            $serviceLocatorDef->getArgument(0)
+        );
+
+        $eventManagerDef = $container->getDefinition('doctrine.dbal.second_connection.event_manager');
+
+        // second connection
+        $this->assertEquals(
+            [
+                'a',
+                'c',
+            ],
+            $eventManagerDef->getArgument(1)
+        );
+
+        $serviceLocatorDef = $container->getDefinition((string) $eventManagerDef->getArgument(0));
+        $this->assertSame(ServiceLocator::class, $serviceLocatorDef->getClass());
+        $this->assertEquals(
+            [
+                'a' => new ServiceClosureArgument(new Reference('a')),
+                'c' => new ServiceClosureArgument(new Reference('c')),
+            ],
+            $serviceLocatorDef->getArgument(0)
        );
    }

@ -261,15 +288,30 @@ class RegisterEventListenersAndSubscribersPassTest extends TestCase

        $this->process($container);

+        $eventManagerDef = $container->getDefinition('doctrine.dbal.default_connection.event_manager');
+
        $this->assertEquals(
            [
-                ['addEventSubscriber', [new Reference('c')]],
-                ['addEventSubscriber', [new Reference('d')]],
-                ['addEventSubscriber', [new Reference('e')]],
-                ['addEventSubscriber', [new Reference('b')]],
-                ['addEventSubscriber', [new Reference('a')]],
+                'c',
+                'd',
+                'e',
+                'b',
+                'a',
            ],
-            $container->getDefinition('doctrine.dbal.default_connection.event_manager')->getMethodCalls()
+            $eventManagerDef->getArgument(1)
+        );
+
+        $serviceLocatorDef = $container->getDefinition((string) $eventManagerDef->getArgument(0));
+        $this->assertSame(ServiceLocator::class, $serviceLocatorDef->getClass());
+        $this->assertEquals(
+            [
+                'a' => new ServiceClosureArgument(new Reference('a')),
+                'b' => new ServiceClosureArgument(new Reference('b')),
+                'c' => new ServiceClosureArgument(new Reference('c')),
+                'd' => new ServiceClosureArgument(new Reference('d')),
+                'e' => new ServiceClosureArgument(new Reference('e')),
+            ],
+            $serviceLocatorDef->getArgument(0)
        );
    }

@ -296,12 +338,12 @@ class RegisterEventListenersAndSubscribersPassTest extends TestCase

        $connections = ['default' => 'doctrine.dbal.default_connection'];

-        $container->register('doctrine.dbal.default_connection.event_manager', 'stdClass')
+        $container->register('doctrine.dbal.default_connection.event_manager', ContainerAwareEventManager::class)
            ->addArgument(new Reference('service_container'));
        $container->register('doctrine.dbal.default_connection', 'stdClass');

        if ($multipleConnections) {
-            $container->register('doctrine.dbal.second_connection.event_manager', 'stdClass')
+            $container->register('doctrine.dbal.second_connection.event_manager', ContainerAwareEventManager::class)
                ->addArgument(new Reference('service_container'));
            $container->register('doctrine.dbal.second_connection', 'stdClass');
            $connections['second'] = 'doctrine.dbal.second_connection';