-
-
Notifications
You must be signed in to change notification settings - Fork 739
[Polyfill] Add UnwrapFutureCompatibleIfRector #2569
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| services: | ||
| Rector\Polyfill\Rector\If_\UnwrapFutureCompatibleIfRector: null |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| services: | ||
| _defaults: | ||
| public: true | ||
| autowire: true | ||
|
|
||
| Rector\Polyfill\: | ||
| resource: '../src' | ||
| exclude: | ||
| - '../src/Rector/**/*Rector.php' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Rector\Polyfill\FeatureSupport; | ||
|
|
||
| use Rector\Php\PhpVersionProvider; | ||
|
|
||
| final class FunctionSupportResolver | ||
| { | ||
| /** | ||
| * @var string[][] | ||
| */ | ||
| private const FUNCTIONS_BY_VERSION = [ | ||
| '5.6' => ['session_abort'], | ||
| ]; | ||
|
|
||
| /** | ||
| * @var PhpVersionProvider | ||
| */ | ||
| private $phpVersionProvider; | ||
|
|
||
| public function __construct(PhpVersionProvider $phpVersionProvider) | ||
| { | ||
| $this->phpVersionProvider = $phpVersionProvider; | ||
| } | ||
|
|
||
| public function isFunctionSupported(string $desiredFunction): bool | ||
| { | ||
| foreach (self::FUNCTIONS_BY_VERSION as $version => $functions) { | ||
| foreach ($functions as $function) { | ||
| if ($desiredFunction !== $function) { | ||
| continue; | ||
| } | ||
|
|
||
| if (! $this->phpVersionProvider->isAtLeast($version)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it would also be helpfull to remove code when explicit php version checks are used (might be a separTe rector then?)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure! I though of that too. Could you help by making a new issue with minimal code (3-lines) in diff a format?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| continue; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Rector\Polyfill\Rector\If_; | ||
|
|
||
| use PhpParser\Node; | ||
| use PhpParser\Node\Stmt\If_; | ||
| use Rector\PhpParser\Node\Manipulator\IfManipulator; | ||
| use Rector\Polyfill\FeatureSupport\FunctionSupportResolver; | ||
| use Rector\Rector\AbstractRector; | ||
| use Rector\RectorDefinition\CodeSample; | ||
| use Rector\RectorDefinition\RectorDefinition; | ||
|
|
||
| /** | ||
| * @see \Rector\Polyfill\Tests\Rector\If_\UnwrapFutureCompatibleIfRector\UnwrapFutureCompatibleIfRectorTest | ||
| */ | ||
| final class UnwrapFutureCompatibleIfRector extends AbstractRector | ||
| { | ||
| /** | ||
| * @var IfManipulator | ||
| */ | ||
| private $ifManipulator; | ||
|
|
||
| /** | ||
| * @var FunctionSupportResolver | ||
| */ | ||
| private $functionSupportResolver; | ||
|
|
||
| public function __construct(IfManipulator $ifManipulator, FunctionSupportResolver $functionSupportResolver) | ||
| { | ||
| $this->ifManipulator = $ifManipulator; | ||
| $this->functionSupportResolver = $functionSupportResolver; | ||
| } | ||
|
|
||
| public function getDefinition(): RectorDefinition | ||
| { | ||
| return new RectorDefinition('Remove functions exists if with else for always existing', [ | ||
| new CodeSample( | ||
| <<<'PHP' | ||
| class SomeClass | ||
| { | ||
| public function run() | ||
| { | ||
| // session locking trough other addons | ||
| if (function_exists('session_abort')) { | ||
| session_abort(); | ||
| } else { | ||
| session_write_close(); | ||
| } | ||
| } | ||
| } | ||
| PHP | ||
| , | ||
| <<<'PHP' | ||
| class SomeClass | ||
| { | ||
| public function run() | ||
| { | ||
| // session locking trough other addons | ||
| session_abort(); | ||
| } | ||
| } | ||
| PHP | ||
|
|
||
| ), | ||
| ]); | ||
| } | ||
|
|
||
| /** | ||
| * @return string[] | ||
| */ | ||
| public function getNodeTypes(): array | ||
| { | ||
| return [If_::class]; | ||
| } | ||
|
|
||
| /** | ||
| * @param If_ $node | ||
| */ | ||
| public function refactor(Node $node): ?Node | ||
| { | ||
| $match = $this->ifManipulator->isIfElseWithFunctionCondition($node, 'function_exists'); | ||
| if ($match === false) { | ||
| return null; | ||
| } | ||
|
|
||
| /** @var Node\Expr\FuncCall $funcCall */ | ||
| $funcCall = $node->cond; | ||
|
|
||
| $functionToExistName = $this->getValue($funcCall->args[0]->value); | ||
| if (! is_string($functionToExistName)) { | ||
| return null; | ||
| } | ||
|
|
||
| if (! $this->functionSupportResolver->isFunctionSupported($functionToExistName)) { | ||
| return null; | ||
| } | ||
|
|
||
| foreach ($node->stmts as $key => $ifStmt) { | ||
| if ($key === 0) { | ||
| // move comment from if to first element to keep it | ||
| $ifStmt->setAttribute('comments', $node->getComments()); | ||
| } | ||
|
|
||
| $this->addNodeAfterNode($ifStmt, $node); | ||
| } | ||
|
|
||
| $this->removeNode($node); | ||
|
|
||
| return null; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| <?php | ||
|
|
||
| namespace Rector\Polyfill\Tests\Rector\If_\UnwrapFutureCompatibleIfRector\Fixture; | ||
|
|
||
| class SomeClass | ||
| { | ||
| public function run() | ||
| { | ||
| // session locking trough other addons | ||
| if (function_exists('session_abort')) { | ||
| session_abort(); | ||
| } else { | ||
| session_write_close(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| ?> | ||
| ----- | ||
| <?php | ||
|
|
||
| namespace Rector\Polyfill\Tests\Rector\If_\UnwrapFutureCompatibleIfRector\Fixture; | ||
|
|
||
| class SomeClass | ||
| { | ||
| public function run() | ||
| { | ||
| // session locking trough other addons | ||
| session_abort(); | ||
| } | ||
| } | ||
|
|
||
| ?> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Rector\Polyfill\Tests\Rector\If_\UnwrapFutureCompatibleIfRector; | ||
|
|
||
| use Iterator; | ||
| use Rector\Polyfill\Rector\If_\UnwrapFutureCompatibleIfRector; | ||
| use Rector\Testing\PHPUnit\AbstractRectorTestCase; | ||
|
|
||
| final class UnwrapFutureCompatibleIfRectorTest extends AbstractRectorTestCase | ||
| { | ||
| /** | ||
| * @dataProvider provideDataForTest() | ||
| */ | ||
| public function test(string $file): void | ||
| { | ||
| $this->doTestFile($file); | ||
| } | ||
|
|
||
| public function provideDataForTest(): Iterator | ||
| { | ||
| return $this->yieldFilesFromDirectory(__DIR__ . '/Fixture'); | ||
| } | ||
|
|
||
| protected function getRectorClass(): string | ||
| { | ||
| return UnwrapFutureCompatibleIfRector::class; | ||
| } | ||
| } |
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.
@staabm Here is just the code example you showed me. I need your help to complete list as far as possible.
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.
I need to think about how to compile such method most easily
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.
After thinking about it I feel we could compile this list from exisiting polyfil libs, e.g.
https://github.com/symfony/polyfill-php56
https://github.com/symfony/polyfill-php70
https://github.com/symfony/polyfill-php71
https://github.com/symfony/polyfill-php72
https://github.com/symfony/polyfill-php73
https://github.com/symfony/polyfill-php74
Uh oh!
There was an error while loading. Please reload this page.
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.
Another thing I checked via github code-search
https://github.com/search?l=PHP&q=function_exists&type=Code
There we can see most examples are like
In these cases we could eleminate the code when we are sure that the function beeing checked is one from the php standard library (so semantically the code is polyfilling, not e.g. guarding against „function already exist“ error because of multiple including the same file)
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.
Great!
Try to keep issues separated, so we don't get stuck with complexity. This is another use case.
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.
I see
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.
There are many new PRs merged every day so there are new conflicts to rebase on.
I'll merge this now to prevent that.
You can send the function list as a new PR targeting this class 👍
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.
@staabm Any update on this? The rule is pretty lonely with just 1 function now :D
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.
Was on vacation till yesterday.. will need a few more days to catch up
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.
here we go #2673