[DependencyInjection] Overriding services autowired by name under _defaults bind not working

[nico-incubiq]

Symfony version(s) affected: 4.1.4

Description
Autowiring allows to bind certain services by name in the _defaults section, under bind in services.yaml. As follow:

services:
    _defaults:
        autowire: true
        bind:
            $isDebug: false

It is common to override base service definitions in the services_test.yaml file for example. So I would have expected to be able to override these default bind too. Like:

services:
    _defaults:
        autowire: true
        bind:
            $isDebug: true

But if you do so, then the container binding pass will throw the following exception: Unused binding "$isDebug" in service "App\Tests\Service\MySuperServiceMock"..

Possible Solution
I tracked it down and this happens because the two definitions of $isDebug are considered as distinct bindings. Consequently ResolveBindingPass::processValue() will bind the first definition, and the second one remains unbound.
The solution would be to make the second definition override the first one earlier on in the process, so there is only one binding when getting to ResolveBindingPass.

Workaround
My current workaround is to only define the binding in the main service file and make it point to a service alias; so that services_test.yaml can override that alias. As follow:

services:
    _defaults:
        autowire: true
        bind:
            $superService: '@app.service.super_service'

    app.services.super_service:
        alias: 'App/Service/SuperService'

[GuilhemN]

Could you share with us more significant parts of your config files please?

Are you trying to override bindings with another config file? Or are you trying to override a service with a different binding?

At least, from what I understand now I think the solution is to be found on your side: we can't make bindings usage detection much more efficient, it is here to help the user detect mistakes and is coded to minimize false positive.

[stof]

defaults are per-file. So any default defined in services_test.yaml will only affect services defined in services_test.yaml.

If what you want is to change the debug flag injected in your normal services, a solution is to use a DI parameter in your binding, and changing the value of the parameter in test (parameters are global in the container):

# services.yaml
parameters:
    app.debug: false

services:
    _defaults:
        autowire: true
        bind:
            $isDebug: '%app.debug%'

# services_test.yaml
parameters:
    app.debug: true

[nico-incubiq]

Thanks @stof that's indeed the solution I found for the scalar $isDebug. And I also used service aliases as the actual code binds services too. @GuilhemN thanks for your reply too, i'll paste a little more below.
I think I misunderstood the "per-file" bit; for me it meant that services in that file would use the default defined here, but it didn't mean I couldn't override it in a different file, as it is custom to do with everything else in these configuration files.

So to confirm with you this is the expected behavior; the following situations are currently not valid:

• Explicitly defining an argument already defined as default bind

# services.yaml
services:
    _defaults:
        autowire: true
        bind:
            # This is only used by service '@App\Service\Grocery', so it's not going to be used anymore in the test environment
            $apiClient: '@eigth_points_guzzle.client.api'

    App\
        resource: '../src/*'
        exclude: '../src/{Entity,Migrations,Tests,Kernel.php}'

# services_test.yaml
services:
    App\Service\Grocery:
        arguments:
            $apiClient: '@test.groceries.api_client.mock'

    test.groceries.api_client.mock:
        class: Test\Mock\Service\GroceryMock

• Overriding a default bind when extending the configuration

# services.yaml
services:
    _defaults:
        autowire: true
        bind:
            $apiClient: '@eigth_points_guzzle.client.api'

# services_test.yaml
services:
    _defaults:
        autowire: true
        bind:
            $apiClient: '@test.groceries.api_client.mock'

[xabbuh]

Your second example indeed is not supposed to work. In your first example I may misunderstand what you mean is not working here, but if I don't miss anything the App\Service\Grocery service should be built as expected as you override the definition coming from the services.yaml file here.

[nico-incubiq]

That example is working fine on production environment, but not in test as it complains about the $apiClient default bind not being used (it was only autowiring to the Grocery service in production) @xabbuh. Tell me if you need something else to help you reproduce the issue.

[oliverde8]

@nico-incubiq That is not the way default works. Default only works in one file.

To do what you wish you can simply override the service with another one :

services: 
    eigth_points_guzzle.client.api: '@test.groceries.api_client.mock'

You can create aliases for the service if you need to do it for your own bundle only.

[nico-incubiq]

Yeah sure @oliverde8 the initial configurations I tried were not refined or anything. I'd totally be okay with just updating the documentation showing how it needs to be done and saying that all other configuration I proposed is unsupported.

But at the moment I still think scenario number 1 should work, there's nothing in the documentation saying it's illegal to override in a different file a service that's originally been using a _default.

• Pushing it a little further, I think example 3 below should be valid too, as the _default bind would definitely be used once in each file and then the service in the test env should override the one in prod env. But this throws the same error Unused binding "$apiClient" in service ".abstract.instanceof.App\Service\Grocery".

# services.yaml
services:
    _defaults:
        autowire: true
        bind:
            $apiClient: '@eigth_points_guzzle.client.api'

    App\Service\Grocery:
        # This gets argument $apiClient

