-
-
Notifications
You must be signed in to change notification settings - Fork 339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DeadCode] Ensure no stmts check on RemovePhpVersionIdCheckRector #2260
Conversation
Fixed 🎉 |
return null; | ||
if ($if->stmts === []) { | ||
$this->removeNode($if); | ||
return $if; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomasVotruba return $if
is needed to avoid "applied rules" not shown in the list, ref
rector-src/src/Rector/AbstractRector.php
Lines 207 to 210 in 2dd8c6b
// nothing to change → continue | |
if ($node === null) { | |
return null; | |
} |
which it only shows when it has node
or array of stmts
returns:
rector-src/src/Rector/AbstractRector.php
Lines 220 to 222 in 2dd8c6b
/** @var Node $originalNode */ | |
$rectorWithLineChange = new RectorWithLineChange($this::class, $originalNode->getLine()); | |
$this->file->addRectorClassWithLine($rectorWithLineChange); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changed should be included for removed nodes too. The AbstractRector
should be updated instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I created PR #2261 for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomasVotruba that seems not doable by only move up the RectorWithLineChange
as the information of Node removed is against original node, which if it returns null, it fallback to clone $node
, which the information is for object before refactor, ref
rector-src/src/Rector/AbstractRector.php
Line 192 in fb8260c
$originalNode = $node->getAttribute(AttributeKey::ORIGINAL_NODE) ?? clone $node; |
it also cause errors in other e2e tests by move it up, ref PR #2261 (comment)
Thanks 👏 |
ref https://github.com/rectorphp/rector-src/pull/2259/files#r867380537