-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci: fix phpstan #131
ci: fix phpstan #131
Conversation
- remove commented out code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am marking this as okay, but please do not merge it as yet. Let's see what Finn says.
- ignore new static()
I've updated the phpstan file to ignore |
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;
In the recent update to 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 |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your work on this @millnut !
We've got a new test failure in the localgov_content_lock and the LocalGovUpdateTest::testUpdate
|
Thanks @finnlewis looks like I missed those I wasn't aware there were modules in |
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. |
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. |
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.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