-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] PhpDumper does not share single-use dependencies when a lazy service is cloned #59706
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
Comments
Up for a PR that'd add a failing test case as a start? |
pvandommelen
added a commit
to pvandommelen/symfony
that referenced
this issue
Feb 7, 2025
…encies by making the inlining behaviour more conservative Fixes symfony#59706
pvandommelen
added a commit
to pvandommelen/symfony
that referenced
this issue
Feb 7, 2025
…encies by making the inlining behaviour more conservative Fixes symfony#59706
pvandommelen
added a commit
to pvandommelen/symfony
that referenced
this issue
Feb 7, 2025
…encies by making the inlining behaviour more conservative Fixes symfony#59706
pvandommelen
added a commit
to pvandommelen/symfony
that referenced
this issue
Feb 7, 2025
…encies by making the inlining behaviour more conservative Fixes symfony#59706
pvandommelen
added a commit
to pvandommelen/symfony
that referenced
this issue
Feb 7, 2025
…encies by making the inlining behaviour more conservative Fixes symfony#59706
nicolas-grekas
added a commit
that referenced
this issue
Feb 10, 2025
… their dependencies when dumped with PhpDumper (pvandommelen) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [DependencyInjection] Fix cloned lazy services not sharing their dependencies when dumped with PhpDumper | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #59706 | License | MIT Fixes referenced services not being shared when lazy services are cloned before being initialized. Adds tests for both the ghost and proxy scenario's. This makes the inlining behaviour more conservative, which impacts the outputs of some other test cases. First commit adds a test, second commit has the fix. Third commit also considers the proxy case, which is also affected. I can squash if necessary. Targeted 6.4, but this also applies to 7.x. Commits ------- a60cff5 [DependencyInjection] Fix cloned lazy services not sharing their dependencies when dumped with PhpDumper
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Symfony version(s) affected
7.x
Description
PhpDumper inlines dependencies when they are private, used only once and non-lazy. When
clone
-ing a lazy service using one of these dependencies, this check is not sufficient and the dependency is constructed twice with the cloned services not sharing the dependency.This is a regression compared to the old method through symfony/proxy-manager-bridge available in 6.x. Using the container directly without using PhpDumper also correctly shares the dependency.
The blog post introducing the new implementation (https://symfony.com/blog/revisiting-lazy-loading-proxies-in-php) explicitly calls out support for clone and wither methods, so I believe the current behaviour where the dependency is not shared is a bug.
How to reproduce
The following script showcases the issue. The last two asserts fail. The asserts pass when the compiled container is used directly without using PhpDumper or when using 6.x with symfony/proxy-manager-bridge.
Possible Solution
Ideally I would expect the service to be constructed when the object is cloned. I don't think PHP offers any method to hook into this process before the clone occurs (
__clone
is only called afterwards) and afterwards is too late.An alternative approach is to make the inlining behaviour more conservative. Currently the assignment to
singleUsePrivateIds
throughisSingleUsePrivateNode()
in thePhpDumper
controls the inlining of service factories. This already checks a lazy flag which considers the target service.I'm unsure what the best solution would be. The check could be expanded to also check incoming edges. Another approach would be to mark the edge as lazy if the source definition is lazy in
AnalyseServiceReferencesPass
instead of only checking the target (possible through reordering when$this->lazy
is reset).Additional Context
No response
The text was updated successfully, but these errors were encountered: