Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] Optimize circular collection by removing flattening#39021
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
| return$this->services['manager3'] =new \stdClass($b); | ||
| $a->listener = [0 => ($this->services['listener3'] ??$this->getListener3Service())]; | ||
| return$instance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The order of injection in this test case has changed:
manager3 is given a dependency that is missing its listener3 dep.
This is an issue to me, since these test cases are precisely about the order of the steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure to get the issue. Here are the 2 methods.
protectedfunctiongetManager3Service($lazyLoad =true) {$a =new \stdClass();$this->services['manager3'] =$instance =new \stdClass($a);$a->listener = [0 => ($this->services['listener3'] ??$this->getListener3Service())];return$instance; }protectedfunctiongetListener3Service() {$this->services['listener3'] =$instance =new \stdClass();$instance->manager = ($this->services['manager3'] ??$this->getManager3Service());return$instance; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Before the change, getManager3 returned$this->services['manager3'] = new \stdClass($b);, with $b being already configured (aka$b->listener set)
In the new method, the listener propery is setafter manager3 got its dep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
hmmm ok, I get the issue: The object ($a) were not fully built at the time it were injected in Manager's constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
And also fix the second change (because I didn't collect ALL circular references for the same node).
Fixtures are identical to previous version. I tested with the projects I used for the 2 previous issues on this.
de7ff0f to85d8004Compared3b951b to136908fComparefabpot commentedNov 9, 2020
Thank you@jderusse. |
… paths (jderusse)This PR was merged into the 4.4 branch.Discussion----------[DependencyInjection] Fix circular detection with multiple paths| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets |Fix#39056| License | MIT| Doc PR | -There are currently 2 kind of issues related to the Dependency Injection:1. performance issue when project contains many loops (#37850)Which has been fixed by#388822. Infinity loop in some case (#38970)Which has been fixed by#38980 and#39021The new issue#39056 has been introduced by#38882 (The performance issue refactor) because in order to optimize loop detection, I take a short cut and choose to not collect ALL the circular loop but only the one that mattersI was wrong. All loops matters.This PR fix my previous refacto to collect ALL the paths, with a low CPU footprintCommits-------1c3721e Fix circular detection with multiple paths
…truct loop (jderusse)This PR was squashed before being merged into the 4.4 branch.Discussion----------[DependencyInjection] Fix circular in DI with lazy + byContruct loop| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets |Fix#39120| License | MIT| Doc PR | -This fix another issue lazy service.It partially revert#38980 and#39021Initially, we trusted lazy services to be lazy and not beeing called while building the services graph=> bug#38970 when lazy deps is injected in a factory, it may be consumed directly to build the object before the graph is fully builtFixed by#38980 => lazy service are considered as "normal service"=> bug#39015 some loop are not resolvable with "normal service", but it shouldn't be an issue when servie proxifyiedFixed by#39021 => lazy service are considered as "normal service" except when proxyfied=> bug#39120 some loop are not resolvable with "normal service", but it shouldn't be an issue because the lazy service is injected in the constructor and userFixed by this PR => that revert to the initial state. lazy service are trusted.But now, The IterratorArgument injected in a factory (single exception) is not more considered as lazyCommits-------54af139 [DependencyInjection] Fix circular in DI with lazy + byContruct loop
Alternative to#39019