8000 [FrameworkBundle] Conflict between generated services ID · Issue #41198 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[FrameworkBundle] Conflict between generated services ID #41198

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
lyrixx opened this issue May 12, 2021 · 21 comments
Closed

[FrameworkBundle] Conflict between generated services ID #41198

lyrixx opened this issue May 12, 2021 · 21 comments
Labels
Bug FrameworkBundle Status: Needs Review

Comments

@lyrixx
Copy link
Member
lyrixx commented May 12, 2021

Symfony version(s) affected: all

Description

Symfony used to namespace all generated ID but it's not the case for some services

And maybe more

How to reproduce

framework:
    http_client:
        scoped_clients:
            github:
                base_uri: http://github.com
    cache:
        pools:
            github: null

With the previous configuration, http_client replaces the cache definition (because http client configuration is processed after cache configuration).

>…goire/dev/labs/symfony/symfony-5.4(master *) bin/console  debug:container github

 // This service is a private alias for the service .debug.github                                                       

Information for Service ".debug.github"
=======================================

 ---------------- -------------------------------------------------- 
  Option           Value                                             
 ---------------- -------------------------------------------------- 
  Service ID       .debug.github                                     
  Class            Symfony\Component\HttpClient\TraceableHttpClient

And If I change the name of the github http client:

>…goire/dev/labs/symfony/symfony-5.4(master *) bin/console  debug:container github

Information for Service "github"
================================

 An adapter that collects data about all cache calls.

 ---------------- -------------------------------------------------- 
  Option           Value                                             
 ---------------- -------------------------------------------------- 
  Service ID       github                                            
  Class            Symfony\Component\Cache\Adapter\TraceableAdapter

Possible Solution

We must namespace all services ID based on semantic configuration like we did in the past with messenger, workflow, etc

Additional context

This issue is a bit annoying with a different use case:

Once I declared a github client, I'm no longer able to use bin/console debug:container github to find all services that match github because a github service exists. Yes, I can still use | grep github, but still :)
@lyrixx lyrixx added the Bug label May 12, 2021
@n0rbyt3
Copy link
Contributor
n0rbyt3 commented May 14, 2021

@lyrixx There will be some BC changes then.

From the HttpClient documentation:

Each scoped client also defines a corresponding named autowiring alias. If you use for example Symfony\Contracts\HttpClient\HttpClientInterface $myApiClient as the type and name of an argument, autowiring will inject the my_api.client service into your autowired classes.

The documentation at this point clears it out: It's better to use <name>.client as naming strategy for scoped clients so they can be injected by name. Previous examples should be adapted (github.client instead of github) to work around this bug.

If the service gets a prefix, it will break autowiring scoped clients by name.

@jderusse
Copy link
Member

👍 for deprecating these services. With autowiring and named autowiring alias they are not needed IMHO

@n0rbyt3
Copy link
Contributor
n0rbyt3 commented May 14, 2021

@jderusse I disagree. The HttpClient is very special. It's decorated multiple times in FrameworkExtension. Also every client gets a special tag for the profiler. Everything is handled with configuration: simple and easy.

The namespace is missing to allow this kind of autowiring. I am working on a project calling multiple APIs and all need their scoped client. How do I inject the specific client if I don't know its service name? Creating the scoped client by hand requires knowledge of its construction and the special tag. Not really useful. One idea might be to create an AbstractScopedHttpClient and each service must implement getBaseUri() method. But the cache pools? Hmm...

@nicolas-grekas
Copy link
Member
nicolas-grekas commented May 14, 2021 •
edited
Loading

For the record, this was discussed at the time and was a conscious decision, to not create any convention. You configure HTTP clients and cache pools giving them a plain service ID and that's the only knowledge you need to have.

I agree there is an inconsistency with how eg workflows are wired, but my preference would be to actually deprecate the suffix/prefix convention there. Eg right now I'm forced to name my argument $foobarStateMachine because of the convention in place, and that's something that is both not flexible and that has to be discovered/teached/learned.

@lyrixx
Copy link
Member Author
lyrixx commented May 16, 2021 •
edited
Loading

Even without a suffix it has to be "discovered/teached/learned". People are not strick with this knowledge 😇
And it is also a convention. To be a configuration, the node should be called "ServiceID" or "service" and not name.

But I agree with you about the hardcoded suffix that is less flexible.

@n0rbyt3
Copy link
Contributor
n0rbyt3 commented May 18, 2021

@nicolas-grekas If you want to deprecate suffix/prefix convention, you need to think about logging, too. Right now you can inject LoggerInterface $fooLogger to log to channel foo without specifying a monolog.logger tag.

@lyrixx
Copy link
Member Author
lyrixx commented May 18, 2021 •
edited
Loading

warning: There is a confusion here between the alias for argument registered, and the service ID.

1️⃣ Here, I was initially talking about the service ID only. This service id is used:

  • when configuring DI and when not using autowire;
  • when people want to decorate the service.
  • in custom compiler pass

2️⃣ The alias for argument is fine, even if I think can enhance it. But the feature must be kept, of course!

3️⃣ But in both case, the same "name" is used for the service id and the alias for argument (except for workflow)

For example: the following configuration

framework:
    cache:
        pools:
            github:
                adapter: cache.adapter.redis
framework:
    workflows:
        rule: [...]

The following generated services:

<service id="github" class="Symfony\Component\Cache\Adapter\TraceableAdapter">
    <!-- ... -->
</service>

