Start running PHPStan on Drupal core (level 0)

+++ b/core/lib/Drupal/Core/Form/FormStateInterface.php
@@ -491,7 +491,7 @@ public static function hasAnyErrors();
-   * @param string $message
+   * @param string|\Drupal\Component\Render\MarkupInterface $message

These changes are tricky. Because I think what we want here is to use the PHP 8 pseudo class stringable. I think PHPStan should recognise when you are passing an object that implements __toString() and think this is okay - or at least make that behaviour configurable.

One thing that PHPStan allows you to do that is neat is set a baseline. So we could start with that and then work towards fix all the errors in the baseline. See https://phpstan.org/user-guide/baseline

The automation of this probably should be a follow-up from #3178845: Run same checks as committers do on DrupalCI

Also I've noticed quite a few PHP projects moving away from phpstan and to psalm

@klausi thanks for link, it looks less terrible then 2 years ago

The testbot is returning with:

19:46:44 [Deprecation] [-ERROR-] No files found for pattern 'jenkins-drupal_patches-72429/artifacts/phpstan/*.xml'. Configuration error?
19:46:44 [Deprecation] Searching for all files in '/var/lib/drupalci/workspace' that match the pattern 'jenkins-drupal_patches-72429/artifacts/phpstan/*.xml'
19:46:44 [Deprecation] Skipping post processing
19:46:44 [Deprecation] No filter has been set, publishing all 0 issues
19:46:44 [Deprecation] Using reference build 'drupal_patches #72426' to compute new, fixed, and outstanding issues
19:46:44 [Deprecation] Issues delta (vs. reference build): outstanding: 0, new: 0, fixed: 0
19:46:44 [Deprecation] No quality gates have been set - skipping
19:46:44 [Deprecation] Health report is disabled - skipping
19:46:44 [Deprecation] Created analysis result for 0 issues (found 0 new issues, fixed 0 issues)
19:46:44 [Deprecation] Attaching ResultAction with ID 'phpstan' to run 'drupal_patches #72429'.

See: https://dispatcher.drupalci.org/job/drupal_patches/72429/console

When I run the command on my local machine I get the following error:

vagrant@drupal92devp:/var/www/drupalvm/drupal$ ./phpstan.sh
Lets run some stans!
Note: Using configuration file /var/www/drupalvm/drupal/core/phpstan.neon.
 594/594 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ -------------------------------------------------------------------- 
  Line   Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php    
 ------ -------------------------------------------------------------------- 
  430    Class Symfony\Component\ExpressionLanguage\Expression not found.    
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
 ------ -------------------------------------------------------------------- 

                                                                                                                 
 [ERROR] Found 1 error                                                                                           

@klausi: Could you explain why the MR has so many changes in it. When I apply your MR without all the added fixes I only get 5 errors:

 594/594 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ ----------------------------------------------------------------------------- 
  Line   Component/Bridge/ZfExtensionManagerSfContainer.php                           
 ------ ----------------------------------------------------------------------------- 
  132    Cannot instantiate interface Laminas\Feed\Reader\ExtensionManagerInterface.  
  132    Cannot instantiate interface Laminas\Feed\Writer\ExtensionManagerInterface.  
 ------ ----------------------------------------------------------------------------- 

 ------ -------------------------------------------------------------------- 
  Line   Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php    
 ------ -------------------------------------------------------------------- 
  430    Class Symfony\Component\ExpressionLanguage\Expression not found.    
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
 ------ -------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------------ 
  Line   Core/Cache/Context/CacheContextsManager.php                                                           
 ------ ------------------------------------------------------------------------------------------------------ 
  265    Access to an undefined property Drupal\Core\Cache\Context\CacheContextsManager::$validContextTokens.  
 ------ ------------------------------------------------------------------------------------------------------ 

 ------ ------------------------------------------------------------------------- 
  Line   Core/Cache/PhpBackend.php                                                
 ------ ------------------------------------------------------------------------- 
  255    Access to an undefined property Drupal\Core\Cache\PhpBackend::$storage.  
 ------ ------------------------------------------------------------------------- 

                                                                                                                 
 [ERROR] Found 5 errors                                                                                          
                                                                                                                 

Additional changes: I started out in this issue with level 1 checking but then found that there is also level 0 in PHPStan. I did not want to throw away the level 1 fixes here since they are all reasonable and I think we can keep them.

