php-parser v5 compat

[staabm]

Bug Report

Subject	Details
Rector version	e.g. v1.0.3

Minimal PHP Code Causing Issue

in a project which contains a dependency to nikic/parser, which leads to install parser v5, we run into a fatal error when in tandem rector is installed.

we see the following error when running a phpunit test-suite, while rector is installed as a dev dependency

PHP Fatal error:  Cannot declare interface PhpParser\Parser, because the name is already in use in /home/runner/work/daiber/daiber/vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/Parser.php on line 6
Script vendor/bin/phpunit handling the phpunit event returned with error code 255

IMO this is a conflict of the project level nikic/php-parser v5 and the in rector embedded nikic/php-parser v4.

Expected Behaviour

I think either the composer.json of rector/rector needs to declare a fixed dependency on nikic/php-parser v4
or rector needs to be compatible with nikic/php-parser v4 and v5 at the same time

[samsonasik]

rector can't support php-parser v5 yet, as rector rely on phpstan that still uses php-parser v4

https://github.com/phpstan/phpstan-src/blob/aedc04923ba337ec287edf00057aa4ab68a0fcf2/composer.json#L24

forcing it to support php-parser v5 will cause error:

Run php ../e2eTestRunner.php
PHP Fatal error:  Class PHPStan\Parser\PhpParserDecorator contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (PhpParser\Parser::getLexer) in phar:///home/runner/work/rector-src/rector-src/vendor/phpstan/phpstan/phpstan.phar/src/Parser/PhpParserDecorator.php on line 11
Fatal error: Class PHPStan\Parser\PhpParserDecorator contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (PhpParser\Parser::getLexer) in phar:///home/runner/work/rector-src/rector-src/vendor/phpstan/phpstan/phpstan.phar/src/Parser/PhpParserDecorator.php on line 11

see original effort for it:

• Try php-parser:dev-master (5-dev) rector-src#2916

[staabm]

as long as rector cannot work with nikic/php-parser v5 I think it should declare a nikic/php-praser v4 dependency in its composer.json (or a v5 conflicts rule).

that way composer will not install a incompatible v5

[samsonasik]

it seems conflicts config can be solution 👍

[staabm]

hmm thinking again about it.. shouldn't the rector release have a prefixed version of nikic/php-parser?

PHP Fatal error:  Cannot declare interface PhpParser\Parser, because the name is already in use in /home/runner/work/daiber/daiber/vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/Parser.php on line 6

the error message tells me that the rector release contains a unprefixed version?

[samsonasik]

it exists in https://github.com/rectorphp/rector/tree/main/vendor/nikic/php-parser and preloaded

• rector/preload.php

  Line 5 in 5ac9de7

  require_once __DIR__ . '/vendor/nikic/php-parser/lib/PhpParser/Node.php';

• rector/bin/rector.php

  Line 98 in 5ac9de7

  require_once __DIR__ . '/../preload.php';

• rector/src/Testing/PHPUnit/AbstractRectorTestCase.php

  Line 162 in 5ac9de7

  require_once __DIR__ . '/../../../preload.php';

Looking at discussion at #8566 (reply in thread), the issue seems due to symfony DebugClassLoader

[staabm]

we don't have the DebugClassLoader in our vendor/.
but you are right, that the description in #8566 sounds like the very same problem

is there a reason php-parser is not prefixed within rector?

[samsonasik]

if it prefixed, user can't create custom rector rule, as it always changed with RectorPrefixYM

[staabm]

I have a feeling that for some reason the preload.php of rector is included while phpunit is running.

and the hardcoded require statements lead to a cannot redeclare error, because the php-parser classes are already loaded by the project dependencies

[staabm]

ok, I got more information:

the preload.php is loaded via the following stacktrace

it is triggered because we have test-cases in our phpunit suite which extends AbstractRectorTestCase and these trigger the preload.php because of some phpunit before-test-hooks

[staabm]

maybe it would make sense to have something like

if (interface_exists('\PhpParser\Node')) {
  return;
}

at the very beginning of preload.php so the require calls are skipped?

---

different take at the problem: shouldn't the preload.php only be used, when rector is running via its own rector command, instead of beeing included by a phpunit binary?

the preload.php is a performance tool and should not be used from AbstractRectorTestCase IMO.

[samsonasik]

Not sure, that need to be tested carefully first in rector-* extension when running tests, as preload.php require phpstan's phpdoc-parser as well

rector/preload.php

Line 256 in 5ac9de7

require_once __DIR__ . '/vendor/phpstan/phpdoc-parser/src/Ast/Node.php';

iirc, without that, that was causing cross version that own phpstan inner phpdoc-parser replace it, make error without it.

[staabm]

continue the discusson from rectorphp/rector-src#5742 (comment)

... a conflict in composer.json with e.g. nikic/php-parser:5.* would lead to a nikic/php-parser:4.* at the project level, which still would trigger "cannot re-declare" errors as soon as a test is contained which extends AbstractRectorTestCase, because of the preload.php greedy autoloading.

I think this won't work.

---

another alternative: what about adding a replace: nikic/php-parser:4.* config in composer.json, which will tell composer that rector does contain a implementation of nikic/php-parser and composer would not need to install another version of it in parallel.

https://getcomposer.org/doc/04-schema.md#replace

I think the main question we need to ask ourselves: is the patched nikic/php-parser rector ships compatible with other tools out there which depend on nikic/php-parser