From fbf0aa275fcc8814be9171cd68a26a6cd5da9ea7 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Fri, 5 Jul 2019 01:19:02 +0300 Subject: [PATCH] [Architecture] Add ConstructorInjectionToActionInjectionRector --- composer.json | 8 +- ...structor-injectin-to-action-injection.yaml | 2 + ecs.yaml | 1 + packages/Architecture/config/config.yaml | 8 + .../src/Cleaner/ClassMethodCleaner.php | 7 + ...ructorInjectionToActionInjectionRector.php | 345 ++++++++++++++++++ ...orInjectionToActionInjectionRectorTest.php | 25 ++ .../Fixture/duplicate.php.inc | 43 +++ .../Fixture/fixture.php.inc | 37 ++ .../Fixture/manage_different_naming.php.inc | 41 +++ .../Fixture/skip_non_action_methods.php.inc | 28 ++ .../Fixture/skip_scalars.php.inc | 21 ++ .../Node/Manipulator/ClassManipulator.php | 30 ++ 13 files changed, 593 insertions(+), 3 deletions(-) create mode 100644 config/set/architecture/constructor-injectin-to-action-injection.yaml create mode 100644 packages/Architecture/config/config.yaml create mode 100644 packages/Architecture/src/Cleaner/ClassMethodCleaner.php create mode 100644 packages/Architecture/src/Rector/Class_/ConstructorInjectionToActionInjectionRector.php create mode 100644 packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/ConstructorInjectionToActionInjectionRectorTest.php create mode 100644 packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/duplicate.php.inc create mode 100644 packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/fixture.php.inc create mode 100644 packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/manage_different_naming.php.inc create mode 100644 packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/skip_non_action_methods.php.inc create mode 100644 packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/skip_scalars.php.inc diff --git a/composer.json b/composer.json index bc2dacb8560..c65e35dfab4 100644 --- a/composer.json +++ b/composer.json @@ -76,7 +76,8 @@ "Rector\\SOLID\\": "packages/SOLID/src", "Rector\\Legacy\\": "packages/Legacy/src", "Rector\\ElasticSearchDSL\\": "packages/ElasticSearchDSL/src", - "Rector\\SymfonyPHPUnit\\": "packages/SymfonyPHPUnit/src" + "Rector\\SymfonyPHPUnit\\": "packages/SymfonyPHPUnit/src", + "Rector\\Architecture\\": "packages/Architecture/src" } }, "autoload-dev": { @@ -114,7 +115,8 @@ "Rector\\SOLID\\Tests\\": "packages/SOLID/tests", "Rector\\Legacy\\Tests\\": "packages/Legacy/tests", "Rector\\ElasticSearchDSL\\Tests\\": "packages/ElasticSearchDSL/tests", - "Rector\\SymfonyPHPUnit\\Tests\\": "packages/SymfonyPHPUnit/tests" + "Rector\\SymfonyPHPUnit\\Tests\\": "packages/SymfonyPHPUnit/tests", + "Rector\\Architecture\\Tests\\": "packages/Architecture/tests" }, "classmap": [ "packages/Symfony/tests/Rector/FrameworkBundle/AbstractToConstructorInjectionRectorSource", @@ -166,4 +168,4 @@ "dev-master": "0.5-dev" } } -} +} \ No newline at end of file diff --git a/config/set/architecture/constructor-injectin-to-action-injection.yaml b/config/set/architecture/constructor-injectin-to-action-injection.yaml new file mode 100644 index 00000000000..f71077fa610 --- /dev/null +++ b/config/set/architecture/constructor-injectin-to-action-injection.yaml @@ -0,0 +1,2 @@ +services: + Rector\Architecture\Rector\Class_\ConstructorInjectionToActionInjectionRector: ~ diff --git a/ecs.yaml b/ecs.yaml index 9949ec7cea5..739d6fba979 100644 --- a/ecs.yaml +++ b/ecs.yaml @@ -102,6 +102,7 @@ parameters: Symplify\CodingStandard\Sniffs\CleanCode\CognitiveComplexitySniff: # tough logic + - 'packages/Architecture/src/Rector/Class_/ConstructorInjectionToActionInjectionRector.php' - 'src/PhpParser/Node/Commander/NodeRemovingCommander.php' - 'packages/BetterPhpDocParser/src/*' - 'packages/Symfony/src/Rector/Class_/MakeCommandLazyRector.php' diff --git a/packages/Architecture/config/config.yaml b/packages/Architecture/config/config.yaml new file mode 100644 index 00000000000..71b850aef99 --- /dev/null +++ b/packages/Architecture/config/config.yaml @@ -0,0 +1,8 @@ +services: + _defaults: + autowire: true + public: true + + Rector\Architecture\: + resource: '../src' + exclude: '../src/{Rector/**/*Rector.php}' diff --git a/packages/Architecture/src/Cleaner/ClassMethodCleaner.php b/packages/Architecture/src/Cleaner/ClassMethodCleaner.php new file mode 100644 index 00000000000..3f72a01d0c4 --- /dev/null +++ b/packages/Architecture/src/Cleaner/ClassMethodCleaner.php @@ -0,0 +1,7 @@ +classManipulator = $classManipulator; + $this->typeAnalyzer = $typeAnalyzer; + } + + public function getDefinition(): RectorDefinition + { + return new RectorDefinition('', [ + new CodeSample( + <<<'CODE_SAMPLE' +final class SomeController +{ + /** + * @var ProductRepository + */ + private $productRepository; + + public function __construct(ProductRepository $productRepository) + { + $this->productRepository = $productRepository; + } + + public function default() + { + $products = $this->productRepository->fetchAll(); + } +} +CODE_SAMPLE + , + <<<'CODE_SAMPLE' +final class SomeController +{ + public function default(ProductRepository $productRepository) + { + $products = $productRepository->fetchAll(); + } +} +CODE_SAMPLE + ), + ]); + } + + /** + * @return string[] + */ + public function getNodeTypes(): array + { + return [Class_::class]; + } + + /** + * @param Class_ $node + */ + public function refactor(Node $node): ?Node + { + $this->reset(); + + // only in controllers + if (! $this->isName($node, '*Controller')) { + return null; + } + + if ($node->isAbstract()) { + return null; + } + + $constructMethod = $node->getMethod('__construct'); + // no constructor, nothing to do + if ($constructMethod === null) { + return null; + } + + // traverse constructor dependencies and names of their properties + $this->collectPropertyFetchToParams($constructMethod); + + // replace them in property fetches with particular class methods and use variable instead + foreach ($node->stmts as $classStmt) { + if (! $classStmt instanceof ClassMethod) { + continue; + } + + if ($this->isName($classStmt, '__construct')) { + continue; + } + + if (! $classStmt->isPublic()) { + continue; + } + + $this->replacePropertyFetchByInjectedVariables($classStmt); + } + + // collect all property fetches that are relevant to original constructor properties + $this->traverseNodesWithCallable($node->stmts, function (Node $node) { + if (! $node instanceof PropertyFetch) { + return null; + } + + // only scan non-action methods + /** @var ClassMethod $methdoNode */ + $methdoNode = $node->getAttribute(AttributeKey::METHOD_NODE); + if ($methdoNode->isPublic()) { + return null; + } + + $usedPropertyFetchName = $this->getName($node); + if (isset($this->propertyFetchToParams[$usedPropertyFetchName])) { + unset($this->propertyFetchToParamsToRemoveFromConstructor[$usedPropertyFetchName]); + } + }); + + $this->removeUnusedPropertiesAndConstructorParams($node, $constructMethod); + + return $node; + } + + private function getPositionStmtByTypeAndName(Class_ $node, string $name, string $type): ?int + { + foreach ($node->stmts as $key => $stmt) { + if (! is_a($stmt, $type, true)) { + continue; + } + + if (! $this->isName($stmt, $name)) { + continue; + } + + return $key; + } + + return null; + } + + private function removeEmptyConstruct(Class_ $class, ClassMethod $constructClassMethod): void + { + if ($constructClassMethod->stmts !== []) { + return; + } + + /** @var int $constructMethodPosition */ + $constructMethodPosition = $this->getPositionStmtByTypeAndName($class, '__construct', ClassMethod::class); + if ($constructMethodPosition) { + unset($class->stmts[$constructMethodPosition]); + } + } + + private function removeUnusedProperties(Class_ $class): void + { + foreach (array_keys($this->propertyFetchToParamsToRemoveFromConstructor) as $propertyFetchName) { + /** @var string $propertyFetchName */ + $this->classManipulator->removeProperty($class, $propertyFetchName); + } + } + + private function replacePropertyFetchByInjectedVariables(ClassMethod $classMethod): void + { + $currentlyAddedLocalVariables = []; + + $this->traverseNodesWithCallable((array) $classMethod->stmts, function (Node $node) use ( + &$currentlyAddedLocalVariables + ): ?Variable { + if (! $node instanceof PropertyFetch) { + return null; + } + + foreach ($this->propertyFetchToParams as $propertyFetchName => $param) { + if ($this->isName($node, $propertyFetchName)) { + $currentlyAddedLocalVariables[] = $param; + + /** @var string $paramName */ + $paramName = $this->getName($param); + return new Variable($paramName); + } + } + + return null; + }); + + foreach ($currentlyAddedLocalVariables as $param) { + // is param already present? + foreach ($classMethod->params as $existingParam) { + if ($this->areNamesEqual($existingParam, $param)) { + continue 2; + } + } + + $classMethod->params[] = $param; + } + } + + private function collectPropertyFetchToParams(ClassMethod $classMethod): void + { + foreach ((array) $classMethod->stmts as $constructorStmt) { + $propertyToVariable = $this->resolveAssignPropertyToVariableOrNull($constructorStmt); + if ($propertyToVariable === null) { + continue; + } + + [$propertyFetchName, $variableName] = $propertyToVariable; + + $param = $this->classManipulator->findMethodParamByName($classMethod, $variableName); + if ($param === null) { + continue; + } + + // random type, we cannot autowire in action + if ($param->type === null) { + continue; + } + + $paramType = $this->getName($param->type); + if ($paramType === null) { + continue; + } + + if ($this->typeAnalyzer->isPhpReservedType($paramType)) { + continue; + } + + // it's a match + $this->propertyFetchToParams[$propertyFetchName] = $param; + } + + $this->propertyFetchToParamsToRemoveFromConstructor = $this->propertyFetchToParams; + } + + private function reset(): void + { + $this->propertyFetchToParams = []; + $this->propertyFetchToParamsToRemoveFromConstructor = []; + } + + private function removeAssignsAndParamsFromConstructor(ClassMethod $classMethod): void + { + foreach ($this->propertyFetchToParamsToRemoveFromConstructor as $propertyFetchToRemove => $paramToRemove) { + // remove unused params in constructor + foreach ($classMethod->params as $key => $constructorParam) { + if (! $this->areNamesEqual($constructorParam, $paramToRemove)) { + continue; + } + + unset($classMethod->params[$key]); + } + + foreach ((array) $classMethod->stmts as $key => $constructorStmt) { + $propertyFetchToVariable = $this->resolveAssignPropertyToVariableOrNull($constructorStmt); + if ($propertyFetchToVariable === null) { + continue; + } + + [$propertyFetchName, ] = $propertyFetchToVariable; + if ($propertyFetchName !== $propertyFetchToRemove) { + continue; + } + + // remove the assign + unset($classMethod->stmts[$key]); + } + } + } + + private function resolveAssignPropertyToVariableOrNull(Node $node): ?array + { + if ($node instanceof Expression) { + $node = $node->expr; + } + + if (! $node instanceof Assign) { + return null; + } + + if (! $node->var instanceof PropertyFetch) { + return null; + } + + if (! $node->expr instanceof Variable) { + return null; + } + + $propertyFetchName = $this->getName($node->var); + $variableName = $this->getName($node->expr); + if ($propertyFetchName === null) { + return null; + } + + if ($variableName === null) { + return null; + } + + return [$propertyFetchName, $variableName]; + } + + private function removeUnusedPropertiesAndConstructorParams(Class_ $class, ClassMethod $classMethod): void + { + if ($this->propertyFetchToParamsToRemoveFromConstructor === []) { + return; + } + + $this->removeAssignsAndParamsFromConstructor($classMethod); + $this->removeUnusedProperties($class); + $this->removeEmptyConstruct($class, $classMethod); + } +} diff --git a/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/ConstructorInjectionToActionInjectionRectorTest.php b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/ConstructorInjectionToActionInjectionRectorTest.php new file mode 100644 index 00000000000..3ac8e9e8924 --- /dev/null +++ b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/ConstructorInjectionToActionInjectionRectorTest.php @@ -0,0 +1,25 @@ +doTestFiles([ + __DIR__ . '/Fixture/fixture.php.inc', + __DIR__ . '/Fixture/duplicate.php.inc', + __DIR__ . '/Fixture/skip_scalars.php.inc', + __DIR__ . '/Fixture/skip_non_action_methods.php.inc', + __DIR__ . '/Fixture/manage_different_naming.php.inc', + ]); + } + + protected function getRectorClass(): string + { + return ConstructorInjectionToActionInjectionRector::class; + } +} diff --git a/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/duplicate.php.inc b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/duplicate.php.inc new file mode 100644 index 00000000000..c07dee09914 --- /dev/null +++ b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/duplicate.php.inc @@ -0,0 +1,43 @@ +productRepository = $productRepository; + } + + public function actionInjection(ProductRepository $productRepository) + { + $products = $productRepository->fetchAll(); + $products = $this->productRepository->fetchAll(); + } +} + +?> +----- +fetchAll(); + $products = $productRepository->fetchAll(); + } +} + +?> diff --git a/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/fixture.php.inc b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/fixture.php.inc new file mode 100644 index 00000000000..60f5946e881 --- /dev/null +++ b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/fixture.php.inc @@ -0,0 +1,37 @@ +productRepository = $productRepository; + } + + public function default() + { + $products = $this->productRepository->fetchAll(); + } +} + +?> +----- +fetchAll(); + } +} + +?> diff --git a/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/manage_different_naming.php.inc b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/manage_different_naming.php.inc new file mode 100644 index 00000000000..56ea2f2c0ad --- /dev/null +++ b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/manage_different_naming.php.inc @@ -0,0 +1,41 @@ +repository = $productRepository; + } + + public function index() + { + return $this->repository->get(5); + } +} + +?> +----- +get(5); + } +} + +?> diff --git a/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/skip_non_action_methods.php.inc b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/skip_non_action_methods.php.inc new file mode 100644 index 00000000000..f808803ac83 --- /dev/null +++ b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/skip_non_action_methods.php.inc @@ -0,0 +1,28 @@ +productRepository = $productRepository; + } + + public function index() + { + return $this->isMatch(5); + } + + private function isMatch($value): bool + { + return (bool) $this->productRepository->findBy($value); + } +} diff --git a/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/skip_scalars.php.inc b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/skip_scalars.php.inc new file mode 100644 index 00000000000..a16a5bcaec7 --- /dev/null +++ b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/skip_scalars.php.inc @@ -0,0 +1,21 @@ +items = $items; + } + + public function actionInjection() + { + $item = $this->items[5]; + } +} diff --git a/src/PhpParser/Node/Manipulator/ClassManipulator.php b/src/PhpParser/Node/Manipulator/ClassManipulator.php index b533d7614e2..73d394aaad8 100644 --- a/src/PhpParser/Node/Manipulator/ClassManipulator.php +++ b/src/PhpParser/Node/Manipulator/ClassManipulator.php @@ -6,6 +6,7 @@ use PhpParser\Node; use PhpParser\Node\Expr\Assign; use PhpParser\Node\Expr\PropertyFetch; use PhpParser\Node\Name; +use PhpParser\Node\Param; use PhpParser\Node\Stmt; use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\ClassLike; @@ -260,6 +261,35 @@ final class ClassManipulator return false; } + public function removeProperty(Class_ $class, string $propertyName): void + { + foreach ($class->stmts as $key => $classStmt) { + if (! $classStmt instanceof Node\Stmt\Property) { + continue; + } + + if (! $this->nameResolver->isName($classStmt, $propertyName)) { + continue; + } + + unset($class->stmts[$key]); + break; + } + } + + public function findMethodParamByName(ClassMethod $classMethod, string $name): ?Param + { + foreach ($classMethod->params as $param) { + if (! $this->nameResolver->isName($param, $name)) { + continue; + } + + return $param; + } + + return null; + } + private function tryInsertBeforeFirstMethod(Class_ $classNode, Stmt $stmt): bool { foreach ($classNode->stmts as $key => $classElementNode) {