I completely understand you and if I where in your position I would like to do the same. Only I think that it is better do do only the things in this issue to make it past level 0 and move the other changes to the followup issue for passing level 1. Sorry :-(

@klausi: Thank you for answering all my questions. PHPStan was new for me. I think we are going to do a lot more PHPStan issues. Do you have any indication how much memory I would need?

It all looks good to me.
For me it is RTBC.

Given we are adding a new dependency to Drupal we need to complete a dependency evaluation. See https://www.drupal.org/about/core/policies/core-dependencies-policies/co... for the type of table and information we need.

We also need a change record and release note describing the new dependency and how core devs can use it. The new script makes things pretty simple.

If core will maintain compatibility wiyh phpstan, contrib modules will have less surface to maintain

For example upgrade status could drop few dependencies #3100308: Make Upgrade Status tests Drupal 9 compatible

I couldn't figure out how to rebase the existing MR so I made a new branch and MR. This updates PHPStan to the latest version, adds a composer run command, adds PHPStan to commit-code-check.sh, adds the .profile extension for scanning and skips *.api.php from being scanned.

If I run it on the full lib directory I get an error that I don't know how to resolve:

Child process error (exit code 255): PHP Fatal error: During class fetch: Uncaught PHPStan\Broker\ClassAutoloadingException: Class Drupal\system\ModuleDependencyMessageTrait not found. in
phar:///.../vendor/phpstan/phpstan/phpstan.phar/src/Reflection/Runtime/RuntimeReflectionProvider.php:133

In the 9.2.x branch I've merged in the changes from #3190393: Start running PHPStan on Drupal core (level 0 - part 2) as well. If we are to add this to the commit check script we can't do this in two parts, we have to clean all this up in one go. It might be worth spinning some of these changes out into separate issues and committing them first.

As there are so many changes in level 0, maybe another way of doing this would be to tackle it as we do with phpcs: enable one rule at a time.

We could run a custom ruleset and fix individual rules in separate issues, that is probably a good way of scoping this anyway?

Child process error (exit code 255): PHP Fatal error: During class fetch: Uncaught PHPStan\Broker\ClassAutoloadingException: Class Drupal\system\ModuleDependencyMessageTrait not found. in
phar:///.../vendor/phpstan/phpstan/phpstan.phar/src/Reflection/Runtime/RuntimeReflectionProvider.php:133

This seems to be because of core/lib/Drupal/Core/Extension/ThemeInstaller.php:

use Drupal\system\ModuleDependencyMessageTrait;

This is core depending on a module, but PHPstan doesn't know how to autoload modules, so it crashes here.

edit: opened #3204220: ModuleDependencyMessageTrait is used by core but belongs to system module

Good catch! I bet there's more places to fix, like usage of core in components

@andypost this is the only case I could find of a module being used from Drupal\Core, and there are only @see references from Drupal\Component to Drupal\Core - no actual dependencies.

With the help of @mglaman's mglaman/phpstan-drupal package, we can autoload module code correctly, which gives a fuller set of results.

The 3178534-third-attempt branch contains only PHPStan and mglaman/phpstan-drupal, with a number of common errors ignored for the time being. This leaves just under 100 errors that seem feasible to fix as a first pass, the full output is attached.

I've opened several issues:

#3204763: Fix mismatched sprintf calls
#3204764: PHPUnit assertions do not return a value
#3205024: Missing use statement in Drupal\Core\Database\Driver\mysql\Connection
#3205026: Missing use statement in Drupal\Core\KeyValueStore\DatabaseStorage
#3205029: DestinationCategoryTest refers to a non-existent class
#3205031: Missing use statement in Drupal\form_test\Form\FormTestVerticalTabsForm
#3205037: Drupal\Tests\Component\Annotation\PluginIdTest tests a non-existent constructor

Some of the issues raised by PHPStan also already have existing issues open:

#3106455: Undefined variable used in /core/modules/system/tests/src/Functional/FileTransfer/TestFileTransfer.php
#2895933: EntityReverse will always use the 'standard' join plugin because of an undefined variable $def
#2935654: Use of undefined $languages variable in NodeListBuilder::buildRow()

Some more individual issues spun off from here

#3210888: Undefined static method Drupal\Core\Database\Connection::serialize()
#3210763: MailManagerTest::testGetInstance() is buggy

This one already has an issue open:

#2595025: None validator incorrectly handles numeric arguments

Marking "needs review" to try and get some eyes on this. The latest run says 5 errors though I only really see 3:

     Internal error: Internal error: Cannot assign an empty string to a string  
     offset in file                                                             
     /var/www/html/core/modules/link/tests/src/Kernel/LinkItemTest.php          
     Run PHPStan with --debug option and post the stack trace to:               
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.md       

     Child process error (exit code 255): PHP Warning:  Undefined array key     
     "class" in                                                                 
     /var/www/html/vendor/mglaman/phpstan-drupal/src/Drupal/ServiceMap.php on   
     line 41                                                                    

     PHP Fatal error:  Declaration of                                           
     Drupal\Core\Database\Statement::fetchAll($mode = null, $column_index =     
     null, $constructor_arguments = null) must be compatible with               
     PDOStatement::fetchAll(int $mode = PDO::FETCH_BOTH, mixed ...$args) in     
     /var/www/html/core/lib/Drupal/Core/Database/Statement.php on line 184      

The second one is https://github.com/mglaman/phpstan-drupal/issues/184, not sure what to do about the other two; I can't reproduce the internal error locally on that file on its own.

1. I am missing in core/scripts/dev/commit-code-check.sh a check that when we update the file core/phpstan.neon.dist we run the phpstan check on all files, not just the files that have changed in the patch or MR. See: #3224583: The testbot does not run PHPCS on all files when core/phpcs.xml.dist is changed.
2. Can we add failures from comment #44 to ignoreErrors list in core/phpstan.neon.dist? I think that the parameter typehint "mixed" is something that will have to wait, because it is PHP 8.0+.

PHPStan has a 1.0 release: https://phpstan.org/blog/phpstan-1-0-released

I'm sure I saw @alexpott or someone similar asking for stable releases if we are to include this into core, so hopefully the release of a 1.0 will help with that.

Pure reroll, rebased on 9.4.x.

@longwave would you mind changing the MR target brach to 9.4.x? This can only be done by the creatore of the MR.

Per #3190585-5: [META] Run PHPStan on Drupal core, I would suggest to rescope this issue to

Add phpstan config to core (level 0). Enable it on CI, and in pre-commit-script, do not fix any problems, just add a baseline. This way we don't introduce any new phpstan level 0 errors. But can keep this part focussed on adding and running phpstan.

Something is wrong with mglaman/phpstan-drupal autoloader I am afraid ... Class PHPStan\Drupal\ServiceMap not found is right, in release 1 the namespace was changed from

namespace PHPStan\Drupal;

[...]

class ServiceMap

to

namespace mglaman\PHPStanDrupal\Drupal;

[...]

class ServiceMap

Edit - I was wrong here, it’s a setup that needed to be adjusted instead

Re. #50, also contrib module testing (that has PhpStan enabled) fails now:

Host commands: cd modules/contrib/image_effects && sudo -u www-data /var/www/html/vendor/bin/phpstan analyse --error-format checkstyle -c /var/lib/drupalci/workdir/phpstan/phpstan.neon . > /var/lib/drupalci/workdir/phpstan/phpstan_results.xml
Return code: 1
Output: 
StdErr: PHP Warning:  Undefined array key "class" in /var/www/html/vendor/mglaman/phpstan-drupal/src/Drupal/ServiceMap.php on line 41
Warning: Undefined array key "class" in /var/www/html/vendor/mglaman/phpstan-drupal/src/Drupal/ServiceMap.php on line 41
Configuration parameters excludes_analyse and excludePaths cannot be used at the same time.

see https://dispatcher.drupalci.org/job/drupal_contrib/506195/artifact/jenki... for instance, the xml results file is 0-size

@mondrake: can you submit an infrastructure issue for this (assuming this is about the drupalci setting to run phpstan)? Thanks!

#52 not sure, I'm pretty new here, right now I think it's an issue with mglaman/phpstan-drupal instead, but need to understand better before raising upstream

Trying to create a baseline with ../vendor/bin/phpstan analyze --generate-baseline fails with

Error: ] An internal error occurred. Baseline could not be generated. Re-run    
         PHPStan without --generate-baseline to see what's going on.            

