TokenPolyfill issue on PHP 7.4 platform

[llaville]

Hello,

I've worked today to make a package compatible with both PHP-Parser 4.18 and 5.0

I've detected an issue on (I know old platform PHP 7.4, but this package is still compatible PHP 7). Here are the details of my analysis

When PHP-Parser 5.0.0 is installed with PHP_CodeSniffer (3.9.0 in my situation), I got a TypeError

TypeError: Argument 1 passed to PhpParser\Internal\TokenPolyfill::__construct() must be of the type int, string given, called in /shared/backups/forks/symbol-parser/vendor/nikic/php-parser/lib/PhpParser/Internal/TokenPolyfill.php on line 202

/shared/backups/forks/symbol-parser/vendor/nikic/php-parser/lib/PhpParser/Internal/TokenPolyfill.php:42
/shared/backups/forks/symbol-parser/vendor/nikic/php-parser/lib/PhpParser/Internal/TokenPolyfill.php:202
/shared/backups/forks/symbol-parser/vendor/nikic/php-parser/lib/PhpParser/Lexer.php:31
/shared/backups/forks/symbol-parser/vendor/nikic/php-parser/lib/PhpParser/Lexer/Emulative.php:71
/shared/backups/forks/symbol-parser/vendor/nikic/php-parser/lib/PhpParser/ParserAbstract.php:182
/shared/backups/forks/symbol-parser/src/Parser/PHP/SymbolNameParser.php:41
/shared/backups/forks/symbol-parser/tests/Integration/Parser/PHP/SymbolNameParserTest.php:42

Version 5.0 is supposed to be compatible with PHP 7.4 min (see https://github.com/nikic/PHP-Parser/blob/v5.0.0/composer.json#L16-L17)

But :

• T_NAME_QUALIFIED https://github.com/nikic/PHP-Parser/blob/v5.0.0/lib/PhpParser/Internal/TokenPolyfill.php#L200 was introduced on PHP 8.0.0beta1
• T_NAME_RELATIVE https://github.com/nikic/PHP-Parser/blob/v5.0.0/lib/PhpParser/Internal/TokenPolyfill.php#L198 was introduced on PHP 8.0.0beta1
• T_NAME_FULLY_QUALIFIED https://github.com/nikic/PHP-Parser/blob/v5.0.0/lib/PhpParser/Internal/TokenPolyfill.php#L196 was introduced on PHP 8.0.0beta1

Reason is PHPCS define these tokens as string.
And TokenPolyfill first argument of class constructor is int

Hope my explains are enough clear ?

[staabm]

there is a related non-released commit on master: ff095c3

maybe this already "fixes" your problem?

also related: #974

[staabm]

@llaville I have opened a issue on the PHPCS repo. maybe you can give more information about your use-case there, so jrfnl can decide whether there is something todo or not.

I just wanted to raise awareness and cannot add more than that.

[llaville]

@staabm Thanks but I don't think it come from PHPCS. I've investigated by debugging code analysed with TokenPolyfill, and there is an issue inside !

[nikic]

I've implemented a check to detect broken libraries like this a while ago (ff095c3) but never released it. It is now released and will hard error early on.

[llaville]

@nikic FYI

BTW

I've put some debugging steps on TokenPolyfill

                    if ($j > $i + 1) {
                        \error_log('@AAA ' . var_export([$i, $j, \T_NS_SEPARATOR, \T_NAME_FULLY_QUALIFIED, \T_NAMESPACE, \T_NAME_RELATIVE, \T_NAME_QUALIFIED], true), 3, '/var/log/php/php-parser.log');
                        if ($id === \T_NS_SEPARATOR) {
                            $id = \T_NAME_FULLY_QUALIFIED;
                        } elseif ($id === \T_NAMESPACE) {
                            $id = \T_NAME_RELATIVE;
                        } else {
                            $id = \T_NAME_QUALIFIED;
                        }
                        if (!\is_int($id)) {
                            \error_log('@BBB ' . var_export(debug_backtrace(), true), 3, '/var/log/php/php-parser.log');
                        }

Code Analysed was

        $code = <<<CODE
        <?php
        declare(strict_types=1);

        namespace Test\Sub;

        class TestClass {
            public function test() {}
        }
        CODE;

When namespace is found

@AAA gave us
[
	0 => 11, 
	1 => 14, 
	2 => 393, 
	3 => 'T_NAME_FULLY_QUALIFIED', 
	4 => 391, 
	5 => -1, 
	6 => 'T_NAME_QUALIFIED'
]

[llaville]

Confirmed by running package unit tests with PHP-Parser 5.0.1

1) ComposerUnused\SymbolParser\Test\Integration\Parser\PHP\SymbolNameParserTest::itShouldParseClasses
Error: Token T_NAME_QUALIFIED has ID of type string, should be int. You may be using a library with broken token emulation

/shared/backups/forks/symbol-parser/vendor/nikic/php-parser/lib/PhpParser/compatibility_tokens.php:30
/shared/backups/forks/symbol-parser/vendor/nikic/php-parser/lib/PhpParser/compatibility_tokens.php:62
/shared/backups/forks/symbol-parser/vendor/nikic/php-parser/lib/PhpParser/Lexer.php:5
/shared/backups/forks/symbol-parser/vendor/composer/ClassLoader.php:578
/shared/backups/forks/symbol-parser/vendor/composer/ClassLoader.php:432
/shared/backups/forks/symbol-parser/vendor/nikic/php-parser/lib/PhpParser/Lexer/Emulative.php:24
/shared/backups/forks/symbol-parser/vendor/composer/ClassLoader.php:578
/shared/backups/forks/symbol-parser/vendor/composer/ClassLoader.php:432
/shared/backups/forks/symbol-parser/vendor/nikic/php-parser/lib/PhpParser/ParserFactory.php:18
/shared/backups/forks/symbol-parser/vendor/nikic/php-parser/lib/PhpParser/ParserFactory.php:32
/shared/backups/forks/symbol-parser/tests/Integration/Parser/PHP/SymbolNameParserTest.php:25

2) ComposerUnused\SymbolParser\Test\Integration\Parser\PHP\SymbolNameParserTest::itShouldParseInterfaces
Error: Undefined constant 'T_NAME_RELATIVE'

