bug #22001 [Doctrine Bridge] fix priority for doctrine event listener…

…s (dmaicher)

This PR was merged into the 2.7 branch.

Discussion
----------

[Doctrine Bridge] fix priority for doctrine event listeners

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #21977
| License       | MIT
| Doc PR        | -

This fixes handling the priorities for doctrine event listeners. As found out by @chapterjason in #21977 the priority was incorrectly handled as soon as a listener had more than one tag (so listening to multiple events).

With this changes all tagged listeners are globally sorted by priority (using the same stable sort approach as in the later available `PriorityTaggedServiceTrait`) and then added one by one to the event manager.

I also updated the tests a bit as it was not covering all cases.

We also have to extend the docs for it I think as it does not mention the `priority` and `lazy` option at all? http://symfony.com/doc/current/doctrine/event_listeners_subscribers.html

Commits
-------

9d9d4ef [Doctrine Bridge] fix priority for doctrine event listeners

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 <jmikola@gmail.com>
  * @author Alexander <iam.asm89@gmail.com>
+ * @author David Maicher <mail@dmaicher.de>
  */
 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;
     }
 }