So we probably need to address at least some of the errors right at the beginning before we even create a baseline.

Re #51, I submitted #3248706: PHPStan fails for contrib due to outdated neon configuration that should only affect deprecation checking for contrib projects with drupalci, which is a different use case for phpstan, but I proposed a fix there.

So re. #51 I also filed https://github.com/mglaman/phpstan-drupal/issues/222 upstream.

I think a review now would be nice.

• we need a new release of mglaman/phpstan-drupal including https://github.com/mglaman/phpstan-drupal/pull/223 to solve an issue with autowiring
• I cannot figure out what is triggering the PHPUnit classes rewrite for BC -- the rewritten classes get in the way of commit-code-checks

I just released https://github.com/mglaman/phpstan-drupal/releases/tag/1.0.1 which fixes autowiring AND includes code for class resolving.

For example:

/**
 * Implements hook_entity_type_alter().
 */
function workspaces_entity_type_alter(array &$entity_types) {
  \Drupal::service('class_resolver')
    ->getInstanceFromDefinition(EntityTypeInfo::class)
    ->entityTypeAlter($entity_types);
}

It understands the class returned from getInstanceFromDefinition

Thanks for reviews @longwave and @alexpott.

1. mglaman/phpstan-drupal updated to 1.0.1 (thanks @mglaman for quick turnaround!) and the missing 'class' error is gone
2. still do not understand why the rewritten PHPUnit classes get in the way of commit-code-checks
3. I do not know how to solve the autoload errors for missing composer/composer, symfony/contracts, psr/log classes (the latter, apparently, intermittent)
4. several pre-level-0 errors are in deprecated code, that will go away in D10. Is this issue still a D9 thing, or when D10 opens we can target PHPStan on D10 only?

I think adding this to 10.x only when it opens might make this easier. So +1 to that idea.

Changed target release per #61.

The phpstan.neon.dist is wrong, in my opinion. Why isn't it including the proper extension.neon?

includes:
	- phpstan-baseline.neon
        - ../vendor/mglaman/phpstan-drupal/extension.neon
  paths:
    - .
    - ../composer

  excludePaths:
    - '*.api.php'
    - 'lib/Drupal/Component/Diff/Engine/*.php'
    - 'modules/migrate_drupal/tests/fixtures/*.php'
    - modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/fruit/ExtendingNonInstalledClass.php
    - tests/Drupal/Tests/Listeners/HtmlOutputPrinter.php
    - '../sites/simpletest/*.php'

It'd be even simpler if phpstan/extension-installer was used (no need to include extension.neon)

This also doesn't include phpstan deprecation rules to catch usages of deprecated code. Maybe that's a follow up?

This also doesn't include phpstan deprecation rules to catch usages of deprecated code. Maybe that's a follow up?

IMHO it should be done here, given we're adding the tools in this issue.

The phpstan.neon.dist is wrong, in my opinion. Why isn't it including the proper extension.neon?

Dunno, can you help? Feel free to push on the fork, BTW

There’s not a phpstan-drupal-deprecations release 1, though, is that right? Latest release is 0.11.1 on packagist.

Yeah, I can help. I just didn't have time today to push, so I wanted to leave a comment. I'll help poke at this tomorrow!

https://packagist.org/packages/phpstan/phpstan-deprecation-rules#1.0.0 there is a 1.0.0. All support (core) extensions had compatible releases.

Another setting to consider: max number of processes. PHPStan supports multiple threads by default and maximizes based on the number of CPUs. It seems like it might be a good idea to set a limit on that.

Documentation: https://phpstan.org/config-reference#parallel-processing

Scheduling logic: https://github.com/phpstan/phpstan-src/blob/a97bdead71d24694dfe2bbe96de8...

CPU count detection logic: https://github.com/phpstan/phpstan-src/blob/6172a47571c790b429d7f341313e...

This one will be hard to debug:

16:57:05  ------ -------------------------------------------------------------------- 
16:57:05   Line   core/modules/views/tests/src/Unit/ViewsTest.php                     
16:57:05  ------ -------------------------------------------------------------------- 
16:57:05          Class Symfony\Contracts\Service\ResetInterface not found.           
16:57:05          💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
16:57:05   11     Class Symfony\Contracts\Service\ResetInterface not found.           
16:57:05          💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
16:57:05  ------ -------------------------------------------------------------------- 

It does not occur for me locally with a clone of the issue fork and composer install:

php vendor/bin/phpstan.phar analyze --configuration ./core/phpstan.neon.dist core/modules/views/tests/src

Out of the latest run https://dispatcher.drupalci.org/job/drupal_patches/103478/console, I think we're now narrowed to a list of errors that must be solved prior to being able to run PHPStan even on level-0.

In particular, we have:

1. some errors that are internal to PHPStan processing. These errors prevent even creating a baseline. ATM there are three of them:

1. Internal error: Internal error: assert($type instanceof ObjectType) in file /var/www/html/core/tests/Drupal/Tests/Core/Form/FormStateTest.php Run PHPStan with --debug option and post the stack trace to: https://github.com/phpstan/phpstan/issues/new?template=Bug_report.md
2. Internal error: Internal error: Cannot assign an empty string to a string offset in file /var/www/html/core/modules/link/tests/src/Kernel/LinkItemTest.php Run PHPStan with --debug option and post the stack trace to: https://github.com/phpstan/phpstan/issues/new?template=Bug_report.md
3. Internal error: Internal error: assert($type instanceof ObjectType) in file /var/www/html/core/lib/Drupal/Core/Extension/ModuleHandler.php Run PHPStan with --debug option and post the stack trace to: https://github.com/phpstan/phpstan/issues/new?template=Bug_report.md

