-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] Allow anonymous DefinitionDecorator resolving #15096
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
what about anonymous services used in the factory or the configurator ? |
} | ||
$argument->setArguments($this->resolveArguments($container, $argument->getArguments())); | ||
$argument->setMethodCalls($this->resolveArguments($container, $argument->getMethodCalls())); | ||
$argument->setProperties($this->resolveArguments($container, $argument->getProperties())); |
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 suggest creating a method resolveDefinition(ContainerBuilder $container, Definition $definition)
resolving everything in the definition to avoid duplication between here and the main loop (and renaming the current resolveDefinition
to resolveDefinitionDecorator
)
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 InlineServiceDefinitionsPass does not try to avoid duplicating the loop. I tried before pushing this one, by they are not the same (e.g. https://github.com/symfony/symfony/pull/15096/files#diff-d81a6a2f4968eb3c7d1511f42bdb8c79R43) even if they look close. I'd prefer keeping them separated, the code is more readable.
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.
well, there is only a small difference, which is what you are doing with the resolved definition in case of DefinitionDecorator. This could be kept in both locations.
Adding support for anonymous decorators in more places would increase the duplication even more
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.
Really, I tried, but this is increasing the cyclomatic complexity for no benefit...
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.
Well, I resorted to merging the loops :)
Please add some tests using anonymous definition decorators in all supported places, not only in the properties |
de6ad35
to
de9335b
Compare
Tests added. |
yes |
it is supported since 2.7 for definitions |
de9335b
to
9280a3e
Compare
Updated for factories and configurators |
9280a3e
to
e5763ce
Compare
Factory/configurator resolution was missing for nested definitions, both in ResolveDefinitionTemplatesPass and InlineServiceDefinitionsPass |
ping @symfony/deciders |
Thank you @nicolas-grekas. |
…tor resolving (nicolas-grekas) This PR was merged into the 2.8 branch. Discussion ---------- [DependencyInjection] Allow anonymous DefinitionDecorator resolving | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This PR allows injecting anonymous DefinitionDecorator into services' arguments/properties, such as: ```php $container->register('foo_service_name', 'FooClass') ->setProperty('bar', new DefinitionDecorator('definition_decorated_service')) ; ``` Commits ------- e5763ce [DependencyInjection] Allow anonymous DefinitionDecorator resolving
This PR allows injecting anonymous DefinitionDecorator into services' arguments/properties, such as: