From b7d541689ff2db33981f46d73fa971365ac78745 Mon Sep 17 00:00:00 2001 From: JDGrimes Date: Fri, 5 Jun 2015 16:39:08 -0400 Subject: [PATCH] Allow for wp_cache_add() and wp_cache_delete() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This sniff checks that any direct database calls are accompanied by caching. Up to now, only `wp_cache_get()` and `wp_cache_set()` were checked for. The sniff requires that these two functions both be called in order for the database call to pass. But it is possible to use `wp_cache_add()` to add to the cache instead of `wp_cache_set()`, so I have updated the sniff to provide for this, checking for either set or add, but either must still be used along with get. There are also cases, such as when a complex `UPDATE` query must be run with `$wpdb->query()`, where this form of get-set/add caching does not apply. In such a case, it is necessary to call `wp_cache_delete()` instead. So I have updated the sniff to allow for the `query()` method to pass when accompanied by a cache delete **or** when accompanied by cache get and set/add. In the process I’ve realized that whenever the `update()`, `delete()`, or `replace()` methods are used, there is also a need to clear the cache. So I have moved these to the list of cacheable functions, leaving only `insert()` as non-cacheable. Like `query()`, these functions will pass the caching part of the sniff if they are accompanied by a cache delete, or if they are accompanied by a cache get and add/set. The reason that the latter is also allowed, is for the case when the cache make be an array of things, only one of which is being updated/deleted. In the process, I found out that there was a method in the list, `execute()` which does not exist as part of `$wpdb`. I don’t know where it came from, but I’ve removed it from the list and added `delete()`, which was previously absent. For those who care, I think that I might even have squeaked a small performance boost out of the sniff in the process. Fixes #397 --- .../Sniffs/VIP/DirectDatabaseQuerySniff.php | 42 +++++++++++------- .../Tests/VIP/DirectDatabaseQueryUnitTest.inc | 43 ++++++++++++++++++- .../Tests/VIP/DirectDatabaseQueryUnitTest.php | 3 ++ 3 files changed, 71 insertions(+), 17 deletions(-) diff --git a/WordPress/Sniffs/VIP/DirectDatabaseQuerySniff.php b/WordPress/Sniffs/VIP/DirectDatabaseQuerySniff.php index 3deec5ade0..45a91305de 100644 --- a/WordPress/Sniffs/VIP/DirectDatabaseQuerySniff.php +++ b/WordPress/Sniffs/VIP/DirectDatabaseQuerySniff.php @@ -12,8 +12,8 @@ class WordPress_Sniffs_VIP_DirectDatabaseQuerySniff implements PHP_CodeSniffer_Sniff { protected $methods = array( - 'cachable' => array( 'get_var', 'get_col', 'get_row', 'get_results', 'query' ), - 'noncachable' => array( 'execute', 'insert', 'update', 'replace' ), + 'cachable' => array( 'delete', 'get_var', 'get_col', 'get_row', 'get_results', 'query', 'replace', 'update' ), + 'noncachable' => array( 'insert' ), ); /** @@ -95,35 +95,45 @@ public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) } $whitelisted_cache = false; - $cached = false; + $cached = $wp_cache_get = false; if ( preg_match( '/cache\s+(ok|pass|clear|whitelist)/i', $endOfLineComment, $matches ) ) { $whitelisted_cache = true; } if ( ! $whitelisted_cache && ! empty( $tokens[$stackPtr]['conditions'] ) ) { - $conditions = $tokens[$stackPtr]['conditions']; - $scope_function = null; - foreach ( $conditions as $condPtr => $condType ) { - if ( $condType == T_FUNCTION ) { - $scope_function = $condPtr; - } - } + $scope_function = $phpcsFile->getCondition( $stackPtr, T_FUNCTION ); if ( $scope_function ) { $scopeStart = $tokens[$scope_function]['scope_opener']; $scopeEnd = $tokens[$scope_function]['scope_closer']; - $wpcacheget = $phpcsFile->findNext( array( T_STRING ), $scopeStart + 1, $stackPtr - 1, null, 'wp_cache_get' ); - $wpcacheset = $phpcsFile->findNext( array( T_STRING ), $stackPtr + 1, $scopeEnd - 1, null, 'wp_cache_set' ); + for ( $i = $scopeStart + 1; $i < $scopeEnd; $i++ ) { + if ( T_STRING === $tokens[ $i ]['code'] ) { + + if ( 'wp_cache_delete' === $tokens[ $i ]['content'] ) { + + if ( in_array( $method, array( 'query', 'update', 'replace', 'delete' ) ) ) { + $cached = true; + break; + } - if ( $wpcacheget && $wpcacheset ) { - $cached = true; + } elseif ( 'wp_cache_get' === $tokens[ $i ]['content'] ) { + + $wp_cache_get = true; + + } elseif ( in_array( $tokens[ $i ]['content'], array( 'wp_cache_set', 'wp_cache_add' ) ) ) { + + if ( $wp_cache_get ) { + $cached = true; + break; + } + } + } } } - } if ( ! $cached && ! $whitelisted_cache ) { - $message = 'Usage of a direct database call without caching is prohibited. Use wp_cache_get / wp_cache_set.'; + $message = 'Usage of a direct database call without caching is prohibited. Use wp_cache_get / wp_cache_set or wp_cache_delete.'; $phpcsFile->addError( $message, $stackPtr, 'NoCaching' ); } diff --git a/WordPress/Tests/VIP/DirectDatabaseQueryUnitTest.inc b/WordPress/Tests/VIP/DirectDatabaseQueryUnitTest.inc index c8139d5283..1cfe139e86 100644 --- a/WordPress/Tests/VIP/DirectDatabaseQueryUnitTest.inc +++ b/WordPress/Tests/VIP/DirectDatabaseQueryUnitTest.inc @@ -61,4 +61,45 @@ function taz() { /* @var wpdb $wpdb */ global $wpdb; echo $wpdb->insert_id; // Good, no actual call, and doesn't need any caching -} \ No newline at end of file +} + +// Some $wpdb methods can pass with only deleting the cache. +function cache_delete_only() { + global $wpdb; + + $data = $where = array(); + + // These methods are allowed to be used with just wp_cache_delete(). + $wpdb->update( $wpdb->users, $data, $where ); // db call ok; OK + $wpdb->replace( $wpdb->users, $data, $where ); // db call ok; OK + $wpdb->delete( $wpdb->users, $data, $where ); // db call ok; OK + $wpdb->query( 'SELECT X FROM Y' ); // db call ok; OK + + $wpdb->get_results( 'SELECT X FROM Y' ); // db call ok; Bad + $wpdb->get_row( 'SELECT X FROM Y' ); // db call ok; Bad + $wpdb->get_col( 'SELECT X FROM Y' ); // db call ok; Bad + + wp_cache_delete( 'key', 'group' ); +} + +// It is OK to use the wp_cache_add() function in place of wp_cache_set(). +function cache_add_instead_of_set() { + global $wpdb; + + $baz = wp_cache_get( 'baz' ); + + if ( false !== $baz ) { + + $data = $where = array(); + + $wpdb->update( $wpdb->users, $data, $where ); // db call ok; OK + $wpdb->replace( $wpdb->users, $data, $where ); // db call ok; OK + $wpdb->delete( $wpdb->users, $data, $where ); // db call ok; OK + $wpdb->query( 'SELECT X FROM Y' ); // db call ok; OK + $wpdb->get_row( 'SELECT X FROM Y' ); // db call ok; OK + $wpdb->get_col( 'SELECT X FROM Y' ); // db call ok; OK + $baz = $wpdb->get_results( $wpdb->prepare( 'SELECT X FROM Y ' ) ); // db call ok; OK + + wp_cache_add( 'baz', $baz ); + } +} diff --git a/WordPress/Tests/VIP/DirectDatabaseQueryUnitTest.php b/WordPress/Tests/VIP/DirectDatabaseQueryUnitTest.php index 5eff41d1e4..f6bf3a376e 100644 --- a/WordPress/Tests/VIP/DirectDatabaseQueryUnitTest.php +++ b/WordPress/Tests/VIP/DirectDatabaseQueryUnitTest.php @@ -30,6 +30,9 @@ public function getErrorList() 32 => 1, 34 => 1, 50 => 1, + 78 => 1, + 79 => 1, + 80 => 1, ); }//end getErrorList()