bug #23008 [EventDispatcher] Handle laziness internally instead of re…

…lying on ClosureProxyArgument (nicolas-grekas)

This PR was merged into the 3.3 branch.

Discussion
----------

[EventDispatcher] Handle laziness internally instead of relying on ClosureProxyArgument

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

If we decide to go this way, we might drop ClosureProxyArgument entirely because we won't need it internally. I'll propose it in another PR for discussion if this one is accepted.

This PR allows "high-order" listeners, as `array(Closure, method)`. Closure is called to get the actual instance when needed only.

How does it look to you? (I'll add tests once confirmed)

Commits
-------

c17a009 [EventDispatcher] Handle laziness internally instead of relying on ClosureProxyArgument

src/Symfony/Component/EventDispatcher/ContainerAwareEventDispatcher.php

@@ -116,7 +116,7 @@ public function removeListener($eventName, $listener)
     public function hasListeners($eventName = null)
     {
         if (null === $eventName) {
-            return (bool) count($this->listenerIds) || (bool) count($this->listeners);
+            return count($this->listenerIds) || count($this->listeners) || parent::hasListeners();
         }
 
         if (isset($this->listenerIds[$eventName])) {

src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php

@@ -11,10 +11,11 @@
 
 namespace Symfony\Component\EventDispatcher\DependencyInjection;
 
-use Symfony\Component\DependencyInjection\Argument\ClosureProxyArgument;
+use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
 use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
+use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\EventDispatcher\EventDispatcher;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 
@@ -78,7 +79,7 @@ public function process(ContainerBuilder $container)
                     $event['method'] = preg_replace('/[^a-z0-9]/i', '', $event['method']);
                 }
 
-                $definition->addMethodCall('addListener', array($event['event'], new ClosureProxyArgument($id, $event['method']), $priority));
+                $definition->addMethodCall('addListener', array($event['event'], array(new ServiceClosureArgument(new Reference($id)), $event['method']), $priority));
             }
         }
 
@@ -103,7 +104,7 @@ public function process(ContainerBuilder $container)
             ExtractingEventDispatcher::$subscriber = $class;
             $extractingDispatcher->addSubscriber($extractingDispatcher);
             foreach ($extractingDispatcher->listeners as $args) {
-                $args[1] = new ClosureProxyArgument($id, $args[1]);
+                $args[1] = array(new ServiceClosureArgument(new Reference($id)), $args[1]);
                 $definition->addMethodCall('addListener', $args);
             }
             $extractingDispatcher->listeners = array();

src/Symfony/Component/EventDispatcher/EventDispatcher.php

@@ -24,6 +24,7 @@
  * @author Fabien Potencier <fabien@symfony.com>
  * @author Jordi Boggiano <j.boggiano@seld.be>
  * @author Jordan Alliot <jordan.alliot@gmail.com>
