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] Factorize compiler passes around new AbstractRecursivePass#21327
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
| }else { | ||
| $arguments[$k] =clone$definition; | ||
| } | ||
| if ($definition->isShared()) { |
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 this be turned into a 1-liner?
$value =$definition->isShared() ?$definition :clone$definition;
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.
no. Shared definitions are returned directly, while non-shared ones are passed to the parent processing
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.
Indeed now - I changed that after@javiereguiluz's comment :)
dbdfab6 to196e9d8Compare| } | ||
| $this->lazy =false; | ||
| foreach ($container->getAliases()as$id =>$alias) { |
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 is moved before the processing of definitions instead of after. Is it expected ? And does it make any difference ?
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.
given that this pass is ready-only, that doesn't make any difference yes
| }else { | ||
| $arguments[$k] =clone$definition; | ||
| } | ||
| if ($definition->isShared()) { |
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.
no. Shared definitions are returned directly, while non-shared ones are passed to the parent processing
| $definition->setFactory($this->updateFactoryReference($replacements,$definition->getFactory())); | ||
| } | ||
| parent::process($container); | ||
| $this->replacements =array(); |
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.
should be in a finally block to ensure it is still reset on exceptions
nicolas-grekasJan 18, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 decided not, to remove one level of indentation: on exception, there would be no circular refs to cleanup - thus no need.
196e9d8 toc44ce27Compare| } | ||
| /** | ||
| * Process a value found in a definition tree. |
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.
Processes
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.
fixed
| * @param mixed $value | ||
| * @param bool $isRoot | ||
| * | ||
| * @return mixed The processed value |
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 be removed as mixed does not really help :)
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 fear that people won't notice that the return value is here at all.
| abstractclass AbstractRecursivePassimplements CompilerPassInterface | ||
| { | ||
| protected$container; | ||
| protected$currentId; |
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.
Wouldn't it be "better" to create getters for those?
nicolas-grekasJan 18, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Some passes set the value also. They are "local vars passed as properties". Cleaned after use. Just saves boilerplates this way. I'd prefer keep them asis.
c44ce27 to6acb80fCompare…rsivePass (nicolas-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[DI] Factorize compiler passes around new AbstractRecursivePass| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -This PR introduces an AbstractRecursivePass that is able to visit the definition tree recursively everywhere a Definition or a Reference can be nested.All existing compiler passes that need recursivity are updated to leverage this new class.This remove a bunch of boilerplate that was previously repeated all over.It also fixes compiler passes that eg missed looking at configurators for no reason (this "fix" is a new feature really).It then applies recursivity to AutowirePass, that does not handle it today, but should.I'm happy that the net result is a loss of 153 lines :)Commits-------6acb80f [DI] Factorize compiler passes around new AbstractRecursivePass
This PR was merged into the 3.3-dev branch.Discussion----------[DependencyInjection] Remove an unused docblock| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aThe removal of this doc block was forgotten in#21327.Commits-------9e33434 [DependencyInjection] Remove an unused docblock
Uh oh!
There was an error while loading.Please reload this page.
This PR introduces an AbstractRecursivePass that is able to visit the definition tree recursively everywhere a Definition or a Reference can be nested.
All existing compiler passes that need recursivity are updated to leverage this new class.
This remove a bunch of boilerplate that was previously repeated all over.
It also fixes compiler passes that eg missed looking at configurators for no reason (this "fix" is a new feature really).
It then applies recursivity to AutowirePass, that does not handle it today, but should.
I'm happy that the net result is a loss of 153 lines :)