From 3c8ed0a42d8ba6b7b2919a8a578332a69be6b959 Mon Sep 17 00:00:00 2001 From: Christophe Avonture Date: Sat, 20 Nov 2021 18:05:39 +0100 Subject: [PATCH 1/7] Add failing test fixture for ChangeReadOnlyVariableWithDefaultValueToConstantRector # Failing Test for ChangeReadOnlyVariableWithDefaultValueToConstantRector Based on https://getrector.org/demo/1ec49451-df66-6316-9733-3b3eb9ff18b0 --- .../Fixture/demo_file.php.inc | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/demo_file.php.inc diff --git a/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/demo_file.php.inc b/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/demo_file.php.inc new file mode 100644 index 00000000000..2eadcfceb79 --- /dev/null +++ b/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/demo_file.php.inc @@ -0,0 +1,52 @@ + "/$id.pdf", + ]; + + return $data; + } +} +?> +----- + "/$id.pdf", + ]; + + return $data; + } +} +?> + +## What is expected + +In fact, the rule `ChangeReadOnlyVariableWithDefaultValueToConstantRector` should not fix this sample file at all. + +`ChangeReadOnlyVariableWithDefaultValueToConstantRector` is for detecting a read-only variable like in the example here below but, in the submitted source here above, no, `$data` is using a parameter given to the function so, it should not be considered as a constant. The rule should not change the code here. + +The origin of the error is for sure the use of the `$id` inside double-quotes and not f.i. using a `sprintf()` function. + +```php +public function run(int $id): array +{ + $data = [ + 'url' => "a_file.pdf", + ]; + + return $data; +} +``` From c4873e0ffda9fd8723ce6c206bdca6a68494ab38 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Mon, 22 Nov 2021 12:11:29 +0700 Subject: [PATCH 2/7] Closes #1282 --- .../Fixture/demo_file.php.inc | 52 ------------------- .../skip_with_encapsed_string_part.php.inc | 15 ++++++ src/NodeManipulator/VariableManipulator.php | 14 +++++ 3 files changed, 29 insertions(+), 52 deletions(-) delete mode 100644 rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/demo_file.php.inc create mode 100644 rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_with_encapsed_string_part.php.inc diff --git a/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/demo_file.php.inc b/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/demo_file.php.inc deleted file mode 100644 index 2eadcfceb79..00000000000 --- a/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/demo_file.php.inc +++ /dev/null @@ -1,52 +0,0 @@ - "/$id.pdf", - ]; - - return $data; - } -} -?> ------ - "/$id.pdf", - ]; - - return $data; - } -} -?> - -## What is expected - -In fact, the rule `ChangeReadOnlyVariableWithDefaultValueToConstantRector` should not fix this sample file at all. - -`ChangeReadOnlyVariableWithDefaultValueToConstantRector` is for detecting a read-only variable like in the example here below but, in the submitted source here above, no, `$data` is using a parameter given to the function so, it should not be considered as a constant. The rule should not change the code here. - -The origin of the error is for sure the use of the `$id` inside double-quotes and not f.i. using a `sprintf()` function. - -```php -public function run(int $id): array -{ - $data = [ - 'url' => "a_file.pdf", - ]; - - return $data; -} -``` diff --git a/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_with_encapsed_string_part.php.inc b/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_with_encapsed_string_part.php.inc new file mode 100644 index 00000000000..9837808fe8e --- /dev/null +++ b/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_with_encapsed_string_part.php.inc @@ -0,0 +1,15 @@ + $id . '/foo' + ]; + + return $data; + } +} diff --git a/src/NodeManipulator/VariableManipulator.php b/src/NodeManipulator/VariableManipulator.php index a582942a58c..512e448c8ca 100644 --- a/src/NodeManipulator/VariableManipulator.php +++ b/src/NodeManipulator/VariableManipulator.php @@ -6,11 +6,14 @@ use PhpParser\Node; use PhpParser\Node\Arg; +use PhpParser\Node\Expr; use PhpParser\Node\Expr\Array_; use PhpParser\Node\Expr\Assign; use PhpParser\Node\Expr\Variable; use PhpParser\Node\Scalar; use PhpParser\Node\Scalar\Encapsed; +use PhpParser\Node\Scalar\EncapsedStringPart; +use PhpParser\Node\Scalar\String_; use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\ClassMethod; use Rector\Core\PhpParser\Comparing\NodeComparator; @@ -67,6 +70,10 @@ function (Node $node) use (&$assignsOfArrayToVariable) { return null; } + if ($node->expr instanceof Array_ && $this->hasEncapsedStringPart($node->expr)) { + return null; + } + $assignsOfArrayToVariable[] = $node; } ); @@ -74,6 +81,13 @@ function (Node $node) use (&$assignsOfArrayToVariable) { return $assignsOfArrayToVariable; } + private function hasEncapsedStringPart(Array_ $expr): bool + { + return (bool) $this->betterNodeFinder->findFirst($expr, function (Node $subNode): bool { + return $subNode instanceof EncapsedStringPart; + }); + } + /** * @param Assign[] $assignsOfArrayToVariable * @return Assign[] From ee7ccc612d9ecd542f2c3f0395589d2132b8bf75 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Mon, 22 Nov 2021 12:12:47 +0700 Subject: [PATCH 3/7] more fixture --- .../skip_with_encapsed_string_part2.php.inc | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_with_encapsed_string_part2.php.inc diff --git a/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_with_encapsed_string_part2.php.inc b/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_with_encapsed_string_part2.php.inc new file mode 100644 index 00000000000..a5dd75ae510 --- /dev/null +++ b/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_with_encapsed_string_part2.php.inc @@ -0,0 +1,15 @@ + "/$id.pdf", + ]; + + return $data; + } +} From 92048f368e5fc5402a7b34d2b772d81ebc2a9620 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Mon, 22 Nov 2021 12:16:22 +0700 Subject: [PATCH 4/7] rename --- .../skip_with_encapsed_string_part.php.inc | 2 +- .../skip_with_encapsed_string_part2.php.inc | 15 --------------- 2 files changed, 1 insertion(+), 16 deletions(-) delete mode 100644 rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_with_encapsed_string_part2.php.inc diff --git a/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_with_encapsed_string_part.php.inc b/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_with_encapsed_string_part.php.inc index 9837808fe8e..a80b2b455a7 100644 --- a/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_with_encapsed_string_part.php.inc +++ b/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_with_encapsed_string_part.php.inc @@ -7,7 +7,7 @@ final class SkipWithEncapsedStringPart public function run(int $id): array { $data = [ - 'url' => $id . '/foo' + 'url' => "/$id.pdf", ]; return $data; diff --git a/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_with_encapsed_string_part2.php.inc b/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_with_encapsed_string_part2.php.inc deleted file mode 100644 index a5dd75ae510..00000000000 --- a/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_with_encapsed_string_part2.php.inc +++ /dev/null @@ -1,15 +0,0 @@ - "/$id.pdf", - ]; - - return $data; - } -} From 54bd1eac8ce58ef759abd7b8ec8251984f72be46 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Mon, 22 Nov 2021 12:17:10 +0700 Subject: [PATCH 5/7] cs --- src/NodeManipulator/VariableManipulator.php | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/NodeManipulator/VariableManipulator.php b/src/NodeManipulator/VariableManipulator.php index 512e448c8ca..a99e6da937b 100644 --- a/src/NodeManipulator/VariableManipulator.php +++ b/src/NodeManipulator/VariableManipulator.php @@ -6,14 +6,12 @@ use PhpParser\Node; use PhpParser\Node\Arg; -use PhpParser\Node\Expr; use PhpParser\Node\Expr\Array_; use PhpParser\Node\Expr\Assign; use PhpParser\Node\Expr\Variable; use PhpParser\Node\Scalar; use PhpParser\Node\Scalar\Encapsed; use PhpParser\Node\Scalar\EncapsedStringPart; -use PhpParser\Node\Scalar\String_; use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\ClassMethod; use Rector\Core\PhpParser\Comparing\NodeComparator; @@ -81,13 +79,6 @@ function (Node $node) use (&$assignsOfArrayToVariable) { return $assignsOfArrayToVariable; } - private function hasEncapsedStringPart(Array_ $expr): bool - { - return (bool) $this->betterNodeFinder->findFirst($expr, function (Node $subNode): bool { - return $subNode instanceof EncapsedStringPart; - }); - } - /** * @param Assign[] $assignsOfArrayToVariable * @return Assign[] @@ -100,6 +91,14 @@ public function filterOutChangedVariables(array $assignsOfArrayToVariable, Class ); } + private function hasEncapsedStringPart(Array_ $array): bool + { + return (bool) $this->betterNodeFinder->findFirst( + $array, + fn (Node $subNode): bool => $subNode instanceof EncapsedStringPart + ); + } + private function isTestCaseExpectedVariable(Variable $variable): bool { $classLike = $this->betterNodeFinder->findParentType($variable, ClassLike::class); From 97ecef4a864eef28c9940cc4141774d2a0f1cd6a Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Mon, 22 Nov 2021 14:08:03 +0700 Subject: [PATCH 6/7] fix Encapsed in string --- .../skip_array_with_encapsed_string_part.php.inc | 15 +++++++++++++++ .../skip_string_with_encapsed_string.php.inc | 12 ++++++++++++ .../skip_string_with_encapsed_string_part.php.inc | 12 ++++++++++++ src/NodeManipulator/VariableManipulator.php | 2 +- 4 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_array_with_encapsed_string_part.php.inc create mode 100644 rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_string_with_encapsed_string.php.inc create mode 100644 rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_string_with_encapsed_string_part.php.inc diff --git a/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_array_with_encapsed_string_part.php.inc b/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_array_with_encapsed_string_part.php.inc new file mode 100644 index 00000000000..b835c7997c9 --- /dev/null +++ b/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_array_with_encapsed_string_part.php.inc @@ -0,0 +1,15 @@ + "$id", + ]; + + return $data; + } +} diff --git a/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_string_with_encapsed_string.php.inc b/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_string_with_encapsed_string.php.inc new file mode 100644 index 00000000000..a0923c14a0d --- /dev/null +++ b/rules-tests/Privatization/Rector/Class_/ChangeReadOnlyVariableWithDefaultValueToConstantRector/Fixture/skip_string_with_encapsed_string.php.inc @@ -0,0 +1,12 @@ +betterNodeFinder->findFirst( $array, - fn (Node $subNode): bool => $subNode instanceof EncapsedStringPart + fn (Node $subNode): bool => $subNode instanceof Encapsed || $subNode instanceof EncapsedStringPart ); } From fcc0dedf76e22f4a257feb3b11eeea826c8a3b15 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Mon, 22 Nov 2021 11:54:24 +0300 Subject: [PATCH 7/7] simplify instanceof --- src/NodeManipulator/VariableManipulator.php | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/NodeManipulator/VariableManipulator.php b/src/NodeManipulator/VariableManipulator.php index 11f1a6c0cda..bccc217d5ff 100644 --- a/src/NodeManipulator/VariableManipulator.php +++ b/src/NodeManipulator/VariableManipulator.php @@ -6,6 +6,7 @@ use PhpParser\Node; use PhpParser\Node\Arg; +use PhpParser\Node\Expr; use PhpParser\Node\Expr\Array_; use PhpParser\Node\Expr\Assign; use PhpParser\Node\Expr\Variable; @@ -56,7 +57,7 @@ function (Node $node) use (&$assignsOfArrayToVariable) { return null; } - if ($node->expr instanceof Encapsed) { + if ($this->hasEncapsedStringPart($node->expr)) { return null; } @@ -68,10 +69,6 @@ function (Node $node) use (&$assignsOfArrayToVariable) { return null; } - if ($node->expr instanceof Array_ && $this->hasEncapsedStringPart($node->expr)) { - return null; - } - $assignsOfArrayToVariable[] = $node; } ); @@ -91,10 +88,10 @@ public function filterOutChangedVariables(array $assignsOfArrayToVariable, Class ); } - private function hasEncapsedStringPart(Array_ $array): bool + private function hasEncapsedStringPart(Expr $expr): bool { return (bool) $this->betterNodeFinder->findFirst( - $array, + $expr, fn (Node $subNode): bool => $subNode instanceof Encapsed || $subNode instanceof EncapsedStringPart ); }