From 9d9d4efb88539b32413d6ac51e9639a0295502cc Mon Sep 17 00:00:00 2001 From: David Maicher Date: Tue, 14 Mar 2017 21:52:39 +0100 Subject: [PATCH] [Doctrine Bridge] fix priority for doctrine event listeners --- ...gisterEventListenersAndSubscribersPass.php | 168 ++++++++++-------- ...erEventListenersAndSubscribersPassTest.php | 156 ++++++++++++---- 2 files changed, 210 insertions(+), 114 deletions(-) diff --git a/src/Symfony/Bridge/Doctrine/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPass.php b/src/Symfony/Bridge/Doctrine/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPass.php index 94f72fd8c8c0..1ec3ba323b78 100644 --- a/src/Symfony/Bridge/Doctrine/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPass.php +++ b/src/Symfony/Bridge/Doctrine/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPass.php @@ -11,27 +11,30 @@ namespace Symfony\Bridge\Doctrine\DependencyInjection\CompilerPass; +use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface; use Symfony\Component\DependencyInjection\ContainerBuilder; +use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException; +use Symfony\Component\DependencyInjection\Exception\RuntimeException; use Symfony\Component\DependencyInjection\Reference; -use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface; /** * Registers event listeners and subscribers to the available doctrine connections. * * @author Jeremy Mikola * @author Alexander + * @author David Maicher */ class RegisterEventListenersAndSubscribersPass implements CompilerPassInterface { + /** + * @var string|string[] + */ private $connections; - private $container; private $eventManagers; private $managerTemplate; private $tagPrefix; /** - * Constructor. - * * @param string $connections Parameter ID for connections * @param string $managerTemplate sprintf() template for generating the event * manager's service ID for a connection name @@ -53,105 +56,112 @@ public function process(ContainerBuilder $container) return; } - $taggedSubscribers = $container->findTaggedServiceIds($this->tagPrefix.'.event_subscriber'); - $taggedListeners = $container->findTaggedServiceIds($this->tagPrefix.'.event_listener'); - - if (empty($taggedSubscribers) && empty($taggedListeners)) { - return; - } - - $this->container = $container; $this->connections = $container->getParameter($this->connections); - $sortFunc = function ($a, $b) { - $a = isset($a['priority']) ? $a['priority'] : 0; - $b = isset($b['priority']) ? $b['priority'] : 0; - - return $a > $b ? -1 : 1; - }; + $this->addTaggedSubscribers($container); + $this->addTaggedListeners($container); + } - if (!empty($taggedSubscribers)) { - $subscribersPerCon = $this->groupByConnection($taggedSubscribers); - foreach ($subscribersPerCon as $con => $subscribers) { - $em = $this->getEventManager($con); + private function addTaggedSubscribers(ContainerBuilder $container) + { + $subscriberTag = $this->tagPrefix.'.event_subscriber'; + $taggedSubscribers = $this->findAndSortTags($subscriberTag, $container); - uasort($subscribers, $sortFunc); - foreach ($subscribers as $id => $instance) { - if ($container->getDefinition($id)->isAbstract()) { - throw new \InvalidArgumentException(sprintf('The abstract service "%s" cannot be tagged as a doctrine event subscriber.', $id)); - } + foreach ($taggedSubscribers as $taggedSubscriber) { + $id = $taggedSubscriber[0]; + $taggedSubscriberDef = $container->getDefinition($id); - $em->addMethodCall('addEventSubscriber', array(new Reference($id))); - } + if ($taggedSubscriberDef->isAbstract()) { + throw new InvalidArgumentException(sprintf('The abstract service "%s" cannot be tagged as a doctrine event subscriber.', $id)); } - } - if (!empty($taggedListeners)) { - $listenersPerCon = $this->groupByConnection($taggedListeners, true); - foreach ($listenersPerCon as $con => $listeners) { - $em = $this->getEventManager($con); - - uasort($listeners, $sortFunc); - foreach ($listeners as $id => $instance) { - if ($container->getDefinition($id)->isAbstract()) { - throw new \InvalidArgumentException(sprintf('The abstract service "%s" cannot be tagged as a doctrine event listener.', $id)); - } - - $em->addMethodCall('addEventListener', array( - array_unique($instance['event']), - isset($instance['lazy']) && $instance['lazy'] ? $id : new Reference($id), - )); + $tag = $taggedSubscriber[1]; + $connections = isset($tag['connection']) ? array($tag['connection']) : array_keys($this->connections); + foreach ($connections as $con) { + if (!isset($this->connections[$con])) { + throw new RuntimeException(sprintf('The Doctrine connection "%s" referenced in service "%s" does not exist. Available connections names: %s', $con, $taggedSubscriber, implode(', ', array_keys($this->connections)))); } + + $this->getEventManagerDef($container, $con)->addMethodCall('addEventSubscriber', array(new Reference($id))); } } } - private function groupByConnection(array $services, $isListener = false) + private function addTaggedListeners(ContainerBuilder $container) { - $grouped = array(); - foreach ($allCons = array_keys($this->connections) as $con) { - $grouped[$con] = array(); - } + $listenerTag = $this->tagPrefix.'.event_listener'; + $taggedListeners = $this->findAndSortTags($listenerTag, $container); + + foreach ($taggedListeners as $taggedListener) { + $id = $taggedListener[0]; + $taggedListenerDef = $container->getDefinition($taggedListener[0]); + if ($taggedListenerDef->isAbstract()) { + throw new InvalidArgumentException(sprintf('The abstract service "%s" cannot be tagged as a doctrine event listener.', $id)); + } - foreach ($services as $id => $instances) { - foreach ($instances as $instance) { - if ($isListener) { - if (!isset($instance['event'])) { - throw new \InvalidArgumentException(sprintf('Doctrine event listener "%s" must specify the "event" attribute.', $id)); - } - $instance['event'] = array($instance['event']); - - if (isset($instance['lazy']) && $instance['lazy']) { - $this->container->getDefinition($id)->setPublic(true); - } + $tag = $taggedListener[1]; + if (!isset($tag['event'])) { + throw new InvalidArgumentException(sprintf('Doctrine event listener "%s" must specify the "event" attribute.', $id)); + } + + $connections = isset($tag['connection']) ? array($tag['connection']) : array_keys($this->connections); + foreach ($connections as $con) { + if (!isset($this->connections[$con])) { + 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)))); } - $cons = isset($instance['connection']) ? array($instance['connection']) : $allCons; - foreach ($cons as $con) { - if (!isset($grouped[$con])) { - 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)))); - } - - if ($isListener && isset($grouped[$con][$id])) { - $grouped[$con][$id]['event'] = array_merge($grouped[$con][$id]['event'], $instance['event']); - } else { - $grouped[$con][$id] = $instance; - } + if ($lazy = isset($tag['lazy']) && $tag['lazy']) { + $taggedListenerDef->setPublic(true); } + + // we add one call per event per service so we have the correct order + $this->getEventManagerDef($container, $con)->addMethodCall('addEventListener', array( + $tag['event'], + $lazy ? $id : new Reference($id), + )); } } + } - return $grouped; + private function getEventManagerDef(ContainerBuilder $container, $name) + { + if (!isset($this->eventManagers[$name])) { + $this->eventManagers[$name] = $container->getDefinition(sprintf($this->managerTemplate, $name)); + } + + return $this->eventManagers[$name]; } - private function getEventManager($name) + /** + * Finds and orders all service tags with the given name by their priority. + * + * The order of additions must be respected for services having the same priority, + * and knowing that the \SplPriorityQueue class does not respect the FIFO method, + * we should not use this class. + * + * @see https://bugs.php.net/bug.php?id=53710 + * @see https://bugs.php.net/bug.php?id=60926 + * + * @param string $tagName + * @param ContainerBuilder $container + * + * @return array + */ + private function findAndSortTags($tagName, ContainerBuilder $container) { - if (null === $this->eventManagers) { - $this->eventManagers = array(); - foreach ($this->connections as $n => $id) { - $this->eventManagers[$n] = $this->container->getDefinition(sprintf($this->managerTemplate, $n)); + $sortedTags = array(); + + foreach ($container->findTaggedServiceIds($tagName) as $serviceId => $tags) { + foreach ($tags as $attributes) { + $priority = isset($attributes['priority']) ? $attributes['priority'] : 0; + $sortedTags[$priority][] = array($serviceId, $attributes); } } - return $this->eventManagers[$name]; + if ($sortedTags) { + krsort($sortedTags); + $sortedTags = call_user_func_array('array_merge', $sortedTags); + } + + return $sortedTags; } } diff --git a/src/Symfony/Bridge/Doctrine/Tests/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPassTest.php b/src/Symfony/Bridge/Doctrine/Tests/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPassTest.php index ba73678541e7..4f196f2ca6a2 100644 --- a/src/Symfony/Bridge/Doctrine/Tests/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPassTest.php +++ b/src/Symfony/Bridge/Doctrine/Tests/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPassTest.php @@ -14,6 +14,7 @@ use Symfony\Bridge\Doctrine\DependencyInjection\CompilerPass\RegisterEventListenersAndSubscribersPass; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\DependencyInjection\Definition; +use Symfony\Component\DependencyInjection\Reference; class RegisterEventListenersAndSubscribersPassTest extends \PHPUnit_Framework_TestCase { @@ -55,12 +56,18 @@ public function testProcessEventListenersWithPriorities() $container ->register('a', 'stdClass') + ->setPublic(false) + ->addTag('doctrine.event_listener', array( + 'event' => 'bar', + )) ->addTag('doctrine.event_listener', array( 'event' => 'foo', 'priority' => -5, )) ->addTag('doctrine.event_listener', array( - 'event' => 'bar', + 'event' => 'foo_bar', + 'priority' => 3, + 'lazy' => true, )) ; $container @@ -69,12 +76,34 @@ public function testProcessEventListenersWithPriorities() 'event' => 'foo', )) ; + $container + ->register('c', 'stdClass') + ->addTag('doctrine.event_listener', array( + 'event' => 'foo_bar', + 'priority' => 4, + )) + ; $this->process($container); - $this->assertEquals(array('b', 'a'), $this->getServiceOrder($container, 'addEventListener')); - - $calls = $container->getDefinition('doctrine.dbal.default_connection.event_manager')->getMethodCalls(); - $this->assertEquals(array('foo', 'bar'), $calls[1][1][0]); + $methodCalls = $container->getDefinition('doctrine.dbal.default_connection.event_manager')->getMethodCalls(); + + $this->assertEquals( + array( + array('addEventListener', array('foo_bar', new Reference('c'))), + array('addEventListener', array('foo_bar', new Reference('a'))), + array('addEventListener', array('bar', new Reference('a'))), + array('addEventListener', array('foo', new Reference('b'))), + array('addEventListener', array('foo', new Reference('a'))), + ), + $methodCalls + ); + + // not lazy so must be reference + $this->assertInstanceOf('Symfony\Component\DependencyInjection\Reference', $methodCalls[0][1][1]); + + // lazy so id instead of reference and must mark service public + $this->assertSame('a', $methodCalls[1][1][1]); + $this->assertTrue($container->getDefinition('a')->isPublic()); } public function testProcessEventListenersWithMultipleConnections() @@ -87,15 +116,86 @@ public function testProcessEventListenersWithMultipleConnections() 'event' => 'onFlush', )) ; + + $container + ->register('b', 'stdClass') + ->addTag('doctrine.event_listener', array( + 'event' => 'onFlush', + 'connection' => 'default', + )) + ; + + $container + ->register('c', 'stdClass') + ->addTag('doctrine.event_listener', array( + 'event' => 'onFlush', + 'connection' => 'second', + )) + ; + $this->process($container); - $callsDefault = $container->getDefinition('doctrine.dbal.default_connection.event_manager')->getMethodCalls(); + $this->assertEquals( + array( + array('addEventListener', array('onFlush', new Reference('a'))), + array('addEventListener', array('onFlush', new Reference('b'))), + ), + $container->getDefinition('doctrine.dbal.default_connection.event_manager')->getMethodCalls() + ); + + $this->assertEquals( + array( + array('addEventListener', array('onFlush', new Reference('a'))), + array('addEventListener', array('onFlush', new Reference('c'))), + ), + $container->getDefinition('doctrine.dbal.second_connection.event_manager')->getMethodCalls() + ); + } - $this->assertEquals('addEventListener', $callsDefault[0][0]); - $this->assertEquals(array('onFlush'), $callsDefault[0][1][0]); + public function testProcessEventSubscribersWithMultipleConnections() + { + $container = $this->createBuilder(true); - $callsSecond = $container->getDefinition('doctrine.dbal.second_connection.event_manager')->getMethodCalls(); - $this->assertEquals($callsDefault, $callsSecond); + $container + ->register('a', 'stdClass') + ->addTag('doctrine.event_subscriber', array( + 'event' => 'onFlush', + )) + ; + + $container + ->register('b', 'stdClass') + ->addTag('doctrine.event_subscriber', array( + 'event' => 'onFlush', + 'connection' => 'default', + )) + ; + + $container + ->register('c', 'stdClass') + ->addTag('doctrine.event_subscriber', array( + 'event' => 'onFlush', + 'connection' => 'second', + )) + ; + + $this->process($container); + + $this->assertEquals( + array( + array('addEventSubscriber', array(new Reference('a'))), + array('addEventSubscriber', array(new Reference('b'))), + ), + $container->getDefinition('doctrine.dbal.default_connection.event_manager')->getMethodCalls() + ); + + $this->assertEquals( + array( + array('addEventSubscriber', array(new Reference('a'))), + array('addEventSubscriber', array(new Reference('c'))), + ), + $container->getDefinition('doctrine.dbal.second_connection.event_manager')->getMethodCalls() + ); } public function testProcessEventSubscribersWithPriorities() @@ -132,11 +232,17 @@ public function testProcessEventSubscribersWithPriorities() ; $this->process($container); - $serviceOrder = $this->getServiceOrder($container, 'addEventSubscriber'); - $unordered = array_splice($serviceOrder, 0, 3); - sort($unordered); - $this->assertEquals(array('c', 'd', 'e'), $unordered); - $this->assertEquals(array('b', 'a'), $serviceOrder); + + $this->assertEquals( + array( + array('addEventSubscriber', array(new Reference('c'))), + array('addEventSubscriber', array(new Reference('d'))), + array('addEventSubscriber', array(new Reference('e'))), + array('addEventSubscriber', array(new Reference('b'))), + array('addEventSubscriber', array(new Reference('a'))), + ), + $container->getDefinition('doctrine.dbal.default_connection.event_manager')->getMethodCalls() + ); } public function testProcessNoTaggedServices() @@ -156,26 +262,6 @@ private function process(ContainerBuilder $container) $pass->process($container); } - private function getServiceOrder(ContainerBuilder $container, $method) - { - $order = array(); - foreach ($container->getDefinition('doctrine.dbal.default_connection.event_manager')->getMethodCalls() as $call) { - list($name, $arguments) = $call; - if ($method !== $name) { - continue; - } - - if ('addEventListener' === $name) { - $order[] = (string) $arguments[1]; - continue; - } - - $order[] = (string) $arguments[0]; - } - - return $order; - } - private function createBuilder($multipleConnections = false) { $container = new ContainerBuilder();