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

Update parser token constant values to ints instead of strings #197

Merged

Conversation

mukuru-shaun
Copy link
Contributor

I just came across a incompatibility with this package, and nikic/php-parser.

This package defines a few "parser token" constants here:

if (!defined('T_NAME_QUALIFIED')) {
define('T_NAME_QUALIFIED', 'T_NAME_QUALIFIED');
}
if (!defined('T_NAME_FULLY_QUALIFIED')) {
define('T_NAME_FULLY_QUALIFIED', 'T_NAME_FULLY_QUALIFIED');
}

nikic/php-parser does something similar (but perhaps more comprehensive?) here: https://github.com/nikic/PHP-Parser/blob/master/lib/PhpParser/compatibility_tokens.php
The nikic/php-parser defines a superset of the tokens defined in this package, with the key difference being they are given int values instead of string values.

This leads to an issue, where if the code in this package is loaded first, the few tokens that this package has defined will have string values, and break when being used with nikic/php-parser as they expect integer values.

The PHP documentation seems to indicate that user land defined parser tokens should indeed be int values: https://www.php.net/manual/en/tokens.php

@phil-davis
Copy link

This fixes my problem reported in nikic/PHP-Parser#974

I can change the values to:

if (!defined('T_NAME_QUALIFIED')) {
    define('T_NAME_QUALIFIED', 393);
}

if (!defined('T_NAME_FULLY_QUALIFIED')) {
    define('T_NAME_FULLY_QUALIFIED', 392);
}

to exactly match the ints used in https://github.com/nikic/PHP-Parser/blob/master/lib/PhpParser/Parser/Php7.php

Or I can use the values suggested in the PR:

if (!defined('T_NAME_QUALIFIED')) {
    define('T_NAME_QUALIFIED', 10001);
}

if (!defined('T_NAME_FULLY_QUALIFIED')) {
    define('T_NAME_FULLY_QUALIFIED', 10002);
}

Both work.

I suppose that the value just needs to a be int that is unique - 2 token names must not have the same value.

@jaapio
Copy link
Member

jaapio commented Jan 10, 2024

Thank you very much for this fix.

I understand the issue, and I agree we should solve this. However I'm in doubt, this is a breaking change for people using this library in the wrong way. It should not really matter as the values of the constants are changing between php versions. It feels like it's safer to use high values that are not used by php itself, as we will avoid conflicts in the future.

Nevertheless it feels like this will be come an issue for a huge number of users having php parser and this library as dependency in their list of dependencies.

Maybe we should just remove the fallback values as this library is not supporting php 7 anymore?

@jaapio jaapio merged commit 8430ca5 into phpDocumentor:1.x Jan 11, 2024
21 checks passed
@mukuru-shaun
Copy link
Contributor Author

Hi @jaapio , thanks so much for getting back to me.

It feels like it's safer to use high values that are not used by php itself, as we will avoid conflicts in the future

Using those high numbers is literally what the PHP documentation recommends, which is why I did that in this PR. I do think it could create an issue where another library sets one of those values to a different token it defines, then there could be edge case issues.
I think the way that php-parser sets the value by checking that they are all set and unique is probably the most comprehensive, but I thought it might be best to PR the simplest fix and we can go from there 😄

@jaapio
Copy link
Member

jaapio commented Jan 11, 2024

The pr is merged and a new version has been tagged. Thanks for this nice patch, I think we should concider dropping this forward compatibility right away when we stop support for php 7.4 which can be done in v2.

I will create an issue for that.

@phil-davis
Copy link

I just came across a incompatibility with this package, and nikic/php-parser

confirmed fixed for me - php-parser and type-resolver are working together. Thanks.

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

Successfully merging this pull request may close these issues.

3 participants