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] Allow autowiring by type + parameter name#28234
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
e448897 to27b52c5Comparenicolas-grekas commentedAug 21, 2018 • 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.
Now with a new |
weaverryan left a comment
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 love this. It solves the last weird autowiring situation in an automated way: classes that have multiple services (cache, log, etc).
We'll need to updatedebug:autowiring to show this. We want to update it anyways to be a bit more "helpful" in general. We need to do that for 4.2.
| $definition->addTag('cache.pool',$pool); | ||
| $container->setDefinition($name,$definition); | ||
| $container->registerAliasForArgument($name, CacheInterface::class); | ||
| $container->registerAliasForArgument($name, CacheItemPoolInterface::class); |
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.
Will this work ok for the$name = '.'.$name.'.inner'; cases above?
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.
good catch, moved up in the foreach!
| /** | ||
| * Registers an alias for autowiring the passed id using named arguments. | ||
| */ | ||
| publicfunctionregisterAliasForArgument(string$id,string$type,string$name =null):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.
Can we doc the arguments? Because we normalize them into lower-camel case, I think it IS worth saying what each argument means
nicolas-grekasAug 21, 2018 • 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 added a docblock description, good enough?
weaverryan left a comment
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.
+1 for me. But, this makes updatingdebug:autowiring an absolute must for 4.2.
sroze left a comment
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.
That's super cool. Though, could you add a test for a service namedsome_service (which is claimed to work on the PhpDoc but tests are only usingsome.service)?
| $container->setAlias('message_bus',$busId)->setPublic(true); | ||
| $container->setAlias(MessageBusInterface::class,$busId); | ||
| }else { | ||
| $container->registerAliasForArgument($busId, MessageBusInterface::class); |
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.
Any reason why we wouldn't register every bus, including the one that has been chosen as the default? The question also works for the lock (because in cache, all of them or registered)
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.
Yes: because the default service is already the one wired... by default :)
sroze commentedAug 22, 2018
I really like it! |
nicolas-grekas commentedAug 23, 2018
done as |
…s-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[DI] Allow autowiring by type + parameter name| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR |symfony/symfony-docs#10206In#27165, we introduced the possibility to bind by type+name:```yamlbind: Psr\Log\LoggerInterface $myLogger: @monolog.logger.my_logger```But we forgot about aliases. For consistency, they could and should allow doing the same. More importantly, this will open up interesting use cases where bundles could provide default values for typed+named arguments (using the new `ContainerBuilder::registerAliasForArgument()` method). E.g:```yamlservices: Psr\Cache\CacheItemPoolInterface $appCacheForecast: @app.cache.forecast```Works also for controller actions and service subscribers (using the real service id as the key).Commits-------c0b8f53 [DI] Allow autowiring by type + parameter name
…rvices and their description (nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[FrameworkBundle] make debug:autowiring list useful services and their description| Q | A| ------------- | ---| Branch? | 4.2| Bug fix? | yes| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#27207| License | MIT| Doc PR | -This PRcloses#27207: we don't need "semantics" anymore. 4.2 has everything already to allow us to separate useful services from the other ones: any autowireable *alias* **is** a useful service!Add autowiring by type + parameter name (#28234) and the story is done: we can easily hint people about which features their bundles provide, without being polluted by for-wiring-only services.Here is a screenshot running this command(before I excluded the aliases for $cacheApp and $cacheSystem):ping@weaverryan as we drafted that together.Fixes a few issues found meanwhile.That should definitely go in 4.2.Commits-------56aab09 [FrameworkBundle] make debug:autowiring list useful services and their description
Uh oh!
There was an error while loading.Please reload this page.
In#27165, we introduced the possibility to bind by type+name:
But we forgot about aliases. For consistency, they could and should allow doing the same. More importantly, this will open up interesting use cases where bundles could provide default values for typed+named arguments (using the new
ContainerBuilder::registerAliasForArgument()method). E.g:Works also for controller actions and service subscribers (using the real service id as the key).