diff --git a/config/sets/symfony/symfony-constructor-injection.php b/config/sets/symfony/symfony-constructor-injection.php index b540ff1c..3fb94dc8 100644 --- a/config/sets/symfony/symfony-constructor-injection.php +++ b/config/sets/symfony/symfony-constructor-injection.php @@ -9,6 +9,10 @@ return static function (RectorConfig $rectorConfig): void { $rectorConfig->rules([ + // modern step-by-step narrow approach + \Rector\Symfony\DependencyInjection\Rector\Class_\ControllerGetByTypeToConstructorInjectionRector::class, + + // legacy rules that require container fetch ContainerGetToConstructorInjectionRector::class, ContainerGetNameToTypeInTestsRector::class, GetToConstructorInjectionRector::class, diff --git a/rules-tests/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector/ControllerGetByTypeToConstructorInjectionRectorTest.php b/rules-tests/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector/ControllerGetByTypeToConstructorInjectionRectorTest.php new file mode 100644 index 00000000..5fdaf7aa --- /dev/null +++ b/rules-tests/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector/ControllerGetByTypeToConstructorInjectionRectorTest.php @@ -0,0 +1,28 @@ +doTestFile($filePath); + } + + public static function provideData(): Iterator + { + return self::yieldFilesFromDirectory(__DIR__ . '/Fixture'); + } + + public function provideConfigFilePath(): string + { + return __DIR__ . '/config/configured_rule.php'; + } +} diff --git a/rules-tests/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector/Fixture/controller_get_with_type.php.inc b/rules-tests/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector/Fixture/controller_get_with_type.php.inc new file mode 100644 index 00000000..c9cca181 --- /dev/null +++ b/rules-tests/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector/Fixture/controller_get_with_type.php.inc @@ -0,0 +1,36 @@ +get(SomeService::class); + } +} + +?> +----- +someService; + } +} + +?> diff --git a/rules-tests/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector/Fixture/skip_another_get.php.inc b/rules-tests/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector/Fixture/skip_another_get.php.inc new file mode 100644 index 00000000..7ea838ae --- /dev/null +++ b/rules-tests/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector/Fixture/skip_another_get.php.inc @@ -0,0 +1,14 @@ +some->get(SomeService::class); + } +} diff --git a/rules-tests/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector/Fixture/skip_non_controller.php.inc b/rules-tests/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector/Fixture/skip_non_controller.php.inc new file mode 100644 index 00000000..a2a13836 --- /dev/null +++ b/rules-tests/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector/Fixture/skip_non_controller.php.inc @@ -0,0 +1,13 @@ +get(SomeService::class); + } +} diff --git a/rules-tests/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector/Source/SomeService.php b/rules-tests/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector/Source/SomeService.php new file mode 100644 index 00000000..aac5f815 --- /dev/null +++ b/rules-tests/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector/Source/SomeService.php @@ -0,0 +1,8 @@ +rule( + \Rector\Symfony\DependencyInjection\Rector\Class_\ControllerGetByTypeToConstructorInjectionRector::class + ); +}; diff --git a/rules-tests/Symfony42/Rector/MethodCall/ContainerGetToConstructorInjectionRector/Fixture/skip_test_case_static_call.php.inc b/rules-tests/Symfony42/Rector/MethodCall/ContainerGetToConstructorInjectionRector/Fixture/skip_test_case_static_call.php.inc deleted file mode 100644 index 52bbe3b7..00000000 --- a/rules-tests/Symfony42/Rector/MethodCall/ContainerGetToConstructorInjectionRector/Fixture/skip_test_case_static_call.php.inc +++ /dev/null @@ -1,13 +0,0 @@ -get('some'); - } -} diff --git a/rules-tests/Symfony42/Rector/MethodCall/ContainerGetToConstructorInjectionRector/Fixture/some_translator_exists3.php.inc b/rules-tests/Symfony42/Rector/MethodCall/ContainerGetToConstructorInjectionRector/Fixture/some_translator_exists3.php.inc deleted file mode 100644 index bd114ac1..00000000 --- a/rules-tests/Symfony42/Rector/MethodCall/ContainerGetToConstructorInjectionRector/Fixture/some_translator_exists3.php.inc +++ /dev/null @@ -1,55 +0,0 @@ -someTranslator = $someTranslator; - $this->someTranslator2 = $someTranslator2; - } - - protected function execute() - { - $someService = $this->getContainer()->get('translator'); - - $someService = $this->getContainer()->get('translator')->translateSomething(); - } -} - -?> ------ -someTranslator = $someTranslator; - $this->someTranslator2 = $someTranslator2; - } - - protected function execute() - { - $someService = $this->someTranslator; - - $someService = $this->someTranslator->translateSomething(); - } -} - -?> diff --git a/rules/DependencyInjection/NodeDecorator/CommandConstructorDecorator.php b/rules/DependencyInjection/NodeDecorator/CommandConstructorDecorator.php new file mode 100644 index 00000000..10cc463e --- /dev/null +++ b/rules/DependencyInjection/NodeDecorator/CommandConstructorDecorator.php @@ -0,0 +1,44 @@ +nodeTypeResolver->isObjectType( + $class, + new ObjectType('Symfony\Component\Console\Command\Command') + )) { + return; + } + + $constuctClassMethod = $class->getMethod(MethodName::CONSTRUCT); + if (! $constuctClassMethod instanceof ClassMethod) { + return; + } + + // empty stmts? add parent::__construct() to setup command + if ((array) $constuctClassMethod->stmts === []) { + $parentConstructStaticCall = new StaticCall(new Name('parent'), '__construct'); + $constuctClassMethod->stmts[] = new Expression($parentConstructStaticCall); + } + } +} diff --git a/rules/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector.php b/rules/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector.php new file mode 100644 index 00000000..9b55e8b7 --- /dev/null +++ b/rules/DependencyInjection/Rector/Class_/ControllerGetByTypeToConstructorInjectionRector.php @@ -0,0 +1,160 @@ +get(SomeType::class)` in controllers to constructor injection (step 1/x)', + [ + new CodeSample( + <<<'CODE_SAMPLE' +use Symfony\Bundle\FrameworkBundle\Controller\Controller; + +final class SomeCommand extends Controller +{ + public function someMethod() + { + $someType = $this->get(SomeType::class); + } +} +CODE_SAMPLE + , + <<<'CODE_SAMPLE' +use Symfony\Bundle\FrameworkBundle\Controller\Controller; + +final class SomeCommand extends Controller +{ + public function __construct(private SomeType $someType) + { + } + + public function someMethod() + { + $someType = $this->someType; + } +} +CODE_SAMPLE + ), + ] + ); + } + + /** + * @return array> + */ + public function getNodeTypes(): array + { + return [Class_::class]; + } + + /** + * @param Class_ $node + */ + public function refactor(Node $node): ?Node + { + if ($this->shouldSkipClass($node)) { + return null; + } + + $propertyMetadatas = []; + + $this->traverseNodesWithCallable($node, function (Node $node) use (&$propertyMetadatas): ?Node { + if (! $node instanceof MethodCall) { + return null; + } + + if ($node->isFirstClassCallable()) { + return null; + } + + if (! $this->isName($node->name, 'get')) { + return null; + } + + if (! $this->isName($node->var, 'this')) { + return null; + } + + if (count($node->getArgs()) !== 1) { + return null; + } + + $firstArg = $node->getArgs()[0]; + if (! $firstArg->value instanceof ClassConstFetch) { + return null; + } + + // must be class const fetch + if (! $this->isName($firstArg->value->name, 'class')) { + return null; + } + + $className = $this->getName($firstArg->value->class); + if (! is_string($className)) { + return null; + } + + $propertyName = $this->propertyNaming->fqnToVariableName($className); + $propertyMetadata = new PropertyMetadata($propertyName, new FullyQualifiedObjectType($className)); + + $propertyMetadatas[] = $propertyMetadata; + return $this->nodeFactory->createPropertyFetch('this', $propertyMetadata->getName()); + }); + + if ($propertyMetadatas === []) { + return null; + } + + foreach ($propertyMetadatas as $propertyMetadata) { + $this->classDependencyManipulator->addConstructorDependency($node, $propertyMetadata); + } + + return $node; + } + + private function shouldSkipClass(Class_ $class): bool + { + // keep it safe + if (! $class->isFinal()) { + return true; + } + + $scope = ScopeFetcher::fetch($class); + + $classReflection = $scope->getClassReflection(); + if (! $classReflection instanceof ClassReflection) { + return true; + } + + return ! $classReflection->isSubclassOf(SymfonyClass::CONTROLLER); + } +} diff --git a/rules/Symfony42/Rector/MethodCall/ContainerGetToConstructorInjectionRector.php b/rules/Symfony42/Rector/MethodCall/ContainerGetToConstructorInjectionRector.php index 95bc8844..614d7aa1 100644 --- a/rules/Symfony42/Rector/MethodCall/ContainerGetToConstructorInjectionRector.php +++ b/rules/Symfony42/Rector/MethodCall/ContainerGetToConstructorInjectionRector.php @@ -7,18 +7,14 @@ use PhpParser\Node; use PhpParser\Node\Expr\ClassConstFetch; use PhpParser\Node\Expr\MethodCall; -use PhpParser\Node\Expr\StaticCall; -use PhpParser\Node\Name; use PhpParser\Node\Stmt\Class_; -use PhpParser\Node\Stmt\ClassMethod; -use PhpParser\Node\Stmt\Expression; use PHPStan\Type\ObjectType; use Rector\NodeManipulator\ClassDependencyManipulator; use Rector\PHPUnit\NodeAnalyzer\TestsNodeAnalyzer; use Rector\PostRector\ValueObject\PropertyMetadata; use Rector\Rector\AbstractRector; +use Rector\Symfony\DependencyInjection\NodeDecorator\CommandConstructorDecorator; use Rector\Symfony\NodeAnalyzer\DependencyInjectionMethodCallAnalyzer; -use Rector\ValueObject\MethodName; use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample; use Symplify\RuleDocGenerator\ValueObject\RuleDefinition; @@ -33,6 +29,7 @@ public function __construct( private readonly DependencyInjectionMethodCallAnalyzer $dependencyInjectionMethodCallAnalyzer, private readonly TestsNodeAnalyzer $testsNodeAnalyzer, private readonly ClassDependencyManipulator $classDependencyManipulator, + private readonly CommandConstructorDecorator $commandConstructorDecorator, ) { } @@ -141,27 +138,8 @@ public function refactor(Node $node): ?Node $this->classDependencyManipulator->addConstructorDependency($class, $propertyMetadata); } - $this->decorateCommandConstructor($class); + $this->commandConstructorDecorator->decorate($class); return $node; } - - private function decorateCommandConstructor(Class_ $class): void - { - // special case for command to keep parent constructor call - if (! $this->isObjectType($class, new ObjectType('Symfony\Component\Console\Command\Command'))) { - return; - } - - $constuctClassMethod = $class->getMethod(MethodName::CONSTRUCT); - if (! $constuctClassMethod instanceof ClassMethod) { - return; - } - - // empty stmts? add parent::__construct() to setup command - if ((array) $constuctClassMethod->stmts === []) { - $parentConstructStaticCall = new StaticCall(new Name('parent'), '__construct'); - $constuctClassMethod->stmts[] = new Expression($parentConstructStaticCall); - } - } } diff --git a/src/Enum/SymfonyClass.php b/src/Enum/SymfonyClass.php index d35f333f..972418f1 100644 --- a/src/Enum/SymfonyClass.php +++ b/src/Enum/SymfonyClass.php @@ -6,6 +6,11 @@ final class SymfonyClass { + /** + * @var string + */ + public const CONTROLLER = 'Symfony\Bundle\FrameworkBundle\Controller\Controller'; + /** * @var string */ diff --git a/src/NodeAnalyzer/ServiceTypeMethodCallResolver.php b/src/NodeAnalyzer/ServiceTypeMethodCallResolver.php index a15582a6..976e910f 100644 --- a/src/NodeAnalyzer/ServiceTypeMethodCallResolver.php +++ b/src/NodeAnalyzer/ServiceTypeMethodCallResolver.php @@ -28,8 +28,8 @@ public function resolve(MethodCall $methodCall): ?Type return new MixedType(); } - $argument = $methodCall->getArgs()[0] - ->value; + $firstArg = $methodCall->getArgs()[0]; + $argument = $firstArg->value; $serviceMap = $this->serviceMapProvider->provide(); if ($argument instanceof String_) {