From dd420c9044296adbc93aa37768c3cdc4135135af Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 11 Apr 2020 12:46:03 +0200 Subject: [PATCH 1/4] Move "is WPDB method call" related utilities to dedicated `WPDBTrait` The "WPDB method call" related utilities are only used by a small set of sniffs, so are better placed in a dedicated trait. This moves the `is_wpdb_method_call()` method to a new `WordPressCS\WordPress\Helpers\WPDBTrait` and starts using that trait in the relevant sniffs. The trait has been made stand-alone, i.e. it does not presume that a sniff implementing it extends the `WordPressCS\WordPress\Sniff` class. Note: a refactor of this method removing the presumption about properties in child classes would still be a good idea. Related to 1465 --- WordPress/Helpers/WPDBTrait.php | 117 ++++++++++++++++++ WordPress/Sniff.php | 87 ------------- .../DB/PreparedSQLPlaceholdersSniff.php | 5 +- WordPress/Sniffs/DB/PreparedSQLSniff.php | 7 +- 4 files changed, 126 insertions(+), 90 deletions(-) create mode 100644 WordPress/Helpers/WPDBTrait.php diff --git a/WordPress/Helpers/WPDBTrait.php b/WordPress/Helpers/WPDBTrait.php new file mode 100644 index 0000000000..6653d58806 --- /dev/null +++ b/WordPress/Helpers/WPDBTrait.php @@ -0,0 +1,117 @@ +getTokens(); + + // Check for wpdb. + if ( ( \T_VARIABLE === $tokens[ $stackPtr ]['code'] && '$wpdb' !== $tokens[ $stackPtr ]['content'] ) + || ( \T_STRING === $tokens[ $stackPtr ]['code'] && 'wpdb' !== $tokens[ $stackPtr ]['content'] ) + ) { + return false; + } + + // Check that this is a method call. + $is_object_call = $phpcsFile->findNext( + array( \T_OBJECT_OPERATOR, \T_DOUBLE_COLON ), + ( $stackPtr + 1 ), + null, + false, + null, + true + ); + if ( false === $is_object_call ) { + return false; + } + + $methodPtr = $phpcsFile->findNext( \T_WHITESPACE, ( $is_object_call + 1 ), null, true, null, true ); + if ( false === $methodPtr ) { + return false; + } + + if ( \T_STRING === $tokens[ $methodPtr ]['code'] && property_exists( $this, 'methodPtr' ) ) { + $this->methodPtr = $methodPtr; + } + + // Find the opening parenthesis. + $opening_paren = $phpcsFile->findNext( \T_WHITESPACE, ( $methodPtr + 1 ), null, true, null, true ); + + if ( false === $opening_paren ) { + return false; + } + + if ( property_exists( $this, 'i' ) ) { + $this->i = $opening_paren; + } + + if ( \T_OPEN_PARENTHESIS !== $tokens[ $opening_paren ]['code'] + || ! isset( $tokens[ $opening_paren ]['parenthesis_closer'] ) + ) { + return false; + } + + // Check that this is one of the methods that we are interested in. + if ( ! isset( $target_methods[ $tokens[ $methodPtr ]['content'] ] ) ) { + return false; + } + + // Find the end of the first parameter. + $end = $phpcsFile->findEndOfStatement( $opening_paren + 1 ); + + if ( \T_COMMA !== $tokens[ $end ]['code'] ) { + ++$end; + } + + if ( property_exists( $this, 'end' ) ) { + $this->end = $end; + } + + return true; + } +} diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 6a79643ca0..67bfb7104f 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1582,93 +1582,6 @@ protected function is_in_array_comparison( $stackPtr ) { return false; } - /** - * Checks whether this is a call to a $wpdb method that we want to sniff. - * - * If available in the child class, the $methodPtr, $i and $end properties are - * automatically set to correspond to the start and end of the method call. - * The $i property is also set if this is not a method call but rather the - * use of a $wpdb property. - * - * @since 0.8.0 - * @since 0.9.0 The return value is now always boolean. The $end and $i member - * vars are automatically updated. - * @since 0.14.0 Moved this method from the `PreparedSQL` sniff to the base WP sniff. - * - * {@internal This method should probably be refactored.}} - * - * @param int $stackPtr The index of the $wpdb variable. - * @param array $target_methods Array of methods. Key(s) should be method name. - * - * @return bool Whether this is a $wpdb method call. - */ - protected function is_wpdb_method_call( $stackPtr, $target_methods ) { - - // Check for wpdb. - if ( ( \T_VARIABLE === $this->tokens[ $stackPtr ]['code'] && '$wpdb' !== $this->tokens[ $stackPtr ]['content'] ) - || ( \T_STRING === $this->tokens[ $stackPtr ]['code'] && 'wpdb' !== $this->tokens[ $stackPtr ]['content'] ) - ) { - return false; - } - - // Check that this is a method call. - $is_object_call = $this->phpcsFile->findNext( - array( \T_OBJECT_OPERATOR, \T_DOUBLE_COLON ), - ( $stackPtr + 1 ), - null, - false, - null, - true - ); - if ( false === $is_object_call ) { - return false; - } - - $methodPtr = $this->phpcsFile->findNext( \T_WHITESPACE, ( $is_object_call + 1 ), null, true, null, true ); - if ( false === $methodPtr ) { - return false; - } - - if ( \T_STRING === $this->tokens[ $methodPtr ]['code'] && property_exists( $this, 'methodPtr' ) ) { - $this->methodPtr = $methodPtr; - } - - // Find the opening parenthesis. - $opening_paren = $this->phpcsFile->findNext( \T_WHITESPACE, ( $methodPtr + 1 ), null, true, null, true ); - - if ( false === $opening_paren ) { - return false; - } - - if ( property_exists( $this, 'i' ) ) { - $this->i = $opening_paren; - } - - if ( \T_OPEN_PARENTHESIS !== $this->tokens[ $opening_paren ]['code'] - || ! isset( $this->tokens[ $opening_paren ]['parenthesis_closer'] ) - ) { - return false; - } - - // Check that this is one of the methods that we are interested in. - if ( ! isset( $target_methods[ $this->tokens[ $methodPtr ]['content'] ] ) ) { - return false; - } - - // Find the end of the first parameter. - $end = $this->phpcsFile->findEndOfStatement( $opening_paren + 1 ); - - if ( \T_COMMA !== $this->tokens[ $end ]['code'] ) { - ++$end; - } - - if ( property_exists( $this, 'end' ) ) { - $this->end = $end; - } - - return true; - } - /** * Determine whether an arbitrary T_STRING token is the use of a global constant. * diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index f6ecdb18d3..18d116a2eb 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -12,6 +12,7 @@ use PHP_CodeSniffer\Util\Tokens; use PHPCSUtils\Utils\PassedParameters; use PHPCSUtils\Utils\TextStrings; +use WordPressCS\WordPress\Helpers\WPDBTrait; use WordPressCS\WordPress\Sniff; /** @@ -43,6 +44,8 @@ */ class PreparedSQLPlaceholdersSniff extends Sniff { + use WPDBTrait; + /** * These regexes were originally copied from https://www.php.net/function.sprintf#93552 * and adjusted for limitations in `$wpdb->prepare()`. @@ -168,7 +171,7 @@ public function register() { */ public function process_token( $stackPtr ) { - if ( ! $this->is_wpdb_method_call( $stackPtr, $this->target_methods ) ) { + if ( ! $this->is_wpdb_method_call( $this->phpcsFile, $stackPtr, $this->target_methods ) ) { return; } diff --git a/WordPress/Sniffs/DB/PreparedSQLSniff.php b/WordPress/Sniffs/DB/PreparedSQLSniff.php index 5fa48a0be8..e9bd2b5578 100644 --- a/WordPress/Sniffs/DB/PreparedSQLSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLSniff.php @@ -11,6 +11,7 @@ use PHP_CodeSniffer\Util\Tokens; use PHPCSUtils\Utils\TextStrings; +use WordPressCS\WordPress\Helpers\WPDBTrait; use WordPressCS\WordPress\Sniff; /** @@ -28,6 +29,8 @@ */ class PreparedSQLSniff extends Sniff { + use WPDBTrait; + /** * The lists of $wpdb methods. * @@ -124,7 +127,7 @@ public function register() { */ public function process_token( $stackPtr ) { - if ( ! $this->is_wpdb_method_call( $stackPtr, $this->methods ) ) { + if ( ! $this->is_wpdb_method_call( $this->phpcsFile, $stackPtr, $this->methods ) ) { return; } @@ -161,7 +164,7 @@ function ( $symbol ) { if ( \T_VARIABLE === $this->tokens[ $this->i ]['code'] ) { if ( '$wpdb' === $this->tokens[ $this->i ]['content'] ) { - $this->is_wpdb_method_call( $this->i, $this->methods ); + $this->is_wpdb_method_call( $this->phpcsFile, $this->i, $this->methods ); continue; } From b7a7fd69ad3c3d6e77f5a1ebe480a35bbcad2943 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 11 Apr 2020 13:04:52 +0200 Subject: [PATCH 2/4] WPDBTrait::is_wpdb_method_call(): use PHPCSUtils --- WordPress/Helpers/WPDBTrait.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/WordPress/Helpers/WPDBTrait.php b/WordPress/Helpers/WPDBTrait.php index 6653d58806..66c3669307 100644 --- a/WordPress/Helpers/WPDBTrait.php +++ b/WordPress/Helpers/WPDBTrait.php @@ -10,6 +10,7 @@ namespace WordPressCS\WordPress\Helpers; use PHP_CodeSniffer\Files\File; +use PHPCSUtils\BackCompat\BCFile; /** * Helper utilities for sniffs which examine WPDB method calls. @@ -102,7 +103,7 @@ protected function is_wpdb_method_call( File $phpcsFile, $stackPtr, $target_meth } // Find the end of the first parameter. - $end = $phpcsFile->findEndOfStatement( $opening_paren + 1 ); + $end = BCFile::findEndOfStatement( $phpcsFile, $opening_paren + 1 ); if ( \T_COMMA !== $tokens[ $end ]['code'] ) { ++$end; From 8fc7d45b65b708184bfc0d905e911e7bb02e3086 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 11 Apr 2020 12:58:29 +0200 Subject: [PATCH 3/4] WPDBTrait::is_wpdb_method_call(): improve code-style independence and support PHP 8.0+ nullsafe object operators The `WPDBTrait::is_wpdb_method_call()` did not properly ignore unexpected whitespace and/or comments. As the check for object operators is now being switched to use the `Collections::objectOperators()` token collection, this automatically also adds support for the PHP 8.0 nullsafe object operator when used with `$wpdb`. Includes unit test via the `PreparedSQL` sniff. --- WordPress/Helpers/WPDBTrait.php | 19 ++++++++----------- WordPress/Tests/DB/PreparedSQLUnitTest.inc | 6 ++++++ WordPress/Tests/DB/PreparedSQLUnitTest.php | 3 ++- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/WordPress/Helpers/WPDBTrait.php b/WordPress/Helpers/WPDBTrait.php index 66c3669307..3ef0f702f2 100644 --- a/WordPress/Helpers/WPDBTrait.php +++ b/WordPress/Helpers/WPDBTrait.php @@ -10,7 +10,9 @@ namespace WordPressCS\WordPress\Helpers; use PHP_CodeSniffer\Files\File; +use PHP_CodeSniffer\Util\Tokens; use PHPCSUtils\BackCompat\BCFile; +use PHPCSUtils\Tokens\Collections; /** * Helper utilities for sniffs which examine WPDB method calls. @@ -59,19 +61,14 @@ protected function is_wpdb_method_call( File $phpcsFile, $stackPtr, $target_meth } // Check that this is a method call. - $is_object_call = $phpcsFile->findNext( - array( \T_OBJECT_OPERATOR, \T_DOUBLE_COLON ), - ( $stackPtr + 1 ), - null, - false, - null, - true - ); - if ( false === $is_object_call ) { + $is_object_call = $phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); + if ( false === $is_object_call + || isset( Collections::objectOperators()[ $tokens[ $is_object_call ]['code'] ] ) === false + ) { return false; } - $methodPtr = $phpcsFile->findNext( \T_WHITESPACE, ( $is_object_call + 1 ), null, true, null, true ); + $methodPtr = $phpcsFile->findNext( Tokens::$emptyTokens, ( $is_object_call + 1 ), null, true, null, true ); if ( false === $methodPtr ) { return false; } @@ -81,7 +78,7 @@ protected function is_wpdb_method_call( File $phpcsFile, $stackPtr, $target_meth } // Find the opening parenthesis. - $opening_paren = $phpcsFile->findNext( \T_WHITESPACE, ( $methodPtr + 1 ), null, true, null, true ); + $opening_paren = $phpcsFile->findNext( Tokens::$emptyTokens, ( $methodPtr + 1 ), null, true, null, true ); if ( false === $opening_paren ) { return false; diff --git a/WordPress/Tests/DB/PreparedSQLUnitTest.inc b/WordPress/Tests/DB/PreparedSQLUnitTest.inc index ef58abc469..701e4f6262 100644 --- a/WordPress/Tests/DB/PreparedSQLUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLUnitTest.inc @@ -111,5 +111,11 @@ $wpdb->query( "SELECT * FROM ${wpdb->{${'a'}}} WHERE post_title LIKE '${title->{ // More defensive variable checking $wpdb->query( "SELECT * FROM $wpdb" ); // Bad x 1, $wpdb on its own is not valid. +$wpdb + -> /*comment*/ query( "SELECT * FROM $wpdb->posts WHERE post_title LIKE '" . $_GET['title'] . "';" ); // Bad. + +$wpdb?->query( "SELECT * FROM $wpdb->posts WHERE post_title LIKE '" . (int) $foo . "';" ); // OK. +$wpdb?->query( "SELECT * FROM $wpdb->posts WHERE post_title LIKE '" . foo() . "';" ); // Bad. + // Don't throw an error during live coding. wpdb::prepare( "SELECT * FROM $wpdb->posts diff --git a/WordPress/Tests/DB/PreparedSQLUnitTest.php b/WordPress/Tests/DB/PreparedSQLUnitTest.php index 0e679f0efc..4a7a4fd50c 100644 --- a/WordPress/Tests/DB/PreparedSQLUnitTest.php +++ b/WordPress/Tests/DB/PreparedSQLUnitTest.php @@ -53,6 +53,8 @@ public function getErrorList() { 108 => 1, 109 => 1, 112 => 1, + 115 => 1, + 118 => 1, ); } @@ -66,5 +68,4 @@ public function getErrorList() { public function getWarningList() { return array(); } - } From 50e97d48370df6f6d768d5f5b903367d4aef098d Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 14 Jul 2020 02:09:56 +0200 Subject: [PATCH 4/4] WPDBTrait::is_wpdb_method_call(): bug fix - check names case-insensitively In PHP class and function names are treated largely case-insensitively (for ascii names), so the method should compare names case-insensitively too. Includes unit tests via the `PreparedSQL` sniff. --- WordPress/Helpers/WPDBTrait.php | 7 ++++--- WordPress/Tests/DB/PreparedSQLUnitTest.inc | 3 +++ WordPress/Tests/DB/PreparedSQLUnitTest.php | 2 ++ 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/WordPress/Helpers/WPDBTrait.php b/WordPress/Helpers/WPDBTrait.php index 3ef0f702f2..3fb1c64947 100644 --- a/WordPress/Helpers/WPDBTrait.php +++ b/WordPress/Helpers/WPDBTrait.php @@ -45,7 +45,8 @@ trait WPDBTrait { * * @param \PHP_CodeSniffer\Files\File $phpcsFile The file being scanned. * @param int $stackPtr The index of the $wpdb variable. - * @param array $target_methods Array of methods. Key(s) should be method name. + * @param array $target_methods Array of methods. Key(s) should be method name + * in lowercase. * * @return bool Whether this is a $wpdb method call. */ @@ -55,7 +56,7 @@ protected function is_wpdb_method_call( File $phpcsFile, $stackPtr, $target_meth // Check for wpdb. if ( ( \T_VARIABLE === $tokens[ $stackPtr ]['code'] && '$wpdb' !== $tokens[ $stackPtr ]['content'] ) - || ( \T_STRING === $tokens[ $stackPtr ]['code'] && 'wpdb' !== $tokens[ $stackPtr ]['content'] ) + || ( \T_STRING === $tokens[ $stackPtr ]['code'] && 'wpdb' !== strtolower( $tokens[ $stackPtr ]['content'] ) ) ) { return false; } @@ -95,7 +96,7 @@ protected function is_wpdb_method_call( File $phpcsFile, $stackPtr, $target_meth } // Check that this is one of the methods that we are interested in. - if ( ! isset( $target_methods[ $tokens[ $methodPtr ]['content'] ] ) ) { + if ( ! isset( $target_methods[ strtolower( $tokens[ $methodPtr ]['content'] ) ] ) ) { return false; } diff --git a/WordPress/Tests/DB/PreparedSQLUnitTest.inc b/WordPress/Tests/DB/PreparedSQLUnitTest.inc index 701e4f6262..01a13f4ff3 100644 --- a/WordPress/Tests/DB/PreparedSQLUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLUnitTest.inc @@ -117,5 +117,8 @@ $wpdb $wpdb?->query( "SELECT * FROM $wpdb->posts WHERE post_title LIKE '" . (int) $foo . "';" ); // OK. $wpdb?->query( "SELECT * FROM $wpdb->posts WHERE post_title LIKE '" . foo() . "';" ); // Bad. +WPDB::prepare( "SELECT * FROM $wpdb->posts WHERE post_title LIKE '" . foo() . "';" ); // Bad. +$wpdb->Query( "SELECT * FROM $wpdb->posts WHERE post_title LIKE '" . foo() . "';" ); // Bad. + // Don't throw an error during live coding. wpdb::prepare( "SELECT * FROM $wpdb->posts diff --git a/WordPress/Tests/DB/PreparedSQLUnitTest.php b/WordPress/Tests/DB/PreparedSQLUnitTest.php index 4a7a4fd50c..59ca42a768 100644 --- a/WordPress/Tests/DB/PreparedSQLUnitTest.php +++ b/WordPress/Tests/DB/PreparedSQLUnitTest.php @@ -55,6 +55,8 @@ public function getErrorList() { 112 => 1, 115 => 1, 118 => 1, + 120 => 1, + 121 => 1, ); }