+ * @author Nicolas Grekas <p@tchwork.com>
  */
 class EventDispatcher implements EventDispatcherInterface
 {
@@ -52,7 +53,7 @@ public function dispatch($eventName, Event $event = null)
     public function getListeners($eventName = null)
     {
         if (null !== $eventName) {
-            if (!isset($this->listeners[$eventName])) {
+            if (empty($this->listeners[$eventName])) {
                 return array();
             }
 
@@ -77,13 +78,23 @@ public function getListeners($eventName = null)
      */
     public function getListenerPriority($eventName, $listener)
     {
-        if (!isset($this->listeners[$eventName])) {
+        if (empty($this->listeners[$eventName])) {
             return;
         }
 
+        if (is_array($listener) && isset($listener[0]) && $listener[0] instanceof \Closure) {
+            $listener[0] = $listener[0]();
+        }
+
         foreach ($this->listeners[$eventName] as $priority => $listeners) {
-            if (false !== in_array($listener, $listeners, true)) {
-                return $priority;
+            foreach ($listeners as $k => $v) {
+                if ($v !== $listener && is_array($v) && isset($v[0]) && $v[0] instanceof \Closure) {
+                    $v[0] = $v[0]();
+                    $this->listeners[$eventName][$priority][$k] = $v;
+                }
+                if ($v === $listener) {
+                    return $priority;
+                }
             }
         }
     }
@@ -93,7 +104,17 @@ public function getListenerPriority($eventName, $listener)
      */
     public function hasListeners($eventName = null)
     {
-        return (bool) $this->getListeners($eventName);
+        if (null !== $eventName) {
+            return !empty($this->listeners[$eventName]);
+        }
+
+        foreach ($this->listeners as $eventListeners) {
+            if ($eventListeners) {
+                return true;
+            }
+        }
+
+        return false;
     }
 
     /**
@@ -110,13 +131,30 @@ public function addListener($eventName, $listener, $priority = 0)
      */
     public function removeListener($eventName, $listener)
     {
-        if (!isset($this->listeners[$eventName])) {
+        if (empty($this->listeners[$eventName])) {
             return;
         }
 
+        if (is_array($listener) && isset($listener[0]) && $listener[0] instanceof \Closure) {
+            $listener[0] = $listener[0]();
+        }
+
         foreach ($this->listeners[$eventName] as $priority => $listeners) {
-            if (false !== ($key = array_search($listener, $listeners, true))) {
-                unset($this->listeners[$eventName][$priority][$key], $this->sorted[$eventName]);
+            foreach ($listeners as $k => $v) {
+                if ($v !== $listener && is_array($v) && isset($v[0]) && $v[0] instanceof \Closure) {
+                    $v[0] = $v[0]();
+                }
+                if ($v === $listener) {
+                    unset($listeners[$k], $this->sorted[$eventName]);
+                } else {
+                    $listeners[$k] = $v;
+                }
+            }
+
+            if ($listeners) {
+                $this->listeners[$eventName][$priority] = $listeners;
+            } else {
+                unset($this->listeners[$eventName][$priority]);
             }
         }
     }
@@ -183,6 +221,16 @@ protected function doDispatch($listeners, $eventName, Event $event)
     private function sortListeners($eventName)
     {
         krsort($this->listeners[$eventName]);
-        $this->sorted[$eventName] = call_user_func_array('array_merge', $this->listeners[$eventName]);
+        $this->sorted[$eventName] = array();
+
+        foreach ($this->listeners[$eventName] as $priority => $listeners) {
+            foreach ($listeners as $k => $listener) {
+                if (is_array($listener) && isset($listener[0]) && $listener[0] instanceof \Closure) {
+                    $listener[0] = $listener[0]();
+                    $this->listeners[$eventName][$priority][$k] = $listener;
+                }
+                $this->sorted[$eventName][] = $listener;
+            }
+        }
     }
 }

src/Symfony/Component/EventDispatcher/Tests/AbstractEventDispatcherTest.php

@@ -302,6 +302,73 @@ public function testHasListenersWithoutEventsReturnsFalseAfterHasListenersWithEv
         $this->assertFalse($this->dispatcher->hasListeners('foo'));
         $this->assertFalse($this->dispatcher->hasListeners());
     }
+
+    public function testHasListenersIsLazy()
+    {
+        $called = 0;
+        $listener = array(function () use (&$called) { ++$called; }, 'onFoo');
+        $this->dispatcher->addListener('foo', $listener);
+        $this->assertTrue($this->dispatcher->hasListeners());
+        $this->assertTrue($this->dispatcher->hasListeners('foo'));
+        $this->assertSame(0, $called);
+    }
+
+    public function testDispatchLazyListener()
+    {
+        $called = 0;
+        $factory = function () use (&$called) {
+            ++$called;
+
+            return new TestWithDispatcher();
+        };
+        $this->dispatcher->addListener('foo', array($factory, 'foo'));
+        $this->assertSame(0, $called);
+        $this->dispatcher->dispatch('foo', new Event());
+        $this->dispatcher->dispatch('foo', new Event());
+        $this->assertSame(1, $called);
+    }
+
+    public function testRemoveFindsLazyListeners()
+    {
+        $test = new TestWithDispatcher();
+        $factory = function () use ($test) { return $test; };
+
+        $this->dispatcher->addListener('foo', array($factory, 'foo'));
+        $this->assertTrue($this->dispatcher->hasListeners('foo'));
+        $this->dispatcher->removeListener('foo', array($test, 'foo'));
+        $this->assertFalse($this->dispatcher->hasListeners('foo'));
+
+        $this->dispatcher->addListener('foo', array($test, 'foo'));
+        $this->assertTrue($this->dispatcher->hasListeners('foo'));
+        $this->dispatcher->removeListener('foo', array($factory, 'foo'));
+        $this->assertFalse($this->dispatcher->hasListeners('foo'));
+    }
+
+    public function testPriorityFindsLazyListeners()
+    {
+        $test = new TestWithDispatcher();
+        $factory = function () use ($test) { return $test; };
+
+        $this->dispatcher->addListener('foo', array($factory, 'foo'), 3);
+        $this->assertSame(3, $this->dispatcher->getListenerPriority('foo', array($test, 'foo')));
+        $this->dispatcher->removeListener('foo', array($factory, 'foo'));
+
+        $this->dispatcher->addListener('foo', array($test, 'foo'), 5);
+        $this->assertSame(5, $this->dispatcher->getListenerPriority('foo', array($factory, 'foo')));
+    }
+
+    public function testGetLazyListeners()
+    {
+        $test = new TestWithDispatcher();
+        $factory = function () use ($test) { return $test; };
+
+        $this->dispatcher->addListener('foo', array($factory, 'foo'), 3);
+        $this->assertSame(array(array($test, 'foo')), $this->dispatcher->getListeners('foo'));
+
+        $this->dispatcher->removeListener('foo', array($test, 'foo'));
+        $this->dispatcher->addListener('bar', array($factory, 'foo'), 3);
+        $this->assertSame(array('bar' => array(array($test, 'foo'))), $this->dispatcher->getListeners());
+    }
 }
 
 class CallableClass

src/Symfony/Component/EventDispatcher/Tests/DependencyInjection/RegisterListenersPassTest.php

@@ -12,8 +12,9 @@
 namespace Symfony\Component\EventDispatcher\Tests\DependencyInjection;
 
 use PHPUnit\Framework\TestCase;
-use Symfony\Component\DependencyInjection\Argument\ClosureProxyArgument;
+use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\EventDispatcher\DependencyInjection\RegisterListenersPass;
 
 class RegisterListenersPassTest extends TestCase
@@ -127,17 +128,17 @@ public function testEventSubscriberResolvableClassName()
         $registerListenersPass->process($container);
 
         $definition = $container->getDefinition('event_dispatcher');
-        $expected_calls = array(
+        $expectedCalls = array(
             array(
                 'addListener',
                 array(
                     'event',
-                    new ClosureProxyArgument('foo', 'onEvent'),
+                    array(new ServiceClosureArgument(new Reference('foo')), 'onEvent'),
                     0,
                 ),
             ),
         );
-        $this->assertEquals($expected_calls, $definition->getMethodCalls());
+        $this->assertEquals($expectedCalls, $definition->getMethodCalls());
     }
 
     /**