#65817 Narrow the accepted and returned types for esc_sql()

[johnbillion]

Narrows array to string[] and adds a PHPStan generic.

I opted to use the @phpstan- prefix for the generic. Still need to make a decision on the preferred approach in general for prefixing tags.

Trac ticket: Core-65817

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props johnbillion, westonruter, irozum.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[westonruter]

Note PHPStan is currently complaining about this line:

Parameter #1 $data of method wpdb::_escape() contains unresolvable type.

I'm working on a solution.

[westonruter]

New commits fix this.

[westonruter]

🤖 Comment from Claude Opus 5

The @phpstan-template T on these two functions reports three errors on wpdb::_escape(). Chasing them ended up somewhere different from where it started, so here is both the result and the route.

1309  Parameter #1 $data of method wpdb::_escape() contains unresolvable type.    [argument.unresolvableType]
1309  Return type of call to method wpdb::_escape() contains unresolvable type.   [method.unresolvableReturnType]
1318  Method wpdb::_escape() should return T of array<string>|string
      but returns array<string>|string.                                          [return.type]

Why a template cannot work here

@phpstan-return T requires PHPStan to prove the returned value is the caller's exact T. _escape() never passes its argument through — it rebuilds it ($data[ $k ] = …) or reassigns it ($data = $this->_real_escape( $data )), and both produce a plain string / array<string>, not T.

This is not about the recursion. Deleting the recursive branch entirely, leaving a flat loop, leaves the error verbatim. Templates carry through pass-through functions, which is exactly why esc_sql() reported nothing while _escape() did. No choice of bound fixes it.

The other two errors come from the recursive branch. tests/phpstan/base.neon sets treatPhpDocTypesAsCertain: false, so PHPStan still analyses if ( is_array( $v ) ) even though $v is a string under the narrowed contract — and it still narrows inside, where \PHPStan\dumpType( $v ) reports *NEVER*. A template parameter cannot be inferred from never.

What is committed

@phpstan-template TKey of array-key
@phpstan-param string|array<TKey, string> $data
@phpstan-return ( $data is string ? string : array<TKey, string> )

on both functions, with the body of _escape() unchanged — the diff is docblocks only, so there is no behaviour to review and nested arrays keep working exactly as before. That was verified by reimplementing the original algorithm alongside and diffing both on flat strings, 2-deep, 4-deep, mixed depths, empty arrays, non-scalar leaves, int/bool/null leaves, and preserved string and int keys: identical on all ten.

Callers get what the template was meant to provide, plus key preservation:

input	result
string	string
string[]	array<string>
array<string, string>	array<string, string>
array{a: string, b: string}	array<'a'|'b', string>
implode( ',', … ) over the result	clean

The ordering is load-bearing

Testing $data is string first is what makes the unchanged body type-check. Inside the dead recursive branch $data is never, and a never argument satisfies whichever case is tested first. With string first the recursive call resolves to string and nothing widens. The reverse order resolves it to an array, widening $data to array<array<string>|string> and contradicting the return type:

• ( $data is string ? string : string[] ) → clean
• ( $data is array ? string[] : string ) → should return array<string>|string but returns array<array<string>|string>|string

There is a comment above the method saying so, because it is not obvious and the failure mode if someone flips it is confusing.

Two dead ends worth recording, so nobody re-walks them. A genuinely recursive type is not expressible: @phpstan-type EscapableData string|array<array-key, EscapableData> is rejected as typeAlias.circular, and any bounded depth is off by one, because the recursive call always produces one level more than declared. And widening to string|mixed[] measures badly — it adds 12 implode expects array<string>, array<mixed> given errors across the core call sites and silences two genuine findings, while giving callers nothing over the current string|array.

Effect on core call sites

Measured across the ten files that call esc_sql(): 8 errors resolved, all implode expects array<string>, array<mixed> given in WP_Site_Query and WP_Network_Query, which call _escape() directly.

Six argument.templateType errors appear in WP_Comment_Query, WP_User_Query, and WP_Date_Query, where the value passed to esc_sql() is mixed so no key type can be inferred. Those same lines already report the argument type for the same reason, so this is a second symptom of an existing gap rather than new breakage. I have a follow-up branch that clears all six by typing the clause arrays in WP_Meta_Query and WP_Date_Query — it removes 51 errors across those ten files — but it touches three unrelated classes and belongs in its own ticket rather than here.

One known limitation

PHPStan validates a conditional return type by checking the body against the union of its cases. The array<TKey, string> case is concrete enough to be checked: probes that deliberately corrupt either the value type or the string case are both reported. An earlier draft used a mixed[] fallback instead, which swallowed any array and made that half of the annotation unfalsifiable — dropping it restored the check.

While in here: no core call site passes a multi-dimensional array to esc_sql(), and every one feeds the result straight to implode(), join(), or sprintf(), which break on nested arrays regardless of how well the leaves were escaped. The recursion in _escape() has no reachable beneficiary in core. Worth knowing before anyone treats the narrowed string[] contract as a regression.

[westonruter]

@irozum As I mentioned in #13023 (comment), it seems like #12975 (review) was written by AI. When you use AI to add reviews, please disclose how you have done so. Otherwise, it is misleading given that your comment says “I” and “me” when actually it was “it”. Please refer to the AI Guidelines.