-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Doctrine] Respect parent class contract in ContainerAwareEventManager #31335
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
a5076ce
to
9d45fac
Compare
Doesn't this change break some desired laziness? |
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 :) |
I've found this problem when try to inject Serializer service into custom DBAL Type using this article . I was surprised that Also there is related issue with ContainerAwareEventManager #31051 , I will try to fix it in this PR.
only if you call |
@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 |
ping @dmaicher then for review :) |
9d45fac
to
9febd51
Compare
182be5b
to
f16e9a5
Compare
@malarzm can you look at the tests? Looks like this PR should fix your issue |
@Koc looking at the tests your PR will fix my issue, thanks a lot! 🍻 |
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. |
43205af
to
28df79d
Compare
28df79d
to
31f5564
Compare
@nicolas-grekas comments fixed, Travis is happy |
31f5564
to
42d6272
Compare
Thank you @Koc. |
…EventManager (Koc) This PR was merged into the 3.4 branch. Discussion ---------- [Doctrine] Respect parent class contract in ContainerAwareEventManager | 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](https://github.com/doctrine/event-manager/blob/master/lib/Doctrine/Common/EventManager.php#L50) `getListeners` method should return array of initialized objects but now it returns array of strings of listener service names. Commits ------- 42d6272 Respect parent class contract in ContainerAwareDoctrineEventManager
Hello, @Koc, could you please have a look at described behavior. To my mind, there is a contradiction in logic. $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 )
Initially, the condition was true both for |
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.