Remove DrupalComponentTestListenerTrait and replace with a PHPStan rule

Postpone on https://github.com/mglaman/phpstan-drupal/issues/626

Should phpstan-drupal own this, or should we stick it in Drupal core and register it as a custom rule? It's a rule for Drupal core development, not contrib or user-land code. I wouldn't mind writing the rule. But it seems "wrong" to have a Drupal core development paradigm in phpstan-drupal.

My 0.02$ - do we need product/framework managers input here?

It’s a positioning topic. This, and fwiw #2972100: Remove usage of uniqid → , aim at introducing phpstan checks on Drupal core, but we do not have any prior examples of something like this being done in Drupal itself. So quite naturally devs are opening issues against mglaman/phpstan-drupal.

On a different approach, phpstan/phpstan-deprecation-rules, even if being actively used in core, was added as a dependency of phpstan-drupal, not of core.

IMHO we need rules to guide where issues should be going to from now on.

But it seems "wrong" to have a Drupal core development paradigm in phpstan-drupal.

Fair question, didn't realize this is indeed a core-only rule.

Putting the rule in core itself and registering in core with phpstan seems reasonable, I "only" see issues with testing the rule, which would mean (AFAICT) core would need Yet Another test-job, now for phpstan, which tests core-specific rules.

EDIT: I agree with (cross-post of) @mondrake that we need some guidance here. Tagging with Needs framework manager review as it's the closest one to what we need I could find.

, I "only" see issues with testing the rule, which would mean (AFAICT) core would need Yet Another test-job, now for phpstan, which tests core-specific rules.

Why? It could be tested with a PHPUnit test

This is in the critical path to PHPUnit 10, since listeners will be going away.

We are not going to get the rule from phpstan-drupal, so let’s have one on our own.

Will look into it the next days.

Update IS and title

I wouldn't mind helping write the PHPStan rule. Did we get a decision on if it can go into Drupal core, or is it being delegated to the extension? The one downside of it not going into the extension is that contrib won't have the rule automatically (unless phpstan-drupal also configures it? It might get weird.)

@mglaman if you can write the rule, I would suggest to do it here now. Then if core committers decide not to include in core, it would be anyway straightforward to move it to phpstan-drupal, I guess.

The compelling argument to do it here, IMHO, is that 'components' is code that should not interact with 'core' code, and this check ensure that tests of components do not extend from core test classes but from PHPUnit TestCase. Should not be relevant for contrib/custom that would probably use composer if they need access to non-Drupal code libraries.

#2972100: Remove usage of uniqid → is different I think, because it'd be debatable if restricting use of PHP functions should be for Drupal core only or also extending to contrib.

One more thing - I would suggest not to use core namespaces from /core/lib/Drupal/Core for this, it's not code used by Drupal at runtime. I would rather go for something in /core/tests/Drupal or even better a new subdirectory /core/tests/PhpStan to prevent mixing PHPUnit related code with PHPStan one.

Sounds good to me! I'll try and take a stab at it this week. When I do I'll assign it to myself

I added ComponentTestDoesNotExtendCoreTest as a rule to replace the test listener, along with a test for the rule.

Nice! Left comments inline. NW for that and for the test failures.

BTW reviwing this I just found this test class Drupal\Tests\Component\DrupalComponentTest which may be something to convert to a PHPStan rule, too, in a follow-up.

Fixed the linting jobs.

This is reviewable now, I think.

The test cannot run in CI until 📌 Downgrade (temporarily) nikic/php-parser to ^4 RTBC , but it runs locally when I downgrade the php parser to v4.
In the test, we need to disable Symfony's DebugClassLoader at setup and re-enable at teardown, since PHPStan's internals invoked during the test itself do not dance well with it.

How about... not putting PHPStan rule tests in core/tests/Drupal but put them in core/tests/PHPStan - and then in there as well as the test have a separate composer.json and phpunit.xml.dist file for running the tests. Then we add a new job to the pipeline in .gitlab/pipeline.yml to run these tests. That way PHPStans dependencies and Drupal's stay decoupled.

#21 sounds good - I will start moving stuff around but then adding the job pipeline is above me.

🙂 unintended, but the last change shows how the rule actually works...

we do not want a PHPStan failure here though, so we need to exclude the fixture from PHPStan checking, and enable rule testing via PHPUnit

I think this now is possibly committable:

1) the test run is green, AND the first Drupal's own PHPStan rule works as proven by #22
2) the test for the rule also works, tested locally, but we need a separate test job in the pipeline to setup PHPUnit to execute it decoupled from Drupal core tests - which will certainly take longer but can be deferred to a follow up

How do I get this to work locally?

$ composer require --dev nikic/php-parser:^4
$ vendor/bin/phpunit -c core core/tests/PHPStan
PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Testing /var/www/html/drupal/core/tests/PHPStan
E                                                                   1 / 1 (100%)

Time: 00:01.504, Memory: 44.00 MB

There was 1 error:

1) Drupal\PHPStan\Tests\ComponentTestDoesNotExtendCoreTestTest::testRule
RuntimeException: The autoloader expected class "Drupal\Tests\UnitTestCase" to be defined in file "/var/www/html/drupal/core/tests/Drupal/Tests/UnitTestCase.php". The file was found but the class was not in it, the class name or namespace probably has a typo.

@longwave hang on ... I'm just making it work :)

Short answer... do the same as the new pipline job...

    - cd core/tests/PHPStan
    - composer install
    - vendor/bin/phpunit tests --coverage-text --colors=never --coverage-cobertura=coverage.cobertura.xml --log-junit junit-reports/recipe-tests.xml

Why does it need it's own composer.json? This seems really "extra". And ideally Drupal core should start providing it's own PHPStan rules for certain things. I don't see how this rule plus testing the rule introduced the need for a new subroot Composer file

@mglaman because testing phpstan rules requires it's dependencies. And it's and core's will not move together. We want to move to PHPUnit 10. PHPStan doesn't support the dependencies that resolves to for us - see 📌 Downgrade (temporarily) nikic/php-parser to ^4 RTBC

Interesting that the test fails but scans don't for this, then. I guess because the APIs (test base classes) are public and working with non-prefixed code it exposes the problem.

Thanks, I didn't easily see _why_ it was happening

And this is because PHPStan packages up it's own dependencies inside the phar file - which imo is a decent decision. But when you you their testing infra you hit a wall because then you have their real dependencies.

I real don't think PHPStan should be deciding what our root dependencies are.

I've put a README.md file in the directory. Atm it is just @todo - but I think we should explain ourselves in there and tell people how to run tests. I'll address this tomorrow as I don't have time right now. Maybe someone will beat me to it.

Let's fill in the readme, at least just mentioning this directory is where we keep Drupal core's custom PHPStan rules. Other than that this looks good, no further comments.

I’ll work on #34

Fleshed out the README, and copied it to a draft CR https://www.drupal.org/node/3444866 → .

I think a 'testing rules' section in the readme would be great, but left a @todo here, if @longwave or @alexpott can help

Added the 'testing rules' section too. No more todos.

Looks great to me!

Given that the custom phpstan rules won't be changing often, and they exist in isolation to Drupal, it seems weird that we would test them on every single ci run. It would make sense if there was a possibility that a change in Drupal could break the phpstan rule, but I don't think that's the case, is it?

You can disregard that last comment, i see the changes key in gitlab ci now.

Parenting

This looks great. Alex Pott pointing out somewhere that if we ran all the component unit tests like we're running the phpstan test, we wouldn't need this phpstan rule at all - bigger change but would be a good follow-up.

Committed/pushed to 11.x, thanks!

mondrake → closed merge request !6823