What is the proper way to maintain custom rectors in own project

[Wirone]

Question

What's the proper way for running tests for custom Rectors stored in our own application?

There is general info about writing own rules, there is mentioned file structure for rules and their tests but unfortunately it does not describe HOW to test such custom rectors. Since nikic/php-parser is part of Rector's own vendor directory I assume it should be used for that, to ensure Rector works properly with our rules. But I don't see how it should be done.

What I have

Looking at documentation and Rector itself I wanted to re-define testing environment based on Rector's, so I created:

utils/rector/phpunit.xml

<?xml version="1.0"?>
<phpunit
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xsi:noNamespaceSchemaLocation="../../vendor/phpunit/phpunit/phpunit.xsd"
    bootstrap="tests/bootstrap.php"
    colors="true"
    executionOrder="defects"
>
    <testsuites>
        <testsuite name="main">
            <directory>tests</directory>
        </testsuite>
    </testsuites>

    <php>
        <ini name="memory_limit" value="-1" />
        <env name="XDEBUG_MODE" value="coverage"/>
        <env name="KERNEL_CACHE_DIRECTORY" value="cache"/>
    </php>
</phpunit>

utils/rector/tests/bootstrap.php

<?php

declare(strict_types=1);

use Rector\Core\Stubs\PHPStanStubLoader;

require_once __DIR__ . '/../../../vendor/rector/rector/src/constants.php';

// make Rector's php-parser a priority to avoid conflict
require_once __DIR__ . '/../../../vendor/rector/rector/preload.php';
require_once __DIR__ . '/../../../vendor/rector/rector/vendor/autoload.php';
require_once __DIR__ . '/../../../vendor/autoload.php';

// silent deprecations, since we test them
error_reporting(E_ALL ^ E_DEPRECATED);

// performance boost
gc_disable();

$phpStanStubLoader = new PHPStanStubLoader();
$phpStanStubLoader->loadStubs();

Unfortunately when I run vendor/bin/phpunit -c utils/rector/phpunit.xml I get:

PHP Fatal error:  Uncaught Error: Class 'PhpParser\Builder\Declaration' not found in /app/vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/Builder/Namespace_.php:10
Stack trace:
#0 /app/vendor/rector/rector/preload.php(11): require_once()
#1 /app/utils/rector/tests/bootstrap.php(10): require_once('/app/...')
#2 /app/vendor/phpunit/phpunit/src/Util/FileLoader.php(66): include_once('/app/...')
#3 /app/vendor/phpunit/phpunit/src/Util/FileLoader.php(54): PHPUnit\Util\FileLoader::load('/app/...')
#4 /app/vendor/phpunit/phpunit/src/TextUI/Command.php(1126): PHPUnit\Util\FileLoader::checkAndLoad('/app/...')
#5 /app/vendor/phpunit/phpunit/src/TextUI/Command.php(922): PHPUnit\TextUI\Command->handleBootstrap('/app/...')
#6 /app/vendor/phpunit/phpunit in /app/vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/Builder/Namespace_.php on line 10

which is weird, because it looks like PhpParser\Builder\Declaration should be loaded through preload.php. I could debug it but I don't want to spend time on it since maybe there's a flow for that but I did not find it somehow.

Summary

There should be more detailed information about the flow for creating and testing custom rules inside own applications.

Thanks in advance!

[TomasVotruba]

Hi, thanks for great question!

At first, I would use normal phpunit.xml, not nested one (nesting expected paths configs often breaks other expected paths).

There could be many other sources for this bug. Could you share the repository in Github? I'll be able to check it locally and find the solution to help you :+1:

[TomasVotruba]

Btw, just a tip: did you know Github can higlight syntax only with explicit language? It makes code nice and more readable :slightly_smiling_face:

[Wirone]

@TomasVotruba here's the reproducer. Maybe I miss something but I could not be able to run those tests (but as I said, I did not spend much time on it).

There is PHPUnit ^8.5 because with ^9.5 there's another problem (Fatal error: Cannot declare interface PhpParser\NodeVisitor, because the name is already in use because nikic/php-parser is installed as PHPUnit's dependency AND as Rector's internal dependency in nested vendor directory). And we use 8.5 so it's closer to my actual runtime where I tried to configure it.

[TomasVotruba]

Thanks, your demo is very useful :+1:

It seems like bug in preload.php in class order. Included classes is missing a class that is included later. I'm investigating...

[TomasVotruba]

I've fixed 2 bugs, should be better now :)

Could you try 0.11.56? I've tested with PHPUnit 8 and 9 and both working

[TomasVotruba]

I've added few best practise updates: https://github.com/Wirone/rector-issue-6709-reproducer/pull/1/files :slightly_smiling_face:

[Wirone]

@TomasVotruba with 0.11.56 it works 🙂 From our actual app:

❯ vendor/bin/phpunit -c utils/rector/phpunit.xml
PHPUnit 8.5.20 by Sebastian Bergmann and contributors.

F                                                                   1 / 1 (100%)

Time: 1.93 seconds, Memory: 76.50 MB

There was 1 failure: (...)

Thank you, now I can work on custom rules that would improve our codebase 👍

PS. It would be good to include testing instructions to documentation.

[samsonasik]

@Wirone feel free to create PR to improve docs

[TomasVotruba]

I'm glad it works 😊

