emogrifier icon indicating copy to clipboard operation
emogrifier copied to clipboard

Psalm errors on Windows with PHIVE Phar

Open JakeQZ opened this issue 2 years ago • 5 comments

These look like genuine errors.

It is mysterious that they are not being reported by Psalm on Linux. It is also mysterious that if Psalm is installed via Composer rather than PHIVE, they are also not reported.

See attached for the actual errors currently reported. windows-php-7.4-psalm-report.txt

JakeQZ avatar Sep 08 '23 01:09 JakeQZ

The errors are also reported on Linux: https://github.com/MyIntervals/emogrifier/actions/runs/6068667790/job/16461949927

My best guess is that it's related to the latest release of rawr/cross-data-providers.

oliverklee avatar Sep 08 '23 12:09 oliverklee

The errors are also reported on Linux: https://github.com/MyIntervals/emogrifier/actions/runs/6068667790/job/16461949927

No, these are the errors addressed by #1221. See the .txt file attached to the OP. There are loads more there not being reported on Linux.

JakeQZ avatar Sep 08 '23 15:09 JakeQZ

Ah, this might also be connected to the PHP version Psalm is run on (as the results probably are somewhat version-specific). On CI, we're running the tests on PHP 8.1. Which version are you using on Windows?

oliverklee avatar Sep 08 '23 15:09 oliverklee

Which version are you using on Windows?

7.4. However, if I install Psalm via Composer (instead of PHIVE), I don't get the errors.

JakeQZ avatar Sep 08 '23 17:09 JakeQZ

Even if Psalm is installed and invoked via PHIVE, the errors are only reported if there isn't also a copy of Psalm installed via Composer. But I'm guessing that may be because the classes that can be autoloaded via Composer are preferred over those in the .phar.

Looking through the errors, most seem to be because Psalm is unable to determine the types of properties and return values of built-in PHP classes and functions (e.g. DOMDocument properties). That oddity must be caused by some combination of the environment (Windows, PHP, PHIVE, and system configuration).

However, with Psalm 5.15.0, I am getting what look like genuine errors regarding failure to check the return value from preg_replace (see attached), e.g.

ERROR: PossiblyNullArgument - src/CssInliner.php:975:39 - Argument 1 of ltrim cannot be null, possibly null value provided (see https://psalm.dev/078)
        $selectorWithoutNots = \ltrim(\preg_replace_callback(
            '/([\\s>+~]?+):not(\\([^()]*+(?:(?2)[^()]*+)*+\\))/i',
            /** @param array<array-key, string> $matches */
            function (array $matches): string {
                return $this->replaceUnmatchableNotComponent($matches);
            },
            ' ' . $selector
        ));

Not sure why these are only being picked up in my configuration, and not otherwise. preg_replace_callback (and preg_replace) may return null in the case of an error (which may be due to catastrophic backtracking).

Pslam has recently introduced more stringent type-checking on calls to the preg_ functions, so it might be expected that we get these errors with the latest version. But strangely, we don't get them on Linux with PHP 8.1, or if Psalm is installed via Composer (on Windows with PHP 7.4).

Hmm... I'll leave this open for now, and open an issue for preg_replace* return values not being checked, which is definitely an issue. I have a workaround to get the same behaviour as on Linux. I'll one day find time to install a newer version of PHP and see what happens with that...

JakeQZ avatar Sep 09 '23 22:09 JakeQZ

Closing, since we now use PHPStan instead of Psalm.

JakeQZ avatar Sep 14 '24 22:09 JakeQZ