Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
a5076ce to9d45facCompareUh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedMay 1, 2019
Doesn't this change break some desired laziness? |
nicolas-grekas commentedMay 1, 2019
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 commentedMay 1, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I've found this problem when try to inject Serializer service into custom DBAL Type usingthis 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 |
malarzm commentedMay 6, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 |
nicolas-grekas commentedMay 6, 2019
ping@dmaicher then for review :) |
9d45fac to9febd51Compare182be5b tof16e9a5CompareKoc commentedMay 6, 2019
@malarzm can you look at the tests? Looks like this PR should fix your issue |
Uh oh!
There was an error while loading.Please reload this page.
malarzm commentedMay 6, 2019
@Koc looking at the tests your PR will fix my issue, thanks a lot! 🍻 |
dmaicher commentedMay 7, 2019
I just tested this change on my big monolith work project and its all good except one test fail: publicfunctiontestListenerIsRegistered():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. |
Uh oh!
There was an error while loading.Please reload this page.
43205af to28df79dCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
28df79d to31f5564CompareKoc commentedMay 9, 2019
@nicolas-grekas comments fixed, Travis is happy |
Uh oh!
There was an error while loading.Please reload this page.
31f5564 to42d6272Comparefabpot commentedMay 18, 2019
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
yaroslavbr commentedDec 6, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 |
Uh oh!
There was an error while loading.Please reload this page.
According to method signature oforiginal EventManager
getListenersmethod should return array of initialized objects but now it returns array of strings of listener service names.