# services_test.yaml
services:
    _defaults:
        autowire: true
        bind:
            $apiClient: '@test.groceries.api_client.mock'

    App\Service\Grocery:
        # This gets argument $apiClient

[stof]

@nico-incubiq you are not overriding a default bind. You are defining a different default bind for the other file. But that one will apply only to services defined in that different file.

File-level defaults cannot be overwritten per env (that's why the solution is to make file-level default use some other global features which can be overridden in other files)

[nicolas-grekas]

Do we agree it would be better to not throw this exception? I feel like so, isn't it?
I don't know how/if we should fix it, but maybe @nico-incubiq you'd like to create a PR with a test case?

[stof]

I think the service override makes the service from the first file disappear from the container before the time where bindings are checked (and so the binding of the first file is not used anymore).

@nicolas-grekas triggering an error for unused file-level bindings might indeed be good for DX when it is actually unused, but I think we have too much false positives. It might indeed be better to remove this error.

[nico-incubiq]

Your explanation about service override applying before the bind checks seems very coherent @stof , could that be a bug here?
I would happily try to help with the tests @nicolas-grekas if we agree on the behaviour.

At least the fact it triggers the error made me realise the configuration was not being applied as I expected.

[GuilhemN]

@nicolas-grekas triggering an error for unused file-level bindings might indeed be good for DX when it is actually unused, but I think we have too much false positives. It might indeed be better to remove this error.

Imo it does more good than harm but that's subjective :)
Whatever you decide I don't think we should make this error more accurate, the only way I see is remembering all removed services containing bindings and checking if they're not using them, but that's very risky, the definition may have changed or be broken so, most likely not something we want to do.

Your explanation about service override applying before the bind checks seems very coherent, could that be a bug here?

That's not a bug, it's just that services are autowired at the very end of the container construction to make sure they're complete.

[nicolas-grekas]

I agree this exception is nice, I was just wondering if we could change the behavior in this very specific context, if possible yes. @stof I'm not sure what you mean with "we have too much false positives". If you're thinking about more, we should find a way to be explicit. Like @GuilhemN, I tend to think catching errors (typos, etc.) is a good thing here by default.

[GuilhemN]

@nicolas-grekas we could also mark bindings of services removed as used (without actually checking) but that could potentially create a lot of false positives.

[stof]

@GuilhemN how would you detect them ? Removed service definitions don't exist anymore, so bindings would not be used by compiler passes in them (the autowiring won't run for removed definitions)

[GuilhemN]

In the ContainerBuilder::removeDefinition method and when overriding a service we could keep a list of bindings those services used (bindings have a unique id, shared for defaults), that would then be passed to the bindings compiler pass in order to ignore them if they're not used elsewhere.

[stof]

@GuilhemN but until autowiring is performed, a service does not use any binding. It has a list of bindings it can use. If the only fact of being able to use a binding (even if you don't actually need it) marks it as used, this error would be useless, as it would only happen if you define a default binding in a file which does not declare any service at all.

[GuilhemN]

@stof only bindings from removed services would not longer be checked (and I don't think services are often removed before bindings are processed, so it shouldn't happen that often), most bindings would be checked as they are now but indeed my proposal isn't perfect and if you remove one service in a file, all its defaults bindings will no longer be checked but I believe it's better than just removing the error while still fixing this issue.

[nicolas-grekas]

Looks like we an idea worth trying IMHO!

[GuilhemN]

I won't have time to do it myself sorry, but I'd be pleased to answer any question about my proposal if someone wants to give it a try :)

[gonzalovilaseca]

@GuilhemN I can give it a try, it would be much easier if you can provide failing tests :-), if you can't, no problem, I'll try to do them.

[GuilhemN]

@gonzalovilaseca if you take these two files, an unexpected exception will be thrown:

# /src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/defaults_bindings.yml
services:
    _defaults:
        bind: { $quz: value }
    
    bar:
        class: Symfony\Component\DependencyInjection\Tests\Fixtures\Bar

    foo:
        class: Symfony\Component\DependencyInjection\Tests\Fixtures\FactoryDummy

# /src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/defaults_bindings2.yml
services:
    _defaults:
        bind: { $quz: ~ }

    bar:
        class: Symfony\Component\DependencyInjection\Tests\Fixtures\Bar

# /src/Symfony/Component/DependencyInjection/Tests/Loader/YamlFileLoaderTest.php
class .. {
    public function testOverriddenDefaultsBindings()
    {
        $container = new ContainerBuilder();
        $loader = new YamlFileLoader($container, new FileLocator(self::$fixturesPath.'/yaml'));
        $loader->load('defaults_bindings.yml');
        $loader->load('defaults_bindings2.yml');
    }

Thanks for giving it a try :)

[nicolas-grekas]

implemented in #29597 if you want to have a look.

[nicolas-grekas]

#29597 has been reverted due to BC breaks it introduced and also to the fact it didn't fix the issue well enough (see last comments there). Help wanted.

[przemyslaw-bogusz]

I will gladly give it a shot. I will make a PR once the reverting PR is merged (#29853). I see it failed the coding standards check, due to the new short array syntax rule.