[DoctrineBridge] Cannot remove Lazy-loaded listeners by object

[VincentLanglet]

Symfony version(s) affected

6.3.x, but maybe lower

Description

Let's say I the following code

foreach ($this->em->getEventManager()->getAllListeners() as $event => $listeners) {
     foreach ($listeners as $listener) {
         $this->em->getEventManager()->removeEventListener($event, $listener);
    }
}

To me this is suppose to remove all the existing event listener.
Seems like with the ContainerAwareEventManager it does nothing.

If I dump the listeners, I get:

[
    'postPersist' => [
        '_service_App\EventListener\MyListener' => the object(App\EventListener\MyListener),
    ],
]

Which means that, when I call removeEventListener($event, $listener), I pass the event and the object but in the remove method, the hash computed is the spl_object_hash and not the one with _service prefix.

symfony/src/Symfony/Bridge/Doctrine/ContainerAwareEventManager.php

Lines 128 to 146 in bd04cda

	public function removeEventListener($events, $listener): void
	{
	if (!$this->initializedSubscribers) {
	$this->initializeSubscribers();
	}
	$hash = $this->getHash($listener);
	foreach ((array) $events as $event) {
	// Check if we actually have this listener associated
	if (isset($this->listeners[$event][$hash])) {
	unset($this->listeners[$event][$hash]);
	}
	if (isset($this->methods[$event][$hash])) {
	unset($this->methods[$event][$hash]);
	}
	}
	}

I would say something is wrong with the initializeListeners method

symfony/src/Symfony/Bridge/Doctrine/ContainerAwareEventManager.php

Lines 166 to 176 in bd04cda

	private function initializeListeners(string $eventName): void
	{
	$this->initialized[$eventName] = true;
	foreach ($this->listeners[$eventName] as $hash => $listener) {
	if (\is_string($listener)) {
	$this->listeners[$eventName][$hash] = $listener = $this->container->get($listener);
	$this->methods[$eventName][$hash] = $this->getMethod($listener, $eventName);
	}
	}
	}

because it transform the $listener as string to the object instance, but without changing the hash.

the $lister property is modified from

[
    'postPersist' => [
        '_service_App\EventListener\MyListener' => "App\EventListener\MyListener",
    ],
]

to

[
    'postPersist' => [
        '_service_App\EventListener\MyListener' => the object(App\EventListener\MyListener),
    ],
]

when it should be

[
    'postPersist' => [
        'someHash' => the object(App\EventListener\MyListener),
    ],
]

Possible Solution

First I thought of something like
Maybe the method should be

    private function initializeListeners(string $eventName): void
    {
        $this->initialized[$eventName] = true;
        foreach ($this->listeners[$eventName] as $hash => $listener) {
            if (\is_string($listener)) {
                $listener = $this->container->get($listener);
                $newHash = $this->getHash($listener);
                unset($this->listeners[$eventName][$hash]);
                
                $this->listeners[$eventName][$newHash] = $listener;
                $this->methods[$eventName][$newHash] = $this->getMethod($listener, $eventName);
            }
        }
    }

But it require listeners to be initialized then.
Which means that:

$listener = new MyListener();
$this->container->set('lazy', $listener);
$this->evm->addEventListener('foo', 'lazy');
$this->evm->removeEventListener('foo', $listener);

won't do nothing when

$listener = new MyListener();
$this->container->set('lazy', $listener);
$this->evm->addEventListener('foo', 'lazy');
$this->evm->getAllListeners(); // Just to initialized
$this->evm->removeEventListener('foo', $listener);

will remove the lazy (now loaded) listener. Which would be weird.

Reproducer

This is a reproducer in the Symfony Tests:

public function testRemoveAllEventListener()
    {
        $this->container->set('lazy', new MyListener());
        $this->evm->addEventListener('foo', 'lazy');
        $this->evm->addEventListener('foo', new MyListener());

        foreach ($this->evm->getAllListeners() as $event => $listeners) {
            foreach ($listeners as $listener) {
                $this->evm->removeEventListener($event, $listener);
            }
        }

        $this->assertSame([], $this->evm->getListeners('foo'));
    }

Additional context

This seems cause by the PR #31335 and reported here #31335 (comment)
@Koc @nicolas-grekas @dmaicher

[dmaicher]

Indeed this seems like a bug to me 😕

I think it makes sense to re-index the listener with the spl_object_hash once it was initialized 👍

Regarding the other issue mentioned: We might have to initialize listeners then for the event(s) so we can properly remove also the lazy instances? Not sure how common it is to remove listeners and how bad the performance penalty would be 🤷‍♂️ Or we can first try to remove it and only if we cannot find the listener then initialize all of them and try again?

[VincentLanglet]

Indeed this seems like a bug to me 😕

I think it makes sense to re-index the listener with the spl_object_hash once it was initialized 👍

When I made some try, it also require to keep a map "oldHash" => "newHash", to avoid issue if someone does:

$listener = new MyListener();
$this->container->set('lazy', $listener);
$this->evm->addEventListener('foo', 'lazy');
$this->evm->getAllListeners(); // Just to initialized
$this->evm->removeEventListener('foo', 'lazy'); // If the hash change, the event listener won't be removed.

Does something like this seems ok to you @dmaicher ? 5.4...VincentLanglet:symfony:removeListeners

[dmaicher]

@VincentLanglet yeah looks good. I will take another look once you opened a PR, ok?