Docs: Clarify return value semantics of wpdb query methods#11855
Docs: Clarify return value semantics of wpdb query methods#11855apermo wants to merge 9 commits into
wpdb query methods#11855Conversation
The `@return` line previously documented only the union type `array|object|null`, without stating which branch is returned in which situation. The user-facing reference on developer.wordpress.org already explains this correctly; this brings the inline docblock in line with that documentation. No behaviour change. See #65261.
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
Can we go the extra mile and provide a
I think this would be: Where |
|
Yeah, makes sense. A couple of small things while drafting it:
Here is what I have so far: While looking at this I noticed something else. The current Two questions:
|
I think it's preferable to fix up PHPStan issues in the course of making other related changes. It reduces the amount of work to make commits.
Yeah, I think a PR that fixes up the return semantics of all methods on WPDB would be good. So if anything, the ticket scope can be changed. |
Adds a `@phpstan-return` conditional type that mirrors the runtime dispatch on `$query` and `$output`, and corrects two longstanding docblock inaccuracies on the surrounding tags: * `@param string $query` becomes `@param string|null $query`. The method signature defaults `$query` to `null`, so `string|null` is the honest type. `wpdb::get_row()` already uses this form. * `@return array|object|null` becomes `@return array|null`. The method never returns a bare `stdClass`; every branch returns an array of rows (of `stdClass`, associative arrays, or numeric arrays) or `null`. The `object` in the union appears to be a copy/paste artefact from `wpdb::get_row()`. The case-insensitive `$output` back-compat path is intentionally not modelled. Anything outside the documented constants folds into the `else` branch and types as `null`. No behaviour change. See #65261.
Documents the per-`$output` return shapes via a PHPStan conditional type that mirrors the runtime dispatch. The existing `@return array|object|null` already accurately describes the possible types, so it is left untouched. The case-insensitive `$output` back-compat path is not modelled; it folds into the `else` branch and types as `null`. No behaviour change. See #65261.
Tightens the element type via PHPStan. Each element of the returned list is the value of column `$x` for a row in the last result, fetched through `wpdb::get_var()`, which returns `string|null`. The list is sequentially indexed from 0, so the refined type is `list<string|null>`. The human-readable `@return array` line is left untouched. No behaviour change. See #65261.
The implementation returns `null` not only on query failure but also when the matched cell value is an empty string. The existing "or null on failure" wording masks the empty-string case, which has caused repeated confusion (see Trac #30257 and the Documentation-Issue-Tracker entry for `get_var()`). This documents the empty-string path and points consumers that need to distinguish failure from an empty result at `$this->last_error`. No conditional `@phpstan-return` is added; the return type does not vary with `$query` or `$x`/`$y`. No behaviour change. See #65261.
apermo
changed the title
wpdb::get_results()wpdb query methods
|
Pushed four follow-up commits, one per method.
Verified with PR title and description are updated to reflect the broader scope. The Trac ticket still says Take a look whenever. |
…`wpdb::get_results()`. Pins `$output` to the literal-string union of the documented constants. Lets PHPStan flag callers that pass typos or the undocumented lowercase form (e.g. `'object'`), which currently works only through the case-insensitive back-compat path in each method. `wpdb::get_results()` accepts `OBJECT`, `OBJECT_K`, `ARRAY_A`, or `ARRAY_N`. `wpdb::get_row()` accepts `OBJECT`, `ARRAY_A`, or `ARRAY_N` (no `OBJECT_K`). No core callers use the lowercase form. No behaviour change. See #65261.
…s()`. `OBJECT` and `OBJECT_K` produce different array shapes and were previously collapsed under one branch: * `OBJECT` returns `$this->last_result`, which is populated by `$this->last_result[ $num_rows ] = $row;` and is therefore a zero-indexed `list<stdClass>`. * `OBJECT_K` builds a fresh array keyed by the value of each row's first column, which is `array<array-key, stdClass>` and is not a list (keys can be string or int, duplicates are discarded). `ARRAY_A` and `ARRAY_N` similarly populate `$new_array[]`, so their outer types are `list<...>` rather than `array<int, ...>`. No behaviour change. See #65261.
The previous `@return` line ran to four sentences, which is at odds with WP-core convention of keeping `@return` terse and moving the rationale into the description block. Moves the empty-string-vs-failure explanation and the `$this->last_error` pointer into the prose paragraph above `@param`, and tightens the `@return` line to a single sentence that still mentions both null causes. No behaviour change. See #65261.
Matches the formatting of the existing `@phpstan-return` precedent on `wp_parse_url()` in `wp-includes/http.php`, where the `@phpstan-` tags follow `@return` directly without an empty line between them. Applied to all three docblocks that now carry `@phpstan-return` in this class: `wpdb::get_row()`, `wpdb::get_col()`, and `wpdb::get_results()`. No behaviour change. See #65261.
|
Pushed four more commits after running the diff through a fresh independent review.
PHPStan verified again: no new errors. The two preexisting ones in PR description updated to reflect all nine commits. Take another look whenever. |
2 participants
Clarifies the inline documentation for the four
wpdbquery methods (get_results(),get_row(),get_col(),get_var()) and adds PHPStan annotations where the runtime dispatch branches by parameter value.Per-commit summary:
get_results()— descriptive text. Documents that an empty array is returned when no rows match or on a database error, and thatnullis returned only when$queryis empty or$outputis not one of the recognized constants. Lifts the developer.wordpress.org wording into the inline docblock.get_results()— PHPStan + type tightening. Adds a@phpstan-returnconditional type that mirrors the runtime dispatch on$queryand$output. Refines@param string $querytostring|null(matchesget_row()'s existing form). Tightens@return array|object|nulltoarray|null— the method never returns a barestdClass; theobjectin the union appears to be a copy/paste artefact fromget_row().get_row()— PHPStan. Adds a@phpstan-returnconditional type for the per-$outputreturn shapes.@return array|object|nullis already accurate here and is left untouched.get_col()— PHPStan. Adds@phpstan-return list<string|null>to tighten the element type. Each element is the result of aget_var()call.get_var()— descriptive text. Clarifies thatnullis also returned when the matched cell value is an empty string (Trac #30257 documented this as intended behavior years ago, but the docblock has continued to say "null on failure" only). Points consumers at$this->last_errorfor distinguishing the two cases.get_row()andget_results()—@phpstan-param. Pins$outputto the literal-string union of the documented constants. Lets PHPStan flag callers that pass typos or the undocumented lowercase form (e.g.'object'). Core has no such callers.get_results()— refined return shapes. SplitsOBJECTandOBJECT_K(list<stdClass>vsarray<array-key, stdClass>— they have demonstrably different shapes). SwitchesARRAY_A/ARRAY_Nouter types fromarray<int, ...>tolist<...>to match the$new_array[]population pattern.get_var()— prose hoist. Moves the empty-string-vs-failure explanation and the$this->last_errorpointer out of the@returnline and into the description block above@param, matching WP-core convention of terse@returnlines with the rationale up top.@returnand@phpstan-tags across all three updated docblocks, matching the formatting of the existing@phpstan-returnprecedent onwp_parse_url()inwp-includes/http.php.None of the commits change behavior; all are docblock-only.
The case-insensitive
$outputback-compat path (strtoupper( $output ) === OBJECT) is intentionally not modelled in the PHPStan annotations. It is hard to express in PHPStan without going into regex territory, and anyone relying on lowercase'object'is using undocumented behavior. It folds into theelsebranch and types asnull. Commit 6's@phpstan-paramnarrowing makes that visible to static analysis.Trac ticket: https://core.trac.wordpress.org/ticket/65261
The ticket originally scoped to
get_results()only. Per discussion with @westonruter on this PR, the scope was broadened to cover all four query methods. The Trac ticket title/description should be updated to match.Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 4.7
Used for: Source-code trace of each query method's failure paths, comparison of the inline docblocks against the developer.wordpress.org reference and against existing PHPStan precedent in core (
wp_parse_url()inwp-includes/http.php), drafting of the revised docblock wording and PHPStan conditional types, an independent fresh-context code review of the resulting diff that surfaced theOBJECT/OBJECT_Kshape split and theget_var()prose hoist, and drafting of this PR description and the Trac ticket. All output was reviewed by me; I take responsibility for the change.This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.