fix: `PhpdocOrderFixer` when `phpstan-` / `psalm-` order is specified #8853

acoulton · 2025-07-16T08:38:49Z

In v3.77.0 PhpdocOrderFixer added default support for ordering phpstan- and psalm- annotations after any of the equivalent non-prefixed tags that are specified. This was implemented in #8777, then optimised (with no behaviour change) in #8812.

However, this produces incorrect diffs (including unexpectedly reordering tags of the same type, and entirely removing multiline returns) if the user has specified an explicit order for these annotations.

This can be seen IRL in https://github.com/Behat/Gherkin/actions/runs/16302601436/job/46040722894 - unchanged code in master that was passing CS at the last merge is now failing and the proposed diff is invalid.

This is happening because the implementation from #8777 adds duplicates for any tags that the user had explicitly configured. E.g. a configuration like ['param', 'phpstan-param', 'return'] was internally processed as ['param', 'phpstan-param', 'phpstan-param', 'return'].

These duplicates then cause annotations to be moved multiple times, in some cases resulting in them being deleted from the output.

This PR fixes the issue by ensuring that annotations are only added to the user-provided order if they are not already specified.

In v3.77.0, PhpdocOrderFixer added default support for ordering phpstan- and psalm- annotations after any of the equivalent non-prefixed tags that are specified. However, this produces incorrect diffs (unexpectedly reordering tags of the same type, and entirely removing multiline returns) if the user has specified an explicit order for these annotations. This commit proves the issue, to be followed by a separate fix.

In v3.77.0, PhpdocOrderFixer added default support for ordering phpstan- and psalm- annotations after any of the equivalent non-prefixed tags that are specified. If the user had already specified any of these tags in the order, the implementation was adding duplicates. E.g. a configuration like `['param', 'phpstan-param', 'return']` was internally processed as `['param', 'phpstan-param', 'phpstan-param', 'return']`. The duplicates were then causing annotations to be moved multiple times, in some cases resulting in them being deleted from the output (as proved by the testcase in the previous commit).

coveralls · 2025-07-16T08:51:14Z

coverage: 94.74% (+0.02%) from 94.716%
when pulling a000746 on acoulton:bug-phpstan-doc-order
into 07eebc9 on PHP-CS-Fixer:master.

acoulton · 2025-07-16T08:52:51Z

As the referenced Github Actions run from Behat/Gherkin will eventually be cleaned up, I have included the content below (I didn't want to add this to the main PR description to keep that readable).

You can see that the fixer is suggesting completely deleting a complex @phpstan-return tag from one method, and in the other file is swapping the order of @phpstan-param tags (which are currently in the same order as the actual method parameters) for no reason.

Output from Behat/Gherkin fixer run in Github Actions

PHP CS Fixer 3.84.0 Alexander by Fabien Potencier, Dariusz Ruminski and contributors.
PHP runtime: 8.3.23
Running analysis on 3 cores with 10 files per process.
Parallel runner is an experimental feature and may be unstable, use it at your own risk. Feedback highly appreciated!
Loaded config default from "/home/runner/work/Gherkin/Gherkin/.php-cs-fixer.dist.php".
............................................................S..  63 / 108 ( 58%)
..................................F.F........                   108 / 108 (100%)
Legend: .-no changes, F-fixed, S-skipped (cached or empty file), I-invalid file syntax (file ignored), E-error
   1) src/Parser.php (phpdoc_order, phpdoc_separation)
      ---------- begin diff ----------
--- /home/runner/work/Gherkin/Gherkin/src/Parser.php
+++ /home/runner/work/Gherkin/Gherkin/src/Parser.php
@@ -123,20 +123,6 @@
      *
      * @return array
      *
-     * @phpstan-return (
-     *     $type is 'TableRow'
-     *         ? TTableRowToken
-     *         : ($type is 'Tag'
-     *             ? TTagToken
-     *             : ($type is 'Step'
-     *                 ? TStepToken
-     *                 : ($type is 'Text'
-     *                     ? TStringValueToken
-     *                     : ($type is TTitleKeyword
-     *                         ? TTitleToken
-     *                         : TNullValueToken|TStringValueToken
-     * )))))
-     *
      * @throws ParserException
      */
     protected function expectTokenType(string $type)

      ----------- end diff -----------

   2) bin/update_cucumber (phpdoc_order)
      ---------- begin diff ----------
--- /home/runner/work/Gherkin/Gherkin/bin/update_cucumber
+++ /home/runner/work/Gherkin/Gherkin/bin/update_cucumber
@@ -164,8 +164,8 @@
     }
 
     /**
+     * @phpstan-param TCurrentVersionArray $currentVersion
      * @phpstan-param non-empty-list<TGitHubReleaseArray> $releases
-     * @phpstan-param TCurrentVersionArray $currentVersion
      *
      * @phpstan-return TGitHubReleaseArray|false
      */