As @samsonasik refers, in my experience, the docs we write suffers author blondness. We did this so many times, the docs we would write would mislead you even more.

If you can propose PR with docs that would be helpful to you too read first, that would be awesome 😎

[Wirone]

@TomasVotruba FYI: the problem came back recently, our tests fail to execute.

Fatal error: Cannot declare interface PhpParser\NodeVisitor, because the name is already in use in /app/vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/NodeVisitor.php on line 6

• rector/rector: 0.12.23
• phpstan/phpstan: 1.6.4
• Composer 2.3.3

[TomasVotruba]

We're struggling with autoloading issues in combination with recent packages you've mentioned. It's rabit hole issue. Could you share minimal reproducer?

[Wirone]

@TomasVotruba I'll try, but I have queue of tasks after vacation and need some time for it. For now I've extracted tests for our custom rules to separate Gitlab CI job with allow_failure: true, since these are not critical and should not fail our pipeline.

[Wirone]

@TomasVotruba as a side question: why don't you prefix PHPStan too? You should probably require specific version that Rector’s code is compatible with, and prefix it like the other dependencies.. Is there a reason why you require phpstan/phpstan as a direct dependency?

[TomasVotruba]

Good question. At first year I recall we tried that to isolate it too. Then most projects had PHPStan, Rector and PHPStan again. With upcoming PHAR and custom it result in so much duplicated work, that was costly and without added value.

In the end the most projects will use both tools, so it make sense to include them just once. Also their symbiosis is stronger, when they're not wrapping each other with extra layers. That makes debugging and using less complex as well.

[Wirone]

But on the other hand it creates race condition problem because changes in PHPStan may lead to Rector errors, then Rector fixes may lead to PHPStan errors and so on... From my perspective PHPStan and Rector are responsible for totally different things. The fact, that Rector internally uses PHPStan is a technical detail that should be totally transparent for me as a consumer. For example, I want to be able to bump to PHPStan v2 without waiting for Rector to be compatible with it. So I don't think it's symbiosis, rather one-side dependency 🙂

Also it's only assumption that if project uses Rector, it uses PHPStan too. Not really, I see scenarios where Psalm is used for static analysis and Rector is included as another tool for refactoring and/or quality keeping. Or maybe SA is not used at all. So I don't think it's good think to require PHPStan on project level.

[TomasVotruba]

I think we're not talking on completely different things. I was explaning why we went with the symbosis, e.g. composer 1-dependency once way. From your comment I understand you suggest node_modlules duplicated-dependency trees way.

Yet, if you can come up better way ideally in working PR, I'm all ears :+1:

[Wirone]

FYI: it started working again with PHPStan 1.6.5 🪄 (CC: @ondrejmirtes) and works on 1.6.7 too.

I think we're not talking on completely different things. I was explaning why we went with the symbosis, e.g. composer 1-dependency once way. From your comment I understand you suggest node_modlules duplicated-dependency trees way.

Yet, if you can come up better way ideally in working PR, I'm all ears 👍

I just wanted to understand why all dependencies are prefixed and built-in into Rector and PHPStan is not. IMHO from Rector's perspective PHPStan is a direct dependency required for proper working and it should be prefixed and built-in too, in version supported at the moment of build. These tools should not be strictly connected because they have different responsibility and it should be possible to upgrade them separately (as I said before: upgrade major version of one of them without waiting for the support in the second one, like it was when PHPStan went in v1).

[ondrejmirtes]

I think the current situation where Rector has phpstan/phpstan in require works best. It would be a non-trivial task to prefix PHPStan, and I would definitely not be keen on supporting PHPStan distribution that was modified this way - there would definitely be new bugs because of that.

[Wirone]

Just FYI: we hit this problem again when upgrade of Codeception bumped nikic/php-parser to v5 in our app, and one of our custom rules started to fail on:

...array_map(
    fn (ClosureUse$use) => $this->determineVariableName($use),
    $functionLike instanceof Closure ? $functionLike->uses : []
),

with:

Parameter #1 $callback of function array_map expects (callable(PhpParser\Node\ClosureUse): mixed)|null, Closure(PhpParser\Node\Expr\ClosureUse): (string|null) given.

The problem here is: Rector bundles parser v4, where PhpParser\Node\Expr\Closure::$uses is a PhpParser\Node\Expr\ClosureUse[] which is actually available in our custom rules, while PHPStan analyses our code having parser v5 in our vendors, and it expects PhpParser\Node\ClosureUse[]. Even if there's an alias defined, it is seen as an error - both on SA level and when running tests (applying rule).

For now I have fn (ClosureUse|DeprecatedClosureUse $use) => $this->determineVariableName($use), + 2 imports (1 aliased), so it works and should work even if Rector bumps parser to v5. But it's a challenge to keep Rector in sync with our app's dependencies and situations like this may lead to many more problems (since v5 has a lot of changes).

Maybe it would be good to prefix bundled PHP parser with stable prefix, that would allow adding it to main autoloader and use in custom rules? Effectively you could have PHP parser under PhpParser\ namespace in your own vendor, and be able to load Parser from e.g. Rector\PhpParser\v4\ namespace pointing to Rector's vendors. This way custom rules would always use the same Parser API that is actually used by Rector?

I don't know if it's a good direction, just thinking out loud. But the problem is real 🙂.

PS. Sorry @TomasVotruba for spoiling your Twitter timeline. Wish you fun and grow there 👍.