Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged

Conversation

@Koc
Copy link
Contributor

@KocKoc commentedApr 30, 2019
edited
Loading

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

According to method signature oforiginal EventManagergetListeners method should return array of initialized objects but now it returns array of strings of listener service names.

@nicolas-grekas
Copy link
Member

Doesn't this change break some desired laziness?

@nicolas-grekasnicolas-grekas added this to the3.4 milestoneMay 1, 2019
@nicolas-grekas
Copy link
Member

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
Copy link
ContributorAuthor

Koc commentedMay 1, 2019
edited
Loading

I've found this problem when try to inject Serializer service into custom DBAL Type usingthis article . I was surprised thatgetListeners('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 callgetListeners without any argument

dmaicher reacted with thumbs up emoji

@malarzm
Copy link
Contributor

malarzm commentedMay 6, 2019
edited
Loading

@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 returnobject[]|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
Copy link
Member

ping@dmaicher then for review :)

dmaicher reacted with thumbs up emoji

@KocKocforce-pushed thebugfix/respect-parent-class-contract branch from9d45fac to9febd51CompareMay 6, 2019 19:31
@KocKocforce-pushed thebugfix/respect-parent-class-contract branch 2 times, most recently from182be5b tof16e9a5CompareMay 6, 2019 19:49
@Koc
Copy link
ContributorAuthor

Koc commentedMay 6, 2019

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

@malarzm
Copy link
Contributor

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

dmaicher reacted with thumbs up emoji

@dmaicher
Copy link
Contributor

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.getListeners() is never called anywhere. So it would only break laziness for people calling it or using bundles/libraries that are calling this somewhere.

@KocKocforce-pushed thebugfix/respect-parent-class-contract branch from43205af to28df79dCompareMay 7, 2019 19:49
@KocKocforce-pushed thebugfix/respect-parent-class-contract branch from28df79d to31f5564CompareMay 9, 2019 12:29
@Koc
Copy link
ContributorAuthor

Koc commentedMay 9, 2019

@nicolas-grekas comments fixed, Travis is happy

@KocKocforce-pushed thebugfix/respect-parent-class-contract branch from31f5564 to42d6272CompareMay 15, 2019 18:39
@fabpot
Copy link
Member

Thank you@Koc.

@fabpotfabpot merged commit42d6272 intosymfony:3.4May 18, 2019
fabpot added a commit that referenced this pull requestMay 18, 2019
…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
This was referencedMay 22, 2019
@KocKoc deleted the bugfix/respect-parent-class-contract branchMay 29, 2019 19:14
@yaroslavbr
Copy link

yaroslavbr commentedDec 6, 2019
edited
Loading

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 foraddEventListener andremoveEventListener so hash was build based on service name and prefix. NowgetListeners 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?

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@dunglasdunglasAwaiting requested review from dunglas

@lyrixxlyrixxAwaiting requested review from lyrixx

@srozesrozeAwaiting requested review from sroze

@xabbuhxabbuhAwaiting requested review from xabbuh

+2 more reviewers

@dmaicherdmaicherdmaicher approved these changes

@malarzmmalarzmmalarzm left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

7 participants

@Koc@nicolas-grekas@malarzm@dmaicher@fabpot@yaroslavbr@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp