diff --git a/packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php b/packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php index 9724950a4e26..21250a85ece1 100644 --- a/packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php +++ b/packages/DeadCode/src/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector.php @@ -5,6 +5,8 @@ namespace Rector\DeadCode\Rector\Class_; use PhpParser\Node; +use PhpParser\Node\Expr\PropertyFetch; +use PhpParser\Node\Expr\StaticPropertyFetch; use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\ClassMethod; use PhpParser\Node\Stmt\Interface_; @@ -15,6 +17,7 @@ use Rector\Rector\AbstractRector; use Rector\RectorDefinition\CodeSample; use Rector\RectorDefinition\RectorDefinition; +use Rector\TypeDeclaration\VendorLock\VendorLockResolver; /** * @sponsor Thanks https://spaceflow.io/ for sponsoring this rule - visit them on https://github.com/SpaceFlow-app @@ -28,9 +31,15 @@ final class RemoveSetterOnlyPropertyAndMethodCallRector extends AbstractRector */ private $propertyManipulator; - public function __construct(PropertyManipulator $propertyManipulator) + /** + * @var VendorLockResolver + */ + private $vendorLockResolver; + + public function __construct(PropertyManipulator $propertyManipulator, VendorLockResolver $vendorLockResolver) { $this->propertyManipulator = $propertyManipulator; + $this->vendorLockResolver = $vendorLockResolver; } public function getDefinition(): RectorDefinition @@ -92,29 +101,25 @@ public function refactor(Node $node): ?Node return null; } - if ($this->propertyManipulator->isPropertyUsedInReadContext($node)) { - return null; - } - $propertyFetches = $this->propertyManipulator->getAllPropertyFetch($node); + $classMethodsToCheck = $this->collectClassMethodsToCheck($propertyFetches); - $methodsToCheck = []; - foreach ($propertyFetches as $propertyFetch) { - $methodName = $propertyFetch->getAttribute(AttributeKey::METHOD_NAME); - if ($methodName !== '__construct') { - //this rector does not remove empty constructors - $methodsToCheck[$methodName] = - $propertyFetch->getAttribute(AttributeKey::METHOD_NODE); - } - } + $vendorLockedClassMethodNames = $this->getVendorLockedClassMethodNames($classMethodsToCheck); - $this->removePropertyAndUsages($node); + $this->removePropertyAndUsages($node, $vendorLockedClassMethodNames); /** @var ClassMethod $method */ - foreach ($methodsToCheck as $method) { - if ($this->methodHasNoStmtsLeft($method)) { - $this->removeClassMethodAndUsages($method); + foreach ($classMethodsToCheck as $method) { + if (! $this->methodHasNoStmtsLeft($method)) { + continue; + } + + $classMethodName = $this->getName($method->name); + if (in_array($classMethodName, $vendorLockedClassMethodNames, true)) { + continue; } + + $this->removeClassMethodAndUsages($method); } return $node; @@ -138,6 +143,63 @@ private function shouldSkipProperty(PropertyProperty $propertyProperty): bool /** @var Class_|Interface_|Trait_|null $classNode */ $classNode = $propertyProperty->getAttribute(AttributeKey::CLASS_NODE); - return $classNode === null || $classNode instanceof Trait_ || $classNode instanceof Interface_; + if ($classNode === null) { + return true; + } + + if ($classNode instanceof Trait_) { + return true; + } + + if ($classNode instanceof Interface_) { + return true; + } + + return $this->propertyManipulator->isPropertyUsedInReadContext($propertyProperty); + } + + /** + * @param PropertyFetch[]|StaticPropertyFetch[] $propertyFetches + * @return ClassMethod[] + */ + private function collectClassMethodsToCheck(array $propertyFetches): array + { + $classMethodsToCheck = []; + + foreach ($propertyFetches as $propertyFetch) { + $methodName = $propertyFetch->getAttribute(AttributeKey::METHOD_NAME); + // this rector does not remove empty constructors + if ($methodName === '__construct') { + continue; + } + + /** @var ClassMethod|null $classMethod */ + $classMethod = $propertyFetch->getAttribute(AttributeKey::METHOD_NODE); + if ($classMethod === null) { + continue; + } + + $classMethodsToCheck[$methodName] = $classMethod; + } + + return $classMethodsToCheck; + } + + /** + * @param ClassMethod[] $methodsToCheck + * @return string[] + */ + private function getVendorLockedClassMethodNames(array $methodsToCheck): array + { + $vendorLockedClassMethodsNames = []; + foreach ($methodsToCheck as $method) { + if (! $this->vendorLockResolver->isClassMethodRemovalVendorLocked($method)) { + continue; + } + + $vendorLockedClassMethodsNames[] = $this->getName($method); + } + + return $vendorLockedClassMethodsNames; } } diff --git a/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/skip_abstract_class_required.php.inc b/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/skip_abstract_class_required.php.inc new file mode 100644 index 000000000000..5b3665c6561b --- /dev/null +++ b/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/skip_abstract_class_required.php.inc @@ -0,0 +1,17 @@ +name = $name; + } +} + +abstract class AbstractClassRequiring +{ + protected abstract function setName(string $name); +} diff --git a/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/skip_interface_required.php.inc b/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/skip_interface_required.php.inc new file mode 100644 index 000000000000..7b4eefacb6a9 --- /dev/null +++ b/packages/DeadCode/tests/Rector/Class_/RemoveSetterOnlyPropertyAndMethodCallRector/Fixture/skip_interface_required.php.inc @@ -0,0 +1,17 @@ +name = $name; + } +} + +interface ParentInterface +{ + public function setName(string $name): void; +} diff --git a/packages/TypeDeclaration/src/VendorLock/VendorLockResolver.php b/packages/TypeDeclaration/src/VendorLock/VendorLockResolver.php index 6a51997e1df3..4653f35e6cc0 100644 --- a/packages/TypeDeclaration/src/VendorLock/VendorLockResolver.php +++ b/packages/TypeDeclaration/src/VendorLock/VendorLockResolver.php @@ -15,6 +15,7 @@ use Rector\NodeTypeResolver\Node\AttributeKey; use Rector\PhpParser\Node\Manipulator\ClassManipulator; use Rector\PhpParser\Node\Resolver\NameResolver; +use ReflectionClass; final class VendorLockResolver { @@ -212,6 +213,65 @@ public function isPropertyChangeVendorLockedIn(Property $property): bool return false; } + /** + * Checks for: + * - interface required methods + * - abstract classes reqired method + * + * Prevent: + * - removing class methods, that breaks the code + */ + public function isClassMethodRemovalVendorLocked(ClassMethod $classMethod): bool + { + $classMethodName = $this->nameResolver->getName($classMethod); + + /** @var Class_|null $class */ + $class = $classMethod->getAttribute(AttributeKey::CLASS_NODE); + if ($class === null) { + return false; + } + + // required by interface? + foreach ($class->implements as $implement) { + $implementedInterfaceName = $this->nameResolver->getName($implement); + + if (interface_exists($implementedInterfaceName)) { + $interfaceMethods = get_class_methods($implementedInterfaceName); + if (in_array($classMethodName, $interfaceMethods, true)) { + return true; + } + } + } + + if ($class->extends === null) { + return false; + } + + /** @var string $className */ + $className = $classMethod->getAttribute(AttributeKey::CLASS_NAME); + + $classParents = class_parents($className); + foreach ($classParents as $classParent) { + if (! class_exists($classParent)) { + continue; + } + + $parentClassReflection = new ReflectionClass($classParent); + if (! $parentClassReflection->hasMethod($classMethodName)) { + continue; + } + + $methodReflection = $parentClassReflection->getMethod($classMethodName); + if (! $methodReflection->isAbstract()) { + continue; + } + + return true; + } + + return false; + } + // Until we have getProperty (https://github.com/nikic/PHP-Parser/pull/646) private function getProperty(ClassLike $classLike, string $name) { diff --git a/src/Rector/AbstractRector/ComplexRemovalTrait.php b/src/Rector/AbstractRector/ComplexRemovalTrait.php index 991e638db217..c9be4f2ca802 100644 --- a/src/Rector/AbstractRector/ComplexRemovalTrait.php +++ b/src/Rector/AbstractRector/ComplexRemovalTrait.php @@ -87,10 +87,22 @@ protected function removeClassMethodAndUsages(ClassMethod $classMethod): void } } - protected function removePropertyAndUsages(PropertyProperty $propertyProperty): void - { + /** + * @param string[] $classMethodNamesToSkip + */ + protected function removePropertyAndUsages( + PropertyProperty $propertyProperty, + array $classMethodNamesToSkip = [] + ): void { + $shouldKeepProperty = false; + $propertyFetches = $this->propertyManipulator->getAllPropertyFetch($propertyProperty); foreach ($propertyFetches as $propertyFetch) { + if ($this->shouldSkipPropertyForClassMethod($propertyFetch, $classMethodNamesToSkip)) { + $shouldKeepProperty = true; + continue; + } + $assign = $propertyFetch->getAttribute(AttributeKey::PARENT_NODE); while ($assign !== null && ! $assign instanceof Assign) { @@ -104,6 +116,10 @@ protected function removePropertyAndUsages(PropertyProperty $propertyProperty): $this->removeAssignNode($assign); } + if ($shouldKeepProperty) { + return; + } + /** @var Property $property */ $property = $propertyProperty->getAttribute(AttributeKey::PARENT_NODE); $this->removeNode($propertyProperty); @@ -223,4 +239,24 @@ private function removeMethodCall(Node $node): void } $this->removeNode($node); } + + /** + * @param StaticPropertyFetch|PropertyFetch $expr + * @param string[] $classMethodNamesToSkip + */ + private function shouldSkipPropertyForClassMethod(Expr $expr, array $classMethodNamesToSkip): bool + { + /** @var ClassMethod|null $classMethodNode */ + $classMethodNode = $expr->getAttribute(AttributeKey::METHOD_NODE); + if ($classMethodNode === null) { + return false; + } + + $classMethodName = $this->getName($classMethodNode); + if ($classMethodName === null) { + return false; + } + + return in_array($classMethodName, $classMethodNamesToSkip, true); + } }