[DependencyInjection] Overriding services autowired by name under _defaults bind not working #28326
Comments
|
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. |
|
defaults are per-file. So any default defined in 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 |
|
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. So to confirm with you this is the expected behavior; the following situations are currently not valid:
# 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
# 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' |
|
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 |
|
That example is working fine on production environment, but not in test as it complains about the |
|
@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. |
|
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.
# 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 |
|
@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) |
|
Do we agree it would be better to not throw this exception? I feel like so, isn't it? |
|
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. |
|
Your explanation about service override applying before the bind checks seems very coherent @stof , could that be a bug here? At least the fact it triggers the error made me realise the configuration was not being applied as I expected. |
Imo it does more good than harm but that's subjective :)
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. |
|
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. |
|
@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. |
|
@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) |
|
In the |
|
@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. |
|
@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. |
|
Looks like we an idea worth trying IMHO! |
|
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 :) |
|
@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. |
|
@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 :) |
|
implemented in #29597 if you want to have a look. |
…d (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [DI] fix reporting bindings on overriden services as unused | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #28326 | License | MIT | Doc PR | - Commits ------- e07ad2b [DI] fix reporting bindings on overriden services as unused
|
#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. |
|
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. |
…er _defaults bind not working
…er _defaults bind not working
…er _defaults bind not working
…er _defaults bind not working
… bind not working (przemyslaw-bogusz, renanbr) This PR was merged into the 3.4 branch. Discussion ---------- [DI] Overriding services autowired by name under _defaults bind not working | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #28326 | License | MIT This is an implementation of ideas and suggestions of @nicolas-grekas and @GuilhemN. Commits ------- 7e805ea more tests 35a40ac [DI] Fixes: #28326 - Overriding services autowired by name under _defaults bind not working
* 3.4: Skip testing the phpunit-bridge on not-master branches when $deps is empty more tests [DI] Fixes: #28326 - Overriding services autowired by name under _defaults bind not working
* 4.2: Skip testing the phpunit-bridge on not-master branches when $deps is empty more tests [DI] Fixes: #28326 - Overriding services autowired by name under _defaults bind not working [DI] fix removing non-shared definition while inlining them
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:
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:
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
$isDebugare considered as distinct bindings. ConsequentlyResolveBindingPass::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:
The text was updated successfully, but these errors were encountered: