Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
This issue is split out from #3426047: Bump PHPStan to level 9 and accept a large baseline
In version 1.10.2 PHPStan introduced the ability to generate a baseline in PHP format. This can help with performance in case of large baseline files by ensuring PHPStan doesn't need to parse a file but can include it as PHP directly.
There's another potential benefit in reducing merge conflicts, though this still has to be confirmed in practice. The theory is that git is not good at diffing a large YAML list (which is the neon format) because the start and end of a rule are not distinct (an example excerpt from the Open Social baseline):
parameters:
ignoreErrors:
-
message: "#^Method Drupal\\\\activity_basics\\\\Plugin\\\\ActivityAction\\\\CreateActivityAction\\:\\:create\\(\\) has no return type specified\\.$#"
count: 1
path: modules/custom/activity_basics/src/Plugin/ActivityAction/CreateActivityAction.php
-
message: "#^Method Drupal\\\\activity_basics\\\\Plugin\\\\ActivityAction\\\\CreateActivityAction\\:\\:create\\(\\) has parameter \\$entity with no type specified\\.$#"
count: 1
path: modules/custom/activity_basics/src/Plugin/ActivityAction/CreateActivityAction.php
-
message: "#^\\\\Drupal calls should be avoided in classes, use dependency injection instead$#"
count: 1
path: modules/custom/activity_basics/src/Plugin/ActivityAction/CreateActivityAction.php
For the PHP format there are more clearly defined boundaries because the start and end of a rule are distinctly delineated by different tokens (the PHP open and close array syntax). Additionally there is at least some logic in git to understand PHP as a language (for example to highlight the function name in a diff -- though the extend of this seems to be undocumented). An excerpt from a level 9 PHP baseline from Drupal Core:
<?php declare(strict_types = 1);
$ignoreErrors = [];
$ignoreErrors[] = [
'message' => '#^Call to function method_exists\\(\\) with \'Composer\\\\\\\\Composer\' and \'getVersion\' will always evaluate to true\\.$#',
'count' => 1,
'path' => __DIR__ . '/../composer/Composer.php',
];
$ignoreErrors[] = [
'message' => '#^Method Drupal\\\\Composer\\\\Composer\\:\\:drupalVersionBranch\\(\\) should return string but returns string\\|null\\.$#',
'count' => 1,
'path' => __DIR__ . '/../composer/Composer.php',
];
$ignoreErrors[] = [
'message' => '#^Parameter \\#2 \\$base_dir of method Drupal\\\\Composer\\\\Generator\\\\ComponentGenerator\\:\\:generate\\(\\) expects string, string\\|false given\\.$#',
'count' => 1,
'path' => __DIR__ . '/../composer/Composer.php',
];
Steps to reproduce
Proposed resolution
Convert the .neon baseline for PHPStan to PHP format.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
The baseline for Drupal Core is now generated using the PHP format.
| Comment | File | Size | Author |
|---|---|---|---|
| #24 | 3426548-nr-bot.txt | 90 bytes | needs-review-queue-bot |
| #19 | 3426548-nr-bot.txt | 90 bytes | needs-review-queue-bot |
Issue fork drupal-3426548
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:
changes
Comments
Comment #3
kingdutchComment #4
andypostIs there perf impact on time of execution?
It needs CR and update docs about new way to generate
Comment #5
kingdutchUsing the following code to time analysis of
bd4b8b98a7(the commit in the MR before the change) and the change itself.Full analysis with baseline in place and without generating a new baseline
Full analysis without baseline in place and generating the baseline from scratch (neon/php used depending on commit tested).
Three runs each (m:ss):
A draft CR has been created. I'm not sure which docs would need to be updated.
Comment #6
andypostI mean official docs but can't find any except https://www.drupal.org/search/site/Phpstan%20%20baseline?f%5B0%5D=ss_met...
This CR should be updated https://www.drupal.org/node/3258232 with a link to new one probably
Comment #7
kingdutchI've created the following documentation page which outlines how PHPStan is used in Drupal core: https://www.drupal.org/docs/develop/development-tools/phpstan/phpstan-in.... I attempted to write it in a way so that it's not outdated when #3426047: Bump PHPStan to level 9 and accept a large baseline lands.
I've added a reference to the new documentation in the CR for this issue: https://www.drupal.org/node/3426891
I've updated the previous CR that andypost linked to also reference the CR for this issue: https://www.drupal.org/node/3258232. The formatting may need some work, I'm not sure if there's a best practice for those kinds of references.
Comment #8
andypostI think the change is minimal and brings 20-25% speed-up for the CI run!
as PHP 8.3 is the default now we can try use PHP-JIT for the job)
Comment #9
mondrake🔝
Comment #10
longwaveGiven the performance improvement I am +1 for landing this, but a bit concerned about conflicting with other ongoing work. We know this will conflict with all other PHPStan issues and require trolling, but do we have a sense of how many other issues will be affected here?
I suppose what I'm asking is: is this safe enough to commit now or should we wait for the disruptive patches beta window? I think the level bump issue will almost certainly have to wait, as I imagine most ongoing MRs will have undiscovered level 2+ issues.
Comment #11
mondrakeMy 0.02 - I think it’s fine to commit now - the RTBC queue is at its lowest since long. Adjusting any pending MR is just a rebase + c/p of the baseline artifact, since we are not changing the rule level.
Comment #12
alexpottWe need to prevent access to the php here from .htaccess and web.config.
We also should have MRs up for 10.3.x and 10.2.x so that we can land this all in 1 go.
Comment #13
longwaveShould we make it a dotfile, ie.
core/.phpstan-baseline.php? AFAIK those are blocked from .htaccess/web.config already.Comment #14
andypostNice idea with dot-file
Comment #15
catchAgreed with #11, we've got a lucky window of a shorter RTBC queue than we've had for months, so this will be less disruptive than it otherwise would be.
Comment #18
kingdutch11.x branch updated. Also updated the documentation and CR to match.
Created 10.2.x and 10.3.x branches with MRs :)
Comment #19
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #20
kingdutchRebased the three branches :D The diff in the new format for the changes on the 10.3.x branch was very readable 🤩
Comment #21
mstrelan commentedSince this file is php do IDEs like PhpStorm try to analyse it? It can obviously be excluded but does that involve manual steps?
Comment #22
kingdutchI can not answer for all IDEs but only for PHPStorm. PHPStorm does seem to index the file. As far as I found there's nothing we can do against this with things we ship in the repo. I also don't know if it's a problem (i.e. in what ways it affects PHPStorm usage).
There are settings in PHPStorm that allow excluding files which are global settings that apply to all projects. So in case the analysis of the file by PHPStorm causes problems for someone it's a one-time fix.
Comment #23
longwaveThis looks good to me, I think all issues have been addressed and the performance improvement should make this worthwhile.
Marking RTBC and will ping committers as otherwise this is going to end up being rerolled over and over, given we are landing other PHPStan fixes at the moment.
Comment #24
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #25
catchcore/scripts/dev/commit-code-check.shalso needs to be updated.Comment #26
kingdutchMoving this back to RTBC as per #23.
#24 is a race condition with a mistake for 6996 which was in my push between #19 and #20 but fixed between #21 and #22. GitLab reports that 6996 is fully mergeable at the moment (contrary to the review bot)
Comment #27
kingdutch#26 crossposted with #25. Moving back to needs review after change in
core/scripts/dev/commit-code-check.shComment #28
longwaveReview was addressed, back to RTBC.
Comment #29
alexpottWe need to add
To .gitattributes so that git doesn't complain about whitespace errors in the new baseline file. This is because PHPStan's baseline generator uses tabs whereas we tell git that that is a whitespace error for PHP. Going to fix this on commit. I checked with @longwave first.
Committed 1ff974793e and pushed to 11.x. Thanks!
Committed 9b890f0414 and pushed to 10.3.x. Thanks!
Committed a3b7bc2 and pushed to 10.2.x. Thanks!
I think we also need to think about #21 a bit more especially before we merge the monster.
Backported to 10.2.x in order to make supporting that branch and 10.3.x and 11.x simpler.
Comment #33
godotislateGetting a failing test: https://git.drupalcode.org/issue/drupal-3427388/-/jobs/1049195/raw