From 1c45ebc08af0744c3af127a82aaa986eee13732b Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 7 Jan 2023 13:17:21 +0100 Subject: [PATCH 1/7] DB/DirectDatabaseQuery: remove redundant code This code is a remnant of the old-style custom ignore comment mechanism which was removed in 2063. --- WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php b/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php index 56ca4ef699..83a0633d11 100644 --- a/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php +++ b/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php @@ -186,18 +186,7 @@ public function process_token( $stackPtr ) { return; } - $endOfStatement = $this->phpcsFile->findNext( \T_SEMICOLON, ( $stackPtr + 1 ), null, false, null, true ); - $endOfLineComment = ''; - for ( $i = ( $endOfStatement + 1 ); $i < $this->phpcsFile->numTokens; $i++ ) { - - if ( $this->tokens[ $i ]['line'] !== $this->tokens[ $endOfStatement ]['line'] ) { - break; - } - - if ( \T_COMMENT === $this->tokens[ $i ]['code'] ) { - $endOfLineComment .= $this->tokens[ $i ]['content']; - } - } + $endOfStatement = $this->phpcsFile->findNext( \T_SEMICOLON, ( $stackPtr + 1 ), null, false, null, true ); // Check for Database Schema Changes. for ( $_pos = ( $stackPtr + 1 ); $_pos < $endOfStatement; $_pos++ ) { From 5f44d4e1623eb71b0a44c3b892fa68339f120cdb Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 7 Jan 2023 12:57:40 +0100 Subject: [PATCH 2/7] DB/DirectDatabaseQuery: add missing test case ... to cover the one uncovered line. --- WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc | 6 ++++++ WordPress/Tests/DB/DirectDatabaseQueryUnitTest.php | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc b/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc index 88f1fba18d..ad2d791830 100644 --- a/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc +++ b/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc @@ -274,3 +274,9 @@ EOD wp_cache_add( 'baz', $baz ); } } + +// Non-cachable call should bow out after `DirectQuery` warning. +function non_cachable() { + global $wpdb; + $wpdb->insert( 'table', array( 'column' => 'foo', 'field' => 'bar' ) ); // Warning direct DB call. +} diff --git a/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.php b/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.php index 02117429f3..b6b8dd2232 100644 --- a/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.php +++ b/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.php @@ -84,7 +84,7 @@ public function getWarningList() { 252 => 1, 265 => 1, 269 => 1, + 281 => 1, ); } - } From 0b002506db9347b46e010b73c4c4f4cdfa1020b4 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 2 Jul 2020 07:01:40 +0200 Subject: [PATCH 3/7] DB/DirectDatabaseQuery: implement PHPCSUtils :point_right: This change will be easier to review while ignoring whitespace changes. --- .../Sniffs/DB/DirectDatabaseQuerySniff.php | 42 +++++++++---------- 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php b/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php index 83a0633d11..ba3a1a99de 100644 --- a/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php +++ b/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php @@ -10,6 +10,8 @@ namespace WordPressCS\WordPress\Sniffs\DB; use PHP_CodeSniffer\Util\Tokens; +use PHPCSUtils\Tokens\Collections; +use PHPCSUtils\Utils\Conditions; use WordPressCS\WordPress\Helpers\RulesetPropertyHelper; use WordPressCS\WordPress\Sniff; @@ -208,36 +210,30 @@ public function process_token( $stackPtr ) { $cached = false; $wp_cache_get = false; - if ( ! empty( $this->tokens[ $stackPtr ]['conditions'] ) ) { - $scope_function = $this->phpcsFile->getCondition( $stackPtr, \T_FUNCTION ); - if ( false === $scope_function ) { - $scope_function = $this->phpcsFile->getCondition( $stackPtr, \T_CLOSURE ); - } - - if ( false !== $scope_function ) { - $scopeStart = $this->tokens[ $scope_function ]['scope_opener']; - $scopeEnd = $this->tokens[ $scope_function ]['scope_closer']; + $scope_function = Conditions::getLastCondition( $this->phpcsFile, $stackPtr, Collections::functionDeclarationTokens() ); + if ( false !== $scope_function ) { + $scopeStart = $this->tokens[ $scope_function ]['scope_opener']; + $scopeEnd = $this->tokens[ $scope_function ]['scope_closer']; - for ( $i = ( $scopeStart + 1 ); $i < $scopeEnd; $i++ ) { - if ( \T_STRING === $this->tokens[ $i ]['code'] ) { + for ( $i = ( $scopeStart + 1 ); $i < $scopeEnd; $i++ ) { + if ( \T_STRING === $this->tokens[ $i ]['code'] ) { - if ( isset( $this->cacheDeleteFunctions[ $this->tokens[ $i ]['content'] ] ) ) { + if ( isset( $this->cacheDeleteFunctions[ $this->tokens[ $i ]['content'] ] ) ) { - if ( \in_array( $method, array( 'query', 'update', 'replace', 'delete' ), true ) ) { - $cached = true; - break; - } - } elseif ( isset( $this->cacheGetFunctions[ $this->tokens[ $i ]['content'] ] ) ) { + if ( \in_array( $method, array( 'query', 'update', 'replace', 'delete' ), true ) ) { + $cached = true; + break; + } + } elseif ( isset( $this->cacheGetFunctions[ $this->tokens[ $i ]['content'] ] ) ) { - $wp_cache_get = true; + $wp_cache_get = true; - } elseif ( isset( $this->cacheSetFunctions[ $this->tokens[ $i ]['content'] ] ) ) { + } elseif ( isset( $this->cacheSetFunctions[ $this->tokens[ $i ]['content'] ] ) ) { - if ( $wp_cache_get ) { - $cached = true; - break; - } + if ( $wp_cache_get ) { + $cached = true; + break; } } } From 3ce178cc5da2482da74e5967937b5d0ce684cd80 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 2 Aug 2020 07:42:20 +0200 Subject: [PATCH 4/7] DB/DirectDatabaseQuery: add support for PHP 8.0+ nullsafe object operator Prevent false negatives in case someone would use the PHP 8.0 nullsafe object operator on the `$wpdb` variable. Also makes the token walking more stable by preventing an unrelated object operator from being recognized as if it were an operator operating on the `$wpdb` object (false positive). Includes tests. --- WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php | 10 +++++++--- WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc | 14 ++++++++++++++ WordPress/Tests/DB/DirectDatabaseQueryUnitTest.php | 2 ++ 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php b/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php index ba3a1a99de..a860dce512 100644 --- a/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php +++ b/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php @@ -174,9 +174,13 @@ public function process_token( $stackPtr ) { return; } - $is_object_call = $this->phpcsFile->findNext( \T_OBJECT_OPERATOR, ( $stackPtr + 1 ), null, false, null, true ); - if ( false === $is_object_call ) { - return; // This is not a call to the wpdb object. + $is_object_call = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); + if ( false === $is_object_call + || ( \T_OBJECT_OPERATOR !== $this->tokens[ $is_object_call ]['code'] + && \T_NULLSAFE_OBJECT_OPERATOR !== $this->tokens[ $is_object_call ]['code'] ) + ) { + // This is not a call to the wpdb object. + return; } $methodPtr = $this->phpcsFile->findNext( array( \T_WHITESPACE ), ( $is_object_call + 1 ), null, true, null, true ); diff --git a/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc b/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc index ad2d791830..5ee85cbac1 100644 --- a/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc +++ b/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc @@ -280,3 +280,17 @@ function non_cachable() { global $wpdb; $wpdb->insert( 'table', array( 'column' => 'foo', 'field' => 'bar' ) ); // Warning direct DB call. } + +// Safeguard recognition of PHP 8.0+ nullsafe object operator. +function nullsafe_object_operator() { + global $wpdb; + $listofthings = $wpdb?->get_col( 'SELECT something FROM somewhere WHERE someotherthing = 1' ); // Warning x 2. + $last = $wpdb?->insert( $wpdb->users, $data, $where ); // Warning x 1. +} + +function stabilize_token_walking($other_db) { + global $wpdb; + // Overwritting $wpdb is bad, of course, but that's not the concern of this sniff. + // This test is making sure that the `$other_db->query` code is not flagged as if it were a call to a `$wpdb` method. + $wpdb = $other_db->query; +} diff --git a/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.php b/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.php index b6b8dd2232..c6b700f30b 100644 --- a/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.php +++ b/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.php @@ -85,6 +85,8 @@ public function getWarningList() { 265 => 1, 269 => 1, 281 => 1, + 287 => 2, + 288 => 1, ); } } From 37003f0588398803cfc9d5be8fab3ee9640b6ad3 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 7 Jan 2023 13:36:50 +0100 Subject: [PATCH 5/7] DB/DirectDatabaseQuery: bug fix - code style independent token walking Don't allow the sniff to get confused by comments. Related to 763 Includes tests. --- WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php | 2 +- WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc | 7 +++++++ WordPress/Tests/DB/DirectDatabaseQueryUnitTest.php | 1 + 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php b/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php index a860dce512..ea7a28ae5f 100644 --- a/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php +++ b/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php @@ -183,7 +183,7 @@ public function process_token( $stackPtr ) { return; } - $methodPtr = $this->phpcsFile->findNext( array( \T_WHITESPACE ), ( $is_object_call + 1 ), null, true, null, true ); + $methodPtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $is_object_call + 1 ), null, true ); $method = $this->tokens[ $methodPtr ]['content']; $this->mergeFunctionLists(); diff --git a/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc b/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc index 5ee85cbac1..f8bd29ffe0 100644 --- a/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc +++ b/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc @@ -294,3 +294,10 @@ function stabilize_token_walking($other_db) { // This test is making sure that the `$other_db->query` code is not flagged as if it were a call to a `$wpdb` method. $wpdb = $other_db->query; } + +function ignore_comments() { + global $wpdb; + $wpdb-> // Let's pretend this is a method-chain (this is the comment which should not confuse the sniff). + insert( 'table', array( 'column' => 'foo', 'field' => 'bar' ) ); // Warning direct DB call. +} + diff --git a/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.php b/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.php index c6b700f30b..44c090d4a8 100644 --- a/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.php +++ b/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.php @@ -87,6 +87,7 @@ public function getWarningList() { 281 => 1, 287 => 2, 288 => 1, + 300 => 1, ); } } From 307ff0b5a949ee7a4b0caa36461751519355633f Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 7 Jan 2023 13:48:29 +0100 Subject: [PATCH 6/7] DB/DirectDatabaseQuery: bug fix - finding end of statement Prevent false positives from walking too far when a database query statement is ended by a PHP close tag. Includes unit tests. --- WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php | 4 ++-- WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc | 6 ++++++ WordPress/Tests/DB/DirectDatabaseQueryUnitTest.php | 1 + 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php b/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php index ea7a28ae5f..1da2b668d8 100644 --- a/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php +++ b/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php @@ -192,11 +192,11 @@ public function process_token( $stackPtr ) { return; } - $endOfStatement = $this->phpcsFile->findNext( \T_SEMICOLON, ( $stackPtr + 1 ), null, false, null, true ); + $endOfStatement = $this->phpcsFile->findNext( array( \T_SEMICOLON, \T_CLOSE_TAG ), ( $stackPtr + 1 ) ); // Check for Database Schema Changes. for ( $_pos = ( $stackPtr + 1 ); $_pos < $endOfStatement; $_pos++ ) { - $_pos = $this->phpcsFile->findNext( Tokens::$textStringTokens, $_pos, $endOfStatement, false, null, true ); + $_pos = $this->phpcsFile->findNext( Tokens::$textStringTokens, $_pos, $endOfStatement ); if ( false === $_pos ) { break; } diff --git a/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc b/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc index f8bd29ffe0..941c6a8c5b 100644 --- a/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc +++ b/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc @@ -301,3 +301,9 @@ function ignore_comments() { insert( 'table', array( 'column' => 'foo', 'field' => 'bar' ) ); // Warning direct DB call. } +function correctly_determine_end_of_statement() { + global $wpdb; + $wpdb->get_col( $query, $col ) ?><-- Warning x 2 --> + 2, 288 => 1, 300 => 1, + 306 => 2, ); } } From 0913f35d3ae5748061e4121c0996a9578e8ed326 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 7 Jan 2023 13:57:16 +0100 Subject: [PATCH 7/7] DB/DirectDatabaseQuery: bug fix - ignore `TRUNCATE` queries Prevent unsolvable false positives for `TRUNCATE` queries. Those cannot be cached and need a direct DB query. (second opinion appreciated!) Includes unit tests. Fixes 1947 --- WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php | 8 +++++++- WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc | 10 ++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php b/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php index 1da2b668d8..2f4d86372b 100644 --- a/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php +++ b/WordPress/Sniffs/DB/DirectDatabaseQuerySniff.php @@ -12,6 +12,7 @@ use PHP_CodeSniffer\Util\Tokens; use PHPCSUtils\Tokens\Collections; use PHPCSUtils\Utils\Conditions; +use PHPCSUtils\Utils\TextStrings; use WordPressCS\WordPress\Helpers\RulesetPropertyHelper; use WordPressCS\WordPress\Sniff; @@ -194,13 +195,18 @@ public function process_token( $stackPtr ) { $endOfStatement = $this->phpcsFile->findNext( array( \T_SEMICOLON, \T_CLOSE_TAG ), ( $stackPtr + 1 ) ); - // Check for Database Schema Changes. + // Check for Database Schema Changes/ table truncation. for ( $_pos = ( $stackPtr + 1 ); $_pos < $endOfStatement; $_pos++ ) { $_pos = $this->phpcsFile->findNext( Tokens::$textStringTokens, $_pos, $endOfStatement ); if ( false === $_pos ) { break; } + if ( strpos( TextStrings::stripQuotes( $this->tokens[ $_pos ]['content'] ), 'TRUNCATE ' ) === 0 ) { + // Ignore queries to truncate the database as caching those is irrelevant and they need a direct db query. + return; + } + if ( preg_match( '#\b(?:ALTER|CREATE|DROP)\b#i', $this->tokens[ $_pos ]['content'] ) > 0 ) { $this->phpcsFile->addWarning( 'Attempting a database schema change is discouraged.', $_pos, 'SchemaChange' ); } diff --git a/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc b/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc index 941c6a8c5b..2e75d64ece 100644 --- a/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc +++ b/WordPress/Tests/DB/DirectDatabaseQueryUnitTest.inc @@ -307,3 +307,13 @@ function correctly_determine_end_of_statement() { query( + $wpdb->prepare( + 'TRUNCATE TABLE `%1$s`', + plugin_get_table_name( 'Name' ) + ) + ); +}