WordPress / WordPress/WordPress-Coding-Standards
`WPDBTrait::is_wpdb_method_call()`: false positive for a method call on a non-wpdb object
Nobody has claimed this yet.
- Dominant language
- PHP
- Stars
- 2.8k
- Forks
- 521
- Avg merge
- 5d 20h
- Merged PRs (30d)
- 1
Description
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()?
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in WordPress/Helpers/WPDBTrait.php around the receiver-token validation described in the issue, then inspect its use by the WordPress.DB.PreparedSQL and WordPress.DB.PreparedSQLPlaceholders sniffs. Exercise the method with the provided my_function()->prepare() example and the $this->wpdb example. Done means unrelated function-call receivers are rejected while intentionally supported wpdb-like receivers retain their expected behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- php
- Domain
- tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100