8000 !service_locator does not use ID of the original service when key not specified · Issue #48454 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

!service_locator does not use ID of the original service when key not specified #48454

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
Bilge opened this issue Dec 3, 2022 · 12 comments
Closed

!service_locator does not use ID of the original service when key not specified #48454

Bilge opened this issue Dec 3, 2022 · 12 comments
Labels
Feature Help wanted Issues and PRs which are looking for volunteers to complete them. Status: Needs Review

Comments

@Bilge
Copy link
Contributor
Bilge commented Dec 3, 2022

Symfony version(s) affected

5.4.16

Description

Everything written in Defining a Service Locator is true and accurate, but what is problematic is what's not written. The astute reader will notice we are given two examples for defining a service locator manually using Symfony\Component\DependencyInjection\ServiceLocator: one with service keys and one without, but there is only one example given using the !service_locator shortcut.

The comment for the example not using keys states: if the element has no key, the ID of the original service is used. This is all good, true and correct. So what happens when we do the same using the !service_locator shortcut? Surely the same behaviour, right? Wrong! When we specify a service locator using !service_locator and no keys, for which there is no example present in the documentation, the services are instead keyed using a contiguous integer sequence starting at zero, which suffice it to say, is completely useless.

How to reproduce

# config/services.yaml
services:
    App\CommandBus:
        arguments:
          - !service_locator
              - '@app.command_handler.foo'
              - '@app.command_handler.bar'

Possible Solution

Use the service ID as the key name, just as with the manual service locator definition pattern.

Additional Context

No response
@Bilge Bilge added the Bug label Dec 3, 2022
@chalasr
Copy link
Member
chalasr commented Dec 3, 2022 •
edited
Loading

👍 I agree. We will need a BC layer though as people probably rely on these numeric keys now. That's a behavior change, I'm re-labelling as feature.

@chalasr chalasr added Feature Help wanted Issues and PRs which are looking for volunteers to complete them. and removed Bug labels Dec 3, 2022
@Bilge
Copy link
Contributor Author
Bilge commented Dec 3, 2022

@chalasr Can you help me understand how this change would be introduced with this compatibility layer?

@chalasr
Copy link
Member
chalasr commented Dec 3, 2022

I don't have the code in mind right now, but I guess we have no other choice than triggering a deprecation notice at the file loader level when keys aren't explicitly set, hinting that it will default to the service id in the next major.
The notice should then just be removed in the next major and service locators be keyed by service id by default.

@Bilge
Copy link
Contributor Author
Bilge commented Dec 3, 2022

So which is the earliest version this fix can go in? I was hoping it could be merged into 5, but from the way you're talking, it can't even be merged into 6.

@chalasr
Copy link
Member
chalasr commented Dec 3, 2022

Indeed, breaking code that use numeric keys in a bugfix release is not doable IMHO

@upyx
Copy link
Contributor
upyx commented Dec 9, 2022

Oh, it's not a bug it's a feature!

@Bilge
Copy link
Contributor Author
Bilge commented Dec 9, 2022 •
edited
Loading

@upyx Would you care to elaborate or are you just trolling?

Aside, I believe this actually was correctly tagged as a bug. What is a bug? I define a bug as actual behaviour that is a departure from the expected behaviour. In this particular case, it's hard to even know what the expected behaviour should be since it isn't documented, but if we extrapolate (as I have done previously) that !service_locator is merely a shorthand for the long-form version of the same, then the expected behaviour is unequivocally that service keys should match service IDs. In that case, this is a bug, Q.E.D.

Moreover, it would be very foolish and difficult to be reliant upon this quirky and undocumented behaviour of chronologically numbering services. The only practical case where this could be useful is if the locator only has one service and you access it by ID 0 (although a locator that is known to only ever contain one service is not actually useful at all), or if you're willing to go one step even more foolish and start assuming that the order you add services to the locator is deterministic and can be relied upon, now and in the future, by accessing multiple services using these numeric indices corresponding to insertion order. Whilst I would like to hope there is nobody doing this, technically it is a BC break and we cannot make that assumption, but for all practical purposes we should expect the surface area to be minimal (notwithstanding anyone who did this deserves their code to break). For this reason I opine we should, at the very least, target v6 with a fix for this.

@nicolas-grekas
Copy link
Member

Up for sending a PR @Bilge?

@Bilge
Copy link
Contributor Author
Bilge commented Dec 14, 2022 •
edited
Loading

@nicolas-grekas Potentially, but can you resolve this discussion as to which version we would be targeting for such a PR?

@nicolas-grekas
Copy link
Member

This looks like a new feature to me.

I get the current behavior might be unexpected, but IMHO, what was not expected is not giving keys to the listed items :)

@upyx
Copy link
Contributor
upyx commented Dec 14, 2022 •
edited
Loading

@upyx Would you care to elaborate or are you just trolling?

Both. It's called irony.

... In this particular case, it's hard to even know what the expected behaviour should be since it isn't documented

So the behaviour is undefined. But you are right about heuristic expectations.

Moreover, it would be very foolish and difficult ...

Unfortunately, that's exactly what people do most of the time...

Since it's marked as "Help wanted", I investigated how it works deeply. And it works the same independently on config type (yaml, xml, php): numeric keys are used. But the documentation for .xml and .php says: "if the element has no key, the ID of the original service is used". Hence, it's definitely a bug. More interestingly, I've tested 6.2, 5.4, 4.4 and 3.4 versions, and this never seems to have worked. So it looks like a bug in documentation.

@nicolas-grekas I did a patch #48653

@upyx upyx mentioned this issue Dec 14, 2022
Closed
@upyx
Copy link
Contributor
upyx commented Dec 15, 2022

A snippet to add this feature to an older Symfony version: https://gist.github.com/upyx/8f2ac31f415659b3ef57c3b199cf7ba3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature Help wanted Issues and PRs which are looking for volunteers to complete them. Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@nicolas-grekas @Bilge @upyx @chalasr @carsonbot
0