[DI] Fix support for multiple tags for locators and iterators

[alex-dev]

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.

[nicolas-grekas]

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 :)

[alex-dev]

• 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?

[nicolas-grekas]

It is during container compilation, not a critical path

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.

[alex-dev]

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:

1. Proxy the computation in a callable or an object;
2. Too many if statements to handle the branching;

Simplest way is likely to be an anonymous class.
It is likely to become a bigger merge nightmare than the previous refactorisation.

[alex-dev]

Restarting from nothing; Only the tests are left.

[alex-dev]

array_key_last... Should I force symfony/dependency-injection to depend of symfony/polyfill-php73 or implement an internal polyfill?

[nicolas-grekas]

use end($array); return key($array);

[nicolas-grekas]

Thank you @alex-dev.