Skip to content
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

Closed
wants to merge 18 commits into from

Conversation

emgk
Copy link
Contributor

@emgk emgk commented Jul 20, 2017

In WordPress-VIP cs, I was getting the warning when using the get_posts() function which saying to include suppress_filters=>false in get_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 if suppress_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.

  1. If there is no suppress_filters argument is passed or suppress_filter is passed but the value is not false then show the default error which is showing currently.

get_posts() is discouraged in favor of creating a new WP_Query() so that Advanced Post Cache will cache the query unless you explicitly supply suppress_filters => false.';

Please guide or correct if I am wrong anywhere.

Thanks

Fixed #660

@jrfnl
Copy link
Member

jrfnl commented Jul 20, 2017

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.
Once that project has finished, your PR will need to be updated to be in line with the changes needed for PHPCS 3.x.

Ref: #1040

@jrfnl jrfnl added this to the 0.14.0 milestone Jul 20, 2017
@emgk
Copy link
Contributor Author

emgk commented Jul 21, 2017

@jrfnl Please do let me know whenever any changes are required on this PR.

Copy link
Member

@jrfnl jrfnl left a 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(
Copy link
Member

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.
Copy link
Member

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
Copy link
Member

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 {
Copy link
Member

@jrfnl jrfnl Jul 21, 2017

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(
Copy link
Member

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 ) ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. There is no need to strtolower() the key or to check its value as there is only one key supplied via getGroups(), so the callback will only be called if that key is encountered.
  2. 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 be false and the suppress_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 to get_posts() without suppress_filters being set would also pass.
  3. get_posts() is now hard-coded into the error message while this message should/could also be thrown for the other two functions.

@emgk
Copy link
Contributor Author

emgk commented Jul 23, 2017

@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().
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Fixed.

Copy link
Member

@jrfnl jrfnl left a 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
*/

Copy link
Member

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;

Copy link
Contributor Author

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 {
Copy link
Member

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
*/

Copy link
Member

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;

Copy link
Contributor Author

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class SuppressFiltersSniff extends AbstractFunctionParameterSniff {

Copy link
Contributor Author

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 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP_CodeSniffer_Tokens => Tokens

Copy link
Contributor Author

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 );

/**
Copy link
Member

@jrfnl jrfnl Jul 27, 2017

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 ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed. to => //

@emgk
Copy link
Contributor Author

emgk commented Jul 27, 2017

@jrfnl I have started working on these changes.

@emgk
Copy link
Contributor Author

emgk commented Jul 29, 2017

@jrfnl I have updated this PR with the current changes made in develop, is it fine or should I have to revert it back ? Please guide

@jrfnl
Copy link
Member

jrfnl commented Aug 6, 2017

@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

@emgk
Copy link
Contributor Author

emgk commented Aug 8, 2017

@jrfnl I am working on this.

@emgk
Copy link
Contributor Author

emgk commented Aug 9, 2017

@jrfnl could you please check ?

@jrfnl jrfnl mentioned this pull request Sep 22, 2017
@jrfnl
Copy link
Member

jrfnl commented Oct 21, 2017

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 develop ? A number of new sniffs have been added to WPCS since you originally pulled yours and your sniff as it currently is does not comply with some of the new rules so that needs to be fixed.

  94 | ERROR   | [x] Expected 1 space after comma in function call; 2 found
     |         |     (Generic.Functions.FunctionCallArgumentSpacing.TooMuchSpaceAfterComma)
  96 | ERROR   | [x] No space found after comma in function call
     |         |     (Generic.Functions.FunctionCallArgumentSpacing.NoSpaceAfterComma)
 104 | ERROR   | [x] No space found after comma in function call
     |         |     (Generic.Functions.FunctionCallArgumentSpacing.NoSpaceAfterComma)
 104 | ERROR   | [x] No space found after comma in function call
     |         |     (Generic.Functions.FunctionCallArgumentSpacing.NoSpaceAfterComma)
 107 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 9 spaces but found 12
     |         |     spaces (Generic.Formatting.MultipleStatementAlignment.NotSameWarning)
 108 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 4 spaces but found 7
     |         |     spaces (Generic.Formatting.MultipleStatementAlignment.NotSameWarning)
 109 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 6 spaces but found 9
     |         |     spaces (Generic.Formatting.MultipleStatementAlignment.NotSameWarning)
 110 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 9 spaces but found 12
     |         |     spaces (Generic.Formatting.MultipleStatementAlignment.NotSameWarning)
 111 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 9 spaces but found 12
     |         |     spaces (Generic.Formatting.MultipleStatementAlignment.NotSameWarning)
 163 | ERROR   | [x] No space found after comma in function call
     |         |     (Generic.Functions.FunctionCallArgumentSpacing.NoSpaceAfterComma)
 165 | WARNING | [x] Equals sign not aligned with surrounding assignments; expected 5 spaces but found 1
     |         |     space (Generic.Formatting.MultipleStatementAlignment.NotSameWarning)

I was also wondering if you could replace the @link in the class documentation with one specifically pointing to a section in the VIP handbook which supports the need for this sniff.

On the page the link points to now, I couldn't find any information about suppress_filter=>false not being allowed.

Copy link
Member

@jrfnl jrfnl left a 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 ] ) {
Copy link
Member

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 ?

  1. This method won't be called unless one of the target functions has been matched.
  2. 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/
Copy link
Member

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
Copy link
Member

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.
Copy link
Member

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 ) {
Copy link
Member

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 );
Copy link
Member

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 );
Copy link
Member

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 ) ) {
Copy link
Member

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.
Copy link
Member

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'] ) {
Copy link
Member

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'] ] )

@jrfnl jrfnl removed this from the 0.14.0 milestone Oct 23, 2017
@jrfnl
Copy link
Member

jrfnl commented Mar 1, 2018

@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 ?

@emgk
Copy link
Contributor Author

emgk commented Mar 8, 2018

Hey @jrfnl, Sorry for the delayed response. yes I am going to work on this PR on this weekend.

@jrfnl
Copy link
Member

jrfnl commented Mar 8, 2018

@emgk No worries. I'll look forward to the new version.

@jrfnl
Copy link
Member

jrfnl commented Jun 11, 2018

@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.

.idea/vcs.xml Outdated
@@ -0,0 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

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.

Copy link
Contributor Author

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.

@emgk
Copy link
Contributor Author

emgk commented Jun 15, 2018

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 ?

$args4 = array();
$args4['suppress_filters'] = false;

get_posts( $args4 );

@jrfnl
Copy link
Member

jrfnl commented Jul 6, 2018

@emgk The WordPress-VIP ruleset in WPCS is going to be deprecated in favour of using the Automattic VIP Coding Standards standard.
See #1309, #1409

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 ?

@tomjn
Copy link
Contributor

tomjn commented Jul 7, 2018

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

@jrfnl
Copy link
Member

jrfnl commented Jul 7, 2018

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 ?

@jrfnl
Copy link
Member

jrfnl commented Nov 11, 2022

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.
Additionally the PR would need an update as the code base has undergone quite some changes in the mean time.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sniff for suppress_filters => false
4 participants