[DI] Fix support for multiple tags for locators and iterators #35342
Conversation
There was a problem hiding this comment.
Hello, thank you for working on this!
I didn't review the logic in details but I think there are some flaws in the current patch that need to be addressed:
- the diff in the PR contains unneeded changes - eg variable declaration reordering. As any unneeded diff is a potential merge conflict when we'll merge changes from 3.4/4.3, we try to avoid them as much as possible.
- because the patch is so big, reviewing is extremely difficult. A PR should do one and only one change. Here, there are two of them: refacto + bug fix. This increases the risk of introducing bugs and the risk of missing the regressions during the review. I'd kindly ask you to rewrite your patch to stick to fixing the logic only.
- Technically, I found a regression and stopped there my review:
$container->getReflectionClass()is now always called inside the foreach loop. The previous logic avoided doing so on purpose when not needed. This behavior should be kept.
Thanks again, I'm looking forward to the next iteration :)
src/Symfony/Component/DependencyInjection/Tests/Compiler/IntegrationTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
- Technically, I found a regression and stopped there my review:
$container->getReflectionClass()is now always called inside the foreach loop. The previous logic avoided doing so on purpose when not needed. This behavior should be kept.
Why should it? It is during container compilation, not a critical path. And the code end up being much easier to follow. Rather than being a mess, everything related to default happens before tag processing, which then becomes repeatable easily.
I'm gonna work a slimmer changeset, but the logic is so tied around the declarations I fear it would complicate the flow even more. Would it be better to split the refacto in a PR to merge and review before this one?
src/Symfony/Component/DependencyInjection/Tests/Compiler/IntegrationTest.php
Outdated
Show resolved
Hide resolved
Actually, it is: the developer experience is directly affected by the compilation time, and making it faster improves the DX. That's why we strive to have the most performant code here also. |
The only applicable ways I know to delay computation are:
Simplest way is likely to be an anonymous class. |
|
Restarting from nothing; Only the tests are left. |
|
array_key_last... Should I force symfony/dependency-injection to depend of symfony/polyfill-php73 or implement an internal polyfill? |
|
use |
|
Thank you @alex-dev. |
…ors (Alexandre Parent) This PR was merged into the 4.4 branch. Discussion ---------- [DI] Fix support for multiple tags for locators and iterators | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Fix #34462, Fix #35326 | License | MIT | Doc PR | none Fix PriorityTaggedServiceTrait::findAndSortTaggedServices to work with multiple explicitely tagged services as would be expected by !tagged_locator. Also reorganize PriorityTaggedServiceTrait::findAndSortTaggedServices to be simpler and easier to understand. Commits ------- 6fc91eb [DI] Fix support for multiple tags for locators and iterators
Fix PriorityTaggedServiceTrait::findAndSortTaggedServices to work with multiple explicitely tagged services as would be expected by !tagged_locator. Also reorganize PriorityTaggedServiceTrait::findAndSortTaggedServices to be simpler and easier to understand.