Skip to content
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

Symfony 6 Rules about ContainerInterface #636

Open
etshy opened this issue Jul 4, 2024 · 6 comments
Open

Symfony 6 Rules about ContainerInterface #636

etshy opened this issue Jul 4, 2024 · 6 comments

Comments

@etshy
Copy link

etshy commented Jul 4, 2024

I'm upgrading a few project from Symfony 5.4 to 6.4. I first try to upgrade to Symfony 6.0 and I don't quite understand the rules about ContainerInterface.

    $rectorConfig->ruleWithConfiguration(ReplaceServiceArgumentRector::class, [
        new ReplaceServiceArgument('Psr\Container\ContainerInterface', new String_('service_container')),
        new ReplaceServiceArgument(
            'Symfony\Component\DependencyInjection\ContainerInterface',
            new String_('service_container')
        ),
    ]);

What's is this supposed to change ?

If we use the interface directly it doesn't change anything so shouldn't it be a class rename from ContainerInterface (both PSR and Symfony) into Symfony\Contracts\Service\ServiceProviderInterface? (not sure about that as ServiceLocator are not the only use for injecting ContainerInterface)
Or at least add the following definitions ?

services:
  Symfony\Component\DependencyInjection\ContainerInterface: '@service_container'
  Psr\Container\ContainerInterface: '@service_container'
@TomasVotruba
Copy link
Member

TomasVotruba commented Jul 4, 2024

If I recall correctly, this was due to interface not being a valid service name anymore. This applied only to PHP files (configs).

If you add aliases to your YAML files like these, it's another way to upgrade, but it only reverts the upgrade path, so not really way to go:

services:
  Symfony\Component\DependencyInjection\ContainerInterface: '@service_container'
  Psr\Container\ContainerInterface: '@service_container'

What's the use case for service using container? Those should be refactored to require particular services over whole container.

@etshy
Copy link
Author

etshy commented Jul 4, 2024

If I recall correctly, this was due to interface not being a valid service name anymore. This applied only to PHP files (configs).
Yeah, ContainerInterface is not "exposed" anymore.

It was to use Locators (bad use of the interface, I guess but symfony docs keep showing this in example, event for 7.x) and some very specific cases but I'm refactoring it to use ServiceProviderInterface instead.
And for the very specific cases, I added the services definitions to keep using ContainerInterface (won't go into details but it's used in files that can't be edited.

But as Symfony keep showing ContainerInterface usage for locator, I thought It could be good to rename ContainerInterface to ServiceProviderInterface, used by ServiceLocator class (and tell synfony to stop showing usage of ContainerInterface ?)

@TomasVotruba
Copy link
Member

Sounds good 👍 Could you share a diff how this change should look like?
What Symfony version should it target?

@etshy
Copy link
Author

etshy commented Jul 4, 2024

Sure
It happened to me with the SymfonySetList::SYMFONY_60 and the removal of ContainerInterface alias is in 6.0 upgrade notes so I'd say symfony: 6.0 ?

And the diff should be something like that (including both ContainerInterface as old content):

- use Psr\Container\ContainerInterface;
// OR
- use Symfony\Component\DependencyInjection\ContainerInterface
+ use Symfony\Contracts\Service\ServiceProviderInterface;

class MyClass
{
    public function __construct(
         #[TaggedLocator(tag: 'app:tag-locator')]
-        private readonly ContainerInterface $locator,
+        private readonly ServiceProviderInterface $locator
    ) {}
}

Not sure if it's possible but to make sure the ContainerInterface is used for locators, we could check the #[TaggedLocator] Attribute or check in the services.yaml ?
To avoid side effects to the maximum

@TomasVotruba
Copy link
Member

Thanks! Looks good to target 6.0. Checking TaggedLocator makes sense as it's available in the code 👍

Would you like to contribute this feature?

@etshy
Copy link
Author

etshy commented Jul 4, 2024

I could take a look and let you know if I can do it in a few days, yeah

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

No branches or pull requests

2 participants