Convert the PHPStan baseline from NEON to PHP

Is there perf impact on time of execution?

It needs CR and update docs about new way to generate

Using the following code to time analysis of bd4b8b98a7 (the commit in the MR before the change) and the change itself.

Full analysis with baseline in place and without generating a new baseline

vendor/bin/phpstan clear-result-cache --memory-limit=-1 && time vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist --memory-limit=-1

Full analysis without baseline in place and generating the baseline from scratch (neon/php used depending on commit tested).

# neon
vendor/bin/phpstan clear-result-cache --memory-limit=-1 && rm core/phpstan-baseline.neon && touch core/phpstan-baseline.neon && time vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist --memory-limit=-1 --generate-baseline=core/phpstan-baseline.neon

# php
vendor/bin/phpstan clear-result-cache --memory-limit=-1 && rm -f core/phpstan-baseline.php && echo "<?php return [];" > core/phpstan-baseline.php && time vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist --memory-limit=-1 --generate-baseline=core/phpstan-baseline.php

Three runs each (m:ss):

• (neon) bd4b8b98a7 full analysis without generating - 1:58/1:55/2:00
• (neon) bd4b8b98a7 full analysis generating baseline - 1:52/1:50/1:37
• (php) 3426548-convert-the-phpstan full analysis without generating - 1:41/1:41/1:33
• (php) 3426548-convert-the-phpstan full analysis generating baseline - 1:31/1:38/1:33

A draft CR has been created. I'm not sure which docs would need to be updated.

I mean official docs but can't find any except https://www.drupal.org/search/site/Phpstan%20%20baseline?f%5B0%5D=ss_met... →

This CR should be updated https://www.drupal.org/node/3258232 → with a link to new one probably

I've created the following documentation page which outlines how PHPStan is used in Drupal core: https://www.drupal.org/docs/develop/development-tools/phpstan/phpstan-in... → . I attempted to write it in a way so that it's not outdated when ✨ [PP-1] Bump PHPStan to level 9 and accept a large baseline Postponed lands.

I've added a reference to the new documentation in the CR for this issue: https://www.drupal.org/node/3426891 →

I've updated the previous CR that andypost linked to also reference the CR for this issue: https://www.drupal.org/node/3258232 → . The formatting may need some work, I'm not sure if there's a best practice for those kinds of references.

I think the change is minimal and brings 20-25% speed-up for the CI run!

as PHP 8.3 is the default now we can try use PHP-JIT for the job)

🔝

Given the performance improvement I am +1 for landing this, but a bit concerned about conflicting with other ongoing work. We know this will conflict with all other PHPStan issues and require trolling, but do we have a sense of how many other issues will be affected here?

I suppose what I'm asking is: is this safe enough to commit now or should we wait for the disruptive patches beta window? I think the level bump issue will almost certainly have to wait, as I imagine most ongoing MRs will have undiscovered level 2+ issues.

My 0.02 - I think it’s fine to commit now - the RTBC queue is at its lowest since long. Adjusting any pending MR is just a rebase + c/p of the baseline artifact, since we are not changing the rule level.

We need to prevent access to the php here from .htaccess and web.config.
We also should have MRs up for 10.3.x and 10.2.x so that we can land this all in 1 go.

Should we make it a dotfile, ie. core/.phpstan-baseline.php? AFAIK those are blocked from .htaccess/web.config already.

Nice idea with dot-file

Agreed with #11, we've got a lucky window of a shorter RTBC queue than we've had for months, so this will be less disruptive than it otherwise would be.

11.x branch updated. Also updated the documentation and CR to match.

Created 10.2.x and 10.3.x branches with MRs :)

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.

Rebased the three branches :D The diff in the new format for the changes on the 10.3.x branch was very readable 🤩

diff --git a/core/.phpstan-baseline.php b/core/.phpstan-baseline.php
index a42a313689..664664da34 100644
--- a/core/.phpstan-baseline.php
+++ b/core/.phpstan-baseline.php
@@ -49,7 +49,9 @@
 	'path' => __DIR__ . '/includes/theme.maintenance.inc',
 ];
 $ignoreErrors[] = [
-	'message' => '#^Call to an undefined static method Doctrine\\\\Common\\\\Annotations\\\\AnnotationRegistry\\:\\:registerLoader\\(\\)\\.$#',
+	'message' => '#^Call to deprecated method registerLoader\\(\\) of class Doctrine\\\\Common\\\\Annotations\\\\AnnotationRegistry\\:
+This method is deprecated and will be removed in
+            doctrine/annotations 2\\.0\\. Annotations will be autoloaded in 2\\.0\\.$#',
 	'count' => 1,
 	'path' => __DIR__ . '/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php',
 ];
