Problem/Motivation

Bump PHPStan to 2.1.34 as

Projects typically see 25 % to 40 % faster analysis times. Please test this release and report back, we're looking forward to your numbers as well!

See https://github.com/phpstan/phpstan/releases/tag/2.1.34

Proposed resolution

composer update mglaman/phpstan-drupal phpstan/extension-installer phpstan/phpstan phpstan/phpstan-deprecation-rules phpstan/phpstan-phpunit

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3568370

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake opened merge request !14411

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review

Indeed PHPStan job execution looks much faster.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Let's do it.

dww’s picture

dww
we/he/they
commented

I tried locally and get the identical composer.lock changes. Pipeline is green. Changes are small and mostly make sense. Not entirely clear why the new version causes us to need to update the baseline, but the original pipeline shows the failure, so it's clearly needed.😅

This is not a fair comparison, but the latest passing phpstan job in this MR took 3 minutes 47 seconds, including:

$ php vendor/bin/phpstan -vvv analyze ...
Result cache not used because the metadata do not match: phpstanVersion, projectConfig, scannedFiles, composerLocks, composerInstalled, executedFilesHashes
...
Elapsed time: 2 minutes 38 seconds

vs. the phpstan job in the latest daily main test pipeline I could find took 2 minutes 26 seconds, including:

$ php vendor/bin/phpstan -vvv analyze ...
Result cache restored in 27.3 seconds. 0 files will be reanalysed.
...
Elapsed time: 46 seconds

How can we compare apples to apples and see the performance gain on consecutive runs where the results are not cached?

Don't want to hold up RTBC for this, probably good to upgrade anyway, but curious to actually see the benefits in action.

Thanks,
-Derek

mondrake’s picture

mondrake
Location 🇮🇹
commented

How can we compare apples to apples and see the performance gain on consecutive runs where the results are not cached?

Check a MR where the PHPStan cache is invalidated. Any one that updates a dependency will.

catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » main
Status: Reviewed & tested by the community » Fixed

It would be great if they've also improved performance when caches are warm - sometimes see it taking 30s or more to restore the cache. Some of the commit log suggested this might be the case.

The MR doesn't apply to main, but rather than moving to needs work I went ahead and ran the commands in the issue summary, then compared against the 11.x diff, and committed that directly (after nearly forgetting to update the phpstan baseline too).

Committed/pushed to main and 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !14411

  • catch committed 0ae7c91c on 11.x 
    task: #3568370 Update PHPStan to 2.1.34

By: mondrake
By: dww

  • catch committed e0549bb5 on main 
    task: #3568370 Update PHPStan to 2.1.34

By: mondrake
By: dww
mondrake’s picture

mondrake
Location 🇮🇹
commented

The main commit lacks the version update in composer.json (main and the Drupal PHPStan rules one) and the metapackages.

Apart from the steps in the IS, we need manual update of the composer.json files and then composer update --lock to update the metapackages.

  • catch committed e4980226 on main 
    Revert "task: #3568370 Update PHPStan to 2.1.34"

This reverts commit...
catch’s picture

catch
he/him
Primary language English
commented
Status: Fixed » Needs work

Oof. Reverted the commit on main, let's do it via MR after all.

mondrake opened merge request !14422

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

14422 targets main.

mondrake’s picture

mondrake
Location 🇮🇹
commented

It would be great if they've also improved performance when caches are warm

One thing that I find inconvenient and a waste of resources is that we need to run PHPStan 2 o 4 times for every run, even if cached:

once to get output in gitlab format

    - php vendor/bin/phpstan -vvv analyze --configuration=$_PHPSTAN_NEON --error-format=gitlab > $_ARTIFACTS_DIR/phpstan-quality-report.json || EXIT_CODE=$?

once to get output in junit format

    - php vendor/bin/phpstan -vvv analyze --configuration=$_PHPSTAN_NEON --no-progress --error-format=junit > $_ARTIFACTS_DIR/phpstan-junit.xml || true

and if there are errors,

once again to report to humans
php vendor/bin/phpstan analyze --configuration=$_PHPSTAN_NEON --no-progress || true
and once more get a new baseline generated for storing in artifacts
php vendor/bin/phpstan analyze --configuration=$_PHPSTAN_NEON --no-progress --generate-baseline=$_PHPSTAN_BASELINE || true

I wonder whether it would make sense to file an issue upstream to ask if it would be possible to run analysis only once and produce the 4 outputs at the same time

dww’s picture

dww
we/he/they
commented

Re #17 that mākea a ton of sense. It’d be great to run once and get 4 different outputs simultaneously.

catch’s picture

catch
he/him
Primary language English
commented

I found a feature request for this from 2020, and it recommended creating your own error formatter https://github.com/phpstan/phpstan/issues/3177

The challenge with doing it in phpstan would be it would also need to support a file destination per error format too.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#3568641: [CI] Introduce our own PHPStan ErrorFormatter to avoid multiple PHPStan executions

Filed #3568641: [CI] Introduce our own PHPStan ErrorFormatter to avoid multiple PHPStan executions as a possible follow up to optimize PHPStan execution time.

godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Reviewed & tested by the community

Diff in MR 14422 against is essentially the same as 0ae7c91c on 11.x, so this lgtm.

  • catch committed c4e9b7e4 on main 
    task: #3568370 Update PHPStan to 2.1.34

By: mondrake
By: dww
By: catch
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !14422

andypost’s picture

andypost
he/him
Primary language Russian
commented

For support 8.5 it need bumo to 2.1.36 minimum #3570713: Bump PHPStan to 2.1.38

EDIT: for 2.1.36 https://github.com/JetBrains/phpstorm-stubs/commit/d955c006e300377c12aba...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.