Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Do not process bindings in AbstractRecursivePass#24602
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
chalasr commentedOct 18, 2017
| Q | A |
|---|---|
| Branch? | 3.4 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #24559 |
| License | MIT |
| Doc PR | n/a |
chalasr commentedOct 18, 2017
Build failure unrelated. |
dmaicher commentedOct 18, 2017
Does this mean that in the example mentioned in#24559 the inlined |
chalasr commentedOct 18, 2017
@dmaicher yes |
dmaicher commentedOct 18, 2017
👍 Should it not be merged into 3.4 instead of master? |
79c4875 to98cb64fComparestof commentedOct 18, 2017
Are you sure about it ? Bindings are used by the autowiring layer, which runsbefore the removal passes. If the autowiring has not used the binding to set a reference, we don't have a usage (and we don't need to prevent removal AFAIK). And if the binding was used to fill an argument, we will have a reference in the arguments (and so processed by the existing code) |
dmaicher commentedOct 18, 2017
@stof if the service was inlined this it not true I believe. It will have an inlined |
stof commentedOct 18, 2017
@dmaicher and this is fine as well, as the argument is still filled properly. It does not need the binding anymore. Actually, I think the issue is that AbstractRecursivePass is always processing bindings in the exact same way it processes arguments, method calls and so on. |
dmaicher commentedOct 18, 2017
@stof ok I see 😉 Then we should indeed not check the References contained in bindings I guess. So the Definitions can be removed correctly for inlined services. |
98cb64f tod86f126Compare72ffcb1 tob175419Comparechalasr commentedOct 18, 2017
PR updated to not process bindings in AbstractRecursivePass. |
| $value->setArguments($this->processValue($value->getArguments())); | ||
| $value->setProperties($this->processValue($value->getProperties())); | ||
| $value->setMethodCalls($this->processValue($value->getMethodCalls())); | ||
| $value->setBindings($this->processValue($value->getBindings())); |
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.
can we have a test case for this?for example within theRemoveUnusedDefinitionsPassTest?
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.
Sorry I mean the test for the pass that checks if services exist ;p
==>CheckExceptionOnInvalidReferenceBehaviorPassTest
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.
Test added
6b2282f to148bdecCompare| { | ||
| $container =newContainerBuilder(); | ||
| $container->register('a','stdClass'); |
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.
this test will now also pass without your change I believe? we should remove this line so the Referencea in the bindings does actually not exist on the container?
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.
oops :) fixed
148bdec to6a6256cComparefabpot commentedOct 18, 2017
Thank you@chalasr. |
…lasr)This PR was merged into the 3.4 branch.Discussion----------[DI] Do not process bindings in AbstractRecursivePass| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#24559| License | MIT| Doc PR | n/aCommits-------6a6256c Do not process bindings in AbstractRecursivePass
This PR was merged into the 3.4 branch.Discussion----------[DI] Fix cannot bind env var| Q | A| ------------- | ---| Branch? | 3.4 <!-- see comment below -->| Bug fix? | yes| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets |#24845 <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/AIn#24602 we removed the processing of bindings from the `AbstractRecursivePass`. But there is actually one case where we want a recursive pass to process them: to resolve env param placeholders.Commits-------f8f3a15 [DI] Fix cannot bind env var