@@ -1356,7 +1358,9 @@
 	'path' => __DIR__ . '/modules/migrate/src/MigrateException.php',
 ];
 $ignoreErrors[] = [
-	'message' => '#^Call to an undefined static method Doctrine\\\\Common\\\\Annotations\\\\AnnotationRegistry\\:\\:registerLoader\\(\\)\\.$#',
+	'message' => '#^Call to deprecated method registerLoader\\(\\) of class Doctrine\\\\Common\\\\Annotations\\\\AnnotationRegistry\\:
+This method is deprecated and will be removed in
+            doctrine/annotations 2\\.0\\. Annotations will be autoloaded in 2\\.0\\.$#',
 	'count' => 1,
 	'path' => __DIR__ . '/modules/migrate/src/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php',
 ];
@@ -2380,12 +2384,6 @@
 	'count' => 1,
 	'path' => __DIR__ . '/tests/Drupal/KernelTests/Core/Config/ConfigInstallTest.php',
 ];
-$ignoreErrors[] = [
-	'message' => '#^Call to deprecated method expectError\\(\\) of class PHPUnit\\\\Framework\\\\TestCase\\:
-https\\://github\\.com/sebastianbergmann/phpunit/issues/5062$#',
-	'count' => 2,
-	'path' => __DIR__ . '/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php',
-];
 $ignoreErrors[] = [
 	'message' => '#^Variable \\$expected_driver might not be defined\\.$#',
 	'count' => 2,
@@ -2443,12 +2441,6 @@
 	'count' => 1,
 	'path' => __DIR__ . '/tests/Drupal/KernelTests/Core/Image/ToolkitGdTest.php',
 ];
-$ignoreErrors[] = [
-	'message' => '#^Call to deprecated method expectErrorMessage\\(\\) of class PHPUnit\\\\Framework\\\\TestCase\\:
-https\\://github\\.com/sebastianbergmann/phpunit/issues/5062$#',
-	'count' => 1,
-	'path' => __DIR__ . '/tests/Drupal/KernelTests/Core/Render/Element/MachineNameTest.php',
-];
 $ignoreErrors[] = [
 	'message' => '#^Variable \\$value in isset\\(\\) always exists and is not nullable\\.$#',
 	'count' => 1,
@@ -2471,7 +2463,9 @@
 	'path' => __DIR__ . '/tests/Drupal/Tests/BrowserTestBase.php',
 ];
 $ignoreErrors[] = [
-	'message' => '#^Call to an undefined static method Doctrine\\\\Common\\\\Annotations\\\\AnnotationRegistry\\:\\:registerAutoloadNamespace\\(\\)\\.$#',
+	'message' => '#^Call to deprecated method registerAutoloadNamespace\\(\\) of class Doctrine\\\\Common\\\\Annotations\\\\AnnotationRegistry\\:
+This method is deprecated and will be removed in
+            doctrine/annotations 2\\.0\\. Annotations will be autoloaded in 2\\.0\\.$#',
 	'count' => 1,
 	'path' => __DIR__ . '/tests/Drupal/Tests/Component/Annotation/Doctrine/DocParserTest.php',
 ];

Since this file is php do IDEs like PhpStorm try to analyse it? It can obviously be excluded but does that involve manual steps?

Since this file is php do IDEs like PhpStorm try to analyse it? It can obviously be excluded but does that involve manual steps?

I can not answer for all IDEs but only for PHPStorm. PHPStorm does seem to index the file. As far as I found there's nothing we can do against this with things we ship in the repo. I also don't know if it's a problem (i.e. in what ways it affects PHPStorm usage).

There are settings in PHPStorm that allow excluding files which are global settings that apply to all projects. So in case the analysis of the file by PHPStorm causes problems for someone it's a one-time fix.

This looks good to me, I think all issues have been addressed and the performance improvement should make this worthwhile.

Marking RTBC and will ping committers as otherwise this is going to end up being rerolled over and over, given we are landing other PHPStan fixes at the moment.

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.

core/scripts/dev/commit-code-check.sh also needs to be updated.

