[Doctrine] Respect parent class contract in ContainerAwareEventManager

[Koc]

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes, failures looks unrelated
Fixed tickets	#31051
License	MIT
Doc PR	-

According to method signature of original EventManager getListeners method should return array of initialized objects but now it returns array of strings of listener service names.

[nicolas-grekas]

Doesn't this change break some desired laziness?

[nicolas-grekas]

now it returns array of strings of listener service names

any link to what "now" means? is that recent behavior change? any bug you experienced or did this come out by reviewing?

context needed please :)

[Koc]

I've found this problem when try to inject Serializer service into custom DBAL Type using this article . I was surprised that getListeners('custom_event') returns array of strings instead of listeners objects.

Also there is related issue with ContainerAwareEventManager #31051 , I will try to fix it in this PR.

Doesn't this change break some desired laziness?

only if you call getListeners without any argument

[malarzm]

@nicolas-grekas this would also fix my #31051 so I'll allow myself to comment :) For me this came up during update from Sf 4.1 to 4.2 where all Doctrine listeners became lazy by default thanks to #27661. Biggest issue is that Doctrine's getListeners method promises to return object[]|object[][] while Symfony implementation is returning strings if listeners were not initialized. So even if this change breaks some desired laziness, it's fixing broken contract. IMO if laziness is to stay as it is, it should use real proxying.

[nicolas-grekas]

ping @dmaicher then for review :)

[Koc]

@malarzm can you look at the tests? Looks like this PR should fix your issue

[malarzm]

@Koc looking at the tests your PR will fix my issue, thanks a lot! 🍻

[dmaicher]

I just tested this change on my big monolith work project and its all good except one test fail:

public function testListenerIsRegistered(): void
{
    $doctrineEventManager = $this->em->getConnection()->getEventManager();
    $preUpdateListeners = $doctrineEventManager->getListeners(Events::preUpdate);
    // this obviously fails now
    $this->assertContains('some_service_id', $preUpdateListeners); 
}

So we are actually relying on the current behavior before this fix 😋 I bet there are other people out there relying on it. This change looks correct to me but we should expect some issues being reported when this is merged & released.

@nicolas-grekas actually laziness is not affected on my project. getListeners() is never called anywhere. So it would only break laziness for people calling it or using bundles/libraries that are calling this somewhere.

[Koc]

@nicolas-grekas comments fixed, Travis is happy

[fabpot]

Thank you @Koc.

[yaroslavbr]

Hello, @Koc, could you please have a look at described behavior. To my mind, there is a contradiction in logic.
This example worked for Symfony 3.4.27 but got broken after the update to 3.4.36.
`ContainerAwareEventManager::addEventListener('postFlush', 'listener_service_name.string');

$listeners = ContainerAwareEventManager::getListeners('postFlush');

//Iterate over fetched listeners and call remove method

ContainerAwareEventManagerremoveEventListener::removeEventListener('postFlush', 'listener_service_name.string');`

This worked before but not after the update. The reason is in the condition (same for addEventListener and removeEventListener )

if (\is_string($listener)) { $hash = '_service_'.$listener; } else { // Picks the hash code related to that listener $hash = spl_object_hash($listener); }

Initially, the condition was true both for addEventListener and removeEventListener so hash was build based on service name and prefix. Now getListeners returns objects so while calling
removeEventListener hash is build based on spl_object_hash which differs from the hash in
addEventListener. So the listener is not removed. Is it expected behavior? To my mind, it is BC brack considering minor version upgrade, is not it?