2. Some errors that can not be added to the baseline file, and therefore prevent a clean (=error code 0) run of PHPStan, like for instance:

• Class Symfony\Contracts\Service\ResetInterface not found.
• Reflection error: Could not locate constant PDO::FETCH_DEFAULT while trying to evaluate constant expression in Drupal\Core\Database\Statement at line 184

Also, we need to fix #3248859: AccessAwareRouter fails PHPStan-0 before.

Replicated error for

Internal error: Internal error: assert($type instanceof ObjectType) in file /var/www/html/core/tests/Drupal/Tests/Core/Form/FormStateTest.php

/Users/mglaman/Drupal/sites/drupal-core-dev/core/tests/Drupal/Tests/Core/Form/FormStateTest.php
Uncaught AssertionError: assert($type instanceof ObjectType) in /Users/mglaman/Drupal/sites/drupal-core-dev/vendor/mglaman/phpstan-drupal/src/Rules/Drupal/LoadIncludes.php:57
#0 /Users/mglaman/Drupal/sites/drupal-core-dev/vendor/mglaman/phpstan-drupal/src/Rules/Drupal/LoadIncludes.php(57): assert(false, 'assert($type in...')

It's due to phpstan-drupal code in handling loadIncludes. Opened an issue: https://github.com/mglaman/phpstan-drupal/issues/232

I just released phpstan-drupal:1.0.3 and pushed the update here, which fixes the two assert($type instanceof ObjectType) errors.

I've created a new branch to check this against D10. Also phpstan and phpstan-drupal are updated, so some things have changed.
The good news is i've managed to get a baseline out of phpstan. With some fixes. In this i found 403 errors.

Running locally i get following errors, which should be fixed first so a baseline could be generated.

Error message "Method Drupal\Core\Routing\AccessAwareRouter::getRouteCollection() should return Symfony\Component\Routing\RouteCollection but return statement is missing." cannot be ignored, use excludePaths  
     instead.                                                                                                                                                                                                         
     Error message "Method Drupal\Core\Routing\AccessAwareRouter::generate() should return string but return statement is missing." cannot be ignored, use excludePaths instead.   
             

#3248859: AccessAwareRouter fails PHPStan-0

     Error message "Method Drupal\FunctionalTests\Bootstrap\ErrorContainer::get() should return object|null but return statement is missing." cannot be ignored, use excludePaths instead.

#3254962: Return type in ErrorContainer

     Error message "Method Drupal\Tests\Component\ProxyBuilder\TestServiceComplexMethod::complexMethod() should return array but return statement is missing." cannot be ignored, use excludePaths instead.           
     Error message "Method Drupal\Tests\Component\ProxyBuilder\TestServiceNullableTypehintSelf::typehintSelf() should return Drupal\Tests\Component\ProxyBuilder\TestServiceNullableTypehintSelf|null but return      
     statement is missing." cannot be ignored, use excludePaths instead.    

     Error message "Method Drupal\Core\File\MimeType\MimeTypeGuesser::guessMimeType() should return string|null but return statement is missing." cannot be ignored, use excludePaths instead.                                                                                                                                                           
     Error message "Method Drupal\Tests\Core\DependencyInjection\Compiler\NewMimeTypeGuesser::guessMimeType() should return string but return statement is missing." cannot be ignored, use excludePaths instead.     
     Error message "Method Drupal\Tests\Core\DependencyInjection\Compiler\NewMimeTypeGuesser::isGuesserSupported() should return bool but return statement is missing." cannot be ignored, use excludePaths instead.  

I included some workarounds for those (which are probably wrong) to have PHPStan working. Issues are created for those.

     Child process error (exit code 255): PHP Fatal error:  Declaration of Drupal\Core\Database\Statement::fetchAll($mode = null, $column_index = null, $constructor_arguments = null) must be compatible with        
     PDOStatement::fetchAll(int $mode = PDO::FETCH_DEFAULT, mixed ...$args) in /app/core/lib/Drupal/Core/Database/Statement.php on line 184                                                                           
     Fatal error: Declaration of Drupal\Core\Database\Statement::fetchAll($mode = null, $column_index = null, $constructor_arguments = null) must be compatible with PDOStatement::fetchAll(int $mode =               
     PDO::FETCH_DEFAULT, mixed ...$args) in /app/core/lib/Drupal/Core/Database/Statement.php on line 184                                                                                                              
                                                                                                                                                                                                                      
     Child process error (exit code 255): PHP Warning:  Undefined property: PhpParser\Node\Stmt\Class_::$namespacedName in /app/vendor/mglaman/phpstan-drupal/src/Rules/Classes/PluginManagerInspectionRule.php on    
     line 31                                                                                                                                                                                                          
     Warning: Undefined property: PhpParser\Node\Stmt\Class_::$namespacedName in /app/vendor/mglaman/phpstan-drupal/src/Rules/Classes/PluginManagerInspectionRule.php on line 31                                      
     PHP Fatal error:  Declaration of Drupal\Core\Database\Statement::fetchAll($mode = null, $column_index = null, $constructor_arguments = null) must be compatible with PDOStatement::fetchAll(int $mode =          
     PDO::FETCH_DEFAULT, mixed ...$args) in /app/core/lib/Drupal/Core/Database/Statement.php on line 184                                                                                                              
     Fatal error: Declaration of Drupal\Core\Database\Statement::fetchAll($mode = null, $column_index = null, $constructor_arguments = null) must be compatible with PDOStatement::fetchAll(int $mode =               
     PDO::FETCH_DEFAULT, mixed ...$args) in /app/core/lib/Drupal/Core/Database/Statement.php on line 184                                                                                                              
                                                                                                                                                                                                                      
     Child process error (exit code 255): PHP Warning:  Undefined array key 0 in /app/vendor/mglaman/phpstan-drupal/src/Rules/Drupal/ModuleLoadInclude.php on line 40                                                 
     Warning: Undefined array key 0 in /app/vendor/mglaman/phpstan-drupal/src/Rules/Drupal/ModuleLoadInclude.php on line 40                                                                                           
     PHP Warning:  Undefined array key 0 in /app/vendor/mglaman/phpstan-drupal/src/Rules/Drupal/LoadIncludes.php on line 50                                                                                           
     Warning: Undefined array key 0 in /app/vendor/mglaman/phpstan-drupal/src/Rules/Drupal/LoadIncludes.php on line 50                                                                                                
     PHP Fatal error:  Declaration of Drupal\Core\Database\Statement::fetchAll($mode = null, $column_index = null, $constructor_arguments = null) must be compatible with PDOStatement::fetchAll(int $mode =          
     PDO::FETCH_DEFAULT, mixed ...$args) in /app/core/lib/Drupal/Core/Database/Statement.php on line 184                                                                                                              
     Fatal error: Declaration of Drupal\Core\Database\Statement::fetchAll($mode = null, $column_index = null, $constructor_arguments = null) must be compatible with PDOStatement::fetchAll(int $mode =               
     PDO::FETCH_DEFAULT, mixed ...$args) in /app/core/lib/Drupal/Core/Database/Statement.php on line 184                                                                                                              

