search

rectorphp / rector

Instant Upgrades and Automated Refactoring of any PHP 5.3+ code
https://getrector.com
MIT License
8.61k stars 680 forks source link

php-parser v5 compat #8567

Open staabm opened 5 months ago

staabm commented 5 months ago

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 commented 5 months ago

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:

staabm commented 5 months ago

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 commented 5 months ago

it seems conflicts config can be solution 👍

staabm commented 5 months ago

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 commented 5 months ago

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

Looking at discussion at https://github.com/rectorphp/rector/discussions/8566#discussioncomment-8842801, the issue seems due to symfony DebugClassLoader

staabm commented 5 months ago

we don't have the DebugClassLoader in our vendor/. but you are right, that the description in https://github.com/rectorphp/rector/discussions/8566 sounds like the very same problem

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

samsonasik commented 5 months ago

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

staabm commented 5 months ago

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 commented 5 months ago

ok, I got more information:

the preload.php is loaded via the following stacktrace

grafik

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 commented 5 months ago

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 commented 5 months ago

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

https://github.com/rectorphp/rector/blob/5ac9de7a83c63cf26327c6a24e3b1cef9797dabd/preload.php#L256

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

staabm commented 5 months ago

continue the discusson from https://github.com/rectorphp/rector-src/pull/5742#discussion_r1530735959

... 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

sebastiaanluca commented 1 month ago

Hi, just chiming in here. We're using a few packages (PHPUnit 11, Spatie typescript transformer) that rely on nikic/php-parser v5. All was fine until a few days ago where we started getting this error:

In ParserAbstract.php line 979:

  [Error]
  Undefined constant PhpParser\Node\Expr\List_::KIND_ARRAY

Took some digging to find the issue, but I just found out that Rector includes its own vendor dir with fixed package versions? 😨 The included nikic/php-parser is autoloaded before the one we have installed as part of composer install, which breaks our tests and multiple other packages.

Why does Rector include its own vendor dir instead of specifying the requirements in its own composer.json and relying on Composer to sort them out (including conflicts)? And in the mean time, how can we work around this besides rm -rf vendor/rector/rector/vendor/nikic in a Composer script?

ondrejmirtes commented 1 week ago

See https://github.com/rectorphp/rector/issues/8815