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 set values of lazy arguments after inlining them#21312
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 commentedJan 16, 2017
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | not yet |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #21299 |
| License | MIT |
| Doc PR | n/a |
nicolas-grekas commentedJan 16, 2017
Missing test ;) |
5523b21 tobb0ad49Comparechalasr commentedJan 16, 2017
Test added! |
nicolas-grekas commentedJan 17, 2017
👍 |
| if (is_array($argument)) { | ||
| $arguments[$k] =$this->inlineArguments($container,$argument); | ||
| }elseif ($argumentinstanceof ArgumentInterface) { | ||
| $argument->setValues($this->inlineArguments($container,$argument->getValues())); |
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.
by removing this, we miss the opportunity to inline inside lazy arguments.
only the call to setValue should be removed in fact
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 didn't dare to ask you about that. Updated
| $this->process($container); | ||
| $values =$container->getDefinition('closure-proxy')->getArguments()[0]->getValues(); |
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.
just usegetArgument(0) here
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.
it was a quick copy-paste, fixed
| $this->assertInstanceOf(Reference::class,$values[0]); | ||
| $this->assertSame('inline', (string)$values[0]); | ||
| $values =$container->getDefinition('iterator')->getArguments()[0]->getValues(); |
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.
same here
| $arguments[$k] =$this->inlineArguments($container,$argument); | ||
| }elseif ($argumentinstanceof ArgumentInterface) { | ||
| $argument->setValues($this->inlineArguments($container,$argument->getValues())); | ||
| $this->inlineArguments($container,$argument->getValues()); |
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.
Is there any case where this could break? For example, if someone uses their own implementation ofArgumentInterface for whatever reason?
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.
To answer my own question: theArgumentInterface wasn't part of any release yet.
xabbuh commentedJan 17, 2017
👍 Status: Reviewed |
fabpot commentedJan 18, 2017
Thank you@chalasr. |
…em (chalasr)This PR was merged into the 3.3-dev branch.Discussion----------[DI] Do not set values of lazy arguments after inlining them| Q | A| ------------- | ---| Branch? | master| Bug fix? | not yet| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#21299| License | MIT| Doc PR | n/aCommits-------04da7c3 [DI] Do not inline values of lazy arguments