In #3400366: Remove DrupalComponentTestListenerTrait and replace with a PHPStan rule, we are trying to replace DrupalComponentTestListenerTrait with a PHPStan rule.
The testing of that rule via PHPUnit fails, though, given PHPStan uses a internally version 4 of nikic/php-parser. See details here: https://github.com/phpstan/phpstan/issues/10620
We have currently locked nikic/php-parser to v5.
However, there is no reason to strictly adhere to that constraint at the moment:
$ composer why nikic/php-parser
phpunit/php-code-coverage 9.2.31 requires nikic/php-parser (^4.18 || ^5.0)
sebastian/complexity 2.0.3 requires nikic/php-parser (^4.18 || ^5.0)
sebastian/lines-of-code 1.0.4 requires nikic/php-parser (^4.18 || ^5.0) Not even under PHPUnit 10:
$ composer why nikic/php-parser
phpunit/php-code-coverage 10.1.14 requires nikic/php-parser (^4.18 || ^5.0)
sebastian/complexity 3.2.0 requires nikic/php-parser (^4.18 || ^5.0)
sebastian/lines-of-code 2.0.2 requires nikic/php-parser (^4.18 || ^5.0) | Comment | File | Size | Author |
|---|---|---|---|
| #17 | 3441353-nr-bot.txt | 90 bytes | needs-review-queue-bot |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !7529
Comments
Comment #2
mondrakeComment #4
mondrakeComment #5
mondrakeComment #6
mondrakeComment #7
spokjeNot a big fan of lowering versions of dependencies, but seeing that:
- None of our other dependencies need it
- Tests are green
- PHPUnit 10 waits for no man...
I'm going to RTBC
Comment #8
spokjeComment #9
alexpottI;ve read https://github.com/phpstan/phpstan/issues/7943 and it states quite clearly that
Reading #3400366: Remove DrupalComponentTestListenerTrait and replace with a PHPStan rule it sounds like the real issue is with DebugClassLoader
Comment #10
mondrake@alexpott the problem is described here https://github.com/phpstan/phpstan/issues/10620#issuecomment-1959440979
Comment #11
mondrakeYou can USE PHPStan for static analysis, but you cannot TEST your own rules.
Comment #12
alexpottSo the solution is also written there...
Given how PHPStan runs we can hit this problem in the future. If we adding PHPStan tests to core then we're going to need to run them separately.
Comment #13
longwaveThis will also prevent us from going to PHPUnit 11, though I suppose we cross that bridge when we come to it: https://github.com/sebastianbergmann/php-code-coverage/blob/main/compose...
But because PHPUnit itself uses
nikic/php-parser, I'm not sure how we run these tests without explicitly installing v4 anyway? PHPUnit will load the code and then PHPStan crashes because of it, as far as I can see, unless I'm missing something. Or do we somehow need to make the autoloader look in the phar first?Comment #14
catchLooks like this needs more discussion.
I think it would be fine to exclude phpstan tests in the main phpunit job and add a new job, which could then manually downgrade nikic/php-parser?
Comment #15
mondrake#13 locally I solved by running
composer require nikic/php-parser:^4, and the test passes (it throws some debugclassloader deprecations that we'll have to silence, but one thing at a time).So we can
a) downgrade like here
b) create a separate job to just run PHPUnit on PHPstan rules, downgrading via
composer require nikic/php-parser:^4on each runc) mark the test skipped in #3400366: Remove DrupalComponentTestListenerTrait and replace with a PHPStan rule while we wait for PHPStan 2
To me this one seemed the lightest to do, and in any case is temp. If we do want to do b) for more separation, then it's another discussion, and I think should happen in #3400366: Remove DrupalComponentTestListenerTrait and replace with a PHPStan rule.
Comment #16
alexpottI'm for B. I think or PHPStan test should not use our root composer.json for installing and testing things. Or perhaps even better and maybe option D, we should not put the PHPStan rules in core. Can we contribute them to mglaman's package or can we add another package.
Comment #17
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #18
mondrakeComment #19
smustgrave commentedWould this need to be done before 10.3?
Comment #20
mondrakeAs discussed in the parent, we will likely go for a decoupling of the PHPStan rule tests from Drupal core test, so this is not going to be necessary.