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
PHPStan released version 1.9.3 (See https://github.com/phpstan/phpstan/releases/tag/1.9.3)
Looking at https://www.drupal.org/node/3060/qa it removes some false positives on Level 1.
Steps to reproduce
See https://www.drupal.org/node/3060/qa and notice the failures on both updated deps jobs for 10.0.x and 10.1.x
Proposed resolution
- Upgrade PHPStan to 1.9.3
- Create a new baseline
- Bump version of PHPStan in composer.lock to 1.9.3, since the newly created baseline won't pass on 1.9.2
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #19 | 3327018-10.1.x-19.patch | 1.52 KB | spokje |
| #2 | 3327018-2.patch | 7.76 KB | spokje |
Comments
Comment #2
spokjeComment #3
spokjeComment #4
spokjeComment #5
longwaveLooks great; seems PHPStan has solved some false positives with "Variable might not be defined" (although seemingly added some new ones, given the $randX vars in StyleTest look OK to me).
Comment #6
longwaveNot sure this can be committed to 10.0.x; while updated PHPStan fails on core it doesn't technically affect anyone. But that means the "updated deps" CI job is a bit pointless on that branch...
Comment #7
spokjeHmm, could you enlighten me with the rules on this?
If we can't commit it in 10.0.x this means we can never update a dependency in 10.0.x? Only when there's a SA out for it or something?
Can't remember the rules being that strict on 9.x branches, AFAIK, we updated dependencies (at least for patch-, maybe even for minor-level version updates) just before every new release?
Comment #8
longwaveThis is the first release we have done with PHPStan in core-dev, so this is the first time we've had to consider changing the baseline in a patch release, and this update might also affect anyone else using core-dev?
https://www.drupal.org/about/core/policies/core-change-policies/allowed-...
This isn't a security update, does it count as an "important bug fix"?
Comment #9
spokjeAh, thanks for the link, somehow I remembered we update all dependencies for every patch release, but it is indeed minor releases only.
> This isn't a security update, does it count as an "important bug fix"?
Hard to say, the world won't end when we don't do it in 10.0.x.
However if we _don't_ do it:
- We're going to have diverging PHPStan baselines, which doesn't make fixing our baseline errors easier, since we're going to need 2 patches (for 10.current and 10.current+1).
- As you correctly noticed, there's no use for the "updated deps" job on 10.current, that should always only be on 10.current+1
I'll return to the simpler things in life and leave the decision on that to Bigger Brains.
Comment #10
spokjePHPStan released 1.9.4 (https://github.com/phpstan/phpstan/releases/tag/1.9.4), but that didn't change the baseline.
Meaning, we can commit the patch and are not forced to bump to 1.9.4 in
composer.lockto make the updated deps test-runs pass.Comment #11
xjmI queued the updated deps job to confirm the issue is solved by the patch.
Comment #13
xjmI accidentally committed this to 10.0.x first, but wasn't about redo an hour-long scan. So it's been cherry-picked forward to 10.1.x.
It also cherry-picked to 9.5.x with some auto-merging. I realize I forgot to queue a 9.5 job earlier. So we'll have to make sure HEAD doesn't fail as a result of me doing that.
Thanks everyone for keeping this updated!
Comment #14
xjmComment #15
longwaveAs far as I can see only the 10.0.x branch was pushed to git. We also don't run PHPStan on 9.5.x, not sure it needs backporting to there at all.
Comment #17
longwaveCommitted and pushed to 10.1.x.
Removing tag for 9.5.1 as I don't see how we can commit it there.
Comment #18
mondrakeSince this changed the baseline, I think it should also have bumped the version in
composer.jsonto ^1.9.3, otherwise someone running with 1.9.2 might incur in errors.Comment #19
spokjeI think I agree with @mondrake on this one, especially seeing that we currently have
^1.8.11in the composer.json, which is a minor below what we're recommending currently.Attached a patch that should up the version.
Comment #20
mondrakeThanks
Comment #22
catchMakes sense to bring the constraint into line too. Committed/pushed to 10.1.x and cherry-picked to 10.0.x, thanks!