[DI] scope singly-implemented interfaces detection by file #33350
Conversation
|
The current logic is on purpose: it makes the aliasing local, while the attached patch makes it global. Each set of resources forms a group, and the "singly implemented" property is defined per-group only. If this matters to you, you need to group loading resources together with a glob pattern. |
|
|
I respect your opinion, but locality is not an excuse, it's a core design principle of the automation features of the DI component. |
|
I checked the doc, there is a tip note on this page about this: PR welcome to improve it! |
"Autonomous", in other words: "local" Yeah, your second comment only shows you didn't read what I wrote - I posted the same link. The green box:
It links to a blog post. |
|
Could you please open a doc issue? I agree a link to a blog post is not the best. |
|
No, sorry - green box in documentation isn't the way to fix this. |
|
That's why a doc issue would be great: to figure out the best way to document this. But anyway, things escalated pretty quickly here so better move on I agree. |
|
It doesn't matter how it is documented. Consider this abstract, simple example: Then for some reason I have this configuration (notice that this is technically allowed) services:
_defaults:
autowire: true
Foo\:
resource: './Foo/*'
Bar\:
resource: './Bar/*'Then after some development/time I decide to add this: Service\Service:
arguments:
$foo: '@Foo\FooInterface'And magically it works and the injected implementation is FooImplementation1, even though it is implied that it should work magically only if there is only one implementation, because if there is more than one it is unclear which one should be used. You are probably not inclined to see this because the most generic use case is to use one resource fo src (aka everything) and then this issue doesn't exist. But there are more advanced use cases and/or ones not utilizing whole framework (only DI component). Besides it is also not looking good in the light of this:
source: https://symfony.com/doc/current/service_container/3.3-di-changes.html |
|
There might be away: as I mentioned before, a critical design principle is that things remain local. But local to what? E.g. rules defined under The current implementation has a blurry scope (" Another issue in the implementation is the call to |
|
I'm reopening so we can consider the proposal with the precisions above. Status: needs work |
|
I addressed the issues in second commit:
Let me know if this is what you meant. |
|
Thank. I think we still need to remove aliases when a 2nd implementation is discovered when calling registerClasses more than one time (which corresponds to your example above.) A test case would help highlight the issue :) |
|
Yes, I also realized that it still needs to be removed - I will think about implementation soon. |
|
Adding aliases is now done at the end of parsing file (aka parseDefinitions).
|
|
It took some time but I managed to make it work.
Let me know if this is enough. |
src/Symfony/Component/DependencyInjection/Loader/FileLoader.php
Outdated
Show resolved
Hide resolved
|
I have added:
I encapsulated resetting logic in method because I didn't want to pass down an additional 3 arrays/parameters. |
|
I know CI checks have failed but I think it is due to reasons unrelated to these changes. |
|
As we can spot during the review, moving the scope to the file level is opening new fancy edge cases. I'm not sure this is really an improvement. But I have an alternative proposal: what about allowing Instead of: App\Adapter\:
resource: '../src/Adapter/*'
App\Port\:
resource: '../src/Port/*'you would do: App\:
resource:
- '../src/Adapter/*'
- '../src/Port/*'? |
|
Hum, my proposal wouldn't work, because the static prefix is used as the PSR-4 prefix. App\:
resource:
- '../src/{Adapter}'
- '../src/{Port}'this does already: App\:
resource: '../src/{Adapter,Port}'I'm stopping here for this idea :) |
CI checks failed, but again it doesn't seem to be relevant. |
That was on consistent with the policy that last declared rules win over previously declared ones. |
00cd673 to
11bd896
Compare
There was a problem hiding this comment.
I just pushed a change to your fork, see the logic around the new registerAliasesForSinglyImplementedInterfaces() method.
I think we're good.
Technically, this is a BC break. But it's one that one can't miss because it will be immediately visible at compile time, and it should be pretty rare.
It's a welcomed tweak to singly-implemented interfaces.
11bd896 to
c1f3970
Compare
|
Looks good to me - it better models the intention/better encapsulates logic 👍 |
|
@daniel-iwaniec could you please check how we could improve the doc in relation to this PR and send one there? |
|
Documentation pull request symfony/symfony-docs#12294 |
|
What's the state of it? Are we waiting for some release cycle, appveyor check or something else? |
|
Thank you @daniel-iwaniec. |
… (daniel-iwaniec, nicolas-grekas)
This PR was merged into the 4.4 branch.
Discussion
----------
[DI] scope singly-implemented interfaces detection by file
| Q | A
| ------------- | ---
| Branch? | 4.4
| Bug fix? | yes
| New feature? | no
| BC breaks? | yes
| Deprecations? | no
| Tests pass? | yes
| License | MIT
[DependencyInjection] fixed handling singly implemented interfaces when importing multiple resources
for example:
```yaml
App\Adapter\:
resource: '../src/Adapter/*'
App\Port\:
resource: '../src/Port/*'
```
this configuration wont create service for interface (in other words singly implemented interface wont be autowired) and this chage fixes it
**Also** this will prevent false positives - for example if I had one implementation in \App\Port namespace and another in \App\Adapter then interface service would still be registered
but that could potentially break exisitng code not aware of this bug
Commits
-------
c1f3970 [DI] add FileLoader::registerAliasesForSinglyImplementedInterfaces()
bec3890 [DI] scope singly-implemented interfaces detection by file
…(daniel-iwaniec) This PR was merged into the 4.4 branch. Discussion ---------- scope singly-implemented interfaces detection by file Documentation change for symfony/symfony#33350 This is probably the only place where automatically registering singly implemented interfaces is referenced. Commits ------- ae343bc scope singly-implemented interfaces detection by file
[DependencyInjection] fixed handling singly implemented interfaces when importing multiple resources
for example:
this configuration wont create service for interface (in other words singly implemented interface wont be autowired) and this chage fixes it
Also this will prevent false positives - for example if I had one implementation in \App\Port namespace and another in \App\Adapter then interface service would still be registered
but that could potentially break exisitng code not aware of this bug