/shared/backups/forks/symbol-parser/vendor/nikic/php-parser/lib/PhpParser/ParserAbstract.php:1219
/shared/backups/forks/symbol-parser/vendor/nikic/php-parser/lib/PhpParser/ParserAbstract.php:159
/shared/backups/forks/symbol-parser/vendor/nikic/php-parser/lib/PhpParser/ParserFactory.php:21
/shared/backups/forks/symbol-parser/vendor/nikic/php-parser/lib/PhpParser/ParserFactory.php:32
/shared/backups/forks/symbol-parser/tests/Integration/Parser/PHP/SymbolNameParserTest.php:53

[jrfnl]

I've implemented a check to detect broken libraries like this a while ago (ff095c3) but never released it. It is now released and will hard error early on.

But does that solve the problem ? As far as I can see, that just moves the problem around and doesn't solve anything...

[llaville]

Answer is into official documentation https://github.com/nikic/PHP-Parser/blob/master/doc/component/Lexer.markdown

We can read

So no solution on my use case

[nikic]

@llaville This refers to something completely unrelated. That point is exactly the same on PHP-Parser 4.0.

The problem here is that PHP-Parser and PHP_CodeSniffer are not compatible with each other. You currently cannot use both at the same time.

[llaville]

But the issue still exists even if I removed PHP_CodeSniffer from context !

[nikic]

@jrfnl It doesn't solve the problem, but it provides a clear error message. It's impossible to "solve" this problem in PHP-Parser, because the requirement for integer token IDs is imposed by PHP itself. It's something I could avoid in my own TokenPolyfill on older versions, but on PHP 8 the native PhpToken is used, and it requires integer token IDs as well.

The only way I can avoid this requirement is by not using the global token constants at all, and instead redefining all token constants myself. I considered doing this (specifically to avoid this kind of problem), but opted not to do so, because I thought there was more value in sticking with PHP's own token format. In any case, it's not something I can change anymore.

[llaville]

I've just found something strange. Need to investigate more again.

On my fork version of composer-unused/symbol-parser (https://github.com/llaville/symbol-parser/tree/php-parser-v5) where I've implemented PHP-Parser v5 compatibility, I run unit tests with PHAR version of PHPUnit 9.6 that raise me errors.

But when I run same tests with phpunit installed as a dev dependency, I got no errors.

[nikic]

That's interesting. I've been trying and failing to reproduce this problem with the dev dependency. With the phar file I see it as well.

[llaville]

That's interesting. I've been trying and failing to reproduce this problem with the dev dependency. With the phar file I see it as well.

Just to avoid to put it on this thread, I've publish my PHPUnit tests comparaison on my PR at composer-unused/symbol-parser#133 (comment)

[nikic]

Ah, it looks like the actual problem is phpdocumentor-type-resolver/Types/ContextFactory.php in the phpunit phar.

[nikic]

The phar contains version 1.6.1, while https://github.com/phpDocumentor/TypeResolver/releases/tag/1.7.4 is the version with php-parser 5 compatibility.

[llaville]

Good catch @nikic :)
That will also explains why GitHub Action failed too
https://github.com/composer-unused/symbol-parser/actions/runs/7990344120/job/21818955371

