diff --git a/config/set/coding-style/coding-style.yaml b/config/set/coding-style/coding-style.yaml index e86ca88c4db2..fd9dd8826b85 100644 --- a/config/set/coding-style/coding-style.yaml +++ b/config/set/coding-style/coding-style.yaml @@ -26,3 +26,4 @@ services: Rector\CodingStyle\Rector\Encapsed\EncapsedStringsToSprintfRector: ~ Rector\CodingStyle\Rector\ClassMethod\NewlineBeforeNewAssignSetRector: ~ Rector\CodingStyle\Rector\String_\ManualJsonStringToJsonEncodeArrayRector: ~ + Rector\CodingStyle\Rector\Class_\AddArrayDefaultToArrayPropertyRector: ~ diff --git a/packages/CodingStyle/src/Rector/Class_/AddArrayDefaultToArrayPropertyRector.php b/packages/CodingStyle/src/Rector/Class_/AddArrayDefaultToArrayPropertyRector.php new file mode 100644 index 000000000000..ba0267fd4eaf --- /dev/null +++ b/packages/CodingStyle/src/Rector/Class_/AddArrayDefaultToArrayPropertyRector.php @@ -0,0 +1,270 @@ +docBlockManipulator = $docBlockManipulator; + } + + public function getDefinition(): RectorDefinition + { + return new RectorDefinition('Adds array default value to property to prevent foreach over null error', [ + new CodeSample( + <<<'CODE_SAMPLE' +class SomeClass +{ + /** + * @var int[] + */ + private $values; + + public function isEmpty() + { + return $this->values === null; + } +} +CODE_SAMPLE + , + <<<'CODE_SAMPLE' +class SomeClass +{ + /** + * @var int[] + */ + private $values = []; + + public function isEmpty() + { + return $this->values === []; + } +} +CODE_SAMPLE + ), + ]); + } + + /** + * @return string[] + */ + public function getNodeTypes(): array + { + return [Class_::class]; + } + + /** + * @param Class_ $node + */ + public function refactor(Node $node): ?Node + { + $changedProperties = $this->collectPropertyNamesWithMissingDefaultArray($node); + + $this->completeDefaultArrayToPropertyNames($node, $changedProperties); + + // $this->variable !== null && count($this->variable) > 0 → count($this->variable) > 0 + $this->clearNotNullBeforeCount($node, $changedProperties); + + // $this->variable === null → $this->variable === [] + $this->replaceNullComparisonOfArrayPropertiesWithArrayComparison($node, $changedProperties); + + return $node; + } + + /** + * @param string[] $changedProperties + */ + private function isLocalPropertyFetchOfNames(Expr $expr, array $changedProperties): bool + { + if (! $expr instanceof PropertyFetch) { + return false; + } + + if (! $this->isName($expr->var, 'this')) { + return false; + } + + return $this->isNames($expr->name, $changedProperties); + } + + /** + * @return string[] + */ + private function collectPropertyNamesWithMissingDefaultArray(Class_ $class): array + { + $propertyNames = []; + $this->traverseNodesWithCallable($class, function (Node $node) use (&$propertyNames) { + if (! $node instanceof PropertyProperty) { + return null; + } + + if ($node->default) { + return null; + } + + /** @var Node\Stmt\Property $property */ + $property = $node->getAttribute(AttributeKey::PARENT_NODE); + + // we need docblock + if ($property->getDocComment() === null) { + return null; + } + + $varTypeInfo = $this->docBlockManipulator->getVarTypeInfo($property); + if ($varTypeInfo === null) { + return null; + } + + if (! $varTypeInfo->isIterable()) { + return null; + } + + // skip nullable + if ($varTypeInfo->isNullable()) { + return null; + } + + $propertyNames[] = $this->getName($node); + + return null; + }); + + return $propertyNames; + } + + /** + * @param string[] $propertyNames + */ + private function completeDefaultArrayToPropertyNames(Class_ $node, array $propertyNames): void + { + $this->traverseNodesWithCallable($node, function (Node $node) use ($propertyNames): ?PropertyProperty { + if (! $node instanceof PropertyProperty) { + return null; + } + + if (! $this->isNames($node, $propertyNames)) { + return null; + } + + $node->default = new Array_(); + + return $node; + }); + } + + /** + * @param string[] $propertyNames + */ + private function replaceNullComparisonOfArrayPropertiesWithArrayComparison( + Class_ $class, + array $propertyNames + ): void { + // replace comparison to "null" with "[]" + $this->traverseNodesWithCallable($class, function (Node $node) use ($propertyNames): ?BinaryOp { + if (! $node instanceof BinaryOp) { + return null; + } + + if ($this->isLocalPropertyFetchOfNames($node->left, $propertyNames) && $this->isNull($node->right)) { + $node->right = new Array_(); + } + + if ($this->isLocalPropertyFetchOfNames($node->right, $propertyNames) && $this->isNull($node->left)) { + $node->left = new Array_(); + } + + return $node; + }); + } + + /** + * @param string[] $propertyNames + */ + private function clearNotNullBeforeCount(Class_ $class, array $propertyNames): void + { + $this->traverseNodesWithCallable($class, function (Node $node) use ($propertyNames) { + if (! $node instanceof BooleanAnd) { + return null; + } + + // $this->value !== null + if (! $this->isLocalPropertyOfNamesNotIdenticalToNull($node->left, $propertyNames)) { + return null; + } + + $isNextNodeCountingProperty = (bool) $this->betterNodeFinder->findFirst($node->right, function (Node $node) use ( + $propertyNames + ): ?bool { + if (! $node instanceof Expr\FuncCall) { + return null; + } + + if (! $this->isName($node, 'count')) { + return null; + } + + if (! isset($node->args[0])) { + return null; + } + + $countedArgument = $node->args[0]->value; + if (! $countedArgument instanceof PropertyFetch) { + return null; + } + + return $this->isNames($countedArgument, $propertyNames); + }); + + if ($isNextNodeCountingProperty === false) { + return null; + } + + return $node->right; + }); + } + + /** + * @param string[] $propertyNames + */ + private function isLocalPropertyOfNamesNotIdenticalToNull(Expr $expr, array $propertyNames): bool + { + if (! $expr instanceof NotIdentical) { + return false; + } + + if ($this->isLocalPropertyFetchOfNames($expr->left, $propertyNames) && $this->isNull($expr->right)) { + return true; + } + + if ($this->isLocalPropertyFetchOfNames($expr->right, $propertyNames) && $this->isNull($expr->left)) { + return true; + } + + return false; + } +} diff --git a/packages/CodingStyle/tests/Rector/Class_/AddArrayDefaultToArrayPropertyRector/AddArrayDefaultToArrayPropertyRectorTest.php b/packages/CodingStyle/tests/Rector/Class_/AddArrayDefaultToArrayPropertyRector/AddArrayDefaultToArrayPropertyRectorTest.php new file mode 100644 index 000000000000..06ff66533f55 --- /dev/null +++ b/packages/CodingStyle/tests/Rector/Class_/AddArrayDefaultToArrayPropertyRector/AddArrayDefaultToArrayPropertyRectorTest.php @@ -0,0 +1,23 @@ +doTestFiles([ + __DIR__ . '/Fixture/fixture.php.inc', + __DIR__ . '/Fixture/count_on_null.php.inc', + __DIR__ . '/Fixture/skip_nullable_array.php.inc', + ]); + } + + protected function getRectorClass(): string + { + return AddArrayDefaultToArrayPropertyRector::class; + } +} diff --git a/packages/CodingStyle/tests/Rector/Class_/AddArrayDefaultToArrayPropertyRector/Fixture/count_on_null.php.inc b/packages/CodingStyle/tests/Rector/Class_/AddArrayDefaultToArrayPropertyRector/Fixture/count_on_null.php.inc new file mode 100644 index 000000000000..d5e72d1b64a4 --- /dev/null +++ b/packages/CodingStyle/tests/Rector/Class_/AddArrayDefaultToArrayPropertyRector/Fixture/count_on_null.php.inc @@ -0,0 +1,47 @@ +values !== null && count($this->values) > 0; + } + + public function isEmptyOtherSide() + { + return null !== $this->values && count($this->values); + } +} + +?> +----- +values) > 0; + } + + public function isEmptyOtherSide() + { + return count($this->values); + } +} + +?> diff --git a/packages/CodingStyle/tests/Rector/Class_/AddArrayDefaultToArrayPropertyRector/Fixture/fixture.php.inc b/packages/CodingStyle/tests/Rector/Class_/AddArrayDefaultToArrayPropertyRector/Fixture/fixture.php.inc new file mode 100644 index 000000000000..fa51ddf1ba91 --- /dev/null +++ b/packages/CodingStyle/tests/Rector/Class_/AddArrayDefaultToArrayPropertyRector/Fixture/fixture.php.inc @@ -0,0 +1,37 @@ +values === null; + } +} + +?> +----- +values === []; + } +} + +?> diff --git a/packages/CodingStyle/tests/Rector/Class_/AddArrayDefaultToArrayPropertyRector/Fixture/skip_nullable_array.php.inc b/packages/CodingStyle/tests/Rector/Class_/AddArrayDefaultToArrayPropertyRector/Fixture/skip_nullable_array.php.inc new file mode 100644 index 000000000000..55e069383413 --- /dev/null +++ b/packages/CodingStyle/tests/Rector/Class_/AddArrayDefaultToArrayPropertyRector/Fixture/skip_nullable_array.php.inc @@ -0,0 +1,16 @@ +values === null; + } +} diff --git a/packages/SOLID/src/Rector/ClassConst/PrivatizeLocalClassConstantRector.php b/packages/SOLID/src/Rector/ClassConst/PrivatizeLocalClassConstantRector.php index a717dfbc0e5f..08c8cb77e650 100644 --- a/packages/SOLID/src/Rector/ClassConst/PrivatizeLocalClassConstantRector.php +++ b/packages/SOLID/src/Rector/ClassConst/PrivatizeLocalClassConstantRector.php @@ -21,12 +21,14 @@ final class PrivatizeLocalClassConstantRector extends AbstractRector /** * @var ClassConstantFetchAnalyzer */ - private $constFetchAnalyzer; - - public function __construct(ParsedNodesByType $parsedNodesByType, ClassConstantFetchAnalyzer $constFetchAnalyzer) - { - $this->parsedNodesByType = $parsedNodesByType;; - $this->constFetchAnalyzer = $constFetchAnalyzer; + private $classConstantFetchAnalyzer; + + public function __construct( + ParsedNodesByType $parsedNodesByType, + ClassConstantFetchAnalyzer $classConstantFetchAnalyzer + ) { + $this->parsedNodesByType = $parsedNodesByType; + $this->classConstantFetchAnalyzer = $classConstantFetchAnalyzer; } public function getDefinition(): RectorDefinition @@ -137,7 +139,7 @@ private function isUsedByChildrenOnly(array $useClasses, string $class): bool private function findClassConstantFetches(string $className, string $constantName): ?array { - $classConstantFetchByClassAndName = $this->constFetchAnalyzer->provideClassConstantFetchByClassAndName(); + $classConstantFetchByClassAndName = $this->classConstantFetchAnalyzer->provideClassConstantFetchByClassAndName(); return $classConstantFetchByClassAndName[$className][$constantName] ?? null; }