Repository navigation
Offer PHPStan's --fix as auto-fixer in the phpstan task #1238
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
base: v2.x
Are you sure you want to change the base?
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 |
|---|---|---|
|
|
@@ -4,14 +4,30 @@ | |
|
|
||
| namespace GrumPHPTest\Unit\Task; | ||
|
|
||
| use GrumPHP\Collection\ProcessArgumentsCollection; | ||
| use GrumPHP\Runner\FixableTaskResult; | ||
| use GrumPHP\Task\Context\GitPreCommitContext; | ||
| use GrumPHP\Task\Context\RunContext; | ||
| use GrumPHP\Task\PhpStan; | ||
| use GrumPHP\Task\TaskInterface; | ||
| use GrumPHP\Test\Task\AbstractExternalTaskTestCase; | ||
| use PHPUnit\Framework\Attributes\DataProvider; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
|
|
||
| class PhpStanTest extends AbstractExternalTaskTestCase | ||
| { | ||
| private const FIXABLE_OUTPUT = <<<'TXT' | ||
| [ERROR] Found 2 errors | ||
|
|
||
| 🔧 1 error can be fixed automatically. Run PHPStan again with --fix. | ||
| TXT; | ||
|
|
||
| private const FIXABLE_IGNORED_OUTPUT = <<<'TXT' | ||
| [ERROR] Found 1 error | ||
|
|
||
| 🔧 1 ignored error can be fixed automatically. Run PHPStan again with --fix-ignored. | ||
|
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. what's the difference between regular fixable and fixable ignored options?
Contributor
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.
I tried combining both ( That's a bug in PHPStan: I reported it in phpstan/phpstan#15704 and opened a fix in phpstan/phpstan-src#7036. Since ignored errors don't make the task fail anyway, this PR only uses |
||
| TXT; | ||
|
|
||
| protected function provideTask(): TaskInterface | ||
| { | ||
| return new PhpStan( | ||
|
|
@@ -66,6 +82,25 @@ function () { | |
| }, | ||
| 'nope' | ||
| ]; | ||
| yield 'exitCode1-with-fixable-errors' => [ | ||
| [], | ||
| self::mockContext(RunContext::class, ['hello.php']), | ||
| function () { | ||
| $this->mockProcessBuilder('phpstan', $process = self::mockProcess(1, self::FIXABLE_OUTPUT)); | ||
| $this->formatter->format($process)->willReturn('nope'); | ||
| }, | ||
| 'nope', | ||
| FixableTaskResult::class | ||
| ]; | ||
| yield 'exitCode1-with-only-fixable-ignored-errors' => [ | ||
| [], | ||
| self::mockContext(RunContext::class, ['hello.php']), | ||
| function () { | ||
| $this->mockProcessBuilder('phpstan', $process = self::mockProcess(1, self::FIXABLE_IGNORED_OUTPUT)); | ||
| $this->formatter->format($process)->willReturn('nope'); | ||
| }, | ||
| 'nope' | ||
| ]; | ||
| } | ||
|
|
||
| public static function providePassesOnStuff(): iterable | ||
|
|
@@ -210,4 +245,79 @@ public static function provideExternalTaskRuns(): iterable | |
| ] | ||
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * When PHPStan reports auto-fixable errors, GrumPHP offers the same command with --fix added. | ||
| * The harness only asserts the analyse command, so the fix command is verified here | ||
| * by inspecting the mutated arguments collection. | ||
| */ | ||
| #[Test] | ||
| #[DataProvider('provideFixCommands')] | ||
| public function it_builds_the_fix_command( | ||
| array $config, | ||
| string $output, | ||
| string $errorOutput, | ||
| array $expectedCommand | ||
| ): void { | ||
| $this->processBuilder->createArgumentsForCommand('phpstan')->willReturn( | ||
| $arguments = new ProcessArgumentsCollection() | ||
| ); | ||
| $this->processBuilder->buildProcess($arguments)->willReturn( | ||
| $process = self::mockProcess(1, $output, $errorOutput) | ||
| ); | ||
| $this->formatter->format($process)->willReturn('failed'); | ||
|
|
||
| $this->configureTask($config)->run(self::mockContext(RunContext::class, ['hello.php'])); | ||
|
|
||
| self::assertSame($expectedCommand, $arguments->getValues()); | ||
| } | ||
|
|
||
| public static function provideFixCommands(): iterable | ||
| { | ||
| yield 'fixable' => [ | ||
| [], | ||
| self::FIXABLE_OUTPUT, | ||
| '', | ||
| ['analyse', '--no-ansi', '--no-interaction', '--no-progress', 'hello.php', '--fix'], | ||
| ]; | ||
| yield 'fixable-keeps-options' => [ | ||
| ['configuration' => 'phpstan.neon', 'level' => 'max'], | ||
| self::FIXABLE_OUTPUT, | ||
| '', | ||
| [ | ||
| 'analyse', | ||
| '--configuration=phpstan.neon', | ||
| '--level=max', | ||
| '--no-ansi', | ||
| '--no-interaction', | ||
| '--no-progress', | ||
| 'hello.php', | ||
| '--fix', | ||
| ], | ||
| ]; | ||
| yield 'fixable-without-grumphp-paths' => [ | ||
| ['use_grumphp_paths' => false], | ||
| self::FIXABLE_OUTPUT, | ||
| '', | ||
| ['analyse', '--no-ansi', '--no-interaction', '--no-progress', '--fix'], | ||
| ]; | ||
| yield 'fixable-hint-on-error-output' => [ | ||
| [], | ||
| '', | ||
| '1 of the errors can be fixed automatically. Run PHPStan again with --fix to fix them first, then fix the rest.', | ||
| ['analyse', '--no-ansi', '--no-interaction', '--no-progress', 'hello.php', '--fix'], | ||
| ]; | ||
| yield 'not-fixable' => [ | ||
| [], | ||
| ' [ERROR] Found 1 error', | ||
| '', | ||
| ['analyse', '--no-ansi', '--no-interaction', '--no-progress', 'hello.php'], | ||
| ]; | ||
| yield 'only-fixable-ignored-errors' => [ | ||
| [], | ||
| self::FIXABLE_IGNORED_OUTPUT, | ||
| '', | ||
| ['analyse', '--no-ansi', '--no-interaction', '--no-progress', 'hello.php'], | ||
| ]; | ||
| } | ||
| } | ||
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'm wondering if this is the best way to detect it is fixable or not.
If they change the text, this will fail.
Most other tasks similar to this one just try to fix the issue without checking if they actually can. So this might be a good addition, i'm just wondering the "how" is good enough or could be detected differently.
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.
Good question. I considered always offering the fixer like ECS/Rector, but for PHPStan that has two downsides:
phpstan analyse --fixthen exits with 1 ("No fixable errors found"), so the fixer reports ✘.--fixlaunches PHPStan Pro (it downloads the Pro executable and starts it) instead of fixing.If PHPStan rewords the hint,
hasFixableErrors()returns false and the task behaves exactly like today (failed result without fixer), so it degrades gracefully instead of breaking.There is a structured alternative: with
--error-format=json, every fixable message has"fixable": true. The options I see:analyse --error-format=jsononly to detect fixable errors. This keeps PHPStan's table output, but costs an extra run (which should mostly be served from the result cache).--error-format=jsonand render the output in GrumPHP, like the phpcs task does with--report-json. No extra run, but PHPStan's own table output is lost.Which one do you prefer? I'll adapt the PR accordingly.