bug #22001 [Doctrine Bridge] fix priority for doctrine event listeners (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 https://github.com/symfony/symfony/issues/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
-------
9d9d4efb88
[Doctrine Bridge] fix priority for doctrine event listeners
This commit is contained in:
commit
ac109f154b
@ -11,27 +11,30 @@
|
||||
|
||||
namespace Symfony\Bridge\Doctrine\DependencyInjection\CompilerPass;
|
||||
|
||||
use Symfony\Component\DependencyInjection\ContainerBuilder;
|
||||
use Symfony\Component\DependencyInjection\Reference;
|
||||
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;
|
||||
|
||||
/**
|
||||
* 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 @@ class RegisterEventListenersAndSubscribersPass implements CompilerPassInterface
|
||||
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;
|
||||
$this->addTaggedSubscribers($container);
|
||||
$this->addTaggedListeners($container);
|
||||
}
|
||||
|
||||
return $a > $b ? -1 : 1;
|
||||
};
|
||||
private function addTaggedSubscribers(ContainerBuilder $container)
|
||||
{
|
||||
$subscriberTag = $this->tagPrefix.'.event_subscriber';
|
||||
$taggedSubscribers = $this->findAndSortTags($subscriberTag, $container);
|
||||
|
||||
if (!empty($taggedSubscribers)) {
|
||||
$subscribersPerCon = $this->groupByConnection($taggedSubscribers);
|
||||
foreach ($subscribersPerCon as $con => $subscribers) {
|
||||
$em = $this->getEventManager($con);
|
||||
foreach ($taggedSubscribers as $taggedSubscriber) {
|
||||
$id = $taggedSubscriber[0];
|
||||
$taggedSubscriberDef = $container->getDefinition($id);
|
||||
|
||||
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));
|
||||
}
|
||||
|
||||
$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 ($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']);
|
||||
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));
|
||||
}
|
||||
|
||||
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 getEventManager($name)
|
||||
private function getEventManagerDef(ContainerBuilder $container, $name)
|
||||
{
|
||||
if (null === $this->eventManagers) {
|
||||
$this->eventManagers = array();
|
||||
foreach ($this->connections as $n => $id) {
|
||||
$this->eventManagers[$n] = $this->container->getDefinition(sprintf($this->managerTemplate, $n));
|
||||
}
|
||||
if (!isset($this->eventManagers[$name])) {
|
||||
$this->eventManagers[$name] = $container->getDefinition(sprintf($this->managerTemplate, $name));
|
||||
}
|
||||
|
||||
return $this->eventManagers[$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)
|
||||
{
|
||||
$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);
|
||||
}
|
||||
}
|
||||
|
||||
if ($sortedTags) {
|
||||
krsort($sortedTags);
|
||||
$sortedTags = call_user_func_array('array_merge', $sortedTags);
|
||||
}
|
||||
|
||||
return $sortedTags;
|
||||
}
|
||||
}
|
||||
|
@ -15,6 +15,7 @@ use PHPUnit\Framework\TestCase;
|
||||
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 TestCase
|
||||
{
|
||||
@ -56,12 +57,18 @@ class RegisterEventListenersAndSubscribersPassTest extends TestCase
|
||||
|
||||
$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
|
||||
@ -70,12 +77,34 @@ class RegisterEventListenersAndSubscribersPassTest extends TestCase
|
||||
'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'));
|
||||
$methodCalls = $container->getDefinition('doctrine.dbal.default_connection.event_manager')->getMethodCalls();
|
||||
|
||||
$calls = $container->getDefinition('doctrine.dbal.default_connection.event_manager')->getMethodCalls();
|
||||
$this->assertEquals(array('foo', 'bar'), $calls[1][1][0]);
|
||||
$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()
|
||||
@ -88,15 +117,86 @@ class RegisterEventListenersAndSubscribersPassTest extends TestCase
|
||||
'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('addEventListener', $callsDefault[0][0]);
|
||||
$this->assertEquals(array('onFlush'), $callsDefault[0][1][0]);
|
||||
$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()
|
||||
);
|
||||
}
|
||||
|
||||
$callsSecond = $container->getDefinition('doctrine.dbal.second_connection.event_manager')->getMethodCalls();
|
||||
$this->assertEquals($callsDefault, $callsSecond);
|
||||
public function testProcessEventSubscribersWithMultipleConnections()
|
||||
{
|
||||
$container = $this->createBuilder(true);
|
||||
|
||||
$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()
|
||||
@ -133,11 +233,17 @@ class RegisterEventListenersAndSubscribersPassTest extends TestCase
|
||||
;
|
||||
|
||||
$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()
|
||||
@ -157,26 +263,6 @@ class RegisterEventListenersAndSubscribersPassTest extends TestCase
|
||||
$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();
|
||||
|
Reference in New Issue
Block a user