From ba5cbf381e8d3cc2752ddfc458ef3f2a7b99c7b0 Mon Sep 17 00:00:00 2001 From: Jeroen Smit Date: Thu, 3 Oct 2019 11:25:00 +0200 Subject: [PATCH 1/2] Added example of broken situation --- ...orInjectionToActionInjectionRectorTest.php | 1 + .../extra_calls_in_constructor.inc.php | 47 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/extra_calls_in_constructor.inc.php diff --git a/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/ConstructorInjectionToActionInjectionRectorTest.php b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/ConstructorInjectionToActionInjectionRectorTest.php index a5de6598863..e2d74cd196c 100644 --- a/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/ConstructorInjectionToActionInjectionRectorTest.php +++ b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/ConstructorInjectionToActionInjectionRectorTest.php @@ -23,6 +23,7 @@ final class ConstructorInjectionToActionInjectionRectorTest extends AbstractRect yield [__DIR__ . '/Fixture/skip_scalars.php.inc']; yield [__DIR__ . '/Fixture/skip_non_action_methods.php.inc']; yield [__DIR__ . '/Fixture/manage_different_naming.php.inc']; + yield [__DIR__ . '/Fixture/extra_calls_in_constructor.inc.php']; } protected function getRectorClass(): string diff --git a/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/extra_calls_in_constructor.inc.php b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/extra_calls_in_constructor.inc.php new file mode 100644 index 00000000000..bc0d9d208cb --- /dev/null +++ b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/extra_calls_in_constructor.inc.php @@ -0,0 +1,47 @@ +productRepository = $productRepository; + $this->default = $productRepository->getDefault(); + } + + public function default() + { + $products = $this->productRepository->fetchAll(); + } +} + +?> +----- +default = $productRepository->getDefault(); + } + public function default(ProductRepository $productRepository) + { + $products = $productRepository->fetchAll(); + } +} + +?> From 2f06f0f836edf920258434bac870dc0555c53c81 Mon Sep 17 00:00:00 2001 From: Jeroen Smit Date: Sat, 5 Oct 2019 20:31:05 +0200 Subject: [PATCH 2/2] Fix for issue #2090 --- ...ructorInjectionToActionInjectionRector.php | 79 ++++++++++--------- ...orInjectionToActionInjectionRectorTest.php | 1 + .../extra_calls_in_constructor.inc.php | 17 +++- .../Fixture/multiple.php.inc | 64 +++++++++++++++ .../Manipulator/ClassMethodManipulator.php | 20 +++++ 5 files changed, 141 insertions(+), 40 deletions(-) create mode 100644 packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/multiple.php.inc diff --git a/packages/Architecture/src/Rector/Class_/ConstructorInjectionToActionInjectionRector.php b/packages/Architecture/src/Rector/Class_/ConstructorInjectionToActionInjectionRector.php index 7705b9d06f1..eb211f0d7a6 100644 --- a/packages/Architecture/src/Rector/Class_/ConstructorInjectionToActionInjectionRector.php +++ b/packages/Architecture/src/Rector/Class_/ConstructorInjectionToActionInjectionRector.php @@ -13,6 +13,7 @@ use PhpParser\Node\Stmt\Expression; use Rector\NodeTypeResolver\Node\AttributeKey; use Rector\Php\TypeAnalyzer; use Rector\PhpParser\Node\Manipulator\ClassManipulator; +use Rector\PhpParser\Node\Manipulator\ClassMethodManipulator; use Rector\Rector\AbstractRector; use Rector\RectorDefinition\CodeSample; use Rector\RectorDefinition\RectorDefinition; @@ -42,10 +43,19 @@ final class ConstructorInjectionToActionInjectionRector extends AbstractRector */ private $propertyFetchToParamsToRemoveFromConstructor = []; - public function __construct(ClassManipulator $classManipulator, TypeAnalyzer $typeAnalyzer) - { + /** + * @var ClassMethodManipulator + */ + private $classMethodManipulator; + + public function __construct( + ClassManipulator $classManipulator, + TypeAnalyzer $typeAnalyzer, + ClassMethodManipulator $classMethodManipulator + ) { $this->classManipulator = $classManipulator; $this->typeAnalyzer = $typeAnalyzer; + $this->classMethodManipulator = $classMethodManipulator; } public function getDefinition(): RectorDefinition @@ -128,7 +138,9 @@ PHP continue; } - $this->replacePropertyFetchByInjectedVariables($classMethod); + foreach ($this->propertyFetchToParams as $propertyFetchName => $param) { + $this->changePropertyUsageToParameter($classMethod, $propertyFetchName, $param); + } } // collect all property fetches that are relevant to original constructor properties @@ -155,25 +167,25 @@ PHP return $node; } - private function replacePropertyFetchByInjectedVariables(ClassMethod $classMethod): void + private function changePropertyUsageToParameter(ClassMethod $classMethod, string $propertyName, Param $param): void { $currentlyAddedLocalVariables = []; $this->traverseNodesWithCallable((array) $classMethod->stmts, function (Node $node) use ( + $propertyName, + $param, &$currentlyAddedLocalVariables ): ?Variable { if (! $node instanceof PropertyFetch) { return null; } - foreach ($this->propertyFetchToParams as $propertyFetchName => $param) { - if ($this->isName($node, $propertyFetchName)) { - $currentlyAddedLocalVariables[] = $param; + if ($this->isName($node, $propertyName)) { + $currentlyAddedLocalVariables[] = $param; - /** @var string $paramName */ - $paramName = $this->getName($param); - return new Variable($paramName); - } + /** @var string $paramName */ + $paramName = $this->getName($param); + return new Variable($paramName); } return null; @@ -270,41 +282,30 @@ PHP private function removeUnusedPropertiesAndConstructorParams(Class_ $class, ClassMethod $classMethod): void { - if ($this->propertyFetchToParamsToRemoveFromConstructor === []) { - return; + $this->removeAssignsFromConstructor($classMethod); + foreach ($this->propertyFetchToParamsToRemoveFromConstructor as $propertyFetchName => $param) { + $this->changePropertyUsageToParameter($classMethod, $propertyFetchName, $param); } - - $this->removeAssignsAndParamsFromConstructor($classMethod); + $this->classMethodManipulator->removeUnusedParameters($classMethod); $this->removeUnusedProperties($class); - $this->removeEmptyConstruct($class, $classMethod); + $this->removeConstructIfEmpty($class, $classMethod); } - private function removeAssignsAndParamsFromConstructor(ClassMethod $classMethod): void + private function removeAssignsFromConstructor(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; } - 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]); + [$propertyFetchName, ] = $propertyFetchToVariable; + if (! isset($this->propertyFetchToParamsToRemoveFromConstructor[$propertyFetchName])) { + continue; } + + // remove the assign + unset($classMethod->stmts[$key]); } } @@ -316,7 +317,7 @@ PHP } } - private function removeEmptyConstruct(Class_ $class, ClassMethod $constructClassMethod): void + private function removeConstructIfEmpty(Class_ $class, ClassMethod $constructClassMethod): void { if ($constructClassMethod->stmts !== []) { return; diff --git a/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/ConstructorInjectionToActionInjectionRectorTest.php b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/ConstructorInjectionToActionInjectionRectorTest.php index e2d74cd196c..0adf31a1de3 100644 --- a/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/ConstructorInjectionToActionInjectionRectorTest.php +++ b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/ConstructorInjectionToActionInjectionRectorTest.php @@ -19,6 +19,7 @@ final class ConstructorInjectionToActionInjectionRectorTest extends AbstractRect public function provideDataForTest(): Iterator { yield [__DIR__ . '/Fixture/fixture.php.inc']; + yield [__DIR__ . '/Fixture/multiple.php.inc']; yield [__DIR__ . '/Fixture/duplicate.php.inc']; yield [__DIR__ . '/Fixture/skip_scalars.php.inc']; yield [__DIR__ . '/Fixture/skip_non_action_methods.php.inc']; diff --git a/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/extra_calls_in_constructor.inc.php b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/extra_calls_in_constructor.inc.php index bc0d9d208cb..0ff8e64592d 100644 --- a/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/extra_calls_in_constructor.inc.php +++ b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/extra_calls_in_constructor.inc.php @@ -14,10 +14,16 @@ final class SomeController */ private $default; + /** + * @var int + */ + private $count; + public function __construct(ProductRepository $productRepository) { $this->productRepository = $productRepository; $this->default = $productRepository->getDefault(); + $this->count = $this->productRepository->getCount(); } public function default() @@ -34,9 +40,18 @@ namespace Rector\Architecture\Tests\Rector\Class_\ConstructorInjectionToActionIn final class SomeController { - public function __construct() + /** + * @var Product + */ + private $default; + /** + * @var int + */ + private $count; + public function __construct(ProductRepository $productRepository) { $this->default = $productRepository->getDefault(); + $this->count = $productRepository->getCount(); } public function default(ProductRepository $productRepository) { diff --git a/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/multiple.php.inc b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/multiple.php.inc new file mode 100644 index 00000000000..b8512c07796 --- /dev/null +++ b/packages/Architecture/tests/Rector/Class_/ConstructorInjectionToActionInjectionRector/Fixture/multiple.php.inc @@ -0,0 +1,64 @@ +productRepository = $productRepository; + $this->responseFactory = $responseFactory; + } + + public function index() + { + return $this->responseFactory->repond($this->isMatch(5)); + } + + private function isMatch($value): bool + { + return (bool) $this->productRepository->findBy($value); + } +} +?> +----- +productRepository = $productRepository; + } + + public function index(ResponseFactory $responseFactory) + { + return $responseFactory->repond($this->isMatch(5)); + } + + private function isMatch($value): bool + { + return (bool) $this->productRepository->findBy($value); + } +} +?> diff --git a/src/PhpParser/Node/Manipulator/ClassMethodManipulator.php b/src/PhpParser/Node/Manipulator/ClassMethodManipulator.php index 0a1c64e678a..bf48abc2514 100644 --- a/src/PhpParser/Node/Manipulator/ClassMethodManipulator.php +++ b/src/PhpParser/Node/Manipulator/ClassMethodManipulator.php @@ -151,6 +151,26 @@ final class ClassMethodManipulator return $paramName; } + public function removeParameter(Param $param, ClassMethod $classMethod): void + { + foreach ($classMethod->params as $key => $constructorParam) { + if (! $this->nameResolver->areNamesEqual($constructorParam, $param)) { + continue; + } + + unset($classMethod->params[$key]); + } + } + + public function removeUnusedParameters(ClassMethod $classMethod): void + { + foreach ($classMethod->getParams() as $param) { + if (! $this->isParameterUsedMethod($param, $classMethod)) { + $this->removeParameter($param, $classMethod); + } + } + } + private function isMethodInParent(string $class, string $method): bool { foreach (class_parents($class) as $parentClass) {