Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions doc/tasks/phpstan.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,3 +75,15 @@ With this parameter you can specify the memory limit.
Since there is no `--changed-files` flag [in PhpStan yet](https://github.com/phpstan/phpstan/issues/934#issuecomment-383002766),
this flags allows you to change what files will be validated.
You can choose to use the paths detected by GrumPHP, or you can choose to fall back on the PhpStan configuration.

## Auto-fixing

Starting with PHPStan 2.3.5, PHPStan marks errors it can fix automatically (🔧) and suggests to run it again with `--fix`.
When PHPStan reports such errors, GrumPHP offers to fix them by running the same `phpstan analyse` command with the `--fix` option.
The fix is applied to the same files that were analysed.
Errors that cannot be fixed automatically are left untouched and still need to be fixed by hand.

Keep in mind that the `--fix` option is experimental in PHPStan.
Besides fixing code, it also removes unmatched `ignoreErrors` entries from your configuration file and baseline, and unmatched `@phpstan-ignore` comments.

Whether GrumPHP asks to fix the errors or fixes them by default can be configured with the [`fixer`](../parameters.md) parameters.
31 changes: 30 additions & 1 deletion src/Task/PhpStan.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

namespace GrumPHP\Task;

use GrumPHP\Fixer\Provider\FixableProcessResultProvider;
use GrumPHP\Formatter\ProcessFormatterInterface;
use GrumPHP\Runner\TaskResult;
use GrumPHP\Runner\TaskResultInterface;
Expand All @@ -12,6 +13,7 @@
use GrumPHP\Task\Context\GitPreCommitContext;
use GrumPHP\Task\Context\RunContext;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\Process\Process;

/**
* @extends AbstractExternalTask<ProcessFormatterInterface>
Expand Down Expand Up @@ -104,9 +106,36 @@ public function run(ContextInterface $context): TaskResultInterface
$process->run();

if (!$process->isSuccessful()) {
return TaskResult::createFailed($this, $context, $this->formatter->format($process));
$failedResult = TaskResult::createFailed($this, $context, $this->formatter->format($process));

if (!$this->hasFixableErrors($process)) {
return $failedResult;
}

return FixableProcessResultProvider::provide(
$failedResult,
function () use ($arguments): Process {
$arguments->add('--fix');

return $this->processBuilder->buildProcess($arguments);
}
);
}

return TaskResult::createPassed($this, $context);
}

/**
* PHPStan (2.3.5+) ends its report with "Run PHPStan again with --fix" when at least one
* reported error can be fixed automatically. Without fixable errors, "phpstan analyse --fix"
* fails with "No fixable errors found", so the fixer is only offered when PHPStan suggests it.
* The hint for ignored errors ("--fix-ignored") is not taken into account.
*/
private function hasFixableErrors(Process $process): bool
{
return 1 === preg_match(
'/Run PHPStan again with --fix(?![\w-])/',

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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:

  1. Unlike ECS/Rector, most PHPStan errors (e.g. type errors) can't be fixed automatically. GrumPHP would ask "I can fix some stuff automatically" on almost every failing run, and phpstan analyse --fix then exits with 1 ("No fixable errors found"), so the fixer reports ✘.
  2. Up to PHPStan 2.1, --fix launches 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:

  • A: keep the hint detection as it is.
  • B: on failure, run a second analyse --error-format=json only to detect fixable errors. This keeps PHPStan's table output, but costs an extra run (which should mostly be served from the result cache).
  • C: switch the task to --error-format=json and render the output in GrumPHP, like the phpcs task does with --report-json. No extra run, but PHPStan's own table output is lost.
  • D: always offer the fixer like ECS/Rector, with the downsides above.

Which one do you prefer? I'll adapt the PR accordingly.

$process->getOutput().PHP_EOL.$process->getErrorOutput()
);
}
}
110 changes: 110 additions & 0 deletions test/Unit/Task/PhpStanTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the difference between regular fixable and fixable ignored options?
Should we support both somehow?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--fix fixes the errors PHPStan reports. --fix-ignored fixes errors that are currently ignored via the baseline, ignoreErrors or @phpstan-ignore, and then removes the ignore entries that are no longer needed. That's mainly useful to shrink a baseline, e.g. after upgrading to PHPStan 2.3.5, where many baselined errors like method.missingOverride became auto-fixable.

I tried combining both (--fix --fix-ignored), but it doesn't work with GrumPHP yet: when only the changed files are passed to PHPStan (the default use_grumphp_paths: true), the ignored errors get fixed but the baseline entries stay, because PHPStan doesn't report unmatched ignores for a list of files, so its later fix rounds never remove them. The next full run (e.g. in CI) then fails with ignore.unmatched. The same happens with --fix-ignored alone.

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 --fix. Once the fix is released, adding --fix-ignored (e.g. as an opt-in option) could be a follow-up.

TXT;

protected function provideTask(): TaskInterface
{
return new PhpStan(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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'],
];
}
}
Loading