Problem/Motivation

----------------------------------------------------------------------------------------------------

Running PHPStan on *all* files.
 ------ --------------------------------------------------------------------
  Line   core/modules/mysql/src/Driver/Database/mysql/Connection.php
 ------ --------------------------------------------------------------------
  125    Array has 3 duplicate keys with value 0
         (\Pdo\Mysql::ATTR_USE_BUFFERED_QUERY, \Pdo\Mysql::ATTR_FOUND_ROWS,
         \Pdo\Mysql::ATTR_MULTI_STATEMENTS).
         🪪  array.duplicateKey
 ------ --------------------------------------------------------------------

Steps to reproduce

Proposed resolution

Resolve it

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3570713

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

larowlan created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Related issues: +#3537713: [12.x] Require PHP 8.5
mondrake’s picture

mondrake
Location 🇮🇹
commented
Priority: Normal » Critical

The bot is systematically killing the RTBC queue right now.

andypost’s picture

andypost
he/him
Primary language Russian
commented

It needs update phpstan at least to 2.1.35 to use proper stubs https://github.com/JetBrains/phpstorm-stubs/commit/d955c006e300377c12aba...

andypost opened merge request !14572

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
Related issues: +#3568370: Update PHPStan to 2.1.34

updated to latest 2.1.38

mondrake’s picture

mondrake
Location 🇮🇹
commented

It does not seem that HEAD pipelines are broken, though. Rather, it’s the check done by the bot that is failing. No?

andypost’s picture

andypost
he/him
Primary language Russian
commented

basically stub shipped with phpstan are bundled into it so 1.x branch makes no sense anymore

andypost’s picture

andypost
he/him
Primary language Russian
commented

Looking at date of release - 2.1.36 is tagged in few hours after fix of stubs

andypost opened merge request !14573

mondrake’s picture

mondrake
Location 🇮🇹
commented

Bumping PHPStan is good in itself, but I still do not understand why MR pipelines targeting main are passing, while the bot is failing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Can someone post what is actually failing? HEAD is not failing - the daily tests are not.

mondrake’s picture

mondrake
Location 🇮🇹
commented

#12 exactly. HEAD is fine. It’s the bot running a script that fails and therefore changing issue status to NW. It happened to half the RTBC issues already.

Example of failure: https://www.drupal.org/project/drupal/issues/3558167#comment-16448917

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

The bot is a script that runs on @nod_'s computer - not sure how it works exactly, maybe it doesn't use the same versions?

I think we need a kill switch for it somewhere so we can turn it off in situations like this.

nod_’s picture

nod_
Primary language French
Location Lille
commented

I stopped the bot for now. I think I'll change the approach and stop testing locally. I might see if I can trigger the verification steps periodically in the CI directly instead of doing that locally.

The bot does a composer/yarn install then runs ./core/scripts/dev/commit-code-check.sh on php 8.4.6.

mondrake’s picture

mondrake
Location 🇮🇹
commented

#15: +1.

We should have one source of truth only, and IMHO that should be the CI results.

Two sources of truth = beginning of holy war 😁

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Priority: Critical » Normal

@nod_ can the bot use .gitlab-ci/php.yml to determine which PHP to use?

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: PHPStan failure in HEAD on PHP 8.5 » Bump PHPStan to 2.1.38
Status: Needs review » Needs work
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review

fixed in main, still unsure about drop 1.x support from 11.x branch

mondrake changed the visibility of the branch 3570713-phpstan-11.x to hidden.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

I've hidden the 11.x branch for now. In main for sure it's high time we drop that.

nicxvan’s picture

nicxvan commented

I'll just chime in to say the bot does occasionally find things that ci doesn't when an mr is behind.

For example when tests began to require the run in separate process attribute it was easy to forget on a branch that hasn't been rebased but the bot would catch it. The error in those cases cutoff have been improved. And a way to pause the bot externally would be good to prevent situations like last night.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Yeah a bot testing RTBCs for code style and stuff is great - it just needs to be configured using details from the branch it is testing.

Committed cc433fe and 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.

  • alexpott committed cc433fe1 on main 
    chore: #3570713 Bump PHPStan to 2.1.38

By: mondrake
By: andypost
mondrake’s picture

mondrake
Location 🇮🇹
commented

Too late... will open a follow up.

nicxvan’s picture

nicxvan commented

it just needs to be configured using details from the branch it is testing.

The target branch right?

mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#3570825: Lock PHPStan and dependencies to latest versions.

Re #26

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@nicxvan nope but an MR change change the minimum PHP for example... which is why the bot broke everything.

cmlara’s picture

cmlara
he/him
commented
Related issues: +#3532698: phpstan dev constraints make it difficult for modules to support 11.2 and below

It appears this may be reverting #3532698: phpstan dev constraints make it difficult for modules to support 11.2 and below

This change will likely force all contrib to upgrade to PHPStan 2.x .

It appears 10.6 (which is supported until December 2026) still only supports PHPStan 1, Contrib will need to support ^1||^2 for at least another year.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

@cmlara this was only committed to main which will become Drupal 12, the 11.x and 10.6.x branches are still open separately? At some point we have to stop supporting PHPStan 1, Drupal 12 seems like a good time to do that.

cmlara’s picture

cmlara
he/him
commented
Related issues: +#3530909: Create metapackage of (loose) contrib test dependencies

IIRC most talk has been about encouraging modules to support D10.6 through D12 in a single release (which is very doable from a Drupal API standpoint for most modules).

The PHPStan config files are often different between the two PHPStan majors, changes, some config options needed in one version of PHPstan would raise fatal errors in the opposite major (I have since learned of a few tricks that could maybe help avoid this, however they seem hacky and possibly are off-api usage of PHPStan).

#3530909: Create metapackage of (loose) contrib test dependencies would be the Ideal solution, with D13 being a fallback (or if you wanted to get hacky, something like a 12.3 release)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I think if you want to run phpstan against mulitple versions of core you are going to need use different baselines. Personally, on my contrib projects (some of which support D9 to D11) I only run PHPStan on the latest Drupal version. Most of them have decent test coverage so I don't feel that PHPStan on older versions adds much and it avoids things like having to maintain multiple baselines because of deprecations.

cmlara’s picture

cmlara
he/him
commented

Diffrent baseline is the least hacky aspect, if using a PHP baseline one can do a simple if statement to include or exclude entries based on version (if even needed, I have a pretty good idea of what errors are per versions, and when not I just run it on the root release and the fix release and look for an error count difference).

Root phpstan.neon changes however can be test breaking between ^1 and ^2 (preventing even running any analysis).

I don't feel that PHPStan on older versions adds much and it avoids things like having to maintain multiple baselines because of deprecations.

Not wrong, especially with how under documented core is especially in older releases, however core shouldn’t be forcing developers into a position where they can’t make a decision for their own.

As written my modules require phpstan ^1 in require dev, if I move it to ^2 all composer stages will fail for 10.6 preventing running PHPUnit tests. I could lie and claim ^1 || ^2 however that sets a poor experience for those running the module (not much better that core does it, however it is easier to explain a single package instead of hundreds of contrib packages).

The above is also ignoring that phpstan-drupal (last I looked) still had faults misidentifying code paths (entityquery access control comes to mind) and I’m giving it more time to stabilize before adopting its 2.x branch.

andypost closed merge request !14572

andypost closed merge request !14573

Status: Fixed » Closed (fixed)

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