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
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .idea/vcs.xml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 1 addition & 11 deletions WordPress/Sniffs/VIP/RestrictedFunctionsSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
* WordPress_Sniffs_WP_AlternativeFunctionsSniff.
* The check for `eval()` now defers to the upstream Squiz.PHP.Eval sniff.
* @since 0.13.0 Class name changed: this class is now namespaced.
* @since 0.14.0 Checks for get_posts() has been moved to the `Security` SuppressFilterSniff.
*/
class RestrictedFunctionsSniff extends AbstractFunctionRestrictionsSniff {

Expand Down Expand Up @@ -159,17 +160,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.

'get_posts' => array(
'type' => 'warning',
'message' => '%s() 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.',
'functions' => array(
'get_posts',
'wp_get_recent_posts',
'get_children',
),
),

'term_exists' => array(
'type' => 'error',
'message' => '%s() is highly discouraged due to not being cached; please use wpcom_vip_term_exists() instead.',
Expand Down
184 changes: 184 additions & 0 deletions WordPress/Sniffs/VIP/SuppressFiltersSniff.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
<?php
/**
* WordPress Coding Standard.
*
* @package WPCS\WordPressCodingStandards
* @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

namespace WordPress\Sniffs\VIP;

use WordPress\AbstractFunctionParameterSniff;
use PHP_CodeSniffer_Tokens as Tokens;

/**
* Checks for suppress_filters=>false being supplied in get_posts(), wp_get_recent_posts() and get_children().
*
* @link https://vip.wordpress.com/documentation/uncached-functions/
*
* @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."

*/
class SuppressFiltersSniff extends AbstractFunctionParameterSniff {

/**
* Functions this sniff is looking for.
*
* @since 0.14.0
*
* @var array
*/
protected $target_functions = array(
'get_posts' => true,
'wp_get_recent_posts' => true,
'get_children' => true,
);

/**
* Process the parameters of a matched function.
*
* @since 0.14.0
*
* @param int $stackPtr The position of the current token in the stack.
* @param array $group_name The name of the group which was matched.
* @param string $matched_content The token content (function name) which was matched.
* @param array $parameters Array with information about the parameters.
*
* @return void
*/
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.

return;
}

// Flag to check if suppress_filters is passed or not.
$array_value = '';

// Retrieve the value parameter's details.
$argument_data = $this->phpcsFile->findNext( Tokens::$emptyTokens, $parameters[1]['start'], ( $parameters[1]['end'] + 1 ), true );

// When the list of argument were passed through variable.
// eg. $args = array( 'foo' => 'bar' ), $args['foo'] => 'bar'.
if ( T_VARIABLE === $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.

AFAICS, this block will break when the variable passed is a class property $this->args or an array index $queries['post']['args'].

Copy link
Contributor Author

@emgk emgk Mar 11, 2018

Choose a reason for hiding this comment

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

@jrfnl Yes, this block is not checking value when passing argument as you mentioned, It would be much appreciated if could give me any reference/sniff file where this already had been done?

$start = 0;

// Check if we are in a function.
$function = $this->phpcsFile->getCondition( $stackPtr, T_FUNCTION );

// If so, we check only within the function, otherwise the whole file.
if ( false !== $function ) {
$start = $this->tokens[ $function ]['scope_opener'];
} else {
// Check if we are in a closure.
$closure = $this->phpcsFile->getCondition( $stackPtr, T_CLOSURE );

// If so, we check only within the closure.
if ( false !== $closure ) {
$start = $this->tokens[ $closure ]['scope_opener'];
}
}

// Move to the next pointer where '=>' or ']' is placed.
$variable = $this->phpcsFile->findNext( T_VARIABLE, $start, $stackPtr );

Copy link
Member

Choose a reason for hiding this comment

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

You could possibly simplify this loop up by using findNext/Previous() with T_VARIABLE and passing it the value you are looking for: https://pear.php.net/package/PHP_CodeSniffer/docs/2.9.1/PHP_CodeSniffer/PHP_CodeSniffer_File.html#methodfindNext

// Check if the variable we're looking for was found.
if ( $this->is_assignment( $variable ) ) {

// Move to the next pointer where '=>' or ']' is placed.
$varData = $this->phpcsFile->findNext( array(
T_CLOSE_SQUARE_BRACKET,
T_DOUBLE_ARROW,
T_ARRAY,
T_OPEN_SHORT_ARRAY,
), $variable );

if ( in_array( $this->tokens[ $varData ]['code'], array(
T_CLOSE_SQUARE_BRACKET,
T_DOUBLE_ARROW,
), true ) ) {

$operator = $varData; // T_DOUBLE_ARROW.

if ( T_CLOSE_SQUARE_BRACKET === $this->tokens[ $varData ]['code'] ) {
$operator = $this->phpcsFile->findNext( T_EQUAL, ( $varData ) );
}

$keyIdx = $this->phpcsFile->findPrevious( array(
T_WHITESPACE,
T_CLOSE_SQUARE_BRACKET,
), ( $operator - 1 ), null, true );

if ( ! is_numeric( $this->tokens[ $keyIdx ]['content'] ) ) {
$valStart = $this->phpcsFile->findNext( array( T_WHITESPACE ), ( $operator + 1 ), null, true );
$valEnd = $this->phpcsFile->findNext( array(
T_COMMA,
T_SEMICOLON,
), ( $valStart + 1 ), null, false, null, true );
$array_value = $this->phpcsFile->getTokensAsString( $valStart, ( $valEnd - $valStart ) );
}

} elseif ( in_array( $this->tokens[ $varData ]['code'], array( T_ARRAY, T_OPEN_SHORT_ARRAY ), true ) ) {
// Store array data into variable so that we can proceed later.
$param_items = $this->get_function_call_parameters( $varData );
// $this->phpcsFile->addWarning( print_r( $param_items, true ), $parameters[1]['start'], 'SuppressFilters' );
}
}
} elseif ( T_ARRAY === $this->tokens[ $argument_data ]['code'] || T_OPEN_SHORT_ARRAY === $this->tokens[ $argument_data ]['code'] ) {

// It covers multiple array variable directly passed to function.
// eg. get_posts( array( 'foo' => 'bar', 'baz' => 'quux' ) ).
$param_items = $this->get_function_call_parameters( $argument_data );

} elseif ( isset( Tokens::$stringTokens[ $this->tokens[ $argument_data ]['code'] ] ) ) {
// It handles when key value comes in '&query=arg' format.
// eg. get_posts( 'foo=bar&baz=quux' ).
$query_args = parse_str( $this->strip_quotes( $this->tokens[ $argument_data ]['content'] ), $arrKey );

if ( isset( $query_args['suppress_filters'] ) ) {
$array_value = $query_args['suppress_filters'];
}
}


// Process multi dimensional array.
if ( ! empty( $param_items ) ) {

// Process the array.
foreach ( $param_items as $item ) {

// If 'raw' contains suppress_filters value.
if ( false !== strpos( $item['raw'], 'suppress_filters' ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

This will also be true when the array item contains a sub-array which will break the below code.
You can get round that by doing a findNext() with T_CONSTANT_ENCAPSED_STRING as the type and passing it suppress_filters as value, as the first thing within this condition and only searching for the double arrow after the found pointer.


// Move to the next pointer where '=>' or ']' is placed.
$variable = $this->phpcsFile->findNext( T_DOUBLE_ARROW, $item['start'], $item['end'] );

// Type of the suppress_filter's value can be.
$accept_type = array(
T_FALSE => T_FALSE,
T_TRUE => T_TRUE,
T_LNUMBER => T_LNUMBER,
);

// Get the value of the suppress_filters array key.
$key_value = $this->phpcsFile->findNext( Tokens::$emptyTokens, $variable + 1, null, true );

// Get the value of the suppress_filters array key.
$key_value1 = $this->phpcsFile->findNext( array_keys($accept_type), $this->tokens[ $key_value ]['nested_parenthesis'] );
if ( isset( $accept_type[ $this->tokens[ $key_value ]['code'] ] ) ) {
$array_value = $this->tokens[ $key_value ]['content'];
}
}
}
}

// In case numeric value is passed.
$item_value = ( '0' === $array_value ) ? 'false' : $this->strip_quotes( $array_value );

if ( 'false' !== $item_value ) {
$this->phpcsFile->addWarning( '%s() 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.', $parameters[1]['start'], 'SuppressFilters', array( $matched_content ) );
}
}

} // End class.
6 changes: 3 additions & 3 deletions WordPress/Tests/VIP/RestrictedFunctionsUnitTest.inc
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,10 @@ get_category_link(); // Ok.
get_cat_ID(); // Ok.
url_to_post_id(); // Error.

get_posts(); // Warning.
wp_get_recent_posts(); // Warning.

get_children(); // Warning.






Expand Down
1 change: 1 addition & 0 deletions WordPress/Tests/VIP/RestrictedFunctionsUnitTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ public function getWarningList() {
54 => 1,
55 => 1,
57 => 1,
76 => 1,
79 => 1,
);

Expand Down
49 changes: 49 additions & 0 deletions WordPress/Tests/VIP/SuppressFiltersUnitTest.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?php

$args3 = array(
'post_type' => 'post',
'orderby' => 'date',
'order' => 'ASC',
); // Bad.

get_posts( $args3 ); // Bad.
wp_get_recent_posts( $args3 ); // Bad.
get_children( $args3 ); // Bad.

get_posts( 'post_type=post&order=ASC' ); // Bad.
wp_get_recent_posts( 'post_type=post&order=ASC'); // Bad.
get_children( 'post_type=post&order=ASC' ); // Bad.

get_posts( 'post_type=post&suppress_filters=false&order=ASC' ); // Ok.
wp_get_recent_posts( 'post_type=post&suppress_filters=false&order=ASC'); // Ok.
get_children( 'post_type=post&suppress_filters=false&order=ASC' ); // Ok.

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

get_posts( $args4 ); // Bad.
wp_get_recent_posts( $args4 ); // Bad.
get_children( $args4 ); // Bad.

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.


$post_args = array();
$post_args['suppress_filters'] = true;

get_posts( $post_args ); // Bad.
wp_get_recent_posts( $post_args ); // Bad.
get_children( $post_args ); // Bad.

function test_suppress_filter() {
$args = array( 'post_type' => 'post', 'suppress_filters' => true );
get_posts( $args ); // Bad
wp_get_recent_posts( $args ); // Bad
get_children( $args ); // Bad

$args_ok = array( 'post_type' => 'post', 'suppress_filters' => false );
get_posts( $args_ok ); // Ok
wp_get_recent_posts( $args_ok ); // Ok
get_children( $args_ok ); // Ok
}
64 changes: 64 additions & 0 deletions WordPress/Tests/VIP/SuppressFiltersUnitTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
<?php
/**
* Unit test class for WordPress Coding Standard.
*
* @package WPCS\WordPressCodingStandards
* @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

namespace WordPress\Tests\VIP;

use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;

/**
* Unit test class for the SuppressFilters sniff.
*
* @package WPCS\WordPressCodingStandards
* @since 0.14.0
*/
class SuppressFiltersUnitTest extends AbstractSniffUnitTest {

/**
* Returns the lines where errors should occur.
*
* @return array <int line number> => <int number of errors>
*/
public function getErrorList() {
return array();

}

/**
* Returns the lines where warnings should occur.
*
* @return array <int line number> => <int number of warnings>
*/
public function getWarningList() {
return array(
9 => 1,
10 => 1,
11 => 1,
13 => 1,
14 => 1,
15 => 1,
24 => 1,
25 => 1,
26 => 1,
28 => 1,
29 => 1,
30 => 1,
35 => 1,
36 => 1,
37 => 1,
41 => 1,
42 => 1,
43 => 1,
46 => 1,
47 => 1,
48 => 1
);

}

} // End class.