[D10] Add the static analyzer tool PHPStan to Backdrop core

[klonos]

This is a fresh feature in Drupal 10, which sounds like something that we should integrate without our GHA: https://www.drupal.org/node/3258232

Description:

The static analyzer tool PHPStan has been added to Drupal Core. We are running the tool with level 0 (the lowest level). The power of static analyzer is that it can find bugs in your code without having to write tests. PHPStan has been added to the script core/scripts/dev/commit-code-check.sh which is part of the testbot.

The PHPStan configuration file is located at: core/phpstan.neon.dist

The PHPStan file with the skipped exceptions is located at: core/phpstan-baseline.neon

CLI Commands

Running PHPStan on drupal core: vendor/bin/phpstan analyze --configuration=core/phpstan.neon.dist Regenerating the baseline: vendor/bin/phpstan analyze --configuration=core/phpstan.neon.dist --generate-baseline ./core/phpstan-baseline.neon

[klonos]

There already exists a GHA: https://github.com/php-actions/phpstan

ping @indigoxela 🙂

[indigoxela]

Hm, out of curiosity I ran phpstan analyze core locally at level 0 - loads of errors. Some false positives caused by api.php and tpl.php files, but also real problems like "Function views_ajax_render not found".

We should probably do it in baby steps. :wink:

[indigoxela]

No need for curl or composer, the GitHub action shivammathur/setup-php provides it via tools:.

      - name: Setup PHP
        uses: shivammathur/setup-php@v2
        with:
          php-version: '8.1'
          tools: phpstan

[indigoxela]

A PR exists for review.

But, we have to decide how to deal with this remaining error, which we can't put in the baseline:

 ------ ---------------------------------------------------------------------------------------------------------------------- 
  Line   theme.inc                                                                                                             
 ------ ---------------------------------------------------------------------------------------------------------------------- 
  432    Return type mixed of method ThemeRegistry::offsetExists() is not covariant with tentative return type bool of method  
         ArrayAccess::offsetExists().                                                                                          
         💡 Make it covariant, or use the #[\ReturnTypeWillChange] attribute to temporarily suppress the error.                
 ------ ----------------------------------------------------------------------------------------------------------------------

Should we use ReturnTypeWillChange - like we already did in BackdropCacheArray::offsetExists?

Another option would be to use an older PHP version for this workflow, but...

[indigoxela]

This is what failures look like on GH: https://github.com/backdrop/backdrop/actions/runs/2705293165

I'm think, the annotations get attached to the PR, when the error is in code that gets added. Didn't test that yet.

[indigoxela]

We have another issue here in the queue to run phpcs, so the question has been raised, if PHPStan and phpcs overlap. They don't as they do pretty different things. And IMO we should have both, static code analysis AND coding standard checks.

I'll move the neon files below our .github folder as requested, so they're not part of released files.

[indigoxela]

Done, neon files moved to the .github folder.

Some screenshots with example code temporarily added to the PR:

Annotation on the summary:

Details (error list):

And now for the cool part: Code added in the current PR gets direct annotations:

[indigoxela]

The baseline files added to this PR, which suppress errors for legacy code, are generated with phpstan:

phpstan analyze -c .github/phpstan/phpstan.neon core/modules/ --generate-baseline=.github/phpstan/phpstan-baseline-modules.neon

I had to split it up in two files, as the whole core run checks 900+ files - and then my dev box runs out of memory.

[indigoxela]

It has been asked, what happens, when one of the ignored errors (baseline) gets fixed. It depends.

With the config parameter reportUnmatchedIgnoredErrors: false nothing happens.

Without that parameter PHPStan informs of the change:

Maybe relevant here: Seems like Github tests a new feature to annotate unchanged files - and this is what it looks like when that happens:

[klonos]

...the question has been raised, if PHPStan and phpcs overlap. They don't as they do pretty different things.

Agreed 👍🏼

Thank you @indigoxela for taking the lead for all these tasks 🙏🏼 - they will greatly help us reduce manual work during reviews, provide more confidence, and reduce the chances of problematic code accidentally slipping into the codebase.

[indigoxela]

... they will greatly help us reduce manual work during reviews

Thank you! Yes, that's the intention behind all my suggested PRs. :wink:

For me this one here is currently "on pause", as there should be more feedback. And it does more than the phpcs PR, which will already provide a lot of help for both, people providing PRs and people reviewing. So my suggested priority is on #5245.

[hosef]

@indigoxela I took a look at your PR and things look good so far. One thing that I did find when testing locally was that there were a lot of errors even when using the files to ignore existing errors, so I think those may need to be regenerated.

[indigoxela]

Yes, they need to get regenerated (hence "needs work"), but I postponed that, because there seem to be some concerns to get this in core. To me it seems like the time for phpstan in B hasn't come yet. :grinning:

[hosef]

I discussed this during the dev meeting today and everyone seemed positive about getting this into core. Generally it seems that as long as we keep the ignore files up to date so that people don't have to mess with them a bunch then it's fine to add this.

[indigoxela]

Another option is to suppress the (not found anymore) nagging.

reportUnmatchedIgnoredErrors: false

See: https://phpstan.org/user-guide/ignoring-errors

[hosef]

That would probably be a good idea, and then we could just generate new files after each release or something like that.

[indigoxela]

Parameter changed, rebased, fresh baseline files generated.

But on PHP 8.1 we now need yet another #[\ReturnTypeWillChange] - I also added the explanation in a comment.

Bad thing: we have two false positives (phpstan 1.8.10), and searching the PHPStan issue queue ... guess whom I found :wink:

• https://github.com/phpstan/phpstan/issues/4565
• https://github.com/phpstan/phpstan/issues/7417

(Spoiler: extended family)

Maybe we just wait a little longer to merge our phpstan action?

Or switch to PHP 8.0 with the check, or use a specific phpstan version (patch level) that doesn't produce the false positive...

[hosef]

If you set the runner to PHP 8.0 does that make the false positives go away?

[indigoxela]

If you set the runner to PHP 8.0 does that make the false positives go away?

On my dev I don't have PHP 8.0 available (anymore), but the false positive goes away with PHP 7.4.

The main idea behind using the latest stable of PHP is, that PHPStan would be able to find deprecation problems, before we run into actual bugs with those. However, if this produces false positives ...

One remaining benefit: we're able to find "dead code", like for instance in views_ui, where a function views_ajax_render() is used - but that doesn't exist.