Skip to content
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

[DI] Allow autowiring by type + parameter name #28234

Merged
merged 1 commit into from Aug 24, 2018

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Aug 20, 2018

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#10206

In #27165, we introduced the possibility to bind by type+name:

bind:
    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:

services:
    Psr\Cache\CacheItemPoolInterface $appCacheForecast: @app.cache.forecast

Works also for controller actions and service subscribers (using the real service id as the key).

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Aug 21, 2018

Now with a new ContainerBuilder::registerAliasForArgument() method used in FrameworkExtension to create such aliases for cache pool, lock resources and messenger buses.
E.g. type hint public function __construct(MessageBusInterface $commandBus) to get the bus declared as "command_bus" in your config.

Copy link
Member

@weaverryan weaverryan left a 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 update debug: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.

@@ -1613,6 +1623,8 @@ private function registerCacheConfiguration(array $config, ContainerBuilder $con

$definition->addTag('cache.pool', $pool);
$container->setDefinition($name, $definition);
$container->registerAliasForArgument($name, CacheInterface::class);
$container->registerAliasForArgument($name, CacheItemPoolInterface::class);
Copy link
Member

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?

Copy link
Member Author

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.
*/
public function registerAliasForArgument(string $id, string $type, string $name = null): Alias
Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member

@weaverryan weaverryan left a 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 updating debug:autowiring an absolute must for 4.2.

Copy link
Contributor

@sroze sroze left a 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 named some_service (which is claimed to work on the PhpDoc but tests are only using some.service)?

@@ -1509,6 +1517,8 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder
if ($busId === $config['default_bus']) {
$container->setAlias('message_bus', $busId)->setPublic(true);
$container->setAlias(MessageBusInterface::class, $busId);
} else {
$container->registerAliasForArgument($busId, MessageBusInterface::class);
Copy link
Contributor

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)

Copy link
Member Author

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
Copy link
Contributor

sroze commented Aug 22, 2018

I really like it!

@nicolas-grekas
Copy link
Member Author

add a test for a service named some_service

done as ContainerBuilderTest::testRegisterAliasForArgument()

@nicolas-grekas nicolas-grekas merged commit c0b8f53 into symfony:master Aug 24, 2018
nicolas-grekas added a commit that referenced this pull request Aug 24, 2018
…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#10206

In #27165, we introduced the possibility to bind by type+name:
```yaml
bind:
    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:
```yaml
services:
    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
@nicolas-grekas nicolas-grekas deleted the di-arg-alias branch August 24, 2018 13:46
nicolas-grekas added a commit that referenced this pull request Oct 29, 2018
…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 PR closes #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):
![image 3](https://user-images.githubusercontent.com/243674/47437006-f41f4380-d7a7-11e8-9f76-7f23e9193ce8.png)

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
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
This was referenced Nov 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants