Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] Use ID of the original service when key not specified in service_locator config#48653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedDec 14, 2022
Hey! Thanks for your PR. You are targeting branch "6.3" but it seems your PR description refers to branch "5.4". Cheers! Carsonbot |
aadc68d toafb0b40Compare| useSymfony\Component\DependencyInjection\Tests\Fixtures\TestDefinition1; | ||
| useSymfony\Component\DependencyInjection\Tests\Fixtures\TestDefinition2; | ||
| require_once__DIR__.'/../Fixtures/includes/autowiring_classes.php'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Since I was also in the middle of trying to figure this out, I know why you did this, but probably most people reading this PR will not. I explained the issuehere and@chalasr has taken adifferent approach to avoid referencing the unloaded class.
However, I think we need to take further action with respect to this bug, because this is indicative that we have tests with hidden dependencies and this particular bug has existed for almost 1.5 years undetected. Relying on the happenstance deterministic (alphabetical) ordering of tests is bad because it means tests cannot be run in isolation, which makes development harder. The best way I know to remedy this is to applyexecutionOrder="random" to thephpunit.xml config, which should be done for all such files in this repository.
Bilge left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should document the new method.
| returnnewReference($id); | ||
| } | ||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In my view, this needs documentation to explain what it is doing and why. The method name certainly gives no indication as to what is going on here.
| /** | |
| /** | |
| * Replaces numeric service IDs with service names. |
The method name could probably be improved along these lines, also, e.g.replaceNumericServiceLocatorKeys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the suggestion!
| }elseif (\is_int($k)) { | ||
| $i =null; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What is this for?
| $services[$k] =$v; | ||
| } | ||
| ksort($services); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If I knew... I extracted this code for reusing. Theksort() was removed from theregister method onc67c2df. I have no idea why it still exists inprocessValue.
Bilge commentedDec 14, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
By the way, there is no YAML fixture that actually tests the |
chalasr commentedDec 14, 2022
Please target 6.3. As discussed in#48454 this does not qualify as a bugfix but a feature as it is changing a behavior introduced 4 years ago that some people might rely on and some others probably worked around it by specifying keys explicitly.
Agreed. PR welcome to add that missing test case (on the lowest maintained branch where it's applicable, probably 5.4). |
afb0b40 to064bee1Comparechalasr commentedDec 20, 2022
Closing as the 7.0 branch does not exist yet. See#48686 |
Bilge commentedDec 20, 2022
Dude... what. Even@nicolas-grekasOK'd the 6.x release implicitly in#48454 by stating it's a new feature, whichaccording to the docs, means the target branch is 6.x.
|
chalasr commentedDec 20, 2022
#48686 implements that feature on 6.x. |
upyx commentedDec 20, 2022
upyx commentedDec 20, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@chalasr@nicolas-grekas |
wouterj commentedDec 20, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This is a feature, but we have to take care of compatibility as to not break applications currently relying on the "odd behavior". Stability of behavior and code is the number 1 priority for anything in Symfony. In this case, it is a bit tricky on how to change the behavior in a smooth way (we can't change behavior in 7.0 without a deprecation in 6.x, but we also can't have deprecations in 6.x that can't be fixed). Fortunately, 6.3 is nearly 6 months away still (and 7.0 almost a year from now), so we have all the time to figure out the best path forwards. In any case,#48686 looks like a more promising PR than this one for 6.x (giving it doesn't change behavior). Let's continue finding the best solution in that PR. |
…ce_locator" config (upyx)This PR was merged into the 6.3 branch.Discussion----------[DependencyInjection] Deprecate integer keys in "service_locator" config| Q | A| ------------- | ---| Branch? | 6.3| Bug fix? | no| New feature? | no| Deprecations? | yes| Tickets | Deprecation befor#48653| License | MIT| Doc PR |symfony/symfony-docs#17576It deprecates undefined/wrong behaviour ofhttps://symfony.com/doc/current/service_container/service_subscribers_locators.html#defining-a-service-locatorCommits-------57c2365 [DependencyInjection] Deprecate integers keys in "service_locator" config
…pyx)This PR was squashed before being merged into the 5.4 branch.Discussion----------[DependencyInjection] Remove not implemented behaviorCurrently numeric keys are used, but it will be deprecated and changed in 7.0.Deprecation:symfony/symfony#48686New behavior:symfony/symfony#48653Commits-------a8770de [DependencyInjection] Remove not implemented behavior
Uh oh!
There was an error while loading.Please reload this page.
It fixes bug/implementes feature described in#48454