From a773eae2370af439f38b45be490fbeda52458483 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sun, 30 Aug 2026 09:38:02 +0200 Subject: [PATCH] [dx] Deprecate ShortenElseIfRector, as merging else/if to elseif creates less readable code and breaks logical reading of the if train Claude-Session: https://claude.ai/code/session_01FLhnMwgwtdvbYJRh9S72UW --- .../Fixture/mirror_comment.php.inc | 35 ------- .../Fixture/mirror_comment2.php.inc | 39 -------- .../Fixture/mirror_comment3.php.inc | 37 -------- .../mirror_comment_deep_comment.php.inc | 59 ------------ .../Fixture/nested_else.php.inc | 71 -------------- .../Fixture/nested_elseif.php.inc | 81 ---------------- .../Fixture/recursive.php.inc | 83 ----------------- .../Fixture/simple.php.inc | 67 -------------- .../Fixture/skip_alternative_syntax.php.inc | 18 ---- .../Fixture/skip_empty_else.php.inc | 21 ----- .../skip_inner_alternative_syntax.php.inc | 16 ---- .../Fixture/skip_multiple_stmts.php.inc | 33 ------- .../Fixture/skip_no_else_if.php.inc | 30 ------ .../Fixture/skip_with_html.php.inc | 35 ------- .../ShortenElseIfRectorTest.php | 28 ------ .../config/configured_rule.php | 9 -- .../Rector/If_/ShortenElseIfRector.php | 92 ++----------------- src/Config/Level/CodeQualityLevel.php | 2 - 18 files changed, 8 insertions(+), 748 deletions(-) delete mode 100644 rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/mirror_comment.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/mirror_comment2.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/mirror_comment3.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/mirror_comment_deep_comment.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/nested_else.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/nested_elseif.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/recursive.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/simple.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_alternative_syntax.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_empty_else.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_inner_alternative_syntax.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_multiple_stmts.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_no_else_if.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_with_html.php.inc delete mode 100644 rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/ShortenElseIfRectorTest.php delete mode 100644 rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/config/configured_rule.php diff --git a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/mirror_comment.php.inc b/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/mirror_comment.php.inc deleted file mode 100644 index df01205447c..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/mirror_comment.php.inc +++ /dev/null @@ -1,35 +0,0 @@ - ------ - diff --git a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/mirror_comment2.php.inc b/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/mirror_comment2.php.inc deleted file mode 100644 index bd234e0fc59..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/mirror_comment2.php.inc +++ /dev/null @@ -1,39 +0,0 @@ - ------ - diff --git a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/mirror_comment3.php.inc b/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/mirror_comment3.php.inc deleted file mode 100644 index 3df2cbb3961..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/mirror_comment3.php.inc +++ /dev/null @@ -1,37 +0,0 @@ - ------ - diff --git a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/mirror_comment_deep_comment.php.inc b/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/mirror_comment_deep_comment.php.inc deleted file mode 100644 index 665ad1aecbd..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/mirror_comment_deep_comment.php.inc +++ /dev/null @@ -1,59 +0,0 @@ - 5) { - echo 'a'; - } else { - // above if comment - if ($a === 5) { - // inside if comment - foreach ($b_arg as $element) { - // inside foreach comment - echo $element; - } - echo 'b'; - } else { - // inside else comment - echo 'c'; - } - } - } -} - -?> ------ - 5) { - echo 'a'; - } elseif ($a === 5) { - // above if comment - // inside if comment - foreach ($b_arg as $element) { - // inside foreach comment - echo $element; - } - echo 'b'; - } else { - // inside else comment - echo 'c'; - } - } -} - -?> diff --git a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/nested_else.php.inc b/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/nested_else.php.inc deleted file mode 100644 index 4326fe191f9..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/nested_else.php.inc +++ /dev/null @@ -1,71 +0,0 @@ -cond1) { - $this->doSomething(); - } else { - if ($this->cond2) { - $this->doSomething(); - } else { - $this->doSomething(); - } - } - } - - public function doSomething() - { - - } -} - -?> ------ -cond1) { - $this->doSomething(); - } elseif ($this->cond2) { - $this->doSomething(); - } else { - $this->doSomething(); - } - } - - public function doSomething() - { - - } -} - -?> diff --git a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/nested_elseif.php.inc b/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/nested_elseif.php.inc deleted file mode 100644 index e10b38335db..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/nested_elseif.php.inc +++ /dev/null @@ -1,81 +0,0 @@ -cond1) { - $this->doSomething(); - } else { - if ($this->cond2) { - $this->doSomething(); - } elseif ($this->cond3) { - $this->doSomething(); - } - } - } - - public function doSomething() - { - - } -} - -?> ------ -cond1) { - $this->doSomething(); - } elseif ($this->cond2) { - $this->doSomething(); - } elseif ($this->cond3) { - $this->doSomething(); - } - } - - public function doSomething() - { - - } -} - -?> diff --git a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/recursive.php.inc b/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/recursive.php.inc deleted file mode 100644 index afe51a5698a..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/recursive.php.inc +++ /dev/null @@ -1,83 +0,0 @@ -cond1) { - $this->doSomething(); - } else { - if ($this->cond2) { - $this->doSomething(); - } else { - if ($this->cond3) { - $this->doSomething(); - } - } - } - } - - public function doSomething() - { - - } -} - -?> ------ -cond1) { - $this->doSomething(); - } elseif ($this->cond2) { - $this->doSomething(); - } elseif ($this->cond3) { - $this->doSomething(); - } - } - - public function doSomething() - { - - } -} - -?> diff --git a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/simple.php.inc b/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/simple.php.inc deleted file mode 100644 index 9e3300b911f..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/simple.php.inc +++ /dev/null @@ -1,67 +0,0 @@ -cond1) { - $this->doSomething(); - } else { - if ($this->cond2) { - $this->doSomething(); - } - } - } - - public function doSomething() - { - - } -} - -?> ------ -cond1) { - $this->doSomething(); - } elseif ($this->cond2) { - $this->doSomething(); - } - } - - public function doSomething() - { - - } -} - -?> diff --git a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_alternative_syntax.php.inc b/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_alternative_syntax.php.inc deleted file mode 100644 index 3edb1782341..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_alternative_syntax.php.inc +++ /dev/null @@ -1,18 +0,0 @@ -cond1) { - return 1; - } else { - // some comment - []; - } - } -} diff --git a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_inner_alternative_syntax.php.inc b/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_inner_alternative_syntax.php.inc deleted file mode 100644 index 83c2c5ef34a..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_inner_alternative_syntax.php.inc +++ /dev/null @@ -1,16 +0,0 @@ -cond1) { - $this->doSomething(); - } else { - if ($this->cond2) { - $this->doSomething(); - } - $this->doSomething(); - } - } - - public function doSomething() - { - - } -} diff --git a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_no_else_if.php.inc b/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_no_else_if.php.inc deleted file mode 100644 index f271d6a9472..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_no_else_if.php.inc +++ /dev/null @@ -1,30 +0,0 @@ -cond1) { - $this->doSomething(); - } else { - $this->doSomething(); - } - } - - public function doSomething() - { - - } -} diff --git a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_with_html.php.inc b/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_with_html.php.inc deleted file mode 100644 index 27b76053e92..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/Fixture/skip_with_html.php.inc +++ /dev/null @@ -1,35 +0,0 @@ - -
- is_main_item) : ?> - -
- icon ? $this->Icon->render($menuItem->icon) : '' ?> - -
- - is_external_url) { - echo $this->Html->link($title, $menuItem->url, [ - 'menu-icon' => $menuItem->icon ?? false, - 'class' => $aClass, - ]); - } elseif ($menuItem->parsed_url && $menuItem->is_url_valid) { - echo $this->Html->linkFromPath($title, $menuItem->parsed_url, [], [ - 'menu-icon' => $menuItem->icon ?? false, - 'class' => $aClass, - ]); - } else { - // Do something? - } - endif; ?> -
- doTestFile($filePath); - } - - public static function provideData(): Iterator - { - return self::yieldFilesFromDirectory(__DIR__ . '/Fixture'); - } - - public function provideConfigFilePath(): string - { - return __DIR__ . '/config/configured_rule.php'; - } -} diff --git a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/config/configured_rule.php b/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/config/configured_rule.php deleted file mode 100644 index 6f875477afc..00000000000 --- a/rules-tests/CodeQuality/Rector/If_/ShortenElseIfRector/config/configured_rule.php +++ /dev/null @@ -1,9 +0,0 @@ -withRules([ShortenElseIfRector::class]); diff --git a/rules/CodeQuality/Rector/If_/ShortenElseIfRector.php b/rules/CodeQuality/Rector/If_/ShortenElseIfRector.php index 63ce3bd9ee0..c823a01d7e0 100644 --- a/rules/CodeQuality/Rector/If_/ShortenElseIfRector.php +++ b/rules/CodeQuality/Rector/If_/ShortenElseIfRector.php @@ -5,21 +5,17 @@ namespace Rector\CodeQuality\Rector\If_; use PhpParser\Node; -use PhpParser\Node\Stmt\Else_; -use PhpParser\Node\Stmt\ElseIf_; use PhpParser\Node\Stmt\If_; -use PhpParser\Node\Stmt\Nop; -use PhpParser\Token; -use Rector\Contract\Rector\HTMLAverseRectorInterface; -use Rector\NodeTypeResolver\Node\AttributeKey; +use Rector\Configuration\Deprecation\Contract\DeprecatedInterface; +use Rector\Exception\ShouldNotHappenException; use Rector\Rector\AbstractRector; use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample; use Symplify\RuleDocGenerator\ValueObject\RuleDefinition; /** - * @see \Rector\Tests\CodeQuality\Rector\If_\ShortenElseIfRector\ShortenElseIfRectorTest + * @deprecated This rule is deprecated, as merging `else`/`if` into `elseif` creates less readable code and breaks the logical reading of the if train. */ -final class ShortenElseIfRector extends AbstractRector implements HTMLAverseRectorInterface +final class ShortenElseIfRector extends AbstractRector implements DeprecatedInterface { public function getRuleDefinition(): RuleDefinition { @@ -71,81 +67,9 @@ public function getNodeTypes(): array */ public function refactor(Node $node): ?Node { - return $this->shortenElseIf($node); - } - - private function shortenElseIf(If_ $node): ?If_ - { - if (! $node->else instanceof Else_) { - return null; - } - - $else = $node->else; - if (count($else->stmts) !== 1) { - return null; - } - - $if = $else->stmts[0]; - if (! $if instanceof If_) { - return null; - } - - // alternative syntax (if/else/endif) cannot be merged to elseif without producing mixed brace/colon syntax - if ($this->isAlternativeSyntax($node) || $this->isAlternativeSyntax($if)) { - return null; - } - - // Try to shorten the nested if before transforming it to elseif - $refactored = $this->shortenElseIf($if); - - if ($refactored instanceof If_) { - $if = $refactored; - } - - if ($if->stmts === []) { - $nop = new Nop(); - $nop->setAttribute(AttributeKey::COMMENTS, $if->getComments()); - $if->stmts[] = $nop; - } else { - $currentStmt = current($if->stmts); - $mergedComments = array_merge($if->getComments(), $currentStmt->getComments()); - $currentStmt->setAttribute(AttributeKey::COMMENTS, $mergedComments); - } - - $node->elseifs[] = new ElseIf_($if->cond, $if->stmts); - - $node->else = $if->else; - - $node->elseifs = array_merge($node->elseifs, $if->elseifs); - - return $node; - } - - private function isAlternativeSyntax(If_ $if): bool - { - $startTokenPos = $if->cond->getEndTokenPos(); - if ($startTokenPos < 0) { - return false; - } - - $oldTokens = $this->getFile() - ->getOldTokens(); - - for ($i = $startTokenPos + 1; isset($oldTokens[$i]); ++$i) { - $token = $oldTokens[$i]; - if (! $token instanceof Token) { - continue; - } - - if ($token->text === ':') { - return true; - } - - if ($token->text === '{') { - return false; - } - } - - return false; + throw new ShouldNotHappenException(sprintf( + '"%s" rule is deprecated, as it creates less readable code and breaks the logical reading of the if train', + self::class + )); } } diff --git a/src/Config/Level/CodeQualityLevel.php b/src/Config/Level/CodeQualityLevel.php index b24712e76c7..ba52e7b8bff 100644 --- a/src/Config/Level/CodeQualityLevel.php +++ b/src/Config/Level/CodeQualityLevel.php @@ -54,7 +54,6 @@ use Rector\CodeQuality\Rector\If_\ArrayExplicitBoolCompareRector; use Rector\CodeQuality\Rector\If_\ConsecutiveNullCompareReturnsToNullCoalesceQueueRector; use Rector\CodeQuality\Rector\If_\ObjectExplicitBoolCompareRector; -use Rector\CodeQuality\Rector\If_\ShortenElseIfRector; use Rector\CodeQuality\Rector\If_\SimplifyIfNotNullReturnRector; use Rector\CodeQuality\Rector\If_\SimplifyIfNullableReturnRector; use Rector\CodeQuality\Rector\If_\SimplifyIfReturnBoolRector; @@ -144,7 +143,6 @@ final class CodeQualityLevel StrlenZeroToIdenticalEmptyStringRector::class, ArrayExplicitBoolCompareRector::class, ObjectExplicitBoolCompareRector::class, - ShortenElseIfRector::class, ThrowWithPreviousExceptionRector::class, RemoveSoleValueSprintfRector::class, ExplicitReturnNullRector::class,