Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

php-parser v5 compat #8567

Open
staabm opened this issue Mar 19, 2024 · 14 comments
Open

php-parser v5 compat #8567

staabm opened this issue Mar 19, 2024 · 14 comments
Labels

Comments

@staabm
Copy link
Contributor

staabm commented Mar 19, 2024

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

@staabm staabm added the bug label Mar 19, 2024
@samsonasik
Copy link
Member

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
Copy link
Contributor Author

staabm commented Mar 19, 2024

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
Copy link
Member

it seems conflicts config can be solution 👍

@staabm
Copy link
Contributor Author

staabm commented Mar 19, 2024

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
Copy link
Member

samsonasik commented Mar 19, 2024

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

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

@staabm
Copy link
Contributor Author

staabm commented Mar 19, 2024

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
Copy link
Member

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

@staabm
Copy link
Contributor Author

staabm commented Mar 19, 2024

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
Copy link
Contributor Author

staabm commented Mar 19, 2024

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
Copy link
Contributor Author

staabm commented Mar 19, 2024

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
Copy link
Member

samsonasik commented Mar 19, 2024

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

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
Copy link
Contributor Author

staabm commented Mar 19, 2024

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

@sebastiaanluca
Copy link

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
Copy link
Contributor

See #8815

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants