Always do a full phpstan analysis on DrupalCI

I think i just saw a problem with only scanning a list of files.
From #2316941: Use the builder pattern to make it easier to create render arrays
There is following patch:
https://www.drupal.org/files/issues/2022-01-21/core-builder-pattern-2316...

This gives below result on phpstan.
https://www.drupal.org/pift-ci-job/2300923

If we take this change as an example:

diff --git a/core/lib/Drupal/Core/Layout/LayoutDefinition.php b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
index d80a8720ca..fa9b83bda0 100644
--- a/core/lib/Drupal/Core/Layout/LayoutDefinition.php
+++ b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
@@ -9,6 +9,7 @@
 use Drupal\Component\Plugin\Definition\PluginDefinition;
 use Drupal\Core\Plugin\Definition\DependentPluginDefinitionInterface;
 use Drupal\Core\Plugin\Definition\DependentPluginDefinitionTrait;
+use Drupal\Core\Render\Element\Image;
 
 /**
  * Provides an implementation of a layout definition and its metadata.
@@ -437,13 +438,12 @@ public function setIconMap($icon_map) {
   public function getIcon($width = 125, $height = 150, $stroke_width = NULL, $padding = NULL) {
     $icon = [];
     if ($icon_path = $this->getIconPath()) {
-      $icon = [
-        '#theme' => 'image',
-        '#uri' => $icon_path,
-        '#width' => $width,
-        '#height' => $height,
-        '#alt' => $this->getLabel(),
-      ];
+      $icon = Image::getBuilder()
+        ->setUri($icon_path)
+        ->setWidth($width)
+        ->setHeight($height)
+        ->setAlt($this->getLabel())
+        ->toRenderable();
     }
     elseif ($icon_map = $this->getIconMap()) {
       $icon_builder = $this->getIconBuilder()

This results in following phpstan error:

Running PHPStan on changed files.
 ------ --------------------------------------------------------------------- 
  Line   lib/Drupal/Core/Layout/LayoutDefinition.php                          
 ------ --------------------------------------------------------------------- 
  441    Call to static method getBuilder() on an unknown class               
         Drupal\Core\Render\Element\Image.                                    
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
 ------ --------------------------------------------------------------------- 

The reason the class is unknown is because it's added in this patch. But phpstan only scans the changed files, and does not scan the Image class, so it cannot see that this class exists.

To add, on the testbot this takes 1'13" (if we start running it now, without the possible cache improvements.)

15:04:43 Running PHPStan on *all* files.

15:05:56
15:05:56 [OK] No errors
15:05:56
15:05:56
15:05:56 PHPStan: passed

What might be still open is to see if we only need to run phpstan on gitlab CI. If this would cause locally a large delay when running on a local computer of all people running this script.

@mondrake when rechecking my initial analysis was indeed incorrect, although the problem still stands.

The patch does the following:

diff --git a/core/lib/Drupal/Core/Layout/LayoutDefinition.php b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
index d80a8720ca..fa9b83bda0 100644
--- a/core/lib/Drupal/Core/Layout/LayoutDefinition.php
+++ b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
@@ -9,6 +9,7 @@
 use Drupal\Component\Plugin\Definition\PluginDefinition;
 use Drupal\Core\Plugin\Definition\DependentPluginDefinitionInterface;
 use Drupal\Core\Plugin\Definition\DependentPluginDefinitionTrait;
+use Drupal\Core\Render\Element\Image;
 
 /**
  * Provides an implementation of a layout definition and its metadata.
@@ -437,13 +438,12 @@ public function setIconMap($icon_map) {
   public function getIcon($width = 125, $height = 150, $stroke_width = NULL, $padding = NULL) {
     $icon = [];
     if ($icon_path = $this->getIconPath()) {
-      $icon = [
-        '#theme' => 'image',
-        '#uri' => $icon_path,
-        '#width' => $width,
-        '#height' => $height,
-        '#alt' => $this->getLabel(),
-      ];
+      $icon = Image::getBuilder()
+        ->setUri($icon_path)
+        ->setWidth($width)
+        ->setHeight($height)
+        ->setAlt($this->getLabel())
+        ->toRenderable();

Add a reference to Image class and use a (new) method getBuilder of the Image class. But indeed PHPstan (with the limited files) is correct that the image Class does not have this method.
However this method is added further in the patch.

diff --git a/core/lib/Drupal/Core/Render/Element/RenderElement.php b/core/lib/Drupal/Core/Render/Element/RenderElement.php
index 35814589b3..8aa918f7f4 100644
--- a/core/lib/Drupal/Core/Render/Element/RenderElement.php
+++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
@@ -7,6 +7,7 @@
 use Drupal\Core\Plugin\PluginBase;
 use Drupal\Core\Render\BubbleableMetadata;
 use Drupal\Core\Render\Element;
+use Drupal\Core\Render\RenderableInterface;
 use Drupal\Core\Url;
 
 /**
@@ -121,7 +122,47 @@
  *
  * @ingroup theme_render
  */
