Skip to content
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

Use of PHP-Parser 5.0 causes fatal when project under test also polyfills tokens #1025

Closed
jrfnl opened this issue Jan 10, 2024 · 4 comments
Closed

Comments

@jrfnl
Copy link
Contributor

jrfnl commented Jan 10, 2024

Q A
php-code-coverage version 9.2.30
PHP version 8.0.30
Driver Xdebug
Xdebug version (if used) 3.3.0
Installation Method Composer
Usage Method PHPUnit
PHPUnit version (if used) 9.6.15

Description

I seem to have caught a weird one...

PHP_CodeSniffer also polyfills PHP tokens and uses text strings for the polyfills. Changing this would be a breaking change and is not currently on the table.

PHP_CodeSniffer uses PHPUnit for its tests and I have recently enabled recording of code coverage.
This worked well until a few days ago when PHP Parser 5.0 got released.

Now, I see the following error, but only when the tests are run with code coverage on PHP 8.0:
PHP Fatal error: Uncaught TypeError: Cannot assign string to property PhpToken::$id of type int in /home/runner/work/PHP_CodeSniffer/PHP_CodeSniffer/vendor/nikic/php-parser/lib/PhpParser/Lexer.php:96

Things seem to work fine with PHP 8.1+.

The error looks to be related to the following change in PHP Parser: https://github.com/nikic/PHP-Parser/blob/master/UPGRADE-5.0.md#changes-to-token-representation

I've already tried my go-to solution of warming the cache prior to running the tests, as previously discussed in #798, unfortunately this does not seem to do the trick, it just moves the error to the cache warming step, even though that step shouldn't be running any of the PHPCS code IIRC.

I've done some test runs on different PHP versions to isolate the problem.
The results can be seen here: https://github.com/PHPCSStandards/PHP_CodeSniffer/actions/runs/7469399693

For now, I'm going to change the GH Actions workflow which is running into this issue to run code coverage on PHP 8.1 instead of PHP 8.0, but I figured it would still be useful to report this.

Additional info

Composer requirements used in the problem run:

    "require": {
        "php": ">=7.2.0",
        "ext-simplexml": "*",
        "ext-tokenizer": "*",
        "ext-xmlwriter": "*"
    },
    "require-dev": {
        "phpunit/phpunit": "^8.0 || ^9.3.4"
    },

PHPUnit config file used: https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/4.x/feature/ci-prevent-issues-with-code-coverage/phpunit.xml.dist

GH Actions script used for the above linked test run: https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/4.x/feature/ci-prevent-issues-with-code-coverage/.github/workflows/test.yml

Please let me know if you need additional information or want me to run some tests.

jrfnl added a commit to PHPCSStandards/PHP_CodeSniffer that referenced this issue Jan 10, 2024
As of PHPUnit 9.3, PHPUnit started using PHP-Parser as a basis to calculate code coverage. PHP Parser also polyfills tokens and that can interfere with the sniffs, but only in code coverage runs.

In PHPUnit 9.3.4 a new feature was added to warm the code cache ahead of running tests recording code coverage. Using that feature should prevent the token interference.

So far, I've not seen problems with this for this codebase, but better safe than sorry, so I'm going to enable cache warming now anyway.

Notes regarding cache-warming:
* The `--coverage-cache` and `--warm-coverage-cache` options are available since PHPUnit 9.3.4 and if these are used on older PHPUnit versions, PHPUnit will error out with an "unrecognized CLI argument" error.
* In other words: these options can only be used with PHPUnit 9.3+, which is why the PHPUnit version is checked and remembered and subsequently used in the conditions.
* Also: running PHPUnit with the `--warm-coverage-cache` option _does not run the tests_. It literally only warms the cache, which is why this is implemented as a separate step in the workflow.
* The cache directory can be configured in the `phpunit.xml[.dist]` file, but only when using the PHPUnit 9.3+ `coverage` XML tag.
    As the PHPUnit config used needs to stay cross-version compatible with older PHPUnit versions, the CLI option is used for now.

Refs:
* sebastianbergmann/php-code-coverage#798 (comment)
* sebastianbergmann/phpunit@9.3.3...9.3.4

Also note that PHP-Parser 5.0 was released a couple of days ago and when used for code coverage runs on PHP 8.0 (as used for the 4.0 branch), this causes problems.

When I merge this commit up to 4.0, I will adjust the GH Actions script for the 4.0 branch to use PHP 8.1 instead of PHP 8.0 to avoid this issue.

The issue has been reported upstream: sebastianbergmann/php-code-coverage#1025
jrfnl added a commit to PHPCSStandards/PHP_CodeSniffer that referenced this issue Jan 10, 2024
As of PHPUnit 9.3, PHPUnit started using PHP-Parser as a basis to calculate code coverage. PHP Parser also polyfills tokens and that can interfere with the sniffs, but only in code coverage runs.

In PHPUnit 9.3.4 a new feature was added to warm the code cache ahead of running tests recording code coverage. Using that feature should prevent the token interference.

So far, I've not seen problems with this for this codebase, but better safe than sorry, so I'm going to enable cache warming now anyway.

