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

Improve sniffs to deal with anonymous functions and classes #813

Merged
merged 2 commits into from
Jan 21, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions WordPress/Sniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -760,8 +760,13 @@ protected function has_nonce_check( $stackPtr ) {

// If we're in a function, only look inside of it.
$f = $this->phpcsFile->getCondition( $stackPtr, T_FUNCTION );
if ( $f ) {
if ( false !== $f ) {
$start = $tokens[ $f ]['scope_opener'];
} else {
$f = $this->phpcsFile->getCondition( $stackPtr, T_CLOSURE );
if ( false !== $f ) {
$start = $tokens[ $f ]['scope_opener'];
}
}

$in_isset = $this->is_in_isset_or_empty( $stackPtr );
Expand Down Expand Up @@ -1110,14 +1115,22 @@ protected function is_validated( $stackPtr, $array_key = null, $in_condition_onl
in the same function/file scope as it is used.
*/

$scope_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 ) {
$scope_start = $this->tokens[ $function ]['scope_opener'];
} else {
$scope_start = 0;
// 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 ) {
$scope_start = $this->tokens[ $closure ]['scope_opener'];
}
}

$scope_end = $stackPtr;
Expand Down Expand Up @@ -1247,7 +1260,7 @@ protected function get_use_type( $stackPtr ) {
}

// USE keywords for traits.
if ( $this->phpcsFile->hasCondition( $stackPtr, array( T_CLASS, T_TRAIT ) ) ) {
if ( $this->phpcsFile->hasCondition( $stackPtr, array( T_CLASS, T_ANON_CLASS, T_TRAIT ) ) ) {
return 'trait';
}

Expand Down
6 changes: 5 additions & 1 deletion WordPress/Sniffs/VIP/DirectDatabaseQuerySniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,11 @@ public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) {
if ( ! $whitelisted_cache && ! empty( $tokens[ $stackPtr ]['conditions'] ) ) {
$scope_function = $phpcsFile->getCondition( $stackPtr, T_FUNCTION );

if ( $scope_function ) {
if ( false === $scope_function ) {
$scope_function = $phpcsFile->getCondition( $stackPtr, T_CLOSURE );
}

if ( false !== $scope_function ) {
$scopeStart = $tokens[ $scope_function ]['scope_opener'];
$scopeEnd = $tokens[ $scope_function ]['scope_closer'];

Expand Down
2 changes: 1 addition & 1 deletion WordPress/Sniffs/Variables/GlobalVariablesSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) {
for ( $ptr = $start; $ptr < $end; $ptr++ ) {

// If the global statement was in the global scope, skip over functions, classes and the likes.
if ( true === $global_scope && in_array( $this->tokens[ $ptr ]['code'], array( T_FUNCTION, T_CLOSURE, T_CLASS, T_INTERFACE, T_TRAIT ), true ) ) {
if ( true === $global_scope && in_array( $this->tokens[ $ptr ]['code'], array( T_FUNCTION, T_CLOSURE, T_CLASS, T_ANON_CLASS, T_INTERFACE, T_TRAIT ), true ) ) {
if ( ! isset( $this->tokens[ $ptr ]['scope_closer'] ) ) {
// Live coding, skip the rest of the file.
return;
Expand Down
4 changes: 3 additions & 1 deletion WordPress/Sniffs/WhiteSpace/ControlStructureSpacingSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,9 @@ public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) {
// We ignore spacing for some structures that tend to have their own rules.
$ignore = array(
T_FUNCTION => true,
T_CLOSURE => true,
T_CLASS => true,
T_ANON_CLASS => true,
T_INTERFACE => true,
T_TRAIT => true,
T_DOC_COMMENT_OPEN_TAG => true,
Expand Down Expand Up @@ -536,7 +538,7 @@ public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) {
// Another control structure's closing brace.
if ( isset( $this->tokens[ $trailingContent ]['scope_condition'] ) ) {
$owner = $this->tokens[ $trailingContent ]['scope_condition'];
if ( in_array( $this->tokens[ $owner ]['code'], array( T_FUNCTION, T_CLOSURE, T_CLASS, T_INTERFACE, T_TRAIT ), true ) ) {
if ( in_array( $this->tokens[ $owner ]['code'], array( T_FUNCTION, T_CLOSURE, T_CLASS, T_ANON_CLASS, T_INTERFACE, T_TRAIT ), true ) ) {
// The next content is the closing brace of a function, class, interface or trait
// so normal function/class rules apply and we can ignore it.
return;
Expand Down
4 changes: 3 additions & 1 deletion WordPress/Sniffs/WhiteSpace/OperatorSpacingSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) {
$bracket = end( $tokens[ $stackPtr ]['nested_parenthesis'] );
if ( isset( $tokens[ $bracket ]['parenthesis_owner'] ) ) {
$function = $tokens[ $bracket ]['parenthesis_owner'];
if ( T_FUNCTION === $tokens[ $function ]['code'] ) {
if ( T_FUNCTION === $tokens[ $function ]['code']
|| T_CLOSURE === $tokens[ $function ]['code']
) {
return;
}
}
Expand Down
10 changes: 10 additions & 0 deletions WordPress/Tests/CSRF/NonceVerificationUnitTest.inc
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,13 @@ function foo_5() {

check_ajax_referer( 'something' );
}

// Test anonymous function - Bad, needs nonce check.
check_ajax_referer( 'something' ); // Nonce check is not in function scope.
$b = function () {
if ( ! isset( $_POST['abc'] ) ) { // Bad.
return;
}

do_something( $_POST['abc'] ); // Bad.
};
16 changes: 9 additions & 7 deletions WordPress/Tests/CSRF/NonceVerificationUnitTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,17 @@ class WordPress_Tests_CSRF_NonceVerificationUnitTest extends AbstractSniffUnitTe
public function getErrorList() {

return array(
5 => 1,
9 => 1,
31 => 1,
44 => 1,
48 => 1,
69 => 1,
89 => 1,
5 => 1,
9 => 1,
31 => 1,
44 => 1,
48 => 1,
69 => 1,
89 => 1,
113 => 1,
114 => 1,
122 => 1,
126 => 1,
);
}

Expand Down
12 changes: 12 additions & 0 deletions WordPress/Tests/VIP/DirectDatabaseQueryUnitTest.inc
Original file line number Diff line number Diff line change
Expand Up @@ -103,3 +103,15 @@ function cache_add_instead_of_set() {
wp_cache_add( 'baz', $baz );
}
}

// Database calls in a closure.
$b = function () {
global $wpdb;

if ( ! ( $listofthings = wp_cache_get( $foo ) ) ) {
$listofthings = $wpdb->get_col( 'SELECT something FROM somewhere WHERE someotherthing = 1' ); // Warning.
wp_cache_set( 'foo', $listofthings );
}

return $listofthings;
};
9 changes: 5 additions & 4 deletions WordPress/Tests/VIP/DirectDatabaseQueryUnitTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,11 @@ public function getErrorList() {
*/
public function getWarningList() {
return array(
6 => 1,
17 => 1,
38 => 1,
50 => 1,
6 => 1,
17 => 1,
38 => 1,
50 => 1,
112 => 1,
);

}
Expand Down
5 changes: 5 additions & 0 deletions WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.inc
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,8 @@ output( "some string \\$_POST[some_var] $_GET[evil]" ); // Bad x2.

echo esc_html( wp_strip_all_tags( wp_unslash( $_GET['a'] ) ) ); // Ok.

// Test validation check vs anonymous functions.
isset( $_POST['abc'] ); // Validation in global scope, not function scope.
$b = function () {
return sanitize_text_field( wp_unslash( $_POST['abc'] ) ); // Error for no validation.
};
1 change: 1 addition & 0 deletions WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public function getErrorList() {
104 => 2,
105 => 1,
114 => 2,
121 => 1,
);

}
Expand Down
12 changes: 12 additions & 0 deletions WordPress/Tests/Variables/GlobalVariablesUnitTest.inc
Original file line number Diff line number Diff line change
Expand Up @@ -117,3 +117,15 @@ class Test_Class_D extends My_TestClass {
}
}
// @codingStandardsChangeSetting WordPress.Variables.GlobalVariables custom_test_class_whitelist null

// Test detecting within and skipping over anonymous classes.
global $year;
add_filter( 'comments_open', new class {
public $year = 2017; // Ok.

public function __construct( $open, $post_id ) {
global $page;
$page = get_post( $post_id ); // Bad.
return 'page' === $page->post_type;
}
}, 10, 2 );
21 changes: 11 additions & 10 deletions WordPress/Tests/Variables/GlobalVariablesUnitTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,17 @@ class WordPress_Tests_Variables_GlobalVariablesUnitTest extends AbstractSniffUni
*/
public function getErrorList() {
return array(
3 => 1,
6 => 1,
16 => 1,
17 => 1,
18 => 1,
25 => 1,
35 => 1,
36 => 1,
54 => 1,
95 => 1,
3 => 1,
6 => 1,
16 => 1,
17 => 1,
18 => 1,
25 => 1,
35 => 1,
36 => 1,
54 => 1,
95 => 1,
128 => 1,
);

}
Expand Down
4 changes: 4 additions & 0 deletions WordPress/Tests/WhiteSpace/OperatorSpacingUnitTest.inc
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,7 @@ for ( $i = 0; $i < sizeof( $posts ); $i++ ) {
if ( ! $var ) {
// ...
}

// Ok: Test skipping of default values in function declarations.
function my_test( $a=true, $b = 123, $c= 'string' ) {}
$a = function ( $a=true, $b = 123, $c= 'string' ) {};
4 changes: 4 additions & 0 deletions WordPress/Tests/WhiteSpace/OperatorSpacingUnitTest.inc.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,7 @@ for ( $i = 0; $i < sizeof( $posts ); $i++ ) {
if ( ! $var ) {
// ...
}

// Ok: Test skipping of default values in function declarations.
function my_test( $a=true, $b = 123, $c= 'string' ) {}
$a = function ( $a=true, $b = 123, $c= 'string' ) {};