Moving this back to RTBC as per #23.

#24 is a race condition with a mistake for 6996 which was in my push between #19 and #20 but fixed between #21 and #22. GitLab reports that 6996 is fully mergeable at the moment (contrary to the review bot)

#26 crossposted with #25. Moving back to needs review after change in core/scripts/dev/commit-code-check.sh

Review was addressed, back to RTBC.

We need to add

diff --git a/.gitattributes b/.gitattributes
index 76ea8feeb1..e7b792f840 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -42,6 +42,9 @@
 *.xml     text eol=lf whitespace=blank-at-eol,-blank-at-eof,-space-before-tab,tab-in-indent,tabwidth=2
 *.yml     text eol=lf whitespace=blank-at-eol,-blank-at-eof,-space-before-tab,tab-in-indent,tabwidth=2
 
+# PHPStan's baseline uses tabs instead of spaces.
+core/.phpstan-baseline.php text eol=lf whitespace=blank-at-eol,-blank-at-eof,-space-before-tab,tabwidth=2 diff=php linguist-language=php
+
 # Define binary file attributes.
 # - Do not treat them as text.
 # - Include binary diff in patches instead of "binary files differ."

To .gitattributes so that git doesn't complain about whitespace errors in the new baseline file. This is because PHPStan's baseline generator uses tabs whereas we tell git that that is a whitespace error for PHP. Going to fix this on commit. I checked with @longwave first.

Committed f9f0971 and pushed to 11.x. Thanks!
Committed 4517fd1 and pushed to 10.3.x. Thanks!
Committed a3b7bc2 and pushed to 10.2.x. Thanks!

I think we also need to think about #21 a bit more especially before we merge the monster.

Backported to 10.2.x in order to make supporting that branch and 10.3.x and 11.x simpler.

Getting a failing test: https://git.drupalcode.org/issue/drupal-3427388/-/jobs/1049195/raw

---- Drupal\Tests\ComposerIntegrationTest ----


Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
[31mFail      Other      phpunit-130.xml      0 Drupal\Tests\ComposerIntegrationTes
[0m    PHPUnit Test failed to complete; Error: PHPUnit 9.6.15 by Sebastian
    Bergmann and contributors.
    
    Testing Drupal\Tests\ComposerIntegrationTest
    ........S...........................F......................       59 / 59
    (100%)
    
    Time: 00:00.042, Memory: 6.00 MB
    
    There was 1 failure:
    
    1) Drupal\Tests\ComposerIntegrationTest::testExpectedScaffoldFiles with
    data set #1 ('.gitattributes', 'assets/scaffold/files/gitattributes',
    '[project-root]')
    Scaffold source and destination files must have the same contents.
    Failed asserting that two strings are equal.
    --- Expected
    +++ Actual
    @@ @@
     *.xml     text eol=lf
    whitespace=blank-at-eol,-blank-at-eof,-space-before-tab,tab-in-indent,tabwidth=2\n
     *.yml     text eol=lf
    whitespace=blank-at-eol,-blank-at-eof,-space-before-tab,tab-in-indent,tabwidth=2\n
     \n
    +# PHPStan's baseline uses tabs instead of spaces.\n
    +core/.phpstan-baseline.php text eol=lf
    whitespace=blank-at-eol,-blank-at-eof,-space-before-tab,tabwidth=2 diff=php
    linguist-language=php\n
    +\n
     # Define binary file attributes.\n
     # - Do not treat them as text.\n
     # - Include binary diff in patches instead of "binary files differ."\n
    
    /builds/issue/drupal-3427388/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
    /builds/issue/drupal-3427388/core/tests/Drupal/Tests/ComposerIntegrationTest.php:205
    /builds/issue/drupal-3427388/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    /builds/issue/drupal-3427388/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
    /builds/issue/drupal-3427388/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
    /builds/issue/drupal-3427388/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
    /builds/issue/drupal-3427388/vendor/phpunit/phpunit/src/TextUI/Command.php:144
    /builds/issue/drupal-3427388/vendor/phpunit/phpunit/src/TextUI/Command.php:97
    
    FAILURES!
    Tests: 59, Assertions: 424, Failures: 1, Skipped: 1.


---- Drupal\Tests\Composer\ComposerTest ----

Kingdutch → closed merge request !6967

Kingdutch → closed merge request !6996

Kingdutch → closed merge request !6995

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