BadFunctions/EasyRFI: add unit tests, includes various bug fixes #72
Conversation
This file contains 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
jrfnl
force-pushed
the
feature/easyrfi-add-unit-tests
branch
2 times, most recently
from
00176a2
to
e12a51b
Compare
As the tokens to search for don't change during a PHPCS run, it is inefficient to use the "expensive" `array_merge()` function 1) within a `while` loop and 2) every time the sniff is triggered by an include/require token. The set of tokens to search for can just as easily be set only once before the sniff is ever triggered and doing so will make the sniff faster.
The PHPCS [`addError()`](https://pear.php.net/package/PHP_CodeSniffer/docs/3.5.4/apidoc/PHP_CodeSniffer/File.html#methodaddError) and [`addWarning()`](https://pear.php.net/package/PHP_CodeSniffer/docs/3.5.4/apidoc/PHP_CodeSniffer/File.html#methodaddWarning) functions have a build-in string replacement `sprintf()`-like functionality, so let's use it.
jrfnl
force-pushed
the
feature/easyrfi-add-unit-tests
branch
from
e12a51b
to
3f2c8c2
Compare
The PHPCS native `PHP_CodeSniffer\Util\Tokens` class contains a number of useful token groups. For this particular sniff, the `Tokens::$includeTokens` applies and contains all the relevant tokens. It is generally a good idea to use the build-in token groups, as when something would change in how PHP and/or PHPCS tokenizes certain constructs, those groups will be updated too.
…tement [1] Prevent false negatives when an `include`/`require` statement combines parentheses with concatentation outside parentheses. Checking whether the next non-empty token is an open parenthesis could cause false negatives as there may be additional paths of the path _after_ the parenthesized part of the statement. The only reason why this wasn't a problem up to now is because of a bug in the sniff determining `$s`. The `findNext()` function searches in the token stack and treats the `$start` parameter as _inclusive_. That means that when searching for the next non-empty token, passing the `$stackPtr` to an include/require statement would _always_ return the `$stackPtr` and not the next non-empty token _after_ the `$stackPtr` which could have been an open parenthesis (or not). Taking the logic related to the parentheses out of the sniff prevents this issue.
…tement [2] PHPCS can be run from within an IDE during live coding. Similarly PHPCS can be run over files containing parse errors. With that in mind, it is best practice to bow out in those cases. Parse error detection should catch those errors. That is not the responsibility of this sniff.
…tement [3] An `include`/`require` statement can be used within template files to include another template and doesn't need a closing semi-colon in that case. That situation was so far not being considered by the sniff and the sniff could in that case search way to far and report on completely unrelated statements after the include.
Putting the `findNext()` in the `while` condition allows to simplify the `if` conditions within the loop.
The only token which can have a `content` of `.` is the `T_STRING_CONCAT` token, so we may as well exclude it from being found.
jrfnl
force-pushed
the
feature/easyrfi-add-unit-tests
branch
from
3f2c8c2
to
d4c9292
Compare
|
@jmarcil Anything I can do to move this (and the other open PRs) forward ? |
|
Anything I can do to move this PR forward ? |
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.
Related to #57, follow up on #70, this PR adds unit tests for the
Security.BadFunctions.EasyRFIsniff.This includes a review of the sniff and making various fixes to it to:
Commit Summary
BadFunctions/EasyRFI: add unit tests
BadFunctions/EasyRFI: efficiency fix
As the tokens to search for don't change during a PHPCS run, it is inefficient to use the "expensive"
array_merge()function 1) within awhileloop and 2) every time the sniff is triggered by an include/require token.The set of tokens to search for can just as easily be set only once before the sniff is ever triggered and doing so will make the sniff faster.
BadFunctions/EasyRFI: use the build-in PHPCS functionality [1]
The PHPCS
addError()andaddWarning()functions have a build-in string replacementsprintf()-like functionality, so let's use it.BadFunctions/EasyRFI: use the build-in PHPCS functionality [2]
The PHPCS native
PHP_CodeSniffer\Util\Tokensclass contains a number of useful token groups.For this particular sniff, the
Tokens::$includeTokensapplies and contains all the relevant tokens.It is generally a good idea to use the build-in token groups, as when something would change in how PHP and/or PHPCS tokenizes certain constructs, those groups will be updated too.
BadFunctions/EasyRFI: bug fix - fix detecting of start/end of the statement [1]
Prevent false negatives when an
include/requirestatement combines parentheses with concatenation outside parentheses.Checking whether the next non-empty token is an open parenthesis could cause false negatives as there may be additional parts of the path after the parenthesized part of the statement.
The only reason why this wasn't a problem up to now is because of a bug in the sniff determining
$s.The
findNext()function searches in the token stack and treats the$startparameter as inclusive.That means that when searching for the next non-empty token, passing the
$stackPtrto an include/require statement would always return the$stackPtrand not the next non-empty token after the$stackPtrwhich could have been an open parenthesis (or not).Taking the logic related to the parentheses out of the sniff prevents this issue.
BadFunctions/EasyRFI: bug fix - fix detecting of start/end of the statement [2]
PHPCS can be run from within an IDE during live coding. Similarly PHPCS can be run over files containing parse errors.
With that in mind, it is best practice to bow out in those cases.
Parse error detection should catch those errors. That is not the responsibility of this sniff.
BadFunctions/EasyRFI: bug fix - fix detecting of start/end of the statement [3]
An
include/requirestatement can be used within template files to include another template and doesn't need a closing semi-colon in that case.That situation was so far not being considered by the sniff and the sniff could in that case search way too far and report on completely unrelated statements after the include.
BadFunctions/EasyRFI: minor code simplification [1]
Putting the
findNext()in thewhilecondition allows to simplify theifconditions within the loop.BadFunctions/EasyRFI: minor code simplification [2]
The only token which can have a
contentof.is theT_STRING_CONCATtoken, so we may as well exclude it from being found.