@@ -200,8 +200,8 @@
     }
 
     /**
+     * @phpstan-param TGitHubReleaseArray $nextVersion
      * @phpstan-param TCurrentVersionArray $currentVersion
-     * @phpstan-param TGitHubReleaseArray $nextVersion
      */
     private function buildCommitMessage(array $currentVersion, array $nextVersion): string
     {
@@ -231,8 +231,8 @@
     }
 
     /**
+     * @phpstan-param TGitHubReleaseArray $nextVersion
      * @phpstan-param TCurrentVersionArray $currentVersion
-     * @phpstan-param TGitHubReleaseArray $nextVersion
      */
     private function updateComposerJsonFile(string $composerFile, array $currentVersion, array $nextVersion): void
     {

      ----------- end diff -----------


Found 2 of 108 files that can be fixed in 1.706 seconds, 16.00 MB memory used
Script php-cs-fixer check --diff --ansi --show-progress=dots --verbose handling the lint event returned with error code 8
Error: Process completed with exit code 8.

gharlan

LGTM

(would be nice to also have some test cases in PhpdocOrderFixerTest for #8777 showing the reordering of the phpstan/psalm tags)

Add some cases to show the reordering of prefixed versions of specified annotations (introduced in PHP-CS-Fixer#8777 with only integration test coverage).

acoulton · 2025-07-16T14:11:53Z

@gharlan thanks - I noticed the coverage in #8777 was a little light, but I wanted to keep this PR small initially.

I've added a commit with a couple more test cases that I think cover the existing behaviour - I could obviously go into a lot more detail / break up the different things those examples are proving, but I'm not sure how complex you want this to get? The examples I've added felt in keeping with the existing test cases, but let me know if you'd like me to do anything more.

I did also notice that #8777 causes some changes in the ordering of unspecified tags e.g. in the pare example with just ['param', 'return'] specified the @custom and @template tags end up in a different place compared to the previous implementation. This is because of how the order array gets split into tags to move before / after the content (because internally the ordering is now ['param', 'phpstan-param', 'psalm-param', 'return', 'phpstan-return', 'psalm-return']). I assume since the test data was changed that is desired / expected (even though it may produce a diff for people updating to / past 3.77.0)?

gharlan · 2025-07-16T14:26:41Z

I've added a commit with a couple more test cases that I think cover the existing behaviour - I could obviously go into a lot more detail / break up the different things those examples are proving, but I'm not sure how complex you want this to get? The examples I've added felt in keeping with the existing test cases, but let me know if you'd like me to do anything more.

Thanks! I think that's enough. 👍

(I'm not a maintainer here, so let's wait for their feedback)

Since php-cs-fixer@3.77.0, phpstan- prefixed annotations are by default ordered after the specified non-prefixed annotations. Therefore, we no longer need to explicitly configure an order for `@phpstan-param` and `@phpstan-return`. Additionally, specifying an order for these properties is currently causing our CI to fail with false diffs, due to a bug in php-cs-fixer. The fix in PHP-CS-Fixer/PHP-CS-Fixer#8853 will solve that, but since our ordering matches the default behaviour we can safely remove the redundant config rather than wait for that fix to get CI green again.

mvorisek · 2025-07-21T10:27:32Z

src/Fixer/Phpdoc/PhpdocOrderFixer.php

-            $configurationOrder[] = 'phpstan-'.$type;
-            $configurationOrder[] = 'psalm-'.$type;
+            if (!\in_array('phpstan-'.$type, $this->configuration['order'], true)) {
+                $configurationOrder[] = 'phpstan-'.$type;


The fallback should be probably added after anything else.

Ex. when order is specified like param, phpstan-param, with the current PR, param, psalm-param, phpstan-param order will be kept, but I would expect param, phpstan-param, psalm-param.

Hmm, that's a reasonable point. Although I think there are potentially other cases where this behaviour can produce slightly-unexpected results especially if you specify order for some variants but not others.

Attempting to check & find the right insertion point for the prefixed variants would add even more overhead to this fixer at runtime - I'm not sure if that's desirable?

IMO the priority is to fix the bug in the current implementation (in particular that it can propose completely deleting annotations).

My gut feel is that it may be appropriate to say that if someone is using param, phpstan-param and psalm-param in the same project then they should either use the default ordering or specify all three in the order they want them.

VincentLanglet · 2025-07-25T16:48:10Z

I also encounter the issue.

I'm using

'phpdoc_order' => ['order' => ['var', 'param', 'throws', 'return', 'phpstan-var', 'psalm-var', 'phpstan-param', 'psalm-param', 'phpstan-return', 'psalm-return']],
'phpdoc_separation' => ['groups' => [
        ['phpstan-template', 'phpstan-template-covariant', 'phpstan-extends', 'phpstan-implements', 'phpstan-var', 'psalm-var', 'phpstan-param', 'psalm-param', 'phpstan-return', 'psalm-return'],
        ['psalm-suppress', 'phpstan-ignore-next-line'],
        ['Assert\\*'],
    ]],

To split classic phpdoc and static analysis phpdoc and now I end up which php cs fixer swapping phpdocs at every run.
Would be nice to prioritize the bugfix.

Friendly ping @keradus since you introduced #8777

acoulton · 2025-07-25T17:20:10Z

FWIW I believe @keradus just cherry-picked the implementation from a contributed PR.

But it would be good to get the bugfix reviewed when possible - I'm very happy to make changes if required.

keradus · 2025-07-29T21:11:43Z

src/Fixer/Phpdoc/PhpdocOrderFixer.php

+            if (!\in_array('phpstan-'.$type, $this->configuration['order'], true)) {
+                $configurationOrder[] = 'phpstan-'.$type;
+            }
+            if (!\in_array('psalm-'.$type, $this->configuration['order'], true)) {
+                $configurationOrder[] = 'psalm-'.$type;
+            }


@mvorisek are you happy with state of this PR?

keradus · 2025-07-29T21:11:59Z

src/Fixer/Phpdoc/PhpdocOrderFixer.php

+            if (!\in_array('phpstan-'.$type, $this->configuration['order'], true)) {
+                $configurationOrder[] = 'phpstan-'.$type;
+            }


@kubawerlos are you happy with state of this PR?

@kubawerlos , you were recently touching logic around this. is this change (whole PR) valid for you ?

In order to be consistent with the previous implementation (grouping the phpdoc and prefix related phpdoc) and avoiding the identified issues when prefix related phpdoc are already specified in the config. I think it's the best solution

kubawerlos · 2025-08-15T06:50:38Z

Thank you @acoulton

acoulton added 2 commits July 16, 2025 09:18

gharlan approved these changes Jul 16, 2025

View reviewed changes

test: Add coverage for default ordering of phpstan-/psalm- prefixes

85d2d4d

Add some cases to show the reordering of prefixed versions of specified annotations (introduced in PHP-CS-Fixer#8777 with only integration test coverage).

acoulton mentioned this pull request Jul 16, 2025

ci: Fix error in cucumber update job due to non-autoloadable classes Behat/Gherkin#374

Merged

acoulton mentioned this pull request Jul 21, 2025

style: Remove redundant phpstan- order config for php-cs-fixer Behat/Gherkin#377

Merged

mvorisek reviewed Jul 21, 2025

View reviewed changes

kubawerlos changed the title ~~fix: PhpdocOrderFixer when phpstan- / psalm- order is specified~~ fix: PhpdocOrderFixer when phpstan- / psalm- order is specified Jul 25, 2025

kubawerlos added the topic/phpdoc PHPDoc tags, Doctrine Annotations etc. label Jul 25, 2025

keradus reviewed Jul 29, 2025

View reviewed changes

VincentLanglet mentioned this pull request Aug 5, 2025

php_order remove phpdoc #8916

Closed

2 tasks

gharlan linked an issue Aug 14, 2025 that may be closed by this pull request

php_order remove phpdoc #8916

Closed

2 tasks

kubawerlos approved these changes Aug 15, 2025

View reviewed changes

Merge branch 'master' into bug-phpstan-doc-order

a000746

kubawerlos merged commit 6326a03 into PHP-CS-Fixer:master Aug 15, 2025
31 checks passed

Uh oh!

fix: PhpdocOrderFixer when phpstan- / psalm- order is specified #8853

fix: PhpdocOrderFixer when phpstan- / psalm- order is specified #8853

Uh oh!

Conversation

acoulton commented Jul 16, 2025

Uh oh!

coveralls commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acoulton commented Jul 16, 2025

Uh oh!

gharlan left a comment

Choose a reason for hiding this comment

Uh oh!

acoulton commented Jul 16, 2025

Uh oh!

gharlan commented Jul 16, 2025

Uh oh!

mvorisek Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

acoulton Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

VincentLanglet commented Jul 25, 2025

Uh oh!

acoulton commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keradus Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

mvorisek Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keradus Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

keradus Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

VincentLanglet Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kubawerlos commented Aug 15, 2025

Uh oh!

Uh oh!

fix: `PhpdocOrderFixer` when `phpstan-` / `psalm-` order is specified #8853

fix: `PhpdocOrderFixer` when `phpstan-` / `psalm-` order is specified #8853

coveralls commented Jul 16, 2025 •

edited

Loading

acoulton commented Jul 25, 2025 •

edited

Loading

mvorisek Jul 29, 2025 •

edited

Loading