Notes regarding cache-warming:
* The `--coverage-cache` and `--warm-coverage-cache` options are available since PHPUnit 9.3.4 and if these are used on older PHPUnit versions, PHPUnit will error out with an "unrecognized CLI argument" error.
* In other words: these options can only be used with PHPUnit 9.3+, which is why the PHPUnit version is checked and remembered and subsequently used in the conditions.
* Also: running PHPUnit with the `--warm-coverage-cache` option _does not run the tests_. It literally only warms the cache, which is why this is implemented as a separate step in the workflow.
* The cache directory can be configured in the `phpunit.xml[.dist]` file, but only when using the PHPUnit 9.3+ `coverage` XML tag.
    As the PHPUnit config used needs to stay cross-version compatible with older PHPUnit versions, the CLI option is used for now.

Refs:
* sebastianbergmann/php-code-coverage#798 (comment)
* sebastianbergmann/phpunit@9.3.3...9.3.4

Also note that PHP-Parser 5.0 was released a couple of days ago and when used for code coverage runs on PHP 8.0 (as used for the 4.0 branch), this causes problems.

When I merge this commit up to 4.0, I will adjust the GH Actions script for the 4.0 branch to use PHP 8.1 instead of PHP 8.0 to avoid this issue.

The issue has been reported upstream: sebastianbergmann/php-code-coverage#1025
@sebastianbergmann
Copy link
Owner

PHPUnit uses php-code-coverage, php-code-coverage uses PHP-Parser. You are testing PHP_CodeSniffer which conflicts with PHP-Parser. I am sorry, but I do not know what I should do about that.

@jrfnl
Copy link
Contributor Author

jrfnl commented Jan 12, 2024

@sebastianbergmann The weird thing about the above is that the conflict apparently only happens on PHP 8.0, not PHP 8.1+, even though the exact same versions of PHPUnit, PHP Code Coverage and PHP Parser are being used on PHP 8.0 vs PHP 8.1+.

That's really the issue I'm reporting.

I'm okay with closing the ticket as, as I wrote above, I can work around it by using PHP 8.1 for code coverage, but figured that some awareness that not everything is as it should be on PHP 8.0 would be a good thing.

@sebastianbergmann
Copy link
Owner

The weird thing about the above is that the conflict apparently only happens on PHP 8.0, not PHP 8.1+, even though the exact same versions of PHPUnit, PHP Code Coverage and PHP Parser are being used on PHP 8.0 vs PHP 8.1+.

There is PHP version-specific code in PHP-Parser's polyfill.

I'm okay with closing the ticket as, as I wrote above

Then I will do just that, sorry :-(

@jrfnl
Copy link
Contributor Author

jrfnl commented Jan 12, 2024

The weird thing about the above is that the conflict apparently only happens on PHP 8.0, not PHP 8.1+, even though the exact same versions of PHPUnit, PHP Code Coverage and PHP Parser are being used on PHP 8.0 vs PHP 8.1+.

There is PHP version-specific code in PHP-Parser's polyfill.

That code applies to all PHP 8.x versions though - PHP_VERSION_ID >= 80000 - and should not cause this issue.

DannyvdSluijs pushed a commit to DannyvdSluijs/PHP_CodeSniffer_Continuation that referenced this issue Apr 29, 2024
As of PHPUnit 9.3, PHPUnit started using PHP-Parser as a basis to calculate code coverage. PHP Parser also polyfills tokens and that can interfere with the sniffs, but only in code coverage runs.

In PHPUnit 9.3.4 a new feature was added to warm the code cache ahead of running tests recording code coverage. Using that feature should prevent the token interference.

So far, I've not seen problems with this for this codebase, but better safe than sorry, so I'm going to enable cache warming now anyway.

Notes regarding cache-warming:
* The `--coverage-cache` and `--warm-coverage-cache` options are available since PHPUnit 9.3.4 and if these are used on older PHPUnit versions, PHPUnit will error out with an "unrecognized CLI argument" error.
* In other words: these options can only be used with PHPUnit 9.3+, which is why the PHPUnit version is checked and remembered and subsequently used in the conditions.
* Also: running PHPUnit with the `--warm-coverage-cache` option _does not run the tests_. It literally only warms the cache, which is why this is implemented as a separate step in the workflow.
* The cache directory can be configured in the `phpunit.xml[.dist]` file, but only when using the PHPUnit 9.3+ `coverage` XML tag.
    As the PHPUnit config used needs to stay cross-version compatible with older PHPUnit versions, the CLI option is used for now.

Refs:
* sebastianbergmann/php-code-coverage#798 (comment)
* sebastianbergmann/phpunit@9.3.3...9.3.4

Also note that PHP-Parser 5.0 was released a couple of days ago and when used for code coverage runs on PHP 8.0 (as used for the 4.0 branch), this causes problems.

When I merge this commit up to 4.0, I will adjust the GH Actions script for the 4.0 branch to use PHP 8.1 instead of PHP 8.0 to avoid this issue.

The issue has been reported upstream: sebastianbergmann/php-code-coverage#1025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants