[DI] Factorize compiler passes around new AbstractRecursivePass #21327
Conversation
| } else { | ||
| $arguments[$k] = clone $definition; | ||
| } | ||
| if ($definition->isShared()) { |
There was a problem hiding this comment.
Can this be turned into a 1-liner?
$value = $definition->isShared() ? $definition : clone $definition;
There was a problem hiding this comment.
no. Shared definitions are returned directly, while non-shared ones are passed to the parent processing
There was a problem hiding this comment.
Indeed now - I changed that after @javiereguiluz's comment :)
nicolas-grekas
force-pushed
the
di-recursive
branch
from
dbdfab6
to
196e9d8
Compare
| } | ||
| } | ||
| } | ||
| $this->lazy = false; | ||
|
|
||
| foreach ($container->getAliases() as $id => $alias) { |
There was a problem hiding this comment.
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.
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.
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.
should be in a finally block to ensure it is still reset on exceptions
There was a problem hiding this comment.
I decided not, to remove one level of indentation: on exception, there would be no circular refs to cleanup - thus no need.
nicolas-grekas
force-pushed
the
di-recursive
branch
from
196e9d8
to
c44ce27
Compare
| } | ||
|
|
||
| /** | ||
| * Process a value found in a definition tree. |
| * @param mixed $value | ||
| * @param bool $isRoot | ||
| * | ||
| * @return mixed The processed value |
There was a problem hiding this comment.
Can be removed as mixed does not really help :)
There was a problem hiding this comment.
I fear that people won't notice that the return value is here at all.
| abstract class AbstractRecursivePass implements CompilerPassInterface | ||
| { | ||
| protected $container; | ||
| protected $currentId; |
There was a problem hiding this comment.
Wouldn't it be "better" to create getters for those?
There was a problem hiding this comment.
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.
nicolas-grekas
force-pushed
the
di-recursive
branch
from
c44ce27
to
6acb80f
Compare
…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/a The removal of this doc block was forgotten in #21327. Commits ------- 9e33434 [DependencyInjection] Remove an unused docblock
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 :)