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

Mark ServiceEntityRepository as lazy #1600

Closed
wants to merge 1 commit into from

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas
Copy link
Member Author

I just realized that lazy services are not compatible with final classes, so this might break some apps that aggressively make their repositories final.
That could be a reason to prefer #1599 instead.
Any opinion about the best way forward?

@ostrolucky ostrolucky changed the base branch from 2.7.x to 2.8.x December 30, 2022 09:43
@ostrolucky
Copy link
Member

Was that working before?

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Dec 30, 2022

Not really: the creation of the circular graph required double instantiations. This was mostly unnoticed but could still lead to strange situations where two different objects could be created (and passed to dependents) for the same service.

@ostrolucky ostrolucky added this to the 2.8.1 milestone Dec 30, 2022
@ostrolucky ostrolucky added the Bug label Dec 30, 2022
Copy link
Member

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a test for this?

@ostrolucky
Copy link
Member

That sounds to me like proxying final repositories before was working before, but it could result in different instances. Did I get it right? If this issue wasn't a downside of #1599, what was a downside of that solution?

@nicolas-grekas
Copy link
Member Author

Proxying final classes works only when there is an interface to rely on. That's not the case for repositories so this never worked.
The downside of #1599 is the dependency on the trait, but I suppose that's a lesser downside than forbidding final classes (I didn't spot this before opening this one.)
I'm reopening #1599

@ostrolucky
Copy link
Member

Ok let's go with #1599

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doctrine Manager not initiliazed in phpunit with php 8.2
3 participants