Move "is WPDB method call" related utilities to dedicated WPDBTrait + improve

[jrfnl]

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

WPDBTrait::is_wpdb_method_call(): use PHPCSUtils

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.

Related to #763

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.

[GaryJones]

Under what conditions would a T_STRING of wpdb be the equivalent of T_VARIABLE of $wpdb? Different PHPCS versions? Use of $this->wpdb? Use of $_GLOBALS['wpdb']? Something else?

[jrfnl]

Good question, which should probably have been asked 5 years ago when that code was added.

$this->wpdb is a typical one, where a plugin stores the global $wpdb variable in a property of their own class instead of having a global statement in every method, though in that case, the strtolower() should probably not be applied.

Another one would be a static method access wpdb::methodname(). And yes, I'm aware that at this time, the WP native wpdb class doesn't contain any static methods. May have been different in the past, may also have been put in place for plugins having their own PluginNamespace\wpdb wrapper class.

[GaryJones]

Does the assigned variable name suggest that this could become a PHPCSUtils method - quite generic?

$is_object_call = PHPCSUtils\Util\Foo::IsObjectCall( $stackPtr, $tokens ); // Or something similar.

[GaryJones]

I mean, reading the rest of the method, there's nothing WPDB-special in it - that only happens in the early checks. I wonder if all of it could be turned into something like:

protected function is_wpdb_method_call( File $phpcsFile, $stackPtr, $target_methods ) {
	// Check if it's wpdb-related
	// Check if it's a method call via PHPCSUtils
}

[jrfnl]

Does the assigned variable name suggest that this could become a PHPCSUtils method - quite generic?

The assigned variable name is the same as it was in the original code in the Sniff class, so no, this is no indication of anything.

I mean, reading the rest of the method, there's nothing WPDB-special in it - that only happens in the early checks

At this time, there is no intention for this to be moved to PHPCSUtils.

This method does weird and horrible stuff with properties in the (child) class using the method and there is no way I will ever allow that kind of code to get into PHPCSUtils.

There may be utilities in PHPCSUtils in the future which could simplify the code in this method, but I highly doubt it, as the code "walks tokens the wrong way" in my opinion (starts at the variable/class, not at the method call) and draws conclusions based on arbitrary local variable names (anything can be called $wpdb) which is unwarranted.

Aside from that, the property assignments being done require information which would be contained within the method in PHPCSUtils and not returned, so that would mean that the method here would need to duplicate what PHPCSUtils would be doing, which doesn't make much sense from a performance point of view.

[jrfnl]

Note: to be honest, as the sniffs using this method look at a specific parameter in a method call, I would much rather advocate for this method to be removed in the future in favour of a rewrite of those sniffs using an AbstractFunctionCallParameter base class combined with the PHPCSUtils PassedParameters class.

It's something I'd consider looking into when we switch to the abstracts in PHPCSUtils once available. If so, we can deprecate the method and remove it in the next major.

[GaryJones]

✅