-
-
Notifications
You must be signed in to change notification settings - Fork 479
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
Added Sniff for the suppress_filters in get_posts() args. #1041
Conversation
Thank you for this PR and for your willingness to contribute to WPCS. FYI: While someone might review your sniff, it will not be merged for a little while yet while we are working on making WPCS compatible with PHPCS 3.x. Ref: #1040 |
@jrfnl Please do let me know whenever any changes are required on this PR. |
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.
Hi Govind, I've had a quick look through and left some initial feedback. Hope it helps.
Aside from the inline remarks: where are the unit tests for this new sniff ?
Let me know if you need some guidance on how to continue.
@@ -158,7 +158,7 @@ public function getGroups() { | |||
), | |||
|
|||
// @todo Introduce a sniff specific to get_posts() that checks for suppress_filters=>false being supplied. | |||
'get_posts' => array( | |||
/*'get_posts' => array( |
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.
If you are truly replacing this whole block with an alternative check, it should be removed, not commented out.
The change should also be annotated in the class changelog at the top of the file and the related unit tests should be (re-)moved from the Tests/VIP/RestrictedFunctionsUnitTest.*
files.
*/ | ||
|
||
/** | ||
* Suppress Filters need to be 'true' when getting posts using get_posts. |
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.
The original check checked this for get_posts()
, wp_get_recent_posts()
and get_children()
.
* | ||
* @package WPCS\WordPressCodingStandards | ||
* | ||
* @since 0.3.0 |
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.
This would become 0.14.0.
You may also want to annotate that this check originally came from the VIP/RestrictedFunctionsSniff
.
* | ||
* @since 0.3.0 | ||
*/ | ||
class WordPress_Sniffs_VIP_SuppressFiltersSniff extends WordPress_AbstractArrayAssignmentRestrictionsSniff { |
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.
Why are you using the WordPress_AbstractArrayAssignmentRestrictionsSniff
for this ?
I'm wondering if there are other functions (WP native or userland) which potentially use suppress_filters
in an array context which would be triggered by this sniff or even just completely unrelated functionality which uses an array key of suppress_filters
.
The sniff as it currently is, doesn't check that the array is used in combination with any of the above mentioned functions and IMHO it should or rather, the function call should be leading with the array value the secondary check.
This might mean that, if a variable is passed to the functions, you need to do some token walking within the scope in which the function is called to see if you can find the variable definition which may or may not contain suppress_filters
.
I suggest you have a look at the WordPress_AbstractFunctionParameterRestrictionsSniff
as a potential alternative starting point.
* This should be overridden in extending classes. | ||
* | ||
* Example: groups => array( | ||
* 'wpdb' => array( |
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.
This comment describes the group format for another abstract sniff, not this one.
*/ | ||
public function callback( $key, $val, $line, $group ) { | ||
$key = strtolower( $key ); | ||
if ( ( 'suppress_filters' === $key && ( 'true' === $val || 1 === $val ) ) ) { |
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.
- There is no need to
strtolower()
the key or to check its value as there is only one key supplied viagetGroups()
, so the callback will only be called if that key is encountered. - The way the check is done now, is not in line with the message being thrown. According to the error message, the only valid value for
suppress_filters
would befalse
and thesuppress_filters
array key must be supplied, everything else should trigger the error message.
Currently, code like this$a['suppress_filters'] = $toggle;
would pass without problem.
Similarly, a function call toget_posts()
withoutsuppress_filters
being set would also pass. get_posts()
is now hard-coded into the error message while this message should/could also be thrown for the other two functions.
@jrfnl I have made changes according to your above comments and pushed the commits along with the test unit files. Please check and do let me know if any changes are required. 👍 |
@@ -8,61 +8,203 @@ | |||
*/ | |||
|
|||
/** | |||
* Suppress Filters need to be 'true' when getting posts using get_posts. | |||
* Support_filters must be false when using get_posts(), wp_get_recent_posts(), get_children(). |
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.
This docs line needs checking.
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.
@GaryJones thanks for your concern. yes, it's still incomplete, I need some guidance on this, like what it should be?
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.
Well, you've switched from Suppress Filters need to be 'true'...' to
Support_filters must be false...`. Even if the change in boolean is intentional, the first word is not accurate as far as I can tell.
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.
I've replaced that first doc comment because it was wrong. if suppress_filters
value is false then it's fine otherwise in case it didn't pass or it is true then the warning will occur. so the doc string can be.
Flag calling get_posts(), wp_get_recent_posts() and get_children() without suppress_filters=>false.
OR
Checks for suppress_filters=>false being supplied in get_posts(), wp_get_recent_posts() and get_children().
Please, correct.
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.
Checks for suppress_filters=>false being supplied in get_posts(), wp_get_recent_posts() and get_children().
This reads better in English.
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.
Thanks, Fixed.
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.
@emgk Could you update the sniff to use namespaces please ? See inline and ticket #1047 for more information and examples.
Also: could you have a look at the code style violations & fix those ?
https://travis-ci.org/WordPress-Coding-Standards/WordPress-Coding-Standards/jobs/257123137#L553
I'll have a more detailed look once those things are fixed.
* @link https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards | ||
* @license https://opensource.org/licenses/MIT MIT | ||
*/ | ||
|
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.
namespace WordPress\Tests\VIP;
use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;
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.
Added
* @package WPCS\WordPressCodingStandards | ||
* @since 0.14.0 | ||
*/ | ||
class WordPress_Tests_VIP_SuppressFiltersTest extends AbstractSniffUnitTest { |
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.
class SuppressFiltersUnitTest extends AbstractSniffUnitTest {
* @link https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards | ||
* @license https://opensource.org/licenses/MIT MIT | ||
*/ | ||
|
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.
namespace WordPress\Sniffs\VIP;
use WordPress\AbstractFunctionParameterSniff;
use PHP_CodeSniffer_Tokens as Tokens;
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.
Added
* | ||
* @since 0.14.0 | ||
*/ | ||
class WordPress_Sniffs_VIP_SuppressFiltersSniff extends WordPress_AbstractFunctionParameterSniff { |
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.
class SuppressFiltersSniff extends AbstractFunctionParameterSniff {
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.
Replaced.
$arrayValue = ''; | ||
|
||
// Retrieve the value parameter's details. | ||
$argumentData = $this->phpcsFile->findNext( PHP_CodeSniffer_Tokens::$emptyTokens, $parameters[1]['start'], ( $parameters[1]['end'] + 1 ), true ); |
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.
PHP_CodeSniffer_Tokens
=> Tokens
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.
Replaced
// Retrieve the value parameter's details. | ||
$argumentData = $this->phpcsFile->findNext( PHP_CodeSniffer_Tokens::$emptyTokens, $parameters[1]['start'], ( $parameters[1]['end'] + 1 ), true ); | ||
|
||
/** |
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.
Here and in a few other places: /**
=> /*
This is not a docblock, but a multi-line inline comment ;-)
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.
Changed. to => //
@jrfnl I have started working on these changes. |
…get_children functions.
fe08bf8
to
b3d04e1
Compare
@jrfnl I have updated this PR with the current changes made in |
@emgk Govind, please have a look at the unit tests which are failing: https://travis-ci.org/WordPress-Coding-Standards/WordPress-Coding-Standards/builds/258087412 |
@jrfnl I am working on this. |
@jrfnl could you please check ? |
Now I've finished work on #1208, I'm finally getting back to this sniff. Sorry for the delay @emgk To start with: could you please rebase the PR on the current
I was also wondering if you could replace the On the page the link points to now, I couldn't find any information about |
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.
@emgk Hi Govind, I've gone through it again and still have quite some feedback. We're getting somewhere, but we're not there yet.
*/ | ||
public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) { | ||
|
||
if ( false === $this->target_functions[ $matched_content ] ) { |
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.
This condition does nothing, so why is it here ?
- This method won't be called unless one of the target functions has been matched.
- None of the target functions is set to a value of
false
.
/** | ||
* Checks for suppress_filters=>false being supplied in get_posts(), wp_get_recent_posts() and get_children(). | ||
* | ||
* @link https://vip.wordpress.com/documentation/vip/code-review-what-we-look-for/ |
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.
Please update this link to one pointing to information about the actual rule.
* | ||
* @package WPCS\WordPressCodingStandards | ||
* | ||
* @since 0.14.0 |
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.
Please add something along the lines of "This check - in a more limited form - was originally contained in the WordPress.VIP.RestrictedFunctions
sniff."
@@ -162,17 +162,6 @@ public function getGroups() { | |||
), | |||
), | |||
|
|||
// @todo Introduce a sniff specific to get_posts() that checks for suppress_filters=>false being supplied. |
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.
I'm still missing a comment in the class docblock annotating that this check was moved to the new sniff.
|
||
// It handles when key value comes in '&query=arg' format. | ||
// eg. get_posts( 'foo=bar&baz=quux' ). | ||
if ( preg_match_all( '#(?:^|&)([a-z_]+)=([^&]*)#i', $this->strip_quotes( $this->tokens[ $argument_data ]['content'] ), $arrKey ) <= 0 ) { |
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.
Why are all query/arg combi's caught and then examined below ?
Why not only search for query/arg combi's where the key is suppress_filters
?
Also: is there any chance of the value being passed as an int ? if so, should it be examined for being 0
or 1
and if so, cast to boolean ?
// '=>' detected. | ||
if ( T_DOUBLE_ARROW === $this->tokens[ $ptr ]['code'] ) { | ||
|
||
$accept_type = array( T_DOUBLE_QUOTED_STRING, T_CONSTANT_ENCAPSED_STRING, T_FALSE, T_TRUE, T_LNUMBER, T_INT_CAST ); |
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.
Why is an T_INT_CAST
an accepted type ? and what about T_DOUBLE_QUOTED_STRING
T_CONSTANT_ENCAPSED_STRING
for that matter ?
I have to admit that the WP_Query
class could do with a stricter type check for the value passed though, so there may well be a valid case for these tokens.
// '=>' detected. | ||
if ( T_DOUBLE_ARROW === $this->tokens[ $ptr ]['code'] ) { | ||
|
||
$accept_type = array( T_DOUBLE_QUOTED_STRING, T_CONSTANT_ENCAPSED_STRING, T_FALSE, T_TRUE, T_LNUMBER, T_INT_CAST ); |
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.
If you change the array format to TOKEN_TYPE => true
, it will allow you to use the faster isset()
instead of in_array()
below.
} | ||
|
||
// If so, expected value was not passed. | ||
if ( ! $is_used || ! empty( $array_value ) ) { |
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.
Being explicit is helpful here: false === $is_used
.
[Edit]: on second thought: this condition will always be true
as either $is_used
will be false
or $array_value
will be filled, so should be removed or am I missing something ?
|
||
get_posts( array( 'post_type' => 'post', 'suppress_filters'=> true ) ); // Bad. | ||
wp_get_recent_posts( array( 'post_type' => 'post', 'suppress_filters'=> true ) ); // Bad. | ||
get_children( array( 'post_type' => 'post', 'suppress_filters'=> true ) ); // Bad. |
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.
I'm missing unit tests for:
- only checking within a function/closure scope
- $args set via an array with value
false
,0
,1
- $args being set using short array syntax from within the function call
- $args being set using short array syntax from outside the function call
etc
Please make sure that what you are doing in the sniff is covered by the unit tests.
// eg. get_posts( array( 'foo' => 'bar', 'baz' => 'quux' ) ). | ||
$param_items = $this->get_function_call_parameters( $argument_data ); | ||
|
||
} elseif ( T_CONSTANT_ENCAPSED_STRING === $this->tokens[ $argument_data ]['code'] || T_DOUBLE_QUOTED_STRING === $this->tokens[ $argument_data ]['code'] ) { |
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.
You can simplify this by using isset( Tokens::$stringTokens[ $this->tokens[ $argument_data ]['code'] ] )
@emgk Just checking: Do you have any intention of updating this PR based on the above feedback or should we close the PR or open it up to one of the WPCS maintainer to update the PR ? |
Hey @jrfnl, Sorry for the delayed response. yes I am going to work on this PR on this weekend. |
@emgk No worries. I'll look forward to the new version. |
@emgk Hi Govind, just checking in again to see how work is progressing. If you haven't got time to finish this, please let us know and we'll see if we can find someone to take over. |
acf07e1
to
6c5d015
Compare
.idea/vcs.xml
Outdated
@@ -0,0 +1,6 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
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.
This file should not be included.
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.
I've just noticed, and removed.
Hi @jrfnl Sorry for delay, I have just worked on the changes you requested for. The PR is almost completed just need your help regarding below things: In below syntax, what is the way to check if the array in variable contains the value I want to check ?
|
@emgk The So, I'm now wondering if this sniff should still go into WPCS or if it should go into the Automattic VIP Coding Standards ? @tomjn I think @emgk has done great work here, though it does need some final touches. Is this already covered in the Automattic VIP Coding Standards sniffs ? In your opinion, should this sniff maybe be pulled there instead ? |
IMO the suppress filters sniff is of value to all not just VIP, it'd be great if this could be included in WPCS under one of the other rulesets rather than the VIP ruleset |
@tomjn Could you please explain this ? Maybe in a new issue in which this can be further discussed ? |
Thank you @emgk for working on this and I'm sorry that it took us so long to get back to it. I believe the discussion points in the underlying ticket #1416 (still) need to be resolved first before this PR can be considered for merging. So for now, I'd like to suggest we close this PR with the option to re-open & rebase in the future. Thank you for your willingness to contribute to WordPressCS. I hope future PRs go more smoothly and I look forward to your continued contribution. |
In WordPress-VIP cs, I was getting the warning when using the
get_posts()
function which saying to includesuppress_filters=>false
inget_posts()
as args.Problem: This warning is not disappearing even when I am passing the
suppress_filter=>false
to it.As mentioned in to do comment inside the https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/WordPress/Sniffs/VIP/RestrictedFunctionsSniff.php#L160 for the
get_posts()
function to make the sniff to check ifsuppress_filter=>false
is passed or not.So here, I have tried to solve this issue, It is still incomplete, tweaks mentioned below can complete this PR.
suppress_filters
argument is passed orsuppress_filter
is passed but the value is not false then show the default error which is showing currently.Please guide or correct if I am wrong anywhere.
Thanks
Fixed #660