Problem/Motivation

phpstan/phpstan released version 1.2.4 (See https://github.com/phpstan/phpstan/releases/tag/1.12.4).

It introduces 2 new errors in the PHPStan analyze phase.

Steps to reproduce

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

 ------ ------------------------------------------------------------------------ 
  Line   core/tests/Drupal/Tests/Core/Update/UpdateRegistryTest.php              
 ------ ------------------------------------------------------------------------ 
  244    Path in include_once()                                                  
         "vfs://drupal/sites/default/modules/module_a/module_a.post_update.php"  
         is not a file or it does not exist.                                     
  245    Path in include_once()                                                  
         "vfs://drupal/sites/default/modules/module_b/module_b.post_update.php"  
         is not a file or it does not exist.                                     
 ------ ------------------------------------------------------------------------ 
 [ERROR] Found 2 errors

Proposed resolution

- Upgrade phpstan/phpstan to 1.2.4
- Bump versions of phpstan/phpstan in composer.* to latest, since the newly created baseline won't pass on any lower version.
- Create a new baseline.
- Create a follow-up issue to deal with the added suppressions.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#38 3475916-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3475916

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:

Comments

spokje created an issue. See original summary.

spokje’s picture

Issue summary: View changes

spokje’s picture

Issue tags: -Needs followup

Personally I believe `vfs` streams are intended to be non-existing and shouldn't be flagged by PHPStan, but more on that in the follow-up #3475920: Handle PHPStan baseline additions about vfs

spokje’s picture

Assigned: spokje » Unassigned
Status: Active » Needs review
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, tests are green, followup for the baseline addition created.

spokje’s picture

Issue summary: View changes
longwave’s picture

Status: Reviewed & tested by the community » Needs work

Needs rebasing.

We probably also want to backport this to 10.4?

spokje’s picture

Well, I'm hoping the upgrade to composer/composer that just landed fixes the composer update issue in the scheduled "updated deps"-runs, since that has been failing for about 2 weeks now with:

$ if [ -n "$COMPOSER_UPDATE" ]; then composer update --optimize-autoloader; composer outdated; fi
> Drupal\Composer\Composer::ensureComposerVersion
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.
  Problem 1
    - Root composer.json requires drupal/core 11.0.x-dev (exact version match), it is satisfiable by drupal/core[11.0.x-dev] from composer repo (https://repo.packagist.org) but drupal/core[dev-main] from path repo (core) has higher repository priority. The packages from the higher priority repository do not match your constraint and are therefore not installable. That repository is canonical so the lower priority repo's packages are not installable. See https://getcomposer.org/repoprio for details and assistance.
  Problem 2
    - Root composer.json requires drupal/core-project-message 11.0.x-dev (exact version match), it is satisfiable by drupal/core-project-message[11.0.x-dev] from composer repo (https://repo.packagist.org) but drupal/core-project-message[dev-main] from path repo (composer/Plugin/ProjectMessage) has higher repository priority. The packages from the higher priority repository do not match your constraint and are therefore not installable. That repository is canonical so the lower priority repo's packages are not installable. See https://getcomposer.org/repoprio for details and assistance.
  Problem 3
    - Root composer.json requires drupal/core-vendor-hardening 11.0.x-dev (exact version match), it is satisfiable by drupal/core-vendor-hardening[11.0.x-dev] from composer repo (https://repo.packagist.org) but drupal/core-vendor-hardening[dev-main] from path repo (composer/Plugin/VendorHardening) has higher repository priority. The packages from the higher priority repository do not match your constraint and are therefore not installable. That repository is canonical so the lower priority repo's packages are not installable. See https://getcomposer.org/repoprio for details and assistance.

I don't _think_ the dependency composer will fix our actual-composer-on-the-test-runner problem, but as a hail mary, it's all I've got.

Can't reproduce any of the above locally.

Looks to me "somehow" the self.version from the above packages lost all its meaning during the update, and composer lands on the if-all-else-fails-fallthrough of dev-main.

Without a fix on the above fixing this issue is basically moot.

longwave’s picture

Over in #3479446: Multiple composer-related tests break with composer 2.8.1 @andypost noted some issues with Composer 2.8 and this is a possible culprit:

https://github.com/composer/composer/pull/12109

Maybe the same issue here?

spokje’s picture

Interesting, thanks @longwave!

Not sure it's exactly the same issue, but at least it gives me/us some pointers on where to look for problems/fixes within composer itself.

spokje’s picture

Status: Needs work » Reviewed & tested by the community

Rebased the MR, back to RTBC.

The actual run with updated deps still fails, but that's something for a new issue IMHO.

spokje’s picture

Title: Bump phpstan/phpstan to latest to make daily "updated deps" QA run pass again » Bump phpstan/phpstan to 1.12.4 to make daily "updated deps" QA run pass again
catch’s picture

Status: Reviewed & tested by the community » Needs work

Not sure why since the rebase was only done a few hours ago, but this appears to need another rebase.

mondrake’s picture

#14: PHPStan baseline changed considerably.

catch’s picture

Oh of course, thanks.

spokje’s picture

Status: Needs work » Reviewed & tested by the community

Sad panda did rebase, all seems well now.

BTW: I had to run a few test-jobs multiple times to get them passing.
Is there a known increase in random test failures lately?

catch’s picture

@spokje there's been a lot but I don't think we have any new issues to track them as such (the one I keep seeing is BlockCacheTest which we already have an issue for but there are others I don't think I've caught at the right time to open an issue).

  • catch committed f6b40af8 on 11.x
    Issue #3475916 by spokje, longwave: Bump phpstan/phpstan to 1.12.4 to...
catch’s picture

Version: 11.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I got a phpstan error that two baseline entries were not found. Just generated the baseline myself again and confirmed those two lines were removed - must be another commit in the past few hours.

Committed/pushed to 11.x, thanks!

I think we should backport this to 10.4.x too.

spokje’s picture

Thanks @catch.

I've got some free time (Mwuahah!), I'll try to, at the very least, create issues for some reoccurring ones.

spokje’s picture

Version: 10.4.x-dev » 11.x-dev
Status: Patch (to be ported) » Reviewed & tested by the community

I was just about to comment that it would surprise me if those two additions to the baseline just vanished, but saw that GitLab CI agrees with me, they're still needed.

 ------ ------------------------------------------------------------------------ 
  Line   core/tests/Drupal/Tests/Core/Update/UpdateRegistryTest.php              
 ------ ------------------------------------------------------------------------ 
  244    Path in include_once()                                                  
         "vfs://drupal/sites/default/modules/module_a/module_a.post_update.php"  
         is not a file or it does not exist.                                     
  245    Path in include_once()                                                  
         "vfs://drupal/sites/default/modules/module_b/module_b.post_update.php"  
         is not a file or it does not exist.                                     
 ------ ------------------------------------------------------------------------ 
 [ERROR] Found 2 errors   

https://git.drupalcode.org/project/drupal/-/pipelines/306083

So I'm afraid we now have a broken 11.x HEAD?
Unsure what status this should have, but I've put the version back to 11.x

(Did you perhaps forgot a composer update somewhere in your procedure to create the new baseline?)

  • catch committed 00bcdbb0 on 11.x
    Issue #3475916 by spokje, longwave: Bump phpstan/phpstan to 1.12.4 to...
catch’s picture

Version: 11.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I forgot to composer install before trying to commit...

Pushed a follow-up with the original, correct, baseline again.

Should be good for backport again now.

spokje’s picture

Status: Patch (to be ported) » Needs review

The 10.4.x branch has a lot more changes in the PHPStan baseline.

I vaguely remember creating an issue about the invalidPregQuote errors that landed in 11.x-land, but wasn't backported to 10.4.x IIRC.

spokje changed the visibility of the branch 11.x to hidden.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Reroll for 10.4 appears good!

catch’s picture

Status: Reviewed & tested by the community » Needs work

rpayanm made their first commit to this issue’s fork.

spokje’s picture

Thanks @rpayanm for merging the latest of 10.4.x back into this MR.

I think this issue is cursed, first the broken HEAD on 11.x and now I somehow managed to create a 10.4.x MR twice without getting it to use the HEAD of that branch, even after I've update fork drupal-3475916.

On top of that, it seems the HEAD of 10.4.x is broken (and let the records show I had nothing to with it ;)

Asked about it in Slack: https://drupal.slack.com/archives/C079NQPQUEN/p1728647212972519

spokje’s picture

Title: Bump phpstan/phpstan to 1.12.4 to make daily "updated deps" QA run pass again » [PP-1] Bump phpstan/phpstan to 1.12.4 to make daily "updated deps" QA run pass again
Status: Needs work » Postponed
Related issues: +#3480180: Recipe validation error in 10.4.x
spokje’s picture

Title: [PP-1] Bump phpstan/phpstan to 1.12.4 to make daily "updated deps" QA run pass again » Bump phpstan/phpstan to 1.12.4 to make daily "updated deps" QA run pass again
Status: Postponed » Needs review

Unblocked.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

10.4 reroll seems good. Verified same version on 11.x too.

needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new90 bytes

The 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.

spokje’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: +no-needs-review-bot

How about no...

  • longwave committed a90082c9 on 10.4.x
    Issue #3475916 by spokje, catch, longwave: Bump phpstan/phpstan to 1.12....

  • longwave committed e19627df on 10.5.x
    Issue #3475916 by spokje, catch, longwave: Bump phpstan/phpstan to 1.12....
longwave’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 10.5.x and 10.4.x.

Was unable to run the full PHPStan check at commit time locally, it hung for over 15 minutes before I gave up - usually it's done in less than 3. I don't think there's anything wrong as the bot here seems happy.

longwave’s picture

Apologies @rpayanm I should have credited you, saving credits here though you are not in the commit message.

Status: Fixed » Closed (fixed)

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