Skip to content

[DI] Empty tags get lost in instanceof conditionals #48388

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
HypeMC opened this issue Nov 29, 2022 · 10 comments
Closed

[DI] Empty tags get lost in instanceof conditionals #48388

HypeMC opened this issue Nov 29, 2022 · 10 comments

Comments

@HypeMC
Copy link
Contributor

HypeMC commented Nov 29, 2022

Symfony version(s) affected

v5.4.16

Description

Symfony v5.1.16 has introduced a BC break in how autoconfiguring works. The issue was caused by #48027 .

How to reproduce

#[AutoconfigureTag('app.foo')]
interface FooInterface
{
}

#[AutoconfigureTag('app.foo', ['alias' => 'bar'])]
class Foo implements FooInterface
{
}
app.foo_locator:
    class: Symfony\Component\DependencyInjection\ServiceLocator
    arguments: [ !tagged_iterator { tag: 'app.foo', index_by: 'alias' } ]

Before #48027 the app.foo_locator service locator would have 2 aliases for the Foo service (which is the behavior I need):

return $container->services['app.foo_locator'] = new \Symfony\Component\DependencyInjection\Argument\ServiceLocator($container->getService, [
    'App\\Service\\Foo' => ['privates', 'App\\Service\\Foo', 'getFooService', true],
    'bar' => ['privates', 'App\\Service\\Foo', 'getFooService', true],
], [
    'App\\Service\\Foo' => 'App\\Service\\Foo',
    'bar' => 'App\\Service\\Foo',
]);

After upgrading to v5.4.16, this is no longer the case:

return $container->services['app.foo_locator'] = new \Symfony\Component\DependencyInjection\Argument\ServiceLocator($container->getService, [
    'bar' => ['privates', 'App\\Service\\Foo', 'getFooService', true],
], [
    'bar' => 'App\\Service\\Foo',
]);

Since the Foo service has an #[AutoconfigureTag] attribute, the FooInterface is skipped during autoconfiguration.

Possible Solution

No response

Additional Context

No response

@HypeMC HypeMC added the Bug label Nov 29, 2022
@nicolas-grekas nicolas-grekas changed the title [DI] BC break in v5.4.16 [DI] Empty tags get lost in instanceof conditionals Dec 4, 2022
nicolas-grekas added a commit that referenced this issue Dec 5, 2022
…re tag when it's already set with attributes" (nicolas-grekas)

This PR was merged into the 5.4 branch.

Discussion
----------

[DependencyInjection] Revert "bug #48027 Don't autoconfigure tag when it's already set with attributes"

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? |
| Tickets       | Fix #48388
| License       | MIT
| Doc PR        | -

Reverting #48027, which is a regression.

Commits
-------

bd2780a Revert "bug #48027 [DependencyInjection] Don't autoconfigure tag when it's already set with attributes (nicolas-grekas)"
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 5, 2022

Me and @Seldaek are wondering about the use case for having two tags. Could anyone share their's?

@fritzmg
Copy link
Contributor

fritzmg commented Dec 5, 2022

@nicolas-grekas this can happen through auto configuration - and also when you need to add autowiring hints for service subscribers. Example:

services:
    _defaults:
        autoconfigure: true

    App\Test:
        tags:
            - { name: container.service_subscriber, id: App\Foobar }
namespace App;

use App\Foobar;
use Symfony\Contracts\Service\ServiceSubscriberInterface;

class Test implements ServiceSubscriberInterface
{
    public static function getSubscribedServices(): array
    {
        return [Foobar::class => Foobar::class];
    }
}
Information for Service "App\Test"
==================================

 ---------------- --------------------------------------------------------------
  Option           Value
 ---------------- --------------------------------------------------------------
  Service ID       App\Test
  Class            App\Test
  Tags             container.service_subscriber (id: App\Foobar)
                   container.service_subscriber
  Public           no
  Synthetic        no
  Lazy             no
  Shared           yes
  Abstract         no
  Autowired        no
  Autoconfigured   yes
 ---------------- --------------------------------------------------------------

i.e.: when autoconfiguration is enabled, Symfony will automatically add an (empty!) container.service_subscriber tag to any service implementing the ServiceSubscriberInterface. However, in ambiguous cases you will need additional autowiring hints so that Symfony knows which service to inject in the container of the service subscriber and thus you manually add the container.service_subscriber tag with the appropriate service ID. You will also need to add the container.service_subscriber multiple times for each autowiring hint.

@HypeMC
Copy link
Contributor Author

HypeMC commented Dec 5, 2022

In my case it's basically a BC layer. Even though the Symfony app itself uses the FQCNs, there are still samo old services (written in languages such as GO) that use the old naming convention & communicate with the Symfony app through a message queue. The idea is to standardize this over time.

@Seldaek
Copy link
Member

Seldaek commented Dec 5, 2022

@fritzmg your use case makes sense, and that kinda matches the issue I had in #48019 - except in that case the double tag actually caused problems for me.

I can totally see that you do want the most specific tags to be added, i.e. in this example you want container.service_subscriber (id: App\Foobar) to be kept, but the simple container.service_subscriber is useless I assume?

So IMO @nicolas-grekas's fix would be fine as long as it ensures the most meaningful tag is kept. i.e. do not auto-configure an empty tag (from an interface or even an attribute?) if there is already a tag present which has meaningful data, but if there is already an autoconfigured tag present with no data then it should be overwritten.

@fritzmg
Copy link
Contributor

fritzmg commented Dec 5, 2022

But that solely depends on the service tag, does it not? I did not do a deeper dive whether the non-specific container.service_subscriber can be omitted if there is a more specific one for example. And besides, certain services will always need to be tagged with multiple tags of the name - but different properties.

@Seldaek
Copy link
Member

Seldaek commented Dec 6, 2022

Yeah if you have multiple with different properties that's fine and the use case makes sense. I just don't see a use case where autoconfiguring a second empty tag makes sense.

Especially with the interface it's quite surprising as it's implicit and might even be a BC break situation if you upgrade and a class becomes autoconfigured suddenly due to a new feature.

@ausi
Copy link
Contributor

ausi commented Dec 6, 2022

i.e. in this example you want container.service_subscriber (id: App\Foobar) to be kept, but the simple container.service_subscriber is useless I assume?

The simple container.service_subscriber is not useless, it has a different meaning than the one with an id. See

foreach ($value->getTag('container.service_subscriber') as $attributes) {
if (!$attributes) {
$autowire = true;
continue;
}
I described this in more detail in #48404

@Seldaek
Copy link
Member

Seldaek commented Dec 7, 2022

Ok I see. Then I guess it's probably not fixable as too much depend on this behavior, no matter how sound it was to begin with.

@ausi
Copy link
Contributor

ausi commented Dec 7, 2022

We could deprecate the empty service_subscriber tag and instruct developers to use something like
{ name: container.service_subscriber, autowire: true } instead. But I have no idea if or how many other tags could be affected by this too.

@fritzmg
Copy link
Contributor

fritzmg commented Dec 7, 2022

Same applies to controller.service_arguments which needs to be either empty - or you need to set at least action, argument and id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants