-
-
Notifications
You must be signed in to change notification settings - Fork 493
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
DB/PreparedSQLPlaceholders: use PHPCSUtils, allow for modern PHP and some bug fixes #2314
Merged
dingo-d
merged 21 commits into
develop
from
feature/db-preparedsqlplaceholders-review-phpcsutils-modern-php
Jul 23, 2023
Merged
DB/PreparedSQLPlaceholders: use PHPCSUtils, allow for modern PHP and some bug fixes #2314
dingo-d
merged 21 commits into
develop
from
feature/db-preparedsqlplaceholders-review-phpcsutils-modern-php
Jul 23, 2023
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
... to covered previously uncovered code. Note: some of the messages generated for these (mostly invalid) code samples are not always that clear, but the fact that the queries are being flagged is still correct.
These variables are conditionally set within the `for()` loop. Just declaring them here to be more explicit/complete with the variables in use within the loop.
This change is already covered by existing tests (test case file line 58 and 259).
In both these cases, a "quick check" for the target function call was done prior to walking the tokens, but the token walking with a negative search for `Tokens::$emptyToken` doesn't contain the risk of walking very far, so this quick check isn't really needed and removing it shouldn't impact performance.
… params [1] The `'raw'` key in the parameter arrays returned from the `PassedParameters` class contains - as per the name - the _raw_ contents of the parameter. Since PHPCSUtils 1.0.0-alpha4, the return array also contain a `'clean'` index, which contains the contents of the parameter cleaned of comments. By switching to using that key, a potential false positive gets fixed. Includes unit test demonstrating the issue and safeguarding the fix.
… params [2] The `'raw'` key in the parameter arrays returned from the `PassedParameters` class contains - as per the name - the _raw_ contents of the parameter. Since PHPCSUtils 1.0.0-alpha4, the return array also contain a `'clean'` index, which contains the contents of the parameter cleaned of comments. By switching to using that key, a potential false negative gets fixed. Includes unit test demonstrating the issue and safeguarding the fix.
… params [3] The `'raw'` key in the parameter arrays returned from the `PassedParameters` class contains - as per the name - the _raw_ contents of the parameter. Since PHPCSUtils 1.0.0-alpha4, the return array also contain a `'clean'` index, which contains the contents of the parameter cleaned of comments. By switching to using that key, a potential false positive gets fixed. Includes unit test demonstrating the issue and safeguarding the fix. Also includes unit tests with an unsupported placeholder in the `array_fill()`, which looks like it was so far untested.
When looking for a function calls to `implode()` or `array_fill()`, the sniff did not allow for fully qualified function calls, which would lead to false positives. Fixed now. Includes unit tests demonstrating the issue and safeguarding the fix.
As things were, the `$skip_to` param would lead to the first token _after_ the parenthesis closer also being skipped. This was not the intention and while - for valid code - this won't lead to false positives/negatives, it should still be fixed.
…ifierWithinIN Throw the error for `IdentifierWithinIN` error on the actual text string token in the parameter instead of on the function call to `implode()` to give a better indication where the actual error occurred. Safeguarded by adjusting one of the pre-existing tests.
…dDynamicPlaceholderGeneration Throw the error for `QuotedDynamicPlaceholderGeneration` error on the actual text string token which contains the open quote instead of on the function call to `implode()` to give a better indication where the actual error occurred. Safeguarded via the existing tests.
When an `implode()` function call is found in a `$query`, the sniff tries to find the preceding text string token to check for quotes around the placeholders. While it is unlikely that an `implode()` wouldn't be preceded by a text string token + concatenation token, it could still be possible. This updates the logic to do a negative search for the previous non-empty, non-concat token instead to prevent the sniff from walking too far. I've not included a test as I couldn't come up with a valid code snippet which would hit a false positive, but it should still be fixed IMO. :point_right: This commit will be easier to review while ignoring whitespace changes.
…n function calls This is already handled correctly by the PHPCSUtils `PassedParameters` class. This commit just adjusts some pre-existing tests to safeguard this for this sniff.
…perator [1] The nullsafe object operator when calling `$wpdb->prepare()` is handled correctly in the `WPDBTrait::is_wpdb_method_call()` method. Adjusted some pre-existing tests to safeguard this.
…perator [2] The sniff looks for variables in a text string, other than `$wpdb`. It doesn't actually look at whether object access is used after the variable. This just adjusts some pre-existing tests to use the nullsafe object operator on variables within the query to safeguard this behaviour against potential future regressions.
… [1] 1. Adjusted the way the correct parameters are retrieved from a call to `wpdb::prepare()` to use the new PHPCSUtils 1.0.0-alpha4 `PassedParameters::getParameterFromStack()` method. 2. Verified the parameter names used are in line with the name as per the WP 6.3 release. WP has been renaming parameters and is probably not done yet, but it doesn't look like those changes (so far) made it into changelog entries.... For the purposes of this exercise, I've taken the _current_ parameter name as the "truth" as support for named parameters hasn't officially been announced yet, so any renames _after_ this moment are the only ones relevant. Also note that `wpdb::prepare()` is a variadic function, which by its nature doesn't support named arguments, but that's not the concern of this sniff. Includes additional unit tests.
… [2] 1. Adjusted the way the correct parameters are retrieved from a call to `implode()` to use the new PHPCSUtils 1.0.0-alpha4 `PassedParameters::getParameterFromStack()` method. 2. The parameter names used for `implode()` are in line with the names as per the PHP 8.0 release. PHP itself renamed a lot of parameters in PHP 8.0. As named parameters did not exist before PHP 8.0, the parameter name as per PHP 8.0 (or above) are the only relevant names. Includes additional unit tests.
… [3] 1. Adjusted the way the correct parameters are retrieved from a call to `array_fill()` to use the PHPCSUtils `PassedParameters::getParameter()` method with named parameter support. 2. The parameter names used for `array_fill()` are in line with the names as per the PHP 8.0 release. PHP itself renamed a lot of parameters in PHP 8.0. As named parameters did not exist before PHP 8.0, the parameter name as per PHP 8.0 (or above) are the only relevant names. Includes additional unit tests.
… [4] `sprintf()` does not support named parameters due to its variadic nature and analyzing code which would use named parameters with `sprintf()` is too prone to incorrect sniff results. So instead of allowing for an incorrect call with named parameters, skip the whole `sprintf()` call if named parameters would be used. Includes additional unit test.
Just a test to safeguard this won't cause any false positives.
jrfnl
changed the title
Feature/db preparedsqlplaceholders review phpcsutils modern php
DB/PreparedSQLPlaceholders: use PHPCSUtils, allow for modern PHP and some bug fixes
Jul 22, 2023
19 tasks
GaryJones
approved these changes
Jul 23, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
dingo-d
approved these changes
Jul 23, 2023
dingo-d
deleted the
feature/db-preparedsqlplaceholders-review-phpcsutils-modern-php
branch
July 23, 2023 16:01
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.
DB/PreparedSQLPlaceholders: add extra tests
... to covered previously uncovered code.
Note: some of the messages generated for these (mostly invalid) code samples are not always that clear, but the fact that the queries are being flagged is still correct.
DB/PreparedSQLPlaceholders: make variables used explicit
These variables are conditionally set within the
for()
loop. Just declaring them here to be more explicit/complete with the variables in use within the loop.DB/PreparedSQLPlaceholders: implement PHPCSUtils
This change is already covered by existing tests (test case file line 58 and 259).
DB/PreparedSQLPlaceholders: minor simplification
In both these cases, a "quick check" for the target function call was done prior to walking the tokens, but the token walking with a negative search for
Tokens::$emptyToken
doesn't contain the risk of walking very far, so this quick check isn't really needed and removing it shouldn't impact performance.DB/PreparedSQLPlaceholders: prevent false positive due to comments in params [1]
The
'raw'
key in the parameter arrays returned from thePassedParameters
class contains - as per the name - the raw contents of the parameter.Since PHPCSUtils 1.0.0-alpha4, the return array also contain a
'clean'
index, which contains the contents of the parameter cleaned of comments.By switching to using that key, a potential false positive gets fixed.
Includes unit test demonstrating the issue and safeguarding the fix.
DB/PreparedSQLPlaceholders: prevent false negative due to comments in params [2]
The
'raw'
key in the parameter arrays returned from thePassedParameters
class contains - as per the name - the raw contents of the parameter.Since PHPCSUtils 1.0.0-alpha4, the return array also contain a
'clean'
index, which contains the contents of the parameter cleaned of comments.By switching to using that key, a potential false negative gets fixed.
Includes unit test demonstrating the issue and safeguarding the fix.
DB/PreparedSQLPlaceholders: prevent false positive due to comments in params [3]
The
'raw'
key in the parameter arrays returned from thePassedParameters
class contains - as per the name - the raw contents of the parameter.Since PHPCSUtils 1.0.0-alpha4, the return array also contain a
'clean'
index, which contains the contents of the parameter cleaned of comments.By switching to using that key, a potential false positive gets fixed.
Includes unit test demonstrating the issue and safeguarding the fix.
Also includes unit tests with an unsupported placeholder in the
array_fill()
, which looks like it was so far untested.DB/PreparedSQLPlaceholders: bug fix - allow for FQN function calls
When looking for a function calls to
implode()
orarray_fill()
, the sniff did not allow for fully qualified function calls, which would lead to false positives.Fixed now.
Includes unit tests demonstrating the issue and safeguarding the fix.
DB/PreparedSQLPlaceholders: bug fix - sniff could skip too much
As things were, the
$skip_to
param would lead to the first token after the parenthesis closer also being skipped. This was not the intention and while - for valid code - this won't lead to false positives/negatives, it should still be fixed.DB/PreparedSQLPlaceholders: improve error message precision for IdentifierWithinIN
Throw the error for
IdentifierWithinIN
error on the actual text string token in the parameter instead of on the function call toimplode()
to give a better indication where the actual error occurred.Safeguarded by adjusting one of the pre-existing tests.
DB/PreparedSQLPlaceholders: improve error message precision for QuotedDynamicPlaceholderGeneration
Throw the error for
QuotedDynamicPlaceholderGeneration
error on the actual text string token which contains the open quote instead of on the function call toimplode()
to give a better indication where the actual error occurred.Safeguarded via the existing tests.
DB/PreparedSQLPlaceholders: prevent token walking too far
When an
implode()
function call is found in a$query
, the sniff tries to find the preceding text string token to check for quotes around the placeholders.While it is unlikely that an
implode()
wouldn't be preceded by a text string token + concatenation token, it could still be possible.This updates the logic to do a negative search for the previous non-empty, non-concat token instead to prevent the sniff from walking too far.
I've not included a test as I couldn't come up with a valid code snippet which would hit a false positive, but it should still be fixed IMO.
👉 This commit will be easier to review while ignoring whitespace changes.
DB/PreparedSQLPlaceholders: add tests with PHP 7.3+ trailing commas in function calls
This is already handled correctly by the PHPCSUtils
PassedParameters
class.This commit just adjusts some pre-existing tests to safeguard this for this sniff.
DB/PreparedSQLPlaceholders: add tests with PHP 8.0+ nullsafe object operator [1]
The nullsafe object operator when calling
$wpdb->prepare()
is handled correctly in theWPDBTrait::is_wpdb_method_call()
method.Adjusted some pre-existing tests to safeguard this.
DB/PreparedSQLPlaceholders: add tests with PHP 8.0+ nullsafe object operator [2]
The sniff looks for variables in a text string, other than
$wpdb
. It doesn't actually look at whether object access is used after the variable.This just adjusts some pre-existing tests to use the nullsafe object operator on variables within the query to safeguard this behaviour against potential future regressions.
DB/PreparedSQLPlaceholders: add support for PHP 8.0+ named parameters [1]
wpdb::prepare()
to use the new PHPCSUtils 1.0.0-alpha4PassedParameters::getParameterFromStack()
method.WP has been renaming parameters and is probably not done yet, but it doesn't look like those changes (so far) made it into changelog entries....
For the purposes of this exercise, I've taken the current parameter name as the "truth" as support for named parameters hasn't officially been announced yet, so any renames after this moment are the only ones relevant.
Also note that
wpdb::prepare()
is a variadic function, which by its nature doesn't support named arguments, but that's not the concern of this sniff.Includes additional unit tests.
DB/PreparedSQLPlaceholders: add support for PHP 8.0+ named parameters [2]
implode()
to use the new PHPCSUtils 1.0.0-alpha4PassedParameters::getParameterFromStack()
method.implode()
are in line with the names as per the PHP 8.0 release.PHP itself renamed a lot of parameters in PHP 8.0. As named parameters did not exist before PHP 8.0, the parameter name as per PHP 8.0 (or above) are the only relevant names.
Includes additional unit tests.
DB/PreparedSQLPlaceholders: add support for PHP 8.0+ named parameters [3]
array_fill()
to use the PHPCSUtilsPassedParameters::getParameter()
method with named parameter support.array_fill()
are in line with the names as per the PHP 8.0 release.PHP itself renamed a lot of parameters in PHP 8.0. As named parameters did not exist before PHP 8.0, the parameter name as per PHP 8.0 (or above) are the only relevant names.
Includes additional unit tests.
DB/PreparedSQLPlaceholders: add support for PHP 8.0+ named parameters [4]
sprintf()
does not support named parameters due to its variadic nature and analyzing code which would use named parameters withsprintf()
is too prone to incorrect sniff results.So instead of allowing for an incorrect call with named parameters, skip the whole
sprintf()
call if named parameters would be used.Includes additional unit test.
DB/PreparedSQLPlaceholders: add tests with PHP 8.1+ first class callable
Just a test to safeguard this won't cause any false positives.
DB/PreparedSQLPlaceholders: minor documentation fix