<service id="state_machine.rule" class="Symfony\Component\Workflow\StateMachine" public="true">
    <!-- ... -->
</service>

and the following generated alias for argument:

 Symfony\Contracts\Cache\CacheInterface $github (github)
 Symfony\Component\Workflow\WorkflowInterface $ruleStateMachine (state_machine.rule)

I search in the framework bundle, and I found:

🅰️ In lock, workflow, rate limiter, we use a prefix

🅱️ In cache, http, messenger: we don't used a prefix.

So the framework configuration is not really consistant here. IMHO it be can be more predictable and consistant.

The @nicolas-grekas solutions might be nice, because the end user have a full control on everything, but at the end, the configuration that we must write is really strange IMHO:

framework:
    messenger:
        default_bus: command.bus
        buses:
            command.bus: []
            query.bus: []
            event.bus: []

The keyword bus is repeated multiple times. When you know how Symfony handle theses names, it becomes clearer, but for a new comer, or for someone who don't know and want to simplify things, it's weird.

So in order to find a solution I propose the following thing:

In 5.4:

  • add for each configuration section a new string entry under name called something like: serviceIdGenerated. The default value must be the same as the current one;
  • in 🅱️ trigger a deprecation if the key is not filled;
  • for the workflow, register another alias for argument with only the name of the workflow, and deprecate the older alias.

In 6.0:

  • drop the deprecations;
  • drop the (old) workflow alias for argument;
  • update default service id for 🅱️ (ie: set a prefix).

Conclusion: in 6.0:

  • use prefix for all generated service ID (no conflict will be possible);
  • the raw name is used for the alias configuration.

With that, I hope both @nicolas-grekas and me will be happy

  • full control of the name;
  • the name is used in alias for argument, so there is no convention (even if this is a convention, but a light one);
  • the service id is prefixed by default, but people can override it if they want

@stof
Copy link
Member
stof commented May 18, 2021 •
edited
Loading

the tricky things about not having a prefix in service ids is that the names you give to your HTTP clients, your cache pools and your messenger transport must now avoid conflicts with all service ids of the container. If you have a cache pool named github and a HTTP client named github, one of them will be overwritten by the other (depending on the order in which FrameworkBundle configures them). This can lead to lots of WTF if you face this, especially when all older components were configured with service id prefixes to avoid issues.

* update default service id for A (ie: set a prefix).

aren't you mixing the A and B cases here ? A already uses a prefix

@stof
Copy link
Member
stof commented May 18, 2021

The WTF behavior might even be worse, as the HttpClientInterface $github autowiring alias would still exist, but referencing a cache pool...

@lyrixx
Copy link
Member Author
lyrixx commented May 18, 2021

@stof you are totally right, that's why I created the issue initially and want to get ride of this! And you are also right about 🅰️ and 🅱️. I'm gonna fix my previous comment. Thanks

@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@carsonbot
Copy link

Could I get an answer? If I do not hear anything I will assume this issue is resolved or abandoned. Please get back to me <3

@carsonbot
Copy link

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Dec 29, 2021 •
edited
Loading

Same topic on #44798 (comment)

What would work I guess:
for each list of services where a name is configured:

  • use a convention to prefix/suffix the name when registering the service (unlike what is done for http_client)
  • do NOT add any convention when registering named autowiring aliases ((unlike what is done for workflows)

I guess we can deprecate the current names by using deprecated aliases.

@lyrixx would that solve your issue?
If yes, up for a PR?

@carsonbot carsonbot removed the Stalled label Dec 29, 2021
@nicolas-grekas nicolas-grekas mentioned this issue Dec 29, 2021
Merged
@lyrixx
Copy link
Member Author
lyrixx commented Dec 29, 2021 •
edited by nicolas-grekas
Loading
  • do NOT add any convention when registering named autowiring aliases (unlike what is done for workflows)

why ? it's very handy. and people already use it a lot (for monolog for instance)

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Dec 29, 2021 •
edited
Loading

Because it reduces the DX by forcing ppl to learn about that prefix/suffix, and also because it forces ppl to use a specific name on their classes, eg $foobarStateMachine

@stof
Copy link
Member
stof commented Dec 29, 2021

I agree that the autowiring alias should be based on the user-defined name only. This is what gives the better DX by giving full control of this alias to the devs. And autowiring aliases don't suffer from conflicts, because the names only need to be unique by type, which is already the case in the config.

@lyrixx
Copy link
Member Author
lyrixx commented Dec 29, 2021

Because it reduces the DX by forcing ppl to learn about that prefix/suffix, and also because it forces ppl to use a specific name on their classes, eg $foobarStateMachine

Oh, I think I misunderstood what you said.

If a workflow is named "foobar", then only the Symfony\....\WorkflowInterface $foobar should be created, and not the Symfony\....\WorkflowInterface $foobarWorkflow, right ?

In that case, I agree with you 👍🏼

@stof
Copy link
Member
stof commented Dec 29, 2021

@lyrixx yes

@tgalopin
Copy link
Contributor

I did what proposed Nicolas (keeping the named autowiring alias to the user-defined name while adding a prefix to the service) in #44798, I agree it's probably the best scenario.

@nicolas-grekas
Copy link
Member

I'm closing because I'm not sure it's worth improving. At least I'm not sure it's worth the cost of annoying ppl with deprecations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug FrameworkBundle Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@nicolas-grekas @lyrixx @stof @jderusse @tgalopin @n0rbyt3 @carsonbot
0