Problem/Motivation

PHPStan released version 1.11.8(See https://github.com/phpstan/phpstan/releases/tag/1.11.8).

After numerous worry-free updates this one uncovers previously unspotted Level 1 issues.

Steps to reproduce

See https://git.drupalcode.org/project/drupal/-/pipelines/234420 and notice the PHP Static Analysis (phpstan) failures on the DEFAULT: Updated dependencies (PHP 8.3 MySQL 8) job for 11.x.

The failures seems to be coming from two enhancements:

Ignored error pattern #^Foo# in path /bar.baz is expected to occur N time, but occurred N+1 times. => Report "missing return" error closer to where the return is missing.
Call to preg_quote() is missing delimiter / to be effective. => https://github.com/phpstan/phpstan/issues/11338

The first type "just" needs a new baseline with the more accurate suppression, which should end up in only the value of count in some suppressions going up.
The second one seems to be easily fixed by adding said delimiter, let's give that a go in this very issue. If it will be more complicated will go for a follow-up and a suppression in this issue.
Turns out explicitly declaring the default value NULL for the $delimiter parameter of preg_quote() appeases PHPStan and makes no changes in the execution of the code (since it's the default value).
Seems like the most safe approach and this is taken in the MR.

Proposed resolution

- Upgrade phpstan/phpstan to 1.11.8
- Create a new baseline, fix Call to preg_quote() is missing delimiter / to be effective.-failures.
- Bump versions of phpstan/phpstan in composer.* to latest, since the newly created baseline won't pass on any lower version.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3463954

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

Spokje created an issue. See original summary.

spokje’s picture

spokje
Primary language Dutch
commented
Issue summary: View changes
spokje’s picture

spokje
Primary language Dutch
commented
Issue summary: View changes

Spokje opened merge request !8938

spokje’s picture

spokje
Primary language Dutch
commented

Hmm, confused now:

1) Drupal\PHPStan\Tests\EnsurePHPStanVersionsMatchTest::testVersions
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'1.11.7'
+'1.11.8'
/builds/issue/drupal-3463954/core/tests/PHPStan/tests/EnsurePHPStanVersionsMatchTest.php:17

vs

https://git.drupalcode.org/project/drupal/-/merge_requests/8938/diffs#14...

spokje’s picture

spokje
Primary language Dutch
commented

Un-confused myself, the problem lies in the other file of the comparison: core/tests/PHPStan/composer.json.

Shouldn't that file be updated automagically when bumping PHPStan in the root composer.json?
Is this worth a follow-up?

spokje’s picture

spokje
Primary language Dutch
commented
Status: Active » Needs review
spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Overall didn't appear to be a lot of missed stuff. Seems pretty straight forward

xjm’s picture

xjm
she/her
Primary language English
commented

For this to be RTBC, we need to prove that it actually solves the problem, eh? ;)

I manually queued the updated deps job for the MR at:
https://git.drupalcode.org/issue/drupal-3463954/-/pipelines/238845

Note that I had to click the button for push access to do this, despite being a project maintainer. Before that, I got this super helpful error message:

An error occurred while making the request.

xjm’s picture

xjm
she/her
Primary language English
commented

Results:
https://git.drupalcode.org/issue/drupal-3463954/-/pipelines/238845

The PHPStan job for updated deps is passing there, but PHPUnit tests failed. Looking into it.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs followup

Failure is:

  Problem 1
    - Root composer.json requires drupal/core-recommended ^11 ->
satisfiable by drupal/core-recommended[11.0].
    - drupal/core-recommended 11.0 requires guzzlehttp/guzzle ~7.8.1 ->
found guzzlehttp/guzzle[7.9.2] but it does not match the constraint.
Failed asserting that 2 matches expected 0.

That... sounds like a problem in the updated deps test logic. I could not find an existing issue.

We could maybe go ahead and commit this regardless, since it makes static analysis work, but I'm scratching my head a bit about how the composer install could work for static analysis but fail for the build test. Going to try requeuing that job to verify that I get the same result....

Edit: The normal PHPUnit suite also passed; it's the build test that failed with the above.

  • catch committed d7fe8641 on 11.x 
    Issue #3463954 by Spokje, xjm: Bump phpstan/phpstan to latest to make...
catch’s picture

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

I'm also confused by those composer errors in the build job, but this is a good step forwards so think we should go ahead here. Committed/pushed to 11.x, thanks!

catch closed merge request !8938

catch’s picture

catch
he/him
Primary language English
commented

Opened #3464995: Updated deps job fails

quietone’s picture

quietone commented
Issue tags: -Needs followup

Status: Fixed » Closed (fixed)

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

andypost’s picture

andypost
he/him
Primary language Russian
commented

I gonna backport it to 10.4 as it become a blocker for composer updates for PHP 8.4 #3478504: upgrade phpstan to 1.11.10 for 10.4.x