(because it use the PHPUnit phar copy : https://github.com/shivammathur/setup-php/blob/main/dist/index.js#L969)

[jrfnl]

@jrfnl It doesn't solve the problem, but it provides a clear error message. It's impossible to "solve" this problem in PHP-Parser, because the requirement for integer token IDs is imposed by PHP itself. It's something I could avoid in my own TokenPolyfill on older versions, but on PHP 8 the native PhpToken is used, and it requires integer token IDs as well.

The only way I can avoid this requirement is by not using the global token constants at all, and instead redefining all token constants myself. I considered doing this (specifically to avoid this kind of problem), but opted not to do so, because I thought there was more value in sticking with PHP's own token format. In any case, it's not something I can change anymore.

Nor I on the PHP_CodeSniffer side as PHPCS has used the string value token polyfills since its early days (~2006) and changing that now would be a breaking change and considering how many external standards there are, not one I really want to contemplate.

Historical context: as the actual value of the PHP native token constants changes depending on the PHP version, polyfilling to the real value was not an option, so at the time (well before my time), the choice was made to polyfill to string values which would never get into a conflict with the PHP native token values.
This was, of course, long before the PhpToken object existed and long before the docs contained the suggestion to use an integer value of 10.000 or higher.

@nikic @llaville Regarding use in combination with PHPUnit - also see sebastianbergmann/php-code-coverage#1025

[jrfnl]

On my fork version of composer-unused/symbol-parser (https://github.com/llaville/symbol-parser/tree/php-parser-v5) where I've implemented PHP-Parser v5 compatibility, I run unit tests with PHAR version of PHPUnit 9.6 that raise me errors.

But when I run same tests with phpunit installed as a dev dependency, I got no errors.

Might depend on which PHPUnit (PHAR) version is being used. The PHARs will have a fixed version of phpDocumenter/TypeResolver included, while Composer will install the latest version of phpDocumenter/TypeResolver in which the token values were changed. See: https://github.com/phpDocumentor/TypeResolver/releases/tag/1.7.4

[nikic]

Nor I on the PHP_CodeSniffer side as PHPCS has used the string value token polyfills since its early days (~2006) and changing that now would be a breaking change and considering how many external standards there are, not one I really want to contemplate.

It's not really obvious to me what backwards compatibility break you are concerned about here: These tokens will already be defined with integer values on newer PHP version, so code already has to handle that just fine. Unless people are using is_string(T_NAME_QUALIFIED) as a particular inventive way to check the PHP version they're running on?

But in any case, it doesn't look like the usage in PHP_CodeSniffer is problematic in practice anyway, as it shouldn't be used at the same time as PHP-Parser. The PHP_CodeSniffer mention here was a red herring.

[jrfnl]

It's not really obvious to me what backwards compatibility break you are concerned about here: These tokens will already be defined with integer values on newer PHP version, so code already has to handle that just fine. Unless people are using is_string(T_NAME_QUALIFIED) as a particular inventive way to check the PHP version they're running on?

Considering the inventive code I've seen over the years, I wouldn't put it passed people...

But in any case, it doesn't look like the usage in PHP_CodeSniffer is problematic in practice anyway, as it shouldn't be used at the same time as PHP-Parser. The PHP_CodeSniffer mention here was a red herring.

Agreed. As long as neither PHPCS nor PHP-Parser has an autoload - files directive which loads the token polyfills (and AFAIK neither do), there shouldn't be a problem.

[llaville]

Might depend on which PHPUnit (PHAR) version is being used. The PHARs will have a fixed version of phpDocumenter/TypeResolver included, while Composer will install the latest version of phpDocumenter/TypeResolver in which the token values were changed. See: https://github.com/phpDocumentor/TypeResolver/releases/tag/1.7.4

Agree, as Nikita already said. But as I used the latest version 9.6.16 of PHPUnit (that support PHP 7.4) which include only version phpdocumentor/type-resolver: 1.6.1 (see phpunit.phar --manifest)

[nikic]

I filed sebastianbergmann/phpunit#5712 to check whether it's possible to update the dependency in the phar. But for your purposes, just switching to use vendor/bin/phpunit is probably the best fix for now :)

[llaville]

@nikic Already on way ;-) composer-unused/symbol-parser#133 (comment)

PS: Solved (see https://github.com/composer-unused/symbol-parser/actions/runs/7996055440)

[llaville]

Good night @nikic / @jrfnl
Thanks for this thread that help me (at least) to learn a lot