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 8.1 | Tokenizer/PHP: readonly vs union types bug fix #3513

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Dec 18, 2021

More follow up on #3480.

Done some more stress testing and found that the retokenization of | to T_TYPE_UNION was broken when combined with the readonly keyword.

Fixed now, includes test.

/cc @kukulich

Done some more stress testing and found that the retokenization of `|` to `T_TYPE_UNION` was broken when combined with the `readonly` keyword.

Fixed now, includes test.
@jrfnl jrfnl changed the title Tokenizer/PHP: readonly vs union types bug fix PHP 8.1 | Tokenizer/PHP: readonly vs union types bug fix Dec 18, 2021
@gsherwood gsherwood added this to the 3.7.0 milestone Dec 19, 2021
@gsherwood gsherwood merged commit a6bc7e4 into squizlabs:master Dec 20, 2021
@gsherwood
Copy link
Member

Thanks for fixing this

@jrfnl jrfnl deleted the feature/tokenizer-php-bugfix-readonly-union-types branch December 20, 2021 13:11
@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 20, 2021

@gsherwood I'm fairly certain I found one more case where T_READONLY should be added to a condition in the PHP Tokenizer class, but I haven't been able to come up with a failing test case for that one yet. Will keep an eye on it:

https://github.com/jrfnl/PHP_CodeSniffer/blob/d5c1cf755070447305e6e847db0299bc4e4c0190/src/Tokenizers/PHP.php#L1577

@gsherwood
Copy link
Member

but I haven't been able to come up with a failing test case for that one yet

I tried a few code snippets but couldn't come up with any failed cases either.

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.

2 participants