-abstract class RenderElement extends PluginBase implements ElementInterface {
+abstract class RenderElement extends PluginBase implements ElementInterface, RenderableInterface {
+
+  protected $renderable = [];
+
+  public static function getBuilder() {
+    return new static([], uniqid(), []);
+  }

To the RenderElement class. But since Image is not part of the items PHPStan scans, phpstan does not know that actualle Image extends RenderElement and thus that the function can be found in the RenderElement class.
It's that combination of issues what causes this IMO.

@mondrake Ow damn! you're right. PHPStan is correct here. Those two classes (Image and TextElement) are not existing.
Sorry for all the noise here. (At least we got a patch ready if we would decide to do a full scan). But the #3 argument off course is invalid in doing so.

Wouldn't we solve the above by removing the phpstan partial file?

Mostly isn't this the guilty line here
reportUnmatchedIgnoredErrors: false

(I still agree on the full run, but above might be a good interim step?)

Forgot about that. Thx for explaining.

An option (although i think it's perhaps far fetched and not optimal) might be to do some baseline file scanning in the commit code script and see there if a changed file has an entry in the baseline file and flag it.
Say i changed file A, and file A is in the baseline, mention / flag it in the commit code script.

Although that would not be perfect. Say i have line 1 of file A in the baseline (line nrs are not in the baseline) and my patch touches lines 5-10 of that file, then i would need to fix the line 1 error in that file also while actually being not in scope for that change.

Did some tests on locally running a full phpstan, and i do confirm this can take some time.
(My laptop is even slower then what catch describes here).

drupal-HEAD {10.0.x} $ time vendor/bin/phpstan analyze  --configuration=core/phpstan.neon.dist
 9754/9754 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
                                                                                                                        
 [OK] No errors                                                                                                         
                                                                                                                        
real    6m43.615s
user    22m41.589s
sys     0m23.777s

drupal-HEAD {10.0.x} $ time vendor/bin/phpstan analyze  --configuration=core/phpstan.neon.dist
 9754/9754 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
                                                                                                                        
 [OK] No errors                                                                                                         
                                                                                                                        
real    0m6.246s
user    0m3.436s
sys     0m1.516s

A cached one is luckily enough much faster. And in most cases this only takes changed files into account.

There are also a lot of possibilities when PHPStan invalidates the cache, so it runs a full scan again.

https://phpstan.org/user-guide/result-cache

• Every 7 days
• Changes in:
• PHPStan version
• PHP version
• Loaded PHP extensions
• Rule level
• Configuration files hashes
• Analysed paths
• composer.lock files hashes
• Stub files hashes
• Bootstrap files hashes
• Autoload file hash
• Errors in the last run
• Dependency tree of project files. If file A.php was modified since the last run, A.php and all the files calling or otherwise referencing all the symbols in A.php are analysed again.

Note: also when running with --debug the cache is not used.

So committers will sometimes have a full non-cached phpstan run. Question is, if this is acceptable for core committers, or do we need to move the phpstan analysis out of the core-commit script, and only run on CI?

By default the cache is stored at %tmpDir%/resultCache.php (/tmp/phpstan on my machine) so that needs to be writable i think.

Rebased the branch, and adjusted to run a full scan on CI, and a partial one otherwise.

Need to test this, but at first sight @longwave that analysis looks correct.
However this will also fail the commit code script (when not run on ci) when a committer want's to commit that patch. In this case the only option for a committer would be to know to commit although the script fails, or run a full scan locally (which would require a committer to do manually, or hack the script). Not sure if this is really a problem.

Not related to phpstan running partial or full. (both give same result).

In our phpstan config we have

excludePaths:
# Skip test fixtures.
- */tests/fixtures/*.php

This skips the AssertHelperTestClass class.

@xjm not fully sure what you mean.
Phpstan here does a partial scan when not on DrupalCI (like it is now) and only does a full scan on DrupalCI.
Or would you like the partial phpstan scan also removed when not on DrupalCI.

Tried to put issue summary back up to date.
For the change record https://www.drupal.org/node/3258232
Nothing needs to change here, this is still correct. This issue only adjust the core-commit-code script.

Added some extra info the the previous change record.

Rebased. This will probably fail now until #3278782: PHPStan baseline is out of sync is committed.

Still hoping to get this in, since it would fix some phpstan-baseline chasing issues.

There seems to be some performance improvements in the latest mglaman/phpstan-drupal version. So if we can update that one prior we can see a full scan takes much less time.

There do seem to be some quite good performance improvement in this upgrade.

After the update a full scan takes 1:15

15:00:21 Running PHPStan on *all* files.
15:01:36  ------ ------------------------------------------------------------------ 

While in #3278916: Update phpstan/phpstan to latest version a full scan takes 2:27

14:33:11 Running PHPStan on *all* files.

14:35:37  ------ ----------------------------------------------------------------------- 

So if we can get #3279840: Update mglaman/phpstan-drupal (and perhaps also #3278916: Update phpstan/phpstan to latest version) this might be acceptable as for the longer DrupalCI runtime (and thus costs).

Now that the updates to phpstan and phpstan-drupal are in. I've rebased this, to let the bot run again.
From my side this is ready now. Giving in mind the performance improvements of phpstan-drupal, this might now be acceptable.

This was in place to then do a full scan on changes in both core/phpstan-baseline.neon and core/phpstan.neon.dist.

For the phpstan.neon.dist file, the configuration i think this makes sense. Do you also wan't this for 'each' baseline change? Since we committed a lot of items to the baseline (to stop introducing new ones), this might make it run locally more often than you would like?
If that's no issue, we might also do this for changes to the composer.lock files, and add that file to the list as well?

Adjusted as requested.

I can see the cautiousness.

As an intermediate step we might do #3259353: Run full phpstan on composer change first. When checking the issues where baseline problems were introduced. I saw following reasons.

* Removal of modules (which can be caught in the composer.lock change).
* Updates of phpstan and drupal-phpstan (which would also be caught)
* Some cases where issues ran against 9.x ci, but not to 10.x, that's something sitll to look out for.

Another option would be (but not sure how feasible) is to check if we can distinguish between a patch run on CI, and a nightly run, then we could do a full scan on the scheduled run. That might also help finding baseline issues earlier.