-
-
Notifications
You must be signed in to change notification settings - Fork 85
Description
Follow up on #1216 which highlighted this problem.
The Problem
There is one place in the PHP tokenizer where (next) tokens are "blanked out" to be ignored by the tokenization loop:
PHP_CodeSniffer/src/Tokenizers/PHP.php
Line 902 in 379692d
$tokens[($stackPtr + 1)] = null; |
The tokenizer handles this correctly at the start of the loop:
PHP_CodeSniffer/src/Tokenizers/PHP.php
Lines 544 to 547 in 379692d
// Special case for tokens we have needed to blank out. | |
if ($tokens[$stackPtr] === null) { | |
continue; | |
} |
However, within the tokenizer loop a lot of the sub-routines create their own "mini-loops" to determine whether retokenization needs to happen and if so, what the retokenization should be.
Both subroutine loops walking forward, as well as backward, could "walk into" one of those blanked out null
tokens, though for backward walking loops this could possibly be avoided by walking back in the $finalTokens
array instead of the original $tokens
, though that may also have other implications.
The Task
Basically every single subroutine loop within the PHP::tokenize()
method needs to be reviewed for the above.
However, as the older parts of the Tokenizer don't have anywhere close to enough tests, it is fragile and the stability of the Tokenizer is crucial for sniffs to continue to function correctly.
So for every change to the Tokenizer, tests are required to be added to cover the change.
And (stretch-goal) adding a complete set of tests to cover the complete sub-routine being touched would be the best outcome, even when the conclusion of the investigation of that sub-routine would be that no changes are needed 😉