Missing priority for events in doctrine bridge #21977

chapterjason · 2017-03-11T18:31:05Z

Q	A
Bug report?	no
Feature request?	yes
BC Break report?	no
RFC?	no
Symfony version	3.2.4

I just have two listeners and they have to be in the correct order. My first approach was to set the priority in the tags for the listeners, I noticed that nothing changed the order. After I checked the code in Symfony/Bridge/Doctrine/ContainerAwareEventManager.php#L101 and compared it to Symfony/Component/Form/FormConfigBuilderInterface.php#L33 I just realized that the priority is missing.

Yeah I know that Component/Form and the Bridge/Doctrine are two different parts.

The text was updated successfully, but these errors were encountered:

javiereguiluz · 2017-03-12T08:55:38Z

You are right: we allow to define priorities everywhere ... so I guess it should be OK to add priorities here too. 👍 from me!

wouterj · 2017-03-12T10:24:47Z

Doctrine is using its own EventManager: https://github.com/doctrine/common/blob/master/lib/Doctrine/Common/EventManager.php This one doesn't have the priority feature, so it's not something we can add in Symfony. It should first be added in the Doctrine EventManager.

javiereguiluz · 2017-03-12T10:35:36Z

@chapterjason after @wouterj's comment, I'm afraid we need to close this as "can't fix" because it depends on Doctrine project. If you still want this feature, please open an issue in https://github.com/doctrine/common/issues

Thanks!

dmaicher · 2017-03-12T16:05:14Z

Aren't the listeners and subscribers already registered in the correct order when specifying priorities on the tag?

https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Doctrine/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPass.php#L67

chapterjason · 2017-03-12T18:19:51Z

Thank you @dmaicher I found out that https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Doctrine/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPass.php#L91 does not take care of priorities what makes no sense cause the priorities will sorted later in https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Doctrine/DependencyInjection/CompilerPass/RegisterEventListenersAndSubscribersPass.php#L95

My prove:
I changed the lines to following

            var_dump(json_encode($taggedListeners));
            $listenersPerCon = $this->groupByConnection($taggedListeners, true);
            var_dump(json_encode($listenersPerCon));

My result:

Symfony\Bridge\Doctrine\DependencyInjection\CompilerPass\RegisterEventListenersAndSubscribersPass.php:92:
{
   "my.listener1":[
      {
         "event":"prePersist"
      },
      {
         "event":"preUpdate",
         "priority":50
      }
   ],
   "my.listener2":[
      {
         "event":"postPersist"
      },
      {
         "event":"preUpdate",
         "priority":100
      },
      {
         "event":"preRemove"
      },
      {
         "event":"postFlush"
      }
   ]
}

Symfony\Bridge\Doctrine\DependencyInjection\CompilerPass\RegisterEventListenersAndSubscribersPass.php:94:
{
   "default":{
      "my.listener1":{
         "event":[
            "prePersist",
            "preUpdate"
         ]
      },
      "my.listener2":{
         "event":[
            "postPersist",
            "preUpdate",
            "preRemove",
            "postFlush"
         ]
      }
   }
}

dmaicher · 2017-03-12T18:43:46Z

@javiereguiluz we have to re-open this unfortunately 😢 I double checked and there is indeed something broken with handling the priorities.

This only ever worked correctly if there is exactly one tag per service as otherwise the priorities are lost or handled incorrectly.

Thanks @chapterjason for debugging it 👍

Test case that we have for it currently:

    public function testProcessEventListenersWithPriorities()
    {
        $container = $this->createBuilder();

        $container
            ->register('a', 'stdClass')
            ->addTag('doctrine.event_listener', array(
                'event' => 'foo',
                'priority' => -5,
            ))
            ->addTag('doctrine.event_listener', array(
                'event' => 'bar',
            ))
        ;
        $container
            ->register('b', 'stdClass')
            ->addTag('doctrine.event_listener', array(
                'event' => 'foo',
            ))
        ;

        $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]);
    }

If we switch the tag order for service a like

        $container
            ->register('a', 'stdClass')
            ->addTag('doctrine.event_listener', array(
                'event' => 'bar',
            ))
            ->addTag('doctrine.event_listener', array(
                'event' => 'foo',
                'priority' => -5,
            ))
        ;

Then the tests suddenly fail as the priorities are lost:

There was 1 failure:

1) Symfony\Bridge\Doctrine\Tests\DependencyInjection\CompilerPass\RegisterEventListenersAndSubscribersPassTest::testProcessEventListenersWithPriorities
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'foo'
-    1 => 'bar'
+    0 => 'bar'
+    1 => 'foo'
 )

dmaicher · 2017-03-14T20:48:53Z

@chapterjason I proposed a fix here: #22001
Maybe you can try it with my changes?

@chapterjason

…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

@chapterjason

…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 symfony/symfony#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

…s (dmaicher) This PR was merged into the 3.4 branch. Discussion ---------- [Doctrine Bridge] fix priority for doctrine event listeners | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21977 | License | MIT | Doc PR | - As discussed in #27126 this ports changes from #22001 to 3.4 that were dropped when merging 2.8 into 3.2 here: dc66960#diff-27d2e9b071d766df504c3fe4131e7abf I took my original changeset from 2.8 and applied all commits since then on top of that. Commits ------- b3ac938 [Doctrine Bridge] fix priority for doctrine event listeners

xabbuh added DoctrineBridge Feature labels Mar 11, 2017

javiereguiluz closed this as completed Mar 12, 2017

chapterjason mentioned this issue Mar 12, 2017

Priority in EventManager doctrine/common#782

Closed

javiereguiluz reopened this Mar 12, 2017

dmaicher mentioned this issue Mar 14, 2017

[Doctrine Bridge] fix priority for doctrine event listeners #22001

Merged

fabpot closed this as completed Mar 17, 2017

This was referenced May 3, 2018

[Doctrine Bridge] return 0 on equality when sorting listeners/subscribers #27126

Closed

[Doctrine Bridge] fix priority for doctrine event listeners #27133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Missing priority for events in doctrine bridge #21977

Missing priority for events in doctrine bridge #21977

chapterjason commented Mar 11, 2017 •

edited

Loading

javiereguiluz commented Mar 12, 2017

wouterj commented Mar 12, 2017 •

edited by javiereguiluz

Loading

javiereguiluz commented Mar 12, 2017

dmaicher commented Mar 12, 2017

chapterjason commented Mar 12, 2017

dmaicher commented Mar 12, 2017 •

edited by javiereguiluz

Loading

dmaicher commented Mar 14, 2017

Missing priority for events in doctrine bridge #21977

Missing priority for events in doctrine bridge #21977

Comments

chapterjason commented Mar 11, 2017 • edited Loading

javiereguiluz commented Mar 12, 2017

wouterj commented Mar 12, 2017 • edited by javiereguiluz Loading

javiereguiluz commented Mar 12, 2017

dmaicher commented Mar 12, 2017

chapterjason commented Mar 12, 2017

dmaicher commented Mar 12, 2017 • edited by javiereguiluz Loading

dmaicher commented Mar 14, 2017

chapterjason commented Mar 11, 2017 •

edited

Loading

wouterj commented Mar 12, 2017 •

edited by javiereguiluz

Loading

dmaicher commented Mar 12, 2017 •

edited by javiereguiluz

Loading