NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[EventDispatcher] fix BC layer#31258

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

nicolas-grekas wants to merge1 commit intosymfony:masterfromnicolas-grekas:ed-fix

Closed

[EventDispatcher] fix BC layer#31258

nicolas-grekas wants to merge1 commit intosymfony:masterfromnicolas-grekas:ed-fix

Conversation

Copy link

Member

nicolas-grekas commentedApr 26, 2019

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

nicolas-grekas added this to thenext milestone

Apr 26, 2019

carsonbot added Status: Needs Review EventDispatcher Bug labels

Apr 26, 2019

Simperfit approved these changes

Apr 26, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Apr 26, 2019

[EventDispatcher] fix BC layer

e9dcea1

nicolas-grekas force-pushed theed-fix branch fromc9f3402 toe9dcea1Compare

April 26, 2019 07:06

nicolas-grekas mentioned this pull request

Apr 26, 2019

[EventDispatcher] Fix TraceableEventDispatcher FC/BC layer#31254

Merged

chalasr reviewed

Apr 26, 2019

View reviewed changes

src/Symfony/Component/EventDispatcher/EventDispatcher.php

		}
		$listener($eventinstanceof Event ?$event :newWrappedEvent($event),$eventName,$this);
		// @deprecated: the ternary operator is part of a BC layer and should be removed in 5.0
		$listener($eventinstanceof Event \|\| !$listenerinstanceof WrappedListener ?$event :newWrappedEvent($event),$eventName,$this);

Copy link

Member

chalasrApr 26, 2019•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

$event instanceof Event is always false because of the early return l234
and!$listener instanceof WrappedListener is always true as listeners are optimized at this point (plain\Closure).
Broken test case:

diff --git a/src/Symfony/Component/EventDispatcher/Tests/Debug/TraceableEventDispatcherTest.php b/src/Symfony/Component/EventDispatcher/Tests/Debug/TraceableEventDispatcherTest.phpindex 8fd0305b0e..db3176dcd1 100644--- a/src/Symfony/Component/EventDispatcher/Tests/Debug/TraceableEventDispatcherTest.php+++ b/src/Symfony/Component/EventDispatcher/Tests/Debug/TraceableEventDispatcherTest.php@@ -18,6 +18,7 @@ use Symfony\Component\EventDispatcher\EventDispatcher; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\EventDispatcher\EventSubscriberInterface; use Symfony\Component\Stopwatch\Stopwatch;+use Symfony\Contracts\EventDispatcher\Event as ContractsEvent;  class TraceableEventDispatcherTest extends TestCase {@@ -151,6 +152,15 @@ class TraceableEventDispatcherTest extends TestCase         $this->assertArrayHasKey('stub', $listeners[0]);     }+    public function testDispatchContractsEvent()+    {+        $tdispatcher = new TraceableEventDispatcher(new EventDispatcher(), new Stopwatch());+        $tdispatcher->addListener('foo', function () {}, 5);+        $tdispatcher->dispatch(new ContractsEvent(), 'foo');+        $listeners = $tdispatcher->getCalledListeners();+        $this->assertArrayHasKey('stub', $listeners[0]);+    }+     public function testGetCalledListenersNested()     {         $tdispatcher = null;