#3254965: Remove deprecated Statement class Duplicate of below
#3210310: Adjust Database API to remove deprecated Drupal 9 code in Drupal 10

The above has issues with the database statement class, which is going to be removed anyhow. So i copied over the previous adjustments but this class is scheduled for removal.

Then there is one issue pointing to LinkItemTest

     Internal error: Internal error: Cannot assign an empty string to a string offset in file /app/core/modules/link/tests/src/Kernel/LinkItemTest.php                                                                
     Run PHPStan with --debug option and post the stack trace to:                                                                                                                                                     
     https://github.com/phpstan/phpstan/issues/new?template=Bug_report.md     

#3254966: PHPStan error LinkItemTest
This is one i don't fully understand, commented out an assert to have it pass, but this is probably not the best appraoch off course.

This issue should then possible be postponed on the above ones, and then the baseline should be inspected to see what we best would fix before committing.

Still playing around with the MR. What i think still needs to change is the usage in drupalci and the commit-code script.

PHPStan recommends to run it on the entire codebase (with a baseline) and never on seperate files. Therefore the usage in the commit-code script looks incorrect to me.
I think we should run it on the entire codebase on the end of the commit code script (which runs also in drupalci).
Also noticing here that i think the baseline is not taken into account, otherwise phstan should return with no errors.

Too many MR branches, now it's hard to follow. Going back to good old patches.

Patch #79 after running COMPOSER_ROOT_VERSION=10.0.x-dev composer update --lock

We should update phpstan/phpstan to 1.2.0 and mglaman/phpstan-drupal to 1.1.3

We should update phpstan/phpstan to 1.2.0 and mglaman/phpstan-drupal to 1.1.3

Attached patch should do just that, and bring composer plugin-api-version back to the original 2.1.0

Try fixing the ErrorContainer issue, that produces errors on PHPStan run

Ignore extending @internal classes.

Ignore extending @internal classes.

