`WPDBTrait::is_wpdb_method_call()`: false positive for a method call on a non-wpdb object

[rodrigoprimo]

Bug Description

WPDBTrait::is_wpdb_method_call(), used by the WordPress.DB.PreparedSQL and WordPress.DB.PreparedSQLPlaceholders sniffs, determines whether a piece of code is a call to one of a set of $wpdb methods.

While writing tests for this method, I noticed that it returns true for a method call made on the return value of a function call, such as my_function()->prepare(). In this case, I believe the method should return false.

The root cause is in how the method validates the token the call is made on. It only checks the content when that token is a T_VARIABLE or a T_STRING, but it performs no check at all for any other token type (WordPress/Helpers/WPDBTrait.php#L69-L73). As a result, a token that is neither a T_VARIABLE nor a T_STRING, such as the T_CLOSE_PARENTHESIS that ends my_function(), passes this check and, as long as it is followed by an object operator and a target method call, is accepted as a $wpdb method call.

Minimal code snippet

my_function()->prepare( 'SELECT * FROM table' );

Expected behavior

is_wpdb_method_call() returns false.

Actual behavior

is_wpdb_method_call() returns true.

Impact

This is currently not reachable through the two sniffs that use the method, so I believe fixing it should be a low priority. Both sniffs register T_VARIABLE and T_STRING, so the method is only ever called on tokens of those two types. Never on a token like the T_CLOSE_PARENTHESIS in my_function() that triggers this bug. So there is no user-facing false positive today.

Open question

A strict fix would require the receiver to be literally the $wpdb variable or the wpdb class name. However, there are receivers that are not the literal global, but that could still legitimately be the wpdb object, and maybe there is an argument to keep supporting them. For example, a property holding the wpdb instance:

$this->wpdb->prepare( "SELECT * FROM table WHERE id = $id" );

$this->wpdb->prepare() is currently recognized as a $wpdb method call (and WordPress.DB.PreparedSQL flags the interpolated variable), and this may be behavior we do not want to change.

So the question is where to draw the line: which receivers that are not the literal $wpdb variable or wpdb class should the method remain flexible about (e.g. $this->wpdb), while still rejecting clearly unrelated receivers such as my_function()?