WordPress / WordPress/WordPress-Coding-Standards

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

Open
#2,759 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Component: Helpers Priority: Low Type: Bug
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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.