Can you summarize where this happened? When we added this rule, I didn't think it would effect core. And it shouldn't trigger in shared namespaces (core modules extending core internals shouldn't error either, I suppose)

#88 sure, see the testrun results of #85, https://dispatcher.drupalci.org/job/drupal_patches/107486/consoleFull

Ignore not found classes, and fixes for Statement / StatementEmpty (due to be removed from D19 though).

Not found classes cannot be ignored - this becomes a stopper. Added more #[\ReturnTypeWillChange] annotations.

Patch looks good.

1. +++ b/core/phpstan.neon.dist
   @@ -0,0 +1,36 @@
   +	- phpstan-baseline.neon
   ...
   +		# @todo files below need to be excluded as they prevent baseline generation.
   +		# Fixing them is a priority.
   

   These lines have tabs.

2. The file core/phpstan-baseline.neon is full of tabs.

3. +++ b/core/drupalci.yml
   @@ -6,12 +6,19 @@
   +          - "cd ${SOURCE_DIR}/core && sudo -u www-data ../vendor/bin/phpstan analyze"
   

   Do we really need the part: "sudo -u www-data"?

4. +++ b/core/drupalci.yml
   @@ -6,12 +6,19 @@
   -        halt-on-fail: true
   +        halt-on-fail: false
   

   This change is out of scope for this issue.

5. +++ b/core/drupalci.yml
   @@ -6,12 +6,19 @@
   +      # Core's code quality is checked by container_command.commit_checks.
   +      # For testing only, run PHPStan across the entire codebase.
   

   So this part does not need to be committed?

6. +++ b/core/lib/Drupal/Core/Database/Statement.php
   @@ -21,7 +21,7 @@
   -class Statement extends \PDOStatement implements StatementInterface {
   +class Statement extends \PDOStatement {
   

   Why are we removing the StatementInterface?

7. +++ b/core/lib/Drupal/Core/Database/Statement.php
   @@ -161,7 +163,7 @@ public function rowCount() {
   -  public function setFetchMode($mode, $a1 = NULL, $a2 = []) {
   +  public function setFetchMode(int $mode, mixed ...$args) {
   
   @@ -170,18 +172,19 @@ public function setFetchMode($mode, $a1 = NULL, $a2 = []) {
   -  public function fetchAll($mode = NULL, $column_index = NULL, $constructor_arguments = NULL) {
   ...
   +  public function fetchAll(int $mode = \PDO::FETCH_DEFAULT, mixed ...$args) {
   

   I think that these changes will cause a BC break. Do we really need them? Can we ask @Beakerboy what it will do to do SQL Server database driver?

8. +++ b/core/phpstan-baseline.neon
   @@ -0,0 +1,1216 @@
   +			message: "#^Drupal\\\\Core\\\\Http\\\\LinkRelationTypeManager must override __construct if using YAML plugins\\.$#"
   +			count: 1
   +			path: lib/Drupal/Core/Http/LinkRelationTypeManager.php
   

   Can you explain to me why we have this exception here? The class is overriding the method __construct().

9. +++ b/core/phpstan-baseline.neon
   @@ -0,0 +1,1216 @@
   +			message: "#^Inner named functions are not supported by PHPStan\\. Consider refactoring to an anonymous function, class method, or a top\\-level\\-defined function\\. See issue \\#165 \\(https\\://github\\.com/phpstan/phpstan/issues/165\\) for more details\\.$#"
   +			count: 2
   +			path: modules/ckeditor5/ckeditor5.module
   

   Can we create a followup issue to fix this problem in PHPStan?

10. +++ b/core/phpstan-baseline.neon
    @@ -0,0 +1,1216 @@
    +			message: "#^Call to sprintf contains 1 placeholder, 0 values given\\.$#"
    +			count: 1
    +			path: modules/ckeditor5/src/Controller/CKEditor5ImageController.php
    

    Could we create a followup issue to fix this problem in PHPStan?

11. +++ b/core/phpstan-baseline.neon
    @@ -0,0 +1,1216 @@
    +			message: "#^Access to an undefined static property static\\(Drupal\\\\Tests\\\\dblog\\\\Functional\\\\DbLogResourceTest\\)\\:\\:\\$entityTypeId\\.$#"
    +			count: 1
    +			path: modules/dblog/tests/src/Functional/DbLogResourceTest.php
    

    This need to be fixed in core/modules/rest/tests/src/Functional/CookieResourceTestTrait.php.

12. +++ b/core/phpstan-baseline.neon
    @@ -0,0 +1,1216 @@
    +			message: "#^Call to an undefined static method static\\(Drupal\\\\Tests\\\\migrate\\\\Kernel\\\\MigrateTestBase\\)\\:\\:migrateDumpAlter\\(\\)\\.$#"
    +			count: 1
    +			path: modules/migrate/tests/src/Kernel/MigrateTestBase.php
    

    This should to be fixed. Can we add the static variable migrateDumpAlter?

13. +++ b/core/phpstan-baseline.neon
    @@ -0,0 +1,1216 @@
    +			message: "#^Class Drupal\\\\block\\\\Entity\\\\Block referenced with incorrect case\\: Drupal\\\\block\\\\entity\\\\Block\\.$#"
    +			count: 2
    +			path: modules/settings_tray/settings_tray.module
    

    Can we fix this one. It does not sound very difficult to fix.

14. +++ b/core/phpstan-baseline.neon
    @@ -0,0 +1,1216 @@
    +			message: "#^Class Drupal\\\\advisory_feed_test\\\\AdvisoriesTestHttpClient extends @final class GuzzleHttp\\\\Client\\.$#"
    +			count: 1
    +			path: modules/system/tests/modules/advisory_feed_test/src/AdvisoriesTestHttpClient.php
    

    Can we create a followup issue to fix this?

15. +++ b/core/phpstan-baseline.neon
    @@ -0,0 +1,1216 @@
    +			message: "#^Result of function usleep \\(void\\) is used\\.$#"
    +			count: 1
    +			path: modules/system/tests/modules/hold_test/src/EventSubscriber/HoldTestSubscriber.php
    

    Can we fix this or create a followup issue for it.

16. +++ b/core/phpstan-baseline.neon
    @@ -0,0 +1,1216 @@
    +			message: "#^Instantiated class Drupal\\\\Tests\\\\system\\\\Functional\\\\FileTransfer\\\\Exception not found\\.$#"
    +			count: 1
    +			path: modules/system/tests/src/Functional/FileTransfer/FileTransferTest.php
    

    Can we fix this or create a followup to fix it. This should not be that difficult to fix.

17. +++ b/core/phpstan-baseline.neon
    @@ -0,0 +1,1216 @@
    +			message: "#^Undefined variable\\: \\$arg$#"
    +			count: 1
    +			path: modules/views/src/Plugin/views/argument_validator/None.php
    

    Can we fix this. Should not be that difficult.

18. +++ b/core/phpstan-baseline.neon
    @@ -0,0 +1,1216 @@
    +			message: "#^Undefined variable\\: \\$def$#"
    +			count: 2
    +			path: modules/views/src/Plugin/views/relationship/EntityReverse.php
    

    Can we fix this. Should be not that difficult.

19. +++ b/core/phpstan-baseline.neon
    @@ -0,0 +1,1216 @@
    +			message: "#^Parameter \\$template of method Drupal\\\\KernelTests\\\\KernelTestBase\\:\\:prepareTemplate\\(\\) has invalid type Text_Template\\.$#"
    +			count: 1
    +			path: tests/Drupal/KernelTests/KernelTestBase.php
    

    Can we fix this. Should not be that difficult.

20. +++ b/core/phpstan-baseline.neon
    @@ -0,0 +1,1216 @@
    +			message: "#^Result of static method Drupal\\\\Composer\\\\Composer\\:\\:ensureComposerVersion\\(\\) \\(void\\) is used\\.$#"
    +			count: 1
    +			path: tests/Drupal/Tests/Composer/ComposerTest.php
    

    Can we fix this. Should not be that difficult.

21. +++ b/core/phpstan-baseline.neon
    @@ -0,0 +1,1216 @@
    +			message: "#^Call to an undefined static method PHPUnit\\\\Util\\\\ErrorHandler\\:\\:handleError\\(\\)\\.$#"
    +			count: 1
    +			path: tests/Drupal/Tests/Listeners/DrupalListener.php
    

    Can we fix this or create a followup to fix this.

22. +++ b/core/phpstan.neon.dist
    @@ -0,0 +1,36 @@
    +		# @todo files below need to be excluded as they prevent baseline generation.
    +		# Fixing them is a priority.
    +    - modules/link/tests/src/Kernel/LinkItemTest.php
    

    Why is this file causing a problem for the baseline generation? What do we need to do to fix it? THere are also tabs on the comment lines.

23. +++ b/core/phpstan.neon.dist
    @@ -0,0 +1,36 @@
    +    # Ignore common errors for now.
    +    - "#^Access to an undefined property#"
    +    - "#^Call to an undefined method#"
    +    - "#^Cannot unset offset#"
    +    - "#should return .* but return statement is missing#"
    +    - "#Drupal calls should be avoided in classes, use dependency injection instead#"
    +    - "#^Plugin definitions cannot be altered.#"
    +    - "#^Missing cache backend declaration for performance.#"
    +    - "#cache tag might be unclear and does not contain the cache key in it.#"
    +    - "#^Class .* extends @internal class#"
    +    - "#^File .* could not be loaded from module_load_include.#"
    

    Which of the following common error need a followup so that they can be removed?

24. In the file core/scripts/dev/commit-code-check.sh I am missing the part where the all files are checked when the files core/phpstan.neon.dist and core/phpstan-baseline.neon are changed. Just like we do with core/phpcs.xml.dist.

Thanks for review @daffie, sorry I forgot to change status to NW, this patch is still far from being committable.

The idea is

• commit a patch with ignores and baseline that can enable running PHPStan level 0 and prevent further level-0 issues to enter the codebase
• keep to a minimum other code changes
• once committed, open followups to clean baseline and ignores to level up the existing codebase to level 0
• once above done, repeat the same for level 1 and so forth

In order to do so, we need to have a zero-error run of PHPStan. Unfortunately (see last test run log https://dispatcher.drupalci.org/job/drupal_patches/107500/consoleFull), this is not the case:

• we still have 29 errors like
  

  20:00:11  ------ --------------------------------------------------------------------- 
  20:00:11   Line   core/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php           
  20:00:11  ------ --------------------------------------------------------------------- 
  20:00:11          Class Symfony\Contracts\Service\ResetInterface not found.            
  20:00:11          💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  20:00:11  ------ --------------------------------------------------------------------- 
  

  that cannot be ignored nor added to the baseline, so we need to solve upfront

• +++ b/core/phpstan.neon.dist
  @@ -0,0 +1,36 @@
  +		# @todo files below need to be excluded as they prevent baseline generation.
  +		# Fixing them is a priority.
  +    - modules/link/tests/src/Kernel/LinkItemTest.php
  +
  

  should be better fixed before commit, it's currently being looked at https://github.com/mglaman/phpstan-drupal/pull/279, once solved we will need updating to a new version of both phpstan/phpstan and mglaman/phpstan-drupal

Forgot to mention... nothing prevents to start tackling any of the baseline or the ignored issues right now. We do not have to wait for this issue to land. If anybody opens an issue, please tagit 'PHPStan-0' so we can have an overview of what's going on.

Can you explain to me why we have this exception here? The class is overriding the method __construct().

I think there's a bug in the PHPStan Drupal rule, an issue would need to be investigated in https://github.com/mglaman/phpstan-drupal/issues

Can we create a followup issue to fix this problem in PHPStan?

These all refer to Drupal issues, right?

Why is this file causing a problem for the baseline generation? What do we need to do to fix it? THere are also tabs on the comment lines.

See https://github.com/mglaman/phpstan-drupal/issues/228, https://www.drupal.org/project/drupal/issues/3254966, https://github.com/phpstan/phpstan/issues/6231

---

This issue should generate the baseline and fixes made afterwards, in my opinion. Unless the crash is critical.

Updated composer.lock with COMPOSER_ROOT_VERSION=10.0.x-dev composer update --lock

Most of my points from comment #94 still stand.

+++ b/core/scripts/dev/commit-code-check.sh
@@ -283,6 +284,19 @@
+  ############################################################################
+  ### PHPSTAN
+  ############################################################################
+  if [[ -f "$TOP_LEVEL/$FILE" ]] && [[ $FILE =~ \.(inc|install|module|php|profile|test|theme)$ ]]; then
+    vendor/bin/phpstan analyze --no-progress --configuration="$TOP_LEVEL/core/phpstan.neon.dist" "$TOP_LEVEL/$FILE"
+    if [ "$?" -ne "0" ]; then
+      # If there are failures set the status to a number other than 0.
+      STATUS=1
+    else
+      printf "PHPStan: $FILE ${green}passed${reset}\n"
+    fi
+  fi

If doing one file at a time takes a long time because of bootstrapping etc... then we should do what we do for cspell and pass all the files in in one go. Plus we should also configure this to do a full scan when the phpstan config files have changed.

This is looking great.

So now we need to implement a full scan if core/phpstan.neon.dist or core/phpstan-baseline.neon changes.

+++ b/core/phpcs.xml.dist
@@ -24,6 +24,9 @@
+  <!--Exclude rewritten PHPUnit classes.-->
+  <exclude-pattern>./sites/simpletest/*</exclude-pattern>
+
   <!-- Only include specific sniffs that pass. This ensures that, if new sniffs are added, HEAD does not fail.-->

I don't think this needs to be here.

Yes, the problem here is that a full scan will not currently pass (see IS point 1), and I have no idea how to fix that - locally it seems OK.

Done #106, now we fail on full scan as per #107. I found in #108 one reason for two remaining issues, for the rest I'm out of ideas ATM.

@mondrake if I run

vendor/bin/phpstan analyze  --configuration=core/phpstan.neon.dist

locally after applying this patch I get no errors.

Maybe the issue summary needs an update - I think you've solved quite a few of the problems listed.

#111 same here... so it is a DrupalCI problem. Concurrency?

Trying to restrict max number of concurrent processes, see #67

Better. Trying to disable concurrency altogether.

Gosh... different levels of parallelism give different results...

Revert phpcs.xml.dist changes

Let's try to debug

I'm copying something I shared in Slack with @alexpott.

We should try setting the cache folder to a relative directory:

parameters:
	tmpDir: tmp

Otherwise the temp folder is generated with sys_get_temp_dir(). My concern was this conflicting with other workspaces in Jekins. But @alexpott suggested

What it might be is the temp dir might be a tmpfs dir and maybe we exhaust it and odd things happen - so changing tmp might be a good idea - can we do that on the command line - on the phpstan invocation? I'm afk atm so can't see easily for myself…

We should try setting the tmpDir param and see if it makes things more reliable.

We need to create a temp directory before running PHPStan then? Named like what and where?

Let's borrow some things from https://git.drupalcode.org/project/infrastructure/-/commit/4379df685f205...

hi @alexpott, from my run #118 with debug on the problematic php file wasn't the problem, but it ran out of memory.

--debug mode does two things:

1. Disables caching
2. Disables concurrency

So, I guess one of the above causes the issue (or their combination + DrupalCI setup)

BTW, running it locally with PHP8.0 cannot fit within 3G, it consumes in the end ~7.0G

@Taran2L yep for a single process you need 7G of memory :)

#121 changes the tmp dir used by setting PHP vars - I tested this locally and it works. We definitely should not be setting a temp dir in the phpstan config file - that'll mess up using the script locally. Anyhow #121 seems to prove that the problem is not due to the temp dir because it does not change the results.

@Taran2L please post interdiff so people can see what you are changing / trying out - not doing that makes people have to work harder.

Here's the missing interdiff... and yeah as pointed out in #125 the temp directory is not the problem. #121 used a custom temp dir for drupalci and it makes no difference.

@alexpott,

- memory consumption: this is quite weird, running multiple processes consumes only ~600M on 8.0 and ~200 on 8.1, but single thread is killing it with ~7G - probably needs to addressed by phpstan

- interdiff - sorry, will do in the future

So, cache directory seems like not an issue, as it fails with an empty cache as well. See #123 and #124

Well, let's maybe try a different PHP version ...

Here's another thought... it's apcu and we're running into evictions that are affecting the composer classmap that's stored there. Let's disable it. To me this explains the different behaviour with different numbers of processes and different PHP versions.

Fingers-crossed.

#697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service introduced two new issues to add to baseline...

 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   modules/system/tests/modules/entity_test/entity_test.install
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------
  59     File core/modules/system/tests/modules/entity_test/entity_test.inc could not be loaded from Drupal\Core\Extension\ModuleHandlerInterface::loadInclude
  61     File core/modules/system/tests/modules/entity_test/entity_test.inc could not be loaded from Drupal\Core\Extension\ModuleHandlerInterface::loadInclude
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------

@alexpott, great find! seems like it's green already, but patch actually comments out a few things, so those have to be brought back

The other thing is that phpstan is fixed to 1.2.0, while 1.3.1 is available; the same with mgalaman/phpstan-drupal

I think we should update before committing. Thoughts?

Sure - this updates all the dependencies added here and updates the baseline to reflect the changes in phpstan-drupal.

We shouldn't be deciding how many processes to run... let's let phpstan do that.

With #131 a run on all the files:

11:09:08 Running PHPStan on *all* files.
11:12:42 
11:12:42  [OK] No errors   

With #132

11:14:35 Running PHPStan on *all* files.
11:15:48 
11:15:48  [OK] No errors   

So that's v nice.

So APCu caching eh? Great find @alexpott, kudos++

Now the patch is 5 times bigger than before, due to increased baseline - is it possible to move some of it to patterns in the ignoreErrors section of phpstan.neon.dist instead?

@mondrake good point about the ignoreErrors... the private call thing in tests is bogus anyway.

+++ b/composer.json
@@ -34,7 +34,10 @@
+        "phpstan/phpstan": "^1.2",
+        "mglaman/phpstan-drupal": "^1.1.3",

IMHO we should commit with composer.json constraints in line with the version we are actually starting using

Sure why not. tbh the whole dev dependency pinning and locks is all interesting.

+++ b/core/phpstan-baseline.neon
@@ -0,0 +1,1752 @@
+		-
+			message: "#^\\#pre_render callback class 'static\\(Drupal\\\\Core\\\\Render\\\\Element\\\\Button\\)' at key '0' does not implement Drupal\\\\Core\\\\Security\\\\TrustedCallbackInterface\\.$#"
+			count: 1
+			path: lib/Drupal/Core/Render/Element/Button.php
+

These reports are not correct. Opened https://github.com/mglaman/phpstan-drupal/pull/292 to deal with this.

reportUnmatchedIgnoredErrors: false

Should that be in the phpstan config?
Having this removed always forces a clean baseline, which we should strive for?

I believe that reportUnmatchedIgnoredErrors was turned off when trying to do scans of only modified files for pre-commit. If pre-commit now does a full scan, we can remove that.

Also:

phpstan-drupal 1.1.6 fixes #138

phpstan 1.3.2 fixes the problem where renamed trait methods were considered private https://github.com/phpstan/phpstan/issues/6306

so we need some bumps

Updating things again.

+++ b/core/scripts/dev/commit-code-check.sh
@@ -126,6 +131,10 @@
+  if [[ $FILE == "core/phpstan-baseline.neon" || $FILE == "core/phpstan.neon.dist" ]]; then
+    PHPSTAN_DIST_FILE_CHANGED=1;
+  fi;

@@ -181,6 +190,28 @@
+if [[ $PHPSTAN_DIST_FILE_CHANGED == "1" ]]; then
+  printf "\nRunning PHPStan on *all* files.\n"
+  php -d apc.enabled=0 -d apc.enable_cli=0 vendor/bin/phpstan analyze --no-progress --configuration="$TOP_LEVEL/core/phpstan.neon.dist"
+else
+  printf "\nRunning PHPStan on changed files.\n"
+  php -d apc.enabled=0 -d apc.enable_cli=0 vendor/bin/phpstan analyze --no-progress --configuration="$TOP_LEVEL/core/phpstan.neon.dist" $ABS_FILES
+fi

This is going to cause problems. This is why we had to add reportUnmatchedIgnoredErrors: false

Everything not hit in the baseline will report as an error if only scanning those files. It's best to just scan all files on precommit. Unless someone can do a fake commit locally and verify.

@mglaman I think we can get the best of both worlds.

Now with commentary to explain what's going on.

Expanding on #143:

What about when editing a file that is extended by other files that are not included in the commit?

It would appear to me that there is a possibility by not scanning the full code base that a list of new PHPStan errors could be committed but not appear until either a baseline is updated or another committer goes to edit a specific file.

For example a couple scenarios I can think of:
Removing a declared property that is not utilized in a Base class however is utilized in an (unedited) extending class? (attempt to access an undefined property error)
Possibly changing the type signatures on method parameters or on its returns in docblocs having a ripple effect? (Parameter type mismatch errors)

@Taran2L it helps to summarize the changes. It looks like you went ahead and tried to remove some reported errors. I think the main goal here is not to fix things but only the minimum required to make PHPStan run, then doing the fixes once this merges.

RE #146 I agree. Pre-commit hooks are for the maintainers when officially committing an issue's patch/MR. Correct? If so, it should run on all the things.