propelorm / Propel2

Propel2 is an open-source high-performance Object-Relational Mapping (ORM) for modern PHP
http://propelorm.org/
MIT License
1.26k stars 398 forks source link

Fixing Psalm CI Issues #1886

Closed oojacoboo closed 2 years ago

oojacoboo commented 2 years ago

For some reason Psalm isn't running the same for CI as local. It's also running again the vendor dir on CI, which is wrong. Additionally, I get a lot of analysis errors locally when targeting the default config. It's possible the CI isn't properly using it either.

codecov-commenter commented 2 years ago

Codecov Report

Merging #1886 (fa07ade) into master (ecec6f2) will not change coverage. The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1886   +/-   ##
=========================================
  Coverage     87.61%   87.61%           
  Complexity     7814     7814           
=========================================
  Files           227      227           
  Lines         21161    21161           
=========================================
  Hits          18541    18541           
  Misses         2620     2620           
Flag Coverage Δ
5-max 87.61% <ø> (ø)
7.4 87.61% <ø> (ø)
agnostic 66.99% <ø> (ø)
mysql 68.88% <ø> (ø)
pgsql 68.89% <ø> (ø)
sqlite 66.72% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ecec6f2...fa07ade. Read the comment docs.

mringler commented 2 years ago

tl;dr: The problem seems to be that Psalm is running in PHP 7.4 mode, but the Symfony package uses PHP 8 features, a simple change to the Github Workflow file should fix it.

Looking at the output from the failed run, we can see that Propel uses a Psalm version which is capable of checking PHP 8 features (v4.23), but it is run in php 7 mode:

==> Setup PHP
✓ PHP Found PHP 8.1.6
...
 - Locking psalm/phar (4.23.0)
...
 - Locking symfony/yaml (v6.1.0)
...
Run composer psalm
> vendor/bin/psalm.phar --config=psalm.xml
Target PHP version: 7.4 (inferred from composer.json)

In the pipeline, PHP 8.1.6 is installed, but psalm still uses 7.4, I guess this is due to the minimal required version in composer.json:

    "require": {
        "php": ">=7.4",
        ...

The error I looked at happens in the Symfony Yaml-Parser (v6.1):

ERROR: ParseError - vendor/symfony/yaml/Parser.php:1047:38 - Syntax error, unexpected T_DOUBLE_ARROW on line 1047 (see https://psalm.dev/173)
                \PREG_INTERNAL_ERROR => 'Internal PCRE error.',

In the Parser code, that line is part of a PHP 8 match statement:

    public static function preg_match(string $pattern, string $subject, array &$matches = null, int $flags = 0, int $offset = 0): int
    {
        if (false === $ret = preg_match($pattern, $subject, $matches, $flags, $offset)) {
            $error = match (preg_last_error()) {
                \PREG_INTERNAL_ERROR => 'Internal PCRE error.', // <--------------- here
                \PREG_BACKTRACK_LIMIT_ERROR => 'pcre.backtrack_limit reached.',
                \PREG_RECURSION_LIMIT_ERROR => 'pcre.recursion_limit reached.',
                \PREG_BAD_UTF8_ERROR => 'Malformed UTF-8 data.',
                \PREG_BAD_UTF8_OFFSET_ERROR => 'Offset doesn\'t correspond to the begin of a valid UTF-8 code point.',
                default => 'Error.',
            };

            throw new ParseException($error);
        }

        return $ret;
    }

All consecutive errors in the file are probably due to that first parse error.

So now I am pretty sure that this is not a problem with the Propel code, but with CI/CD setup.

I think the fix is to add a command line parameter to the psalm command in the Github Workflow file (ci.yml) in line 161, something like this should work:

            - name: Psalm
              run: composer psalm -- --php-version=${{ matrix.php-version }}

Being part of the hoi polloi, I cannot change this.

@dereuromark can you try that out, see if it fixes the issue?

oojacoboo commented 2 years ago

@mringler thanks for helping dig into this. Took a little massaging, but looks like I got it working/passing.