From 4dcdf4c890bdde269a37cb840d3595edd6e4b1d9 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Wed, 11 Sep 2019 16:10:47 +0200 Subject: [PATCH 1/2] cleanup --- .../src/PhpDocInfo/PhpDocInfo.php | 17 +++++++++++++++++ .../DoctrineRelationTagValueNodeInterface.php | 4 +++- .../Property/PropertyTypeDeclarationRector.php | 4 +++- .../Property/InjectAnnotationClassRector.php | 2 +- 4 files changed, 24 insertions(+), 3 deletions(-) diff --git a/packages/BetterPhpDocParser/src/PhpDocInfo/PhpDocInfo.php b/packages/BetterPhpDocParser/src/PhpDocInfo/PhpDocInfo.php index 7da5920175c..2c5e3935514 100644 --- a/packages/BetterPhpDocParser/src/PhpDocInfo/PhpDocInfo.php +++ b/packages/BetterPhpDocParser/src/PhpDocInfo/PhpDocInfo.php @@ -15,6 +15,7 @@ use Rector\BetterPhpDocParser\Attributes\Ast\PhpDoc\AttributeAwarePhpDocNode; use Rector\BetterPhpDocParser\Attributes\Ast\PhpDoc\AttributeAwareReturnTagValueNode; use Rector\BetterPhpDocParser\Attributes\Ast\PhpDoc\AttributeAwareVarTagValueNode; use Rector\BetterPhpDocParser\Attributes\Contract\Ast\AttributeAwareNodeInterface; +use Rector\Exception\ShouldNotHappenException; use Rector\NodeTypeResolver\StaticTypeMapper; /** @@ -189,6 +190,8 @@ final class PhpDocInfo public function getByType(string $type): ?PhpDocTagValueNode { + $this->ensureTypeIsTagValueNode($type, __METHOD__); + foreach ($this->phpDocNode->children as $phpDocChildNode) { if ($phpDocChildNode instanceof PhpDocTagNode) { if (is_a($phpDocChildNode->value, $type, true)) { @@ -213,4 +216,18 @@ final class PhpDocInfo return null; } + + private function ensureTypeIsTagValueNode(string $type, string $location): void + { + if (is_a($type, PhpDocTagValueNode::class, true)) { + return; + } + + throw new ShouldNotHappenException(sprintf( + 'Type "%s" passed to "%s()" method must be child of "%s"', + $type, + $location, + PhpDocTagValueNode::class + )); + } } diff --git a/packages/DoctrinePhpDocParser/src/Contract/Ast/PhpDoc/DoctrineRelationTagValueNodeInterface.php b/packages/DoctrinePhpDocParser/src/Contract/Ast/PhpDoc/DoctrineRelationTagValueNodeInterface.php index 3047b47a8f4..2a4ce3b1d71 100644 --- a/packages/DoctrinePhpDocParser/src/Contract/Ast/PhpDoc/DoctrineRelationTagValueNodeInterface.php +++ b/packages/DoctrinePhpDocParser/src/Contract/Ast/PhpDoc/DoctrineRelationTagValueNodeInterface.php @@ -2,7 +2,9 @@ namespace Rector\DoctrinePhpDocParser\Contract\Ast\PhpDoc; -interface DoctrineRelationTagValueNodeInterface +use PHPStan\PhpDocParser\Ast\PhpDoc\PhpDocTagValueNode; + +interface DoctrineRelationTagValueNodeInterface extends PhpDocTagValueNode { public function getTargetEntity(): ?string; diff --git a/packages/TypeDeclaration/src/Rector/Property/PropertyTypeDeclarationRector.php b/packages/TypeDeclaration/src/Rector/Property/PropertyTypeDeclarationRector.php index 5fecf195f97..aaa93accf5a 100644 --- a/packages/TypeDeclaration/src/Rector/Property/PropertyTypeDeclarationRector.php +++ b/packages/TypeDeclaration/src/Rector/Property/PropertyTypeDeclarationRector.php @@ -57,7 +57,9 @@ final class PropertyTypeDeclarationRector extends AbstractRector return null; } - if ($this->docBlockManipulator->hasTag($node, '@var')) { + // is already set + $currentVarType = $this->docBlockManipulator->getVarType($node); + if (! $currentVarType instanceof MixedType) { return null; } diff --git a/src/Rector/Property/InjectAnnotationClassRector.php b/src/Rector/Property/InjectAnnotationClassRector.php index 3a306bcdf5c..5a7d3210ec1 100644 --- a/src/Rector/Property/InjectAnnotationClassRector.php +++ b/src/Rector/Property/InjectAnnotationClassRector.php @@ -165,7 +165,7 @@ CODE_SAMPLE return null; } - if (! $this->docBlockManipulator->hasTag($property, 'var')) { + if (! $this->docBlockManipulator->getVarType()) { $this->docBlockManipulator->changeVarTag($property, $type); } From 6f983ad302cc82f8fa0b30c39fc6a356d651ab62 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Wed, 11 Sep 2019 20:09:16 +0200 Subject: [PATCH 2/2] improve getByType() --- .../Doctrine/DoctrineEntityManipulator.php | 4 +-- .../NodeAnalyzer/DocBlockManipulator.php | 5 +++ ...ertyInjectToConstructorInjectionRector.php | 36 ++++++++++--------- .../Property/InjectAnnotationClassRector.php | 6 ++-- ...InjectToConstructorInjectionRectorTest.php | 11 ++---- 5 files changed, 31 insertions(+), 31 deletions(-) diff --git a/packages/DeadCode/src/Doctrine/DoctrineEntityManipulator.php b/packages/DeadCode/src/Doctrine/DoctrineEntityManipulator.php index 462e9499fa6..577af866d81 100644 --- a/packages/DeadCode/src/Doctrine/DoctrineEntityManipulator.php +++ b/packages/DeadCode/src/Doctrine/DoctrineEntityManipulator.php @@ -3,10 +3,10 @@ namespace Rector\DeadCode\Doctrine; use Doctrine\ORM\Mapping\Entity; -use Doctrine\ORM\Mapping\InheritanceType; use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\Property; use Rector\DoctrinePhpDocParser\Ast\PhpDoc\Class_\EntityTagValueNode; +use Rector\DoctrinePhpDocParser\Ast\PhpDoc\Class_\InheritanceTypeTagValueNode; use Rector\DoctrinePhpDocParser\Contract\Ast\PhpDoc\DoctrineRelationTagValueNodeInterface; use Rector\DoctrinePhpDocParser\Contract\Ast\PhpDoc\InversedByNodeInterface; use Rector\DoctrinePhpDocParser\Contract\Ast\PhpDoc\MappedByNodeInterface; @@ -71,7 +71,7 @@ final class DoctrineEntityManipulator } // is parent entity - if ($this->docBlockManipulator->hasTag($class, InheritanceType::class)) { + if ($this->docBlockManipulator->hasTag($class, InheritanceTypeTagValueNode::class)) { return false; } diff --git a/packages/NodeTypeResolver/src/PhpDoc/NodeAnalyzer/DocBlockManipulator.php b/packages/NodeTypeResolver/src/PhpDoc/NodeAnalyzer/DocBlockManipulator.php index 8bcf76fad77..6be2cc2f6c0 100644 --- a/packages/NodeTypeResolver/src/PhpDoc/NodeAnalyzer/DocBlockManipulator.php +++ b/packages/NodeTypeResolver/src/PhpDoc/NodeAnalyzer/DocBlockManipulator.php @@ -109,6 +109,11 @@ final class DocBlockManipulator return true; } + // allow only class nodes further + if (! class_exists($name)) { + return false; + } + // advanced check, e.g. for "Namespaced\Annotations\DI" $phpDocInfo = $this->createPhpDocInfoFromNode($node); diff --git a/src/Rector/Architecture/DependencyInjection/AnnotatedPropertyInjectToConstructorInjectionRector.php b/src/Rector/Architecture/DependencyInjection/AnnotatedPropertyInjectToConstructorInjectionRector.php index b6dd65a438b..4f39f974890 100644 --- a/src/Rector/Architecture/DependencyInjection/AnnotatedPropertyInjectToConstructorInjectionRector.php +++ b/src/Rector/Architecture/DependencyInjection/AnnotatedPropertyInjectToConstructorInjectionRector.php @@ -9,7 +9,7 @@ use PHPStan\Type\UnionType; use Rector\NodeTypeResolver\Node\AttributeKey; use Rector\NodeTypeResolver\PhpDoc\NodeAnalyzer\DocBlockManipulator; use Rector\Rector\AbstractRector; -use Rector\RectorDefinition\ConfiguredCodeSample; +use Rector\RectorDefinition\CodeSample; use Rector\RectorDefinition\RectorDefinition; /** @@ -26,17 +26,16 @@ final class AnnotatedPropertyInjectToConstructorInjectionRector extends Abstract /** * @var string */ - private $annotation; + private const INJECT_ANNOTATION = 'inject'; /** * @var DocBlockManipulator */ private $docBlockManipulator; - public function __construct(DocBlockManipulator $docBlockManipulator, string $annotation = 'inject') + public function __construct(DocBlockManipulator $docBlockManipulator) { $this->docBlockManipulator = $docBlockManipulator; - $this->annotation = $annotation; } public function getDefinition(): RectorDefinition @@ -44,7 +43,7 @@ final class AnnotatedPropertyInjectToConstructorInjectionRector extends Abstract return new RectorDefinition( 'Turns non-private properties with `@annotation` to private properties and constructor injection', [ - new ConfiguredCodeSample( + new CodeSample( <<<'CODE_SAMPLE' /** * @var SomeService @@ -64,10 +63,6 @@ public function __construct(SomeService $someService) $this->someService = $someService; } CODE_SAMPLE - , - [ - '$annotation' => 'inject', - ] ), ] ); @@ -86,16 +81,11 @@ CODE_SAMPLE */ public function refactor(Node $node): ?Node { - if (! $this->docBlockManipulator->hasTag($node, $this->annotation)) { + if ($this->shouldSkipProperty($node)) { return null; } - // it needs @var tag as well, to get the type - if (! $this->docBlockManipulator->hasTag($node, 'var')) { - return null; - } - - $this->docBlockManipulator->removeTagFromNode($node, $this->annotation); + $this->docBlockManipulator->removeTagFromNode($node, self::INJECT_ANNOTATION); // set to private $this->makePrivate($node); @@ -124,4 +114,18 @@ CODE_SAMPLE $this->addPropertyToClass($classNode, $propertyType, $propertyName); } + + private function shouldSkipProperty(Node $node): bool + { + if (! $this->docBlockManipulator->hasTag($node, self::INJECT_ANNOTATION)) { + return true; + } + + // it needs @var tag as well, to get the type + if (! $this->docBlockManipulator->hasTag($node, 'var')) { + return true; + } + + return false; + } } diff --git a/src/Rector/Property/InjectAnnotationClassRector.php b/src/Rector/Property/InjectAnnotationClassRector.php index 5a7d3210ec1..e4357061aec 100644 --- a/src/Rector/Property/InjectAnnotationClassRector.php +++ b/src/Rector/Property/InjectAnnotationClassRector.php @@ -142,6 +142,7 @@ CODE_SAMPLE $tagClass = $this->annotationToTagClass[$annotationClass]; $injectTagValueNode = $phpDocInfo->getByType($tagClass); + if ($injectTagValueNode === null) { continue; } @@ -165,10 +166,7 @@ CODE_SAMPLE return null; } - if (! $this->docBlockManipulator->getVarType()) { - $this->docBlockManipulator->changeVarTag($property, $type); - } - + $this->docBlockManipulator->changeVarTag($property, $type); $this->docBlockManipulator->removeTagFromNode($property, $tagClass); $classNode = $property->getAttribute(AttributeKey::CLASS_NODE); diff --git a/tests/Rector/Architecture/DependencyInjection/AnnotatedPropertyInjectToConstructorInjectionRector/AnnotatedPropertyInjectToConstructorInjectionRectorTest.php b/tests/Rector/Architecture/DependencyInjection/AnnotatedPropertyInjectToConstructorInjectionRector/AnnotatedPropertyInjectToConstructorInjectionRectorTest.php index 4cd9065ba05..487dde92428 100644 --- a/tests/Rector/Architecture/DependencyInjection/AnnotatedPropertyInjectToConstructorInjectionRector/AnnotatedPropertyInjectToConstructorInjectionRectorTest.php +++ b/tests/Rector/Architecture/DependencyInjection/AnnotatedPropertyInjectToConstructorInjectionRector/AnnotatedPropertyInjectToConstructorInjectionRectorTest.php @@ -24,15 +24,8 @@ final class AnnotatedPropertyInjectToConstructorInjectionRectorTest extends Abst ]); } - /** - * @return mixed[] - */ - protected function getRectorsWithConfiguration(): array + protected function getRectorClass(): string { - return [ - AnnotatedPropertyInjectToConstructorInjectionRector::class => [ - '$annotation' => 'inject', - ], - ]; + return AnnotatedPropertyInjectToConstructorInjectionRector::class; } }