Sundry improvements of the VIP.DirectDatabaseQuery sniff #400
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is more in line with similar lists in other sniffs.
This code was added long ago (aa57302), but since then the sniff has evolved to the point that it is no longer needed. Specifically, the sniff is much more restrictive about when it flags `$wpdb`, limiting the errors to a predefined list of methods. One reason that duplicate messages used to be given on the same line was when you called `$wpdb->query( $wpdb->prepare( /* … */ ) );`, and both methods was be flagged. Because the `prepare()` method is no longer flagged, this problem no longer occurs. For good measure, I’ve also modified the sniff to return the `$endOfStatment` pointer, which will tell PHPCS not to sniff any more code until after the semicolon at the end of the statement. This would also prevent duplicate errors, and may even provide a small performance benefit as well.
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
This lets us merge the two lists of methods just once, and use `isset()` instead of `is_array()`, both of which provide improved performance.
westonruter
added a commit
that referenced
this pull request
Jun 5, 2015
Sundry improvements of the VIP.DirectDatabaseQuery sniff
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See the commit messages for more info.
Fixes #397