8000 [DI][Autowiring] autowiring-types & FQCN · Issue #21772 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

[DI][Autowiring] autowiring-types & FQCN #21772

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.

Sign up for GitHub

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

Jump to bottom
Closed
theofidry opened this issue Feb 27, 2017 · 8 comments
Closed

[DI][Autowiring] autowiring-types & FQCN #21772

theofidry opened this issue Feb 27, 2017 · 8 comments
Labels
Bug DependencyInjection Status: Needs Review Unconfirmed

Comments

@theofidry
Copy link
Contributor
Q A
Bug report? yes
Feature request? no
BC Break report? yes
RFC? no
Symfony version 3.3 (master)

If you have the following:

services:
  FooInterface: '@Foo1'
  Foo1:
    #...
  Foo2:
    #...
    autowiring_types: ['FooInterface']

  Bar:
    class: Bar
    autowire: true

With:

class Bar {
  function __construct(FooInterface $foo) {...}
}

Then the situation is ambiguous for Bar: should $foo points to Foo1 or Foo2.

In Symfony 3.2.1 at least, I've seen a case where $foo points to Foo2 (i.e. the autowired type) but then if you do a $container->get('FooInterface') you will have Foo1 which besides making the situation confusing makes it extremely hard to debug and detect.

I cannot afford to upgrade on that project, so maybe this case is handled differently since #21494 has been merged, but I didn't see a test for that case in master and IHMO this is a case where a confusion remains and should throw an exception right off the bat and force the user to remove the ambiguity.
@GuilhemN
Copy link
Contributor
GuilhemN commented Feb 27, 2017 •
edited
Loading

Seems hard to fix without breaking bc (unless we consider this as a bug fix?).
In any case, this will be solved in 4.0 as only aliases will be considered.

Edit: In fact if I remember well, aliases always win, so I guess it fixes the ambiguity?

@theofidry
Copy link
Contributor Author

Prior to #21494 the aliases don't work for autowiring (at least interfaces, it might work to some degree with abstract or concrete classes), you have to use autowiring_types. And deprecating the later only comes with that PR as well.

To be honest, even if there is a hard BC, it's such a confusing case it should be considered as a bug and be fixed still IMO.

@dunglas
Copy link
Member
dunglas commented Feb 28, 2017 •
edited
Loading

For me the behavior you describe isn't ambigus at all. Aliases and autowiring_types were totally different things before 3.3.
The FooInterface typehint returns @Foo2 because it's related to PHP type resolution. It has nothing to do with service identifiers and it's totally wanted.
The service identifier FooInterface (just a string, it can be anything else, and it is not related to PHP types) returns @Foo1 because it's an alias of it.

#21494 just add a new shortcut on top of this (and by the way make the syntax more concise by adding a new behavior to aliases).

IMO, there is nothing to change in 3.2-.

@dunglas
Copy link
Member
dunglas commented Feb 28, 2017 •
edited
Loading

By the way, you cannot (and it was never allowed) to do $container->get('anything') when anything isn't a service identifier. The fact that some services have the type anything doesn't matter. Only the identifier matters for get(), not the type.

A PR to add a method like getServiceOfType('FooInterface') was proposed but enventually refused. So the behavior you describe has never been supported (and is still not).
< 8000 /div>

@theofidry
Copy link
Contributor Author
theofidry commented Feb 28, 2017 •
edited
Loading

I'm not following how it's not ambiguous. After #21494, if Foo2 didn't have autowiring_types, Bar would get Foo1.

Prior to #21494, the case above simply doesn't work). And if I take the original example Foo1 is given as expected.

However #21494, we deprecate autowiring_types in favour of being to rely on a service ID (alias or not). Yet if we use both which points at different services, autowiring_types takes priority (to be confirmed, be it that way or the other way around, it doesn't matter): this is where I think it's now ambiguous.

By the way, you cannot (and it was never allowed) to do $container->get('anything') when anything isn't defined. The fact than some services have the type anything doesn't matter. Only the identifier matter for get(), not the type.

Hm not sure where I implied that, I'm doing $container->get(FooInterface::class) because the services are registered by their FQCN so I know it's their ID. I'm not mistaking it for the Laravel DI where it's "try to get me the service of that instance".

@dunglas
Copy link
Member
dunglas commented Feb 28, 2017

Yet if we use both which points at different services, autowiring_types takes priority: this is where I think it's now ambiguous.

I don't understand why. How do you think it should behave? The current behavior looks perfectly logical to me but maybe it's not someone not aware of internal mechanisms. However the situation you describe should not be frequent, in 3.3 users should only use aliases and not use autowiring_types anymore.

@theofidry
Copy link
Contributor Author

How do you think it should behave?

Clearly there is two ways to go:

  • Keep the current behaviour, in which case a test case is missing to ensure it didn't change and avoid any regressions
  • Consider the case as ambiguous and throw an exception to force the user to fix it. In the example above that translates to one fo the following:
    • Make FooInterface point to Foo2
    • Move the autowiring_type from Foo2 to Foo1
    • Remove FooInterface
    • Remove the autowiring type

The current behavior looks perfectly logical to me but maybe it's not someone not aware of internal mechanisms

Precisely. In insight, it wasn't that hard to debug: check typehints in AutowirePass. But more than debugging, unless you do an integration test along the lines of instantiating your autowired services and double checking the dependencies injected - which I don't expect anyone to do, this kind of issue will go unnoticed before noticing a weird behaviour during other integration tests if you're well covered or runtime otherwise.

The current behavior looks perfectly logical to me but maybe it's not someone not aware of internal mechanisms

I hope non existent right now, unless you jump straight to master and mix both. However when someone will upgrade to 3.3, unless they diligently remove any usage of autowiring_types in favour of aliases, they might start to mix both and this risk highly increase in that scenario.

Thinking of your comment from before:

A PR to add a method like getServiceOfType('FooInterface') was proposed but enventually refused. So the behavior you describe has never been supported (and is still not).

Admittedly this could help if you are using autowiring_types. But it wasn't trivial to implement and #21494 removes any need for it. This method is not useful unless you have autowiring_types, except when you typehint an abstract class for example and only have one service implementing it. It however would still be limited to types, maybe something making more sense would be something like getServiceForAutowiring($value) and getParameterForAutowiring(). But I think this should be discussed in a dedicated issue and not here.

@theofidry
Copy link
Contributor Author

After some discussion off, in 3.3 with the example above, $foo of Bar will point to Foo1 and not Foo2.

So two things:

  • Technically it's a BC break with autowiring_types. As prior to 3.3 aliases with interfaces were broken anyway and the usage of autowiring_types not wide-spread, this is ok.
  • The case remains ambiguous IMO, but make more sense and I think it's ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review Unconfirmed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@dunglas @javiereguiluz @theofidry @GuilhemN @carsonbot
0