rectorphp / rector-symfony

Rector upgrade rules for Symfony
http://getrector.com
MIT License
179 stars 86 forks source link

Symfony 6 Rules about ContainerInterface #636

Open etshy opened 2 months ago

etshy commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

etshy commented 2 months ago

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 commented 2 months ago

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

Would you like to contribute this feature?

etshy commented 2 months ago

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