localgovdrupal / localgov_project

Project template for Drupal 10 sites built with the LocalGov Drupal distribution.
https://localgovdrupal.org
GNU General Public License v2.0
10 stars 8 forks source link

ci: fix phpstan #131

Closed millnut closed 10 months ago

millnut commented 11 months ago

While digging into PHP 8.2 issues that need fixing, I thought it was strange that PHPStan reported nothing, so it appears that it is missing the level parameter in the config which then causes it to not report PHP 8.2 related issues.

Access to an undefined property in PHPStan is basically the Dynamic properties deprecated in PHP 8.2

See: https://phpstan.org/blog/phpstan-is-ready-for-php-8-2

I've added this as level 1 to match Drupal core and now we can see PHP 8.2 issues picked up in the phpstan checks before unit tests and be more aligned with Drupal core on check level.

Drupal check uses level 2 but let's mirror core for now.

It would be good to get some thoughts on bumping to level 2, I think that we can leave for now but might be worth looking at in future.

Once this runs on the action, I should be able to cross-reference with what has been fixed more easily

millnut commented 11 months ago

I've updated the phpstan file to ignore Unsafe usage of new static based on the info here https://www.drupal.org/docs/develop/development-tools/phpstan/handling-unsafe-usage-of-new-static and how core handles it https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/phpstan.neon.dist?ref_type=heads#L32

millnut commented 11 months ago

Hi @stephen-cox I've removed the level from PHPStan and added a comment; while the level change worked great for identifying PHP 8.2 issues to be resolved it has a side-effect of highlighting classes that are in an optional module and an optional composer suggestion such as;

group_alert_banner.module                       
 ------ ----------------------------------------------------------------------------------------------------------------- 
  37     Access to constant MICROSITES_CONTROLLER_ROLE on an unknown class Drupal\localgov_microsites_group\RolesHelper.

In the recent update to localgov_alert_banner.

Rather than creating noise in the workflows, maybe the increase to level 1 is something we can look into at a later date and dig into the various cases we want to allow/ignore in future.

I still think the changes in this PR (formatting, comment removal, and the note on why level is not used currently) should still be merged to give historical context as to why it is not set.

cc: @finnlewis

finnlewis commented 10 months ago

Discussing this at Merge Monday, @Adnan-cds points out that the failing tests here are due to localgov_alert_banner has tests that need group module, which is in require-dev.

We're happy to merge this and address those tests in separately.

finnlewis commented 10 months ago

We've got a new test failure in the localgov_content_lock and the LocalGovUpdateTest::testUpdate


...............................................................  63 / 164 ( 38%)
............................................................... [12](https://github.com/localgovdrupal/localgov_project/actions/runs/6497345874/job/17646297928?pr=131#step:5:13)6 / 164 ( 76%)
.........E...............S...E........                          164 / 164 (100%)

Time: 22:06.991, Memory: 14.00 MB

There were 2 errors:

1) Drupal\Tests\localgov_content_lock\Functional\ContentLockTest::testContentLockConfiguration
Exception: Deprecated function: Creation of dynamic property Drupal\content_lock\ContentLock\ContentLock::$time is deprecated
Drupal\content_lock\ContentLock\ContentLock->__construct()() (Line: [13](https://github.com/localgovdrupal/localgov_project/actions/runs/6497345874/job/17646297928?pr=131#step:5:14)9)

/var/www/html/web/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:47
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:209
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:158
/var/www/html/vendor/guzzlehttp/promises/src/TaskQueue.php:52
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:251
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:227
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:272
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:229
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:69
/var/www/html/vendor/guzzlehttp/guzzle/src/Client.php:189
/var/www/html/web/core/tests/Drupal/Tests/DrupalTestBrowser.php:137
/var/www/html/vendor/symfony/browser-kit/AbstractBrowser.php:403
/var/www/html/vendor/symfony/browser-kit/AbstractBrowser.php:603
/var/www/html/vendor/symfony/browser-kit/AbstractBrowser.php:421
/var/www/html/vendor/behat/mink-browserkit-driver/src/BrowserKitDriver.php:111
/var/www/html/vendor/behat/mink/src/Session.php:148
/var/www/html/web/core/tests/Drupal/Tests/BrowserTestBase.php:281
/var/www/html/web/core/tests/Drupal/Tests/BrowserTestBase.php:251
/var/www/html/web/core/tests/Drupal/Tests/BrowserTestBase.php:370
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

2) Drupal\Tests\localgov\Functional\LocalGovUpdateTest::testUpdate
Exception: Deprecated function: Creation of dynamic property Drupal\geofield_map\MapThemerPluginManager::$geofieldMapSettings is deprecated
Drupal\geofield_map\MapThemerPluginManager->__construct()() (Line: 72)

/var/www/html/web/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:47
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:209
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:158
/var/www/html/vendor/guzzlehttp/promises/src/TaskQueue.php:52
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:251
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:227
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:272
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:229
/var/www/html/vendor/guzzlehttp/promises/src/Promise.php:69
/var/www/html/vendor/guzzlehttp/guzzle/src/Client.php:189
/var/www/html/web/core/tests/Drupal/Tests/DrupalTestBrowser.php:137
/var/www/html/vendor/symfony/browser-kit/AbstractBrowser.php:403
/var/www/html/vendor/behat/mink-browserkit-driver/src/BrowserKitDriver.php:111
/var/www/html/vendor/behat/mink/src/Session.php:[14](https://github.com/localgovdrupal/localgov_project/actions/runs/6497345874/job/17646297928?pr=131#step:5:15)8
/var/www/html/web/core/tests/Drupal/Tests/UiHelperTrait.php:[23](https://github.com/localgovdrupal/localgov_project/actions/runs/6497345874/job/17646297928?pr=131#step:5:24)7
/var/www/html/web/core/tests/Drupal/Tests/UiHelperTrait.php:451
/var/www/html/web/core/tests/Drupal/Tests/UpdatePathTestTrait.php:53
/var/www/html/web/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php:[27](https://github.com/localgovdrupal/localgov_project/actions/runs/6497345874/job/17646297928?pr=131#step:5:28)0
/var/www/html/web/profiles/contrib/localgov/tests/src/Functional/LocalGovUpdateTest.php:79
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:7[28](https://github.com/localgovdrupal/localgov_project/actions/runs/6497345874/job/17646297928?pr=131#step:5:29)
millnut commented 10 months ago

Thanks @finnlewis looks like I missed those I wasn't aware there were modules in localgov install profile; I should have some time tomorrow to fix those

millnut commented 10 months ago

Hey @finnlewis I've looked at both failures and they are not from LocalGov Drupal code but from contrib modules.

Content Lock has a patch for PHP 8.2; I don't think we should add patches for PHP compatibility LocalGov Drupal side, so I've chased on that issue for a tagged release and the patch fixes the issue.

Geofield Map is a bit more tricky, we currently use the v2 branch of geofield_map however it is no longer supported in favour of the v3 branch. The v3 branch fixes the PHP 8.2 error. Geofield Map is used by the localgov_geo module and there was discussion around removing this module so we should probably discuss this more on Monday on whether we remove or upgrade to v3.

finnlewis commented 10 months ago

Discussing in Merge Monday.

We're going to merge this and tag a beta release.

The failing tests are upstream contrib PHP 8.2 issues. We will outline known issues in release notes.