Skip to content
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

[Doctrine] Respect parent class contract in ContainerAwareEventManager #31335

Merged
merged 1 commit into from May 18, 2019

Conversation

@Koc
Copy link
Contributor

commented Apr 30, 2019

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.

@Koc Koc force-pushed the Koc:bugfix/respect-parent-class-contract branch from a5076ce to 9d45fac Apr 30, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Doesn't this change break some desired laziness?

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone May 1, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 1, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@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

This comment has been minimized.

Copy link
Member

commented May 6, 2019

ping @dmaicher then for review :)

@Koc Koc force-pushed the Koc:bugfix/respect-parent-class-contract branch from 9d45fac to 9febd51 May 6, 2019

@Koc Koc requested review from dunglas, lyrixx, sroze and xabbuh as code owners May 6, 2019

@Koc Koc force-pushed the Koc:bugfix/respect-parent-class-contract branch 2 times, most recently from 182be5b to f16e9a5 May 6, 2019

@Koc

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

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

@malarzm

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

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

@dmaicher

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

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 Koc force-pushed the Koc:bugfix/respect-parent-class-contract branch from 43205af to 28df79d May 7, 2019

@Koc Koc force-pushed the Koc:bugfix/respect-parent-class-contract branch from 28df79d to 31f5564 May 9, 2019

@Koc

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@nicolas-grekas comments fixed, Travis is happy

@Koc Koc force-pushed the Koc:bugfix/respect-parent-class-contract branch from 31f5564 to 42d6272 May 15, 2019

@fabpot

fabpot approved these changes May 18, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented May 18, 2019

Thank you @Koc.

@fabpot fabpot merged commit 42d6272 into symfony:3.4 May 18, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request May 18, 2019

bug #31335 [Doctrine] Respect parent class contract in ContainerAware…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.