NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Autowiring] Add interface FQCN as a valid service name to avoid collisions#21132

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

Jump to bottom

Closed

theofidry wants to merge3 commits intosymfony:masterfromtheofidry:bugfix/autowire-interface

Closed

[Autowiring] Add interface FQCN as a valid service name to avoid collisions#21132

theofidry wants to merge3 commits intosymfony:masterfromtheofidry:bugfix/autowire-interface

Conversation

Copy link

Contributor

theofidry commentedJan 2, 2017

Q	A
Branch?	master? (not sure)
Bug fix?	yes/no (not sure)
New feature?	yes
BC breaks?	possibly
Deprecations?	no
Tests pass?	yes
Fixed tickets	none
License	MIT
Doc PR	TODO

As of now, if you have:

interface CollisionInterface {}class Aimplements CollisionInterface {}class UseCollision{function__construct(CollisionInterface$collision) {}}

services:app.a:class:Aautowire:trueapp.b:class:ACollisionInterface:'@app.b'app.use_collision:class:UseCollisionautowire:true

It would fail, because you haveapp.a andapp.b which are both implementingCollisionInterface. However as you can see, in this scenario this is a bit stupid, as if you bother doingCollisionInterface: '@app.b', this is equivalent to (or rather should be IMO) have:

services:#...app.b:class:Aautowired_types:[ CollisionInterface ]

You might wonder what's the diff and why bother yourself with that, the diff IMO is the location of your service definitions. For example I may try to follow a more DDD oriented architecture, e.g. the hexagonal architecture, where I have:

config/  domain.yml # service definitions related to the domain layer  infrastructure.yml # service definitions related to the infrastructure layersrc/  Domain/    CollisionInterface.php  Infrastructure/    A.php    UseCollision.php

and try to have something like:

# config/domain.ymlservices:App\Domain\CollisionInterface:'@app.b'

# config/infrastructure.ymlapp.a:class:Aautowire:trueapp.b:class:Aapp.use_collision:class:UseCollisionautowire:true

I am not sure if this can be caused any BC and if this should be considered as a feature or a bugfix.

/cc@dunglas

Add interface FQCN as a valid service name to avoid collisions

93f181d

carsonbot added Status: Needs Review Bug Feature labels

Jan 2, 2017

Copy link

Contributor

GuilhemN commentedJan 2, 2017

Sounds hard to support without having a dedicated system inContainerBuilder. For example we can't support the following using the current autowiring types system as the service is not yet defined:

services:CollisionInterface:'@app.b'app.a:class:Aautowire:trueapp.b:class:Aapp.use_collision:class:UseCollisionautowire:true

nicolas-grekas added this to the3.x milestone

Jan 2, 2017

Copy link

Member

dunglas commentedJan 3, 2017

Shouldn't we deprecate theautowiring_types key to only use the `CollisionInterface: '@app.b' syntax now?

dunglas requested changes

Jan 3, 2017

View reviewed changes

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php Outdated

		privatefunctioncreateAutowiredDefinition(\ReflectionClass$typeHint,$id)
		{
		if (isset($this->ambiguousServiceTypes[$typeHint->name])) {
		if (isset($this->ambiguousServiceTypes[$typeHintName =$typeHint->name])) {

Copy link

Member

dunglasJan 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why is necessary to create the$typeHintName variable? It makes the code harder to read for no benefit over using the prop directly.

src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php Outdated

		{
		$container =newContainerBuilder();

		$container->register('c1',__NAMESPACE__.'\CollisionA');

Copy link

Member

dunglasJan 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You can useCollisionA::class for Symfony 3+. Same of lines below.

Copy link

ContributorAuthor

theofidry commentedJan 3, 2017

Shouldn't we deprecate the autowiring_types key to only use the `CollisionInterface: '@app.b' syntax now?

I'm for it, but should it be done here or another PR? Also should this be merged in master or an older branch?

Address comments

6a8306f

Copy link

ContributorAuthor

theofidry commentedJan 3, 2017•
edited
Loading

@GuilhemN I'm not sure to understand your concern, all services are registered being process that way so the order does not matter.

Copy link

Contributor

GuilhemN commentedJan 3, 2017•
edited
Loading

@theofidry oops, I didn't realize it was a PR and I was thinking you'll try to implement it otherwise... 😐

LGTM 👍 I'm also for deprecating theautowiring_types key.

theofidry commented

Jan 6, 2017

View reviewed changes

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php Outdated

		$matchingServices =implode(',',$this->ambiguousServiceTypes[$typeHint->name]);

		thrownewRuntimeException(sprintf('Unable to autowire argument of type "%s" for the service "%s". Multiple services exist for this %s (%s).',$typeHint->name,$id,$classOrInterface,$matchingServices),1);
		thrownewRuntimeException(sprintf('Unable to autowire argument of type "%s" for the service "%s". Multiple services exist for this %s (%s).',$typeHint->name,$id,$classOrInterface,$matchingServices));

Copy link

ContributorAuthor

theofidryJan 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

needs to be reverted

Revert the change to the exception

24d3ff4

Copy link

ContributorAuthor

theofidry commentedJan 6, 2017

@dunglas updated, but I'm still unsure on which branch we should merge this as if we want to deprecateautowiring_types, it would need to be merged on the oldest possible, as you cannot do withoutautowiring_types without this PR.

Copy link

Member

nicolas-grekas commentedJan 6, 2017

the deprecation part is for master for sure
so you may need to split this PR in two if a bug needs to be fixed in a lower branch

Copy link

ContributorAuthor

theofidry commentedJan 6, 2017•
edited
Loading

@nicolas-grekas I'm actually not sure if this should be considered a feature or a bug (hence my question of which branch to target). The PR doesn't need to be split, deprecatingautowiring_types is not done yet and I plan to do it in another PR.

Copy link

Member

dunglas commentedJan 6, 2017

👍

GuilhemN reviewed

Jan 9, 2017

View reviewed changes

src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

		privatefunctioncreateAutowiredDefinition(\ReflectionClass$typeHint,$id)
		{
		if (isset($this->ambiguousServiceTypes[$typeHint->name])) {
		if ($this->container->has($typeHint->name)) {