Fix tokenizer bug - Heredoc in if condition broke scope mapping#295
Draft
fredden wants to merge 2 commits intoPHPCSStandards:4.xfrom
Draft
Fix tokenizer bug - Heredoc in if condition broke scope mapping#295fredden wants to merge 2 commits intoPHPCSStandards:4.xfrom
if condition broke scope mapping#295fredden wants to merge 2 commits intoPHPCSStandards:4.xfrom
Conversation
jrfnl
reviewed
Feb 6, 2024
Member
jrfnl
left a comment
There was a problem hiding this comment.
Thanks for this PR @fredden !
I haven't done a proper review yet, but am wondering if squizlabs/PHP_CodeSniffer#3750 could be addressed as well as it seems like a related case ?
For now, I've just left a few small notes inline.
I would love to see more extensive tests for this method, preferably pulled & merged before this PR, but will accept this PR with the current tests as it's better than none anyway.
Member
Author
|
This code change introduces a bug where context conditions are no longer being added applied to the heredoc. More tests covering the existing behaviour are required to safeguard changes in the tokenizer. I'll mark this pull request as draft until we have better confidence in the tests for the tokenizer. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
I have been looking into #149. I started by creating a failing test which demonstrated the problem, and then worked on making that test pass. This pull request should fix the problem described in #149.
When the tokenizer creates its map of scopes, goes through each token in turn until it finds another scope opener. It then uses recursion to map each set of open/close parenthesises to the corresponding opener (eg,
ifstatement). The case of a heredoc embedded within an open/close pair of parenthesises was not handled. There are already special cases foruseandnamespace. I have added to this list so that heredocs are also acceptable in this context.PHP_CodeSniffer/src/Tokenizers/Tokenizer.php
Lines 1128 to 1144 in c4251a1
If the scope opener isn't handled, the code assumes that a closer will never be found, and that there must be a parse error. This isn't the case with a heredoc. Adding support for heredocs seems to make sense in this context.
PHP_CodeSniffer/src/Tokenizers/Tokenizer.php
Lines 1194 to 1195 in c4251a1
The tests here could be much more robust / extensive. I've opted for a simple test here, and will leave writing very verbose tests for #146 for now. If it would be preferable to add more tests here, please let me know.
Suggested changelog entry
ifconditions orfunctiondefinitions).Related issues/external references
Fixes #149
Types of changes
PR checklist