#62 Add Psalm support. #66

hkirsman · 2021-09-24T17:04:02Z

You can try this out on Drupal 9 project:

composer require wunderio/code-quality:dev-feature/62-add-psalm-task --dev

or in Lando prefix with lando. Psalm requires PHP's mbstring and in my experience I was missing it locally but Lando had it.

lando composer require wunderio/code-quality:dev-feature/62-add-psalm-task --dev

Lock files can be tricky and every project is different. On my 9.2.6 installation I did this:

lando composer require wunderio/code-quality:dev-feature/62-add-psalm-task composer/xdebug-handler:1.4.6 composer/composer:2.0.2  --dev

Check that first line in grumphp.yml is grumphp: and not parameters:

In Grumphp add psalm task and extension lines ("psalm: ~" under "tasks:" and "- Wunderio\GrumPHP\Task\Psalm\PsalmExtensionLoader" under "extensions:")

Copy the psalm.xml config to project root

cp vendor/wunderio/code-quality/config/psalm.xml ./psalm.xml
You might see issues with tests not finding dependencies. ERROR: MissingDependency - web/modules/custom/redirect_old_tua/tests/src/HomePageTest.php:13:28 - Drupal\Tests\BrowserTestBase depends on class or interface phpunit\framework\testcase that does not exist (see https://psalm.dev/157)
class HomePageTest extends BrowserTestBase {

For that install testing Drupals testing support:

composer require drupal/core-dev:X.X.X --dev

As I don't have php locally I'm also using Landos php with this grumphp config:

grumphp:
git_hook_variables:
EXEC_GRUMPHP_COMMAND: 'lando php'

and re-init grumphp

lando grumphp git:init

Where X.X.X is your current core version.

hkirsman · 2021-09-25T10:26:52Z

Seems as if we also need https://github.com/fenetikm/autoload-drupal because otherwise it complains missing classes although they are there (and dev-master because it supports Composer 2). I've added this with last commit.

In composer.json under extras add

    "autoload-drupal": {
        "modules": [
            "web/modules/custom/",
            "web/modules/contrib/",
            "web/core/modules/"
        ],
        "classmap": [
            "web/core/tests/Drupal/Tests"
        ]
    }

Run

lando composer dump-autoload

And finally try to commit code or run

lando grumphp run

hkirsman · 2021-09-25T10:37:26Z

Could also be we need to test https://github.com/mortenson/psalm-plugin-drupal

hkirsman · 2021-09-25T14:32:08Z

https://github.com/fenetikm/autoload-drupal seems to be quite good but doesn't seem to load .module files so I'm seeing undefined constant error with my code.

This one says it solves the .module and .theme loading https://github.com/mortenson/psalm-plugin-drupal

hkirsman · 2021-09-25T19:01:32Z

For temporary fix it seems I'd need to create file something like autoload-extras-for-psalm.php

<?php

$vendorDir = dirname(dirname(__FILE__));
$baseDir = dirname($vendorDir);
$projectRoot = realpath($baseDir . '/..');

// @todo: find all .module and .theme files and require here.
require_once $projectRoot . '/web/modules/contrib/adobe_captivate/adobe_captivate.module';

In psalm.xml add autoloader parameter:

<psalm
    errorLevel="6"
    resolveFromConfigFile="true"
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xmlns="https://getpsalm.org/schema/config"
    xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
    autoloader="vendor/wunderio/code-quality/autoload-extras-for-psalm.php"
>

hkirsman · 2021-09-26T10:34:56Z

Hm, can't get this to work.

One more thing. We already have mglaman/phpstan-drupal installed and this seems promising: https://github.com/mglaman/phpstan-drupal/blob/main/drupal-autoloader.php

hkirsman · 2021-09-27T19:49:18Z

I've taken the https://github.com/mglaman/phpstan-drupal/blob/main/drupal-autoloader.php and refactored it a bit. To me it seems to work nicely.

hkirsman · 2021-09-28T08:07:04Z

Should be ok now for testing. See initial comment.

hkirsman · 2022-03-11T13:56:33Z

I'm getting

ERROR: UndefinedInterfaceMethod - web/modules/custom/tekla_apireference/src/Form/APIReferenceDocumentRevisionRevertTranslationForm.php:101:54 - Method Drupal\Core\Entity\EntityInterface::getTranslation does not exist (see https://psalm.dev/181)
    $latest_revision_translation = $latest_revision->getTranslation($this->langcode);

This is the code:

    $latest_revision = $this->apiReferenceDocumentStorage->load($revision->id());
    $latest_revision_translation = $latest_revision->getTranslation($this->langcode);

I don't see it's actually a valid error. PhpStorm find the interface and if I try to add it then PhpStorm also yells that I can't add it again.

So I'll disable this error:

    <issueHandlers>
      <UndefinedInterfaceMethod errorLevel="suppress" />
    </issueHandlers>

hkirsman · 2022-03-11T15:07:46Z

Another one:

ERROR: ImplicitToStringCast - web/modules/custom/tekla_apireference/src/Form/APIReferenceDocumentImportForm.php:137:58 - Argument 2 of Drupal\Core\Form\FormStateInterface::setErrorByName expects string, Drupal\Core\StringTranslation\TranslatableMarkup provided with a __toString method (see https://psalm.dev/060)
      $form_state->setErrorByName('source_package_file', t('Import could not continue because directory %directory could not be created for extraction.', [
        '%directory' => $extract_path,
      ]));

But we're not going to do something like t('Foobar')->____toString(). That would output string but with the placeholders. I'll create ignore for that specific error.

All in all I'm adding these ignores to config:

    <issueHandlers>
      <UndefinedInterfaceMethod errorLevel="suppress" />
      <ImplicitToStringCast errorLevel="suppress" />
    </issueHandlers>

k4lv15

Hey, @hkirsman! I've been testing this feature on one of the customer's projects for some time. Seems to be working nicely in overall 🙂 👍 But since commit 5a9c35c I'm facing a few issues, please see my other two comments below.

Also, one thing that is not working is PSALM's Docblock suppression (/** @psalm-suppress <SomeError> */) as PHPCS task throws Drupal.Commenting.InlineComment.DocBlock error whenever such docblock is used. In my project I've added a rule to phpcs.xml file to ignore such errors completely (see my snippet below), but a better option would prolly be to ignore only @psalm-suppress docblock comments. Not sure how, but I guess something similar has been done with @var docblocks.

<!-- Mark inline DocBlock comments only as a warning as otherwise using @psalm-suppress annotation comments leads to PHPCS errors-->
<rule ref="Drupal.Commenting.InlineComment.DocBlock">
    <severity>4</severity>
</rule>

UPDATE:
PSALM's Docblock suppression is now allowed and works since latest commit (361795e) 🎉

k4lv15 · 2022-03-11T10:01:47Z

src/Task/tasks.yml

+      defaults: null
+      allowed_types: ['string', 'null' ]
+    threads:
+      defaults: 6


Running ./vendor/bin/grumphp run --tasks=psalm gives "The pcntl & posix extensions must be loaded in order for Psalm to be able to use multiple processes." Happens both locally (using MacOs BigSur) and on CircleCI (using eu.gcr.io/silta-images/php:7.4-fpm-v0.1 for PHP container). If I override the default value of threads parameter back to 1, then it works again.
UPDATE:
Locally, if I use custom lando tooling that utilises lando's appserver for running for GrumPHP commands, everything works just fine:

grumphp: description: Runs GrumPHP commands cmd: - appserver: ./vendor/bin/grumphp

This works nicely now both on Lando and CircleCI :)

k4lv15 · 2022-03-14T11:06:16Z

src/Task/Psalm/PsalmTask.php

+    $arguments->addOptionalArgument('--report=%s', $config['report']);
+    $arguments->addOptionalArgument('--no-cache', $config['no_cache']);
+    $arguments->addOptionalArgument('--threads=%d', $config['threads']);
+    $arguments->add('--find-unused-code');


When testing on one of client's projects, UnusedVariable errors can't be ignored if using /** @psalm-suppress UnusedVariable */ annotation-comment before the variable., as --find-unused-code seems to have higher priority. It can be changed by ignoring particular errors in psalm.xml file by adding

<issueHandlers> <UnusedVariable errorLevel="info" /> </issueHandlers>

but prolly all new UnusedVariable errors will be ignored from now on, which is not great, I guess.

Why not remove that variable or is false positive?

It seems like a false positive to me as the variable is used multiple times after defined. Also PHPStorm doesn't mark it as unused which happened in all other cases I fixed. What's different is that it's defined as a reference to another variable (see the example code below). Maybe PSALM doesn't like that for some reason.

$subform1 =& $form['field_content_row']['widget'][$key]['subform']; if (!empty($subform1['field_block_reference']['widget'])) { foreach (Element::children($subform1['field_block_reference']['widget']) as $key1) { ...

UPDATE
Managed to ignore the error by adding @psalm-suppress UnusedVariable for the function which had that variable, at least better option then ignoring such errors completely 🙂 So seems that only inline usages of @psalm-suppress doesn't work.

k4lv15

Looks good, all of the mentioned issues have been fixed ✅
NOTE: Set-up was tested on a real customer project and worked nicely ✅
Green light from me 🟢 And great work! 💪 💪 💪

hkirsman requested review from guncha25, mikkmiggur and ragnarkurmwunder September 24, 2021 17:04

hkirsman changed the title ~~#62 Add Psalm task.~~ #62 Add Psalm support. Sep 25, 2021

hkirsman mentioned this pull request Sep 25, 2021

Implement Psalm the Bug-finder #62

Open

hkirsman added 5 commits March 2, 2022 16:34

#62 Add Psalm task.

9ff8308

#62 Add Drupal 8 Composer Autoloader.

c618c5c

#62 Add custom Drupal autoloader for Psalm.

282aa40

#62 Increase Psalm strictness (from 6 to 4).

a112b4f

#62 Fix Drupal autoloader base path detection.

5a54029

hkirsman force-pushed the feature/62-add-psalm-task branch from b67cded to 5a54029 Compare March 2, 2022 14:36

hkirsman added 3 commits March 3, 2022 11:09

#62 Fix autoloader.

53825a3

#62 Enable Psalm to find unused variables.

5a9c35c

#62 Increase Psalm thread for faster scan.

af7cbc8

#62 Add 2 ignores to psalm.xml.

b4df640

k4lv15 reviewed Mar 14, 2022

View reviewed changes

#62 Replace phpcs comment sniffer with custom code.

942a0f2

hkirsman force-pushed the feature/62-add-psalm-task branch from 61f093c to 942a0f2 Compare March 15, 2022 07:42

hkirsman added 2 commits March 15, 2022 09:55

#62 Fix coding standards in WunderInlineCommentSniff.

b0e5d6a

#62 Allow PSALM @psalm-suppress comment block.

361795e

hkirsman force-pushed the feature/62-add-psalm-task branch from 3effb7b to 361795e Compare March 15, 2022 07:58

hkirsman added 2 commits March 16, 2022 15:41

#62 Force Psalm to 1 thead if pcntl or posix is missing.

67318ba

#62 Add contrib and core ignore to Psalm.

e3a9b08

k4lv15 approved these changes Mar 31, 2022

View reviewed changes

hkirsman merged commit 85a8935 into master Apr 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#62 Add Psalm support. #66

#62 Add Psalm support. #66

hkirsman commented Sep 24, 2021 •

edited

Loading

hkirsman commented Sep 25, 2021 •

edited

Loading

hkirsman commented Sep 25, 2021 •

edited

Loading

hkirsman commented Sep 25, 2021

hkirsman commented Sep 25, 2021

hkirsman commented Sep 26, 2021

hkirsman commented Sep 27, 2021

hkirsman commented Sep 28, 2021

hkirsman commented Mar 11, 2022

hkirsman commented Mar 11, 2022

k4lv15 left a comment •

edited

Loading

k4lv15 Mar 11, 2022

k4lv15 Mar 21, 2022

k4lv15 Mar 14, 2022

hkirsman Mar 14, 2022

k4lv15 Mar 14, 2022 •

edited

Loading

k4lv15 left a comment

#62 Add Psalm support. #66

#62 Add Psalm support. #66

Conversation

hkirsman commented Sep 24, 2021 • edited Loading

hkirsman commented Sep 25, 2021 • edited Loading

hkirsman commented Sep 25, 2021 • edited Loading

hkirsman commented Sep 25, 2021

hkirsman commented Sep 25, 2021

hkirsman commented Sep 26, 2021

hkirsman commented Sep 27, 2021

hkirsman commented Sep 28, 2021

hkirsman commented Mar 11, 2022

hkirsman commented Mar 11, 2022

k4lv15 left a comment • edited Loading

Choose a reason for hiding this comment

k4lv15 Mar 11, 2022

Choose a reason for hiding this comment

k4lv15 Mar 21, 2022

Choose a reason for hiding this comment

k4lv15 Mar 14, 2022

Choose a reason for hiding this comment

hkirsman Mar 14, 2022

Choose a reason for hiding this comment

k4lv15 Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

k4lv15 left a comment

Choose a reason for hiding this comment

hkirsman commented Sep 24, 2021 •

edited

Loading

hkirsman commented Sep 25, 2021 •

edited

Loading

hkirsman commented Sep 25, 2021 •

edited

Loading

k4lv15 left a comment •

edited

Loading

k4lv15 Mar 14, 2022 •

edited

Loading