Restore void on the dual-mode $display functions with conditional return types - #13359
Restore void on the dual-mode $display functions with conditional return types#13359westonruter wants to merge 11 commits into
void on the dual-mode $display functions with conditional return types#13359Conversation
r61768 replaced `string|void` with `string|null` across the template tags that either echo their result or return it, and r63379 followed by adding the trailing `return null;` that the annotation then obliged. Both steps were mechanically correct but lost information: under `string|null`, PHPStan treats the display-mode result as a legitimate value, so consuming a meaningless one no longer reports anything. Restore `string|void` on the nine functions r63379 touched and pin the duality down with a conditional `@phpstan-return`, so display mode resolves to `void` and retrieval mode to `string` — or to `string|null` where a failure path bails before the display branch. PHPStan raises `function.void` at call sites again, while retrieval-mode calls keep their usable type. The trailing `return null;` statements go away, since `void` in the union licenses falling off the end. The failure bails in `single_post_title()`, `post_type_archive_title()`, `single_term_title()` and `edit_term_link()` go back to a bare `return;`; the conditional return type is what now makes them read as nothing when displaying and as null when retrieving. `single_cat_title()` and `single_tag_title()` delegate to `single_term_title()`, so they take the same annotation. Their one-line body becomes an early return, because returning the delegate's value unconditionally never returns void and PHPStan reports the `void` in the union as unused. `single_month_title()` is deliberately left alone: its display branch legitimately returns `false` on failure, so it cannot resolve to plain `void` and would gain the accuracy without the detection. Full PHPStan runs before and after report an identical 27,183 errors, and the `return.missing` baseline stays deleted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ay` tags
A sweep of core for the same echo-or-return shape turned up five more
functions, and with them the reason the pattern kept going unnoticed:
`@return void|string` on its own conveys nothing to PHPStan. The
`function.void` report fires only when the resolved return type is exactly
`void`, and a plain union never resolves to that. So `comment_class()`,
`the_title()`, `wp_loginout()` and `wp_register()` have carried the `void`
through every annotation sweep while getting no analysis out of it.
Give all four the conditional `@phpstan-return` that actually does the
work, and reorder the union to `string|void` to match the functions the
previous commit touched. `the_title()` bails early when the title is
empty, so its retrieval branch is `string|null` rather than `string`.
`wp_update_php_annotation()` is documented `string|null … null otherwise`,
the same wording r61768 left behind elsewhere, and needs a body change to
follow: its trailing `return null;` is reached in both modes, so it moves
to an early bare `return;` on the missing-annotation path and the echoing
path now falls off the end. Behavior is unchanged.
Deliberately excluded are the functions that echo and then return the
value unconditionally, where the result is always meaningful --
`wp_nonce_field()`, `checked()` and its siblings, `menu_page_url()`,
`timer_stop()` among them -- along with those returning a meaningful
`false` or `true` while displaying, such as `single_month_title()` and the
`WP_Scripts` and `WP_Styles` `print_*()` methods.
The dozen or so tags taking `echo` inside an `$args` array are left alone
for the reason `wp_list_users()` was: `$args` accepts a query string as
well as an array, so an `array{echo: false}` condition would resolve to
the `void` branch for the still-common `'echo=0'` call style and report
correct code as an error.
Full PHPStan runs before and after report the same error set.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`WP_Styles::print_inline_style()`, `WP_Scripts::print_extra_script()` and the deprecated `WP_Scripts::print_scripts_l10n()` all document their return the wrong way round. Each says the markup comes back when `$display` is true, but the string is returned on the `! $display` branch and the printing branch returns `true`. The description has read this way since the `$display` parameter was introduced, so anyone consulting it to decide which argument to pass was told the opposite of what the code does. Swap `true` for `false` in the three descriptions, and pin the two behaviours apart with a conditional `@phpstan-return`, since the plain unions collapse the distinction the same way the `$display` template tags did. `print_inline_style()` now resolves to `bool` when printing and `string|false` when retrieving, rather than `string|bool` either way, and `print_extra_script()` to `true|null` and `string|null` rather than `bool|string|null`. The narrower retrieval types matter at the two internal call sites that pass `false` and then use the result as a string. `print_inline_script()` and `print_translations()` are left alone. Both print and then return the same value, so their existing `string|false` is accurate in either mode and there is nothing for a condition to separate. The pre-existing `return.type` report on `print_extra_script()`, which stems from `WP_Dependencies::get_data()` returning mixed, is unchanged apart from restating the narrower expected type. The error set is otherwise identical before and after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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. |
There was a problem hiding this comment.
Pull request overview
This PR updates PHPDoc/PHPStan typing for WordPress “dual-mode” template functions that either echo output ($display = true) or return it ($display = false), restoring void semantics via conditional @phpstan-return annotations so PHPStan can flag misuse of display-mode results. It also corrects return-value documentation/typing for a few WP_Styles/WP_Scripts $display-controlled methods.
Changes:
- Restores
string|voidstyle PHPDoc unions for$displayfunctions and adds conditional@phpstan-returnannotations to distinguish echo vs return modes. - Removes trailing
return null;statements where “falling off the end” is intended for display mode. - Fixes inverted/overbroad return documentation for
WP_Styles::print_inline_style()andWP_Scripts::{print_extra_script,print_scripts_l10n}()and adds conditional@phpstan-returnnarrowing.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/wp-includes/post-template.php | Adds conditional PHPStan return typing for the_title() dual-mode behavior. |
| src/wp-includes/link-template.php | Updates edit_term_link(), next_posts(), previous_posts() return docs and removes trailing return null; in display mode. |
| src/wp-includes/general-template.php | Updates multiple title/date-related $display functions with `string |
| src/wp-includes/functions.php | Adjusts wp_update_php_annotation() to bail early and adds conditional @phpstan-return typing. |
| src/wp-includes/comment-template.php | Updates comment_class() return typing with conditional @phpstan-return. |
| src/wp-includes/class-wp-styles.php | Corrects print_inline_style() return documentation and adds conditional narrowing. |
| src/wp-includes/class-wp-scripts.php | Corrects/clarifies $display-dependent return docs and adds conditional narrowing for extra-script printing methods. |
Suppressed comments (4)
src/wp-includes/general-template.php:1781
- The docblock return type omits
null, butsingle_cat_title()can returnnullin retrieval mode because it delegates tosingle_term_title(), which bails out withreturn;on failure/empty term name. Since the conditional@phpstan-returnalready indicatesstring|nullfor$display = false, the public@returnshould includenulltoo.
* @return string|void Title when retrieving.
* @phpstan-return ( $display is true ? void : string|null )
src/wp-includes/general-template.php:1803
- The docblock return type omits
null, butsingle_tag_title()can returnnullin retrieval mode because it delegates tosingle_term_title(), which bails out withreturn;on failure/empty term name. Since the conditional@phpstan-returnalready indicatesstring|nullfor$display = false, the public@returnshould includenulltoo.
* @return string|void Title when retrieving.
* @phpstan-return ( $display is true ? void : string|null )
src/wp-includes/general-template.php:1825
- The docblock return type omits
null, butsingle_term_title()can returnnull(viareturn;) when there is no queried term, when not in a supported taxonomy context, or when the term name is empty. Since the conditional@phpstan-returnalready indicatesstring|nullfor$display = false, the public@returnshould includenullto keep the documentation accurate.
* @return string|void Title when retrieving.
* @phpstan-return ( $display is true ? void : string|null )
src/wp-includes/general-template.php:1738
- The docblock return type omits
null, butpost_type_archive_title()can returnnullon failure (e.g. when not on a post type archive). Since the conditional@phpstan-returnalready indicatesstring|nullfor$display = false, the public@returnshould includenullto keep the documentation accurate.
* @return string|void Title when retrieving, nothing when displaying or on failure.
* @phpstan-return ( $display is true ? void : string|null )
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…late tags
The thirteen template tags taking their print-or-return flag inside an
`$args` array were previously passed over on the grounds that `$args`
accepts a query string as well as an array, so an `array{echo: false}`
condition would resolve to the `void` branch for the still-common
`'echo=0'` call style and report correct code as an error. Testing rather
than reasoning about it shows the objection is avoidable, and that two of
the assumptions behind it were wrong.
Array shapes in a conditional are not sealed, so a caller passing
`array( 'echo' => false, 'aria_label' => 'a' )` matches
`array{ echo: false, ... }` as intended. And the query-string case is
handled by a third branch: when `$args` is neither the falsy-flag shape
nor an array, the type stays a union and nothing is reported. What that
branch gives up is only the undecidable call styles; the bare
`the_title_attribute()`, the empty array and an explicit truthy flag all
still resolve to `void` and are reported when consumed.
The flag also has to be matched as `false|0|''|'0'` rather than `false`,
since these tags variously default it to `true` or to `1` and callers
follow suit. Matching only `false` reports `array( 'echo' => 0 )` as void
while it actually returns the markup.
Covered are `the_title_attribute()`, `get_search_form()`, `get_calendar()`
(whose flag is `display`), `wp_login_form()`, `wp_get_archives()`,
`wp_list_pages()`, `wp_page_menu()`, `wp_list_comments()`,
`wp_list_bookmarks()`, `wp_list_authors()`, `wp_list_users()`,
`wp_tag_cloud()` and `paginate_comments_links()`. The last two answer to
`format` and `type` as well, either of which returns an array even while
printing, so their conditions nest that dimension first.
`wp_list_users()` also loses the trailing `return null;` r63378 gave it,
so that its printing path is genuinely void.
For the three tags whose `$args` is documented as an array, the
`$args is array` guard is always true and PHPStan says so, so those take
the plain two-branch form.
`wp_dropdown_languages()` turns out not to belong to this group at all: it
prints and then returns the markup regardless of the flag. Its `void` is
correct, for the bail on a missing `id` or `name`, and only the
description needed saying.
Left alone are `wp_list_categories()` and `wp_nav_menu()`, which return a
meaningful `false` on a missing taxonomy and a missing menu respectively,
on a path shared by both modes. Their printing branch is therefore
`false|void` rather than `void` and cannot be reported, the same reason
`single_month_title()` was passed over.
Narrowing `paginate_comments_links()` to `string[]` also resolves three
existing errors, two of them in Twenty Twenty, where the result of a call
passing `echo => false` and no `type` was still typed as possibly an array.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eight of the conditional return types gave the retrieval branch as `string|null` while the `@return` above them said `string|void`, so the two tags disagreed about the third outcome. The `null` came from the failure paths, which bail with a bare `return;` and so are void, not a returned null. Saying `void` in both places settles it, and matches the shape the `echo` argument tags already use, where PHPStan itself reported the `null` as never returned. Callers are unaffected: `void` in a branch that is not the whole type resolves to `null` at the call site, so retrieval-mode calls still type as `string|null` exactly as before, and display mode still resolves to plain `void` and is still reported when consumed. Affected are `single_post_title()`, `post_type_archive_title()`, `single_cat_title()`, `single_tag_title()`, `single_term_title()`, `edit_term_link()`, `the_title()` and `wp_update_php_annotation()`. Those whose description did not mention the failure case now say so, since the type alone no longer hints at it. `WP_Scripts::print_extra_script()` and `print_scripts_l10n()` keep `string|null`, correctly: their bail is an explicit `return null;` and the `@return` above already carries the `null`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…her than bail `single_cat_title()` and `single_tag_title()` were given the same `string|void` retrieval branch as the functions that bail with a bare `return;`, but neither of them bails. Their retrieval path is a single `return single_term_title( $prefix, false );`, which hands back that function's `string|null` as an actual value, so `null` is what the branch should say. Callers see no difference, since a `void` in one branch of a conditional resolves to `null` at the call site either way. The point is that the annotation should describe what the function does, and only a bare `return;` justifies writing `void`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The conditional return types had settled into two shapes. Where the function bails before the print-or-return split, the retrieval branch read `string|void`, because the bail is a bare `return;` and that is void rather than a returned null. Where it does not bail, the branch read plain `string`. The first shape is defensible but reads oddly, since `void` then appears on both sides of the condition and it is not obvious that only one of them is doing the work. Settle on a single shape instead: one branch is exactly `void`, meaning the call has no value to give, and the other is the retrieval type unioned with `null` where a bail exists. To make the second half true, the bails reachable in retrieval mode now say `return null;` rather than falling out of the function, because in that mode the null is a value the caller observes. Bare `return;` is kept where it remains correct: the paths inside `if ( $args['display'] )` in `get_calendar()`, which run only after the markup has been echoed and can never be reached by a caller expecting a value. This does not resurrect what r63379 added and this branch removed. Those were trailing `return null;` statements on the printing path, asserting a value where the caller is not looking. The bails converted here sit before the split and are reached in both modes. Covered are `single_post_title()`, `post_type_archive_title()`, `single_term_title()`, `edit_term_link()`, `the_title()`, `wp_update_php_annotation()`, `the_title_attribute()`, `get_calendar()`, `wp_get_archives()`, `wp_list_comments()`, `wp_tag_cloud()` and `paginate_comments_links()`, with descriptions reworded to name the null case now that the type no longer implies it. Behavior is unchanged, as a bare `return;` and `return null;` both yield null. The reports are unchanged too: all fourteen functions still resolve to plain `void` in printing mode and are reported when consumed, and every retrieval-mode call still types exactly as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A sweep for `void` sitting in a union with no conditional to resolve it turns up one more function of this shape in core. `trackback_url()` echoes the trackback URL or returns it, exactly like the tags already covered, and it has no bail, so the condition is the simple one: `void` when printing, `string` when not. Its `@param` said the argument was "Not used.", which is wrong and was actively misleading now that the return type depends on it. It is read twice, once to warn that retrieving the value this way has been deprecated since 2.5.0 and again to choose between echoing and returning. The description now leads with "Deprecated." and points at `get_trackback_url()`, following `the_author()`, whose identically named argument is documented that way, and a changelog entry records the deprecation in the terse form `_wp_can_use_pcre_u()` uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bundled theme has one function of the same shape as the template tags covered here: it echoes the site description or returns it, depending on a `$display` argument, and bails with a bare `return;` when the site has no description. It takes the same treatment, `void` when printing and `string|null` when not, with the bail made explicit since in retrieval mode that null is the value the caller receives. `twentytwenty_site_logo()` is the near neighbor and is left alone. It returns an empty string when the site has no title, on a path shared by both modes, so its printing branch is `string|void` rather than `void` and cannot be reported. Changing that `return '';` would alter what callers receive, which is not worth doing for an annotation. `twentytwenty_get_post_meta()` and `twentytwentyfive_format_binding()` carry `void` in a union too, but neither takes a print-or-return argument; the `void` covers their bails, and there is nothing for a condition to switch on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@IanDelMar @apermo I'd appreciate your review of this. |
There was a problem hiding this comment.
🟡 Changes recommended
get_search_form()’s new @phpstan-return does not account for legacy non-array arguments (e.g. get_search_form(false)), causing an incorrect void resolution in PHPStan.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
The condition added for this function only described the array form, so `get_search_form( false )` resolved to `void` while at runtime it returns the markup. The function still honors the boolean `$echo` flag that r44956 replaced with `$args`, casting a non-array argument to bool and using it as the flag, and the condition skipped straight past that. An earlier guard would have covered it, but was dropped because PHPStan reported `$args is array` as always true. That report was the symptom rather than the problem: `@param array $args` is what makes the branch look unreachable, while a call passing `false` is still resolved against the argument's own type, so the branch that mattered was removed and the wrong one kept. Declaring the legacy form in `@phpstan-param` makes both agree. `get_search_form( false )` now resolves to `string`, an explicit `true` and a bare call still resolve to `void`, and a variable of unknown boolean value resolves to the union, so nothing is reported where the flag cannot be determined. `get_calendar()` accepts legacy positional arguments too, but its first one sets `initial` rather than `display`, so `get_calendar( false )` prints and resolving it to `void` is already right. `wp_login_form()` has no such back-compat branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The changes are predominantly docblock/type-annotation refinements with low behavioral risk, and the remaining notes are minor clarity/consistency improvements.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
src/wp-includes/functions.php:8749
- The early bail when no annotation is available currently returns
nulleven when$displayis true, while the@phpstan-returnmodels display-mode calls asvoid. Returningvoidin display mode (andnullonly when retrieving) keeps the implementation consistent with the documented dual-mode return intent.
src/wp-includes/link-template.php:2547 next_posts()returns an empty string when no next page exists (it maps anulllink to''), so the docblock should mention that$display = falsecan yield an empty string rather than a URL.
src/wp-includes/post-template.php:49- The empty-title early bail uses
return null;even when$displayis true, but the accompanying@phpstan-returnmodels the display-mode call asvoid. To keep the implementation aligned with the intended dual-mode semantics, returnvoidwhen displaying and only returnnullin retrieval mode.
This issue also appears on line 99 of the same file.
src/wp-includes/post-template.php:101
- Like
the_title(), this empty-title bail returnsnulleven when$parsed_args['echo']is truthy. Since the docblock/@phpstan-returntreat the echoing branch asvoid, consider returningvoidwhen echoing and only returningnullfor the non-echoing path.
if ( strlen( $title ) === 0 ) {
return null;
}
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
`next_posts()` promises "the link URL for next posts page" when retrieving, but it maps a missing link to an empty string, so that is what a caller gets on the last page. `previous_posts()` has the identical shape, and `wp_register()` promises a registration or admin link while setting the value to an empty string when registration is disabled and the visitor is logged out, or when a logged-in user cannot reach the dashboard. The types were already right, since an empty string is a string. It is the descriptions that promised something the functions do not always deliver, and a caller reading only the summary would not think to guard against it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🤖 Comment by Claude Opus 5 Replying to the four suppressed notes in @Copilot's review, since none of them carry a thread to answer in. The
|
apermo
left a comment
apermo
left a comment
There was a problem hiding this comment.
I'll double check my own comments, other than that, looking good to me.
| * @param bool $display Optional. Whether to print the inline style | ||
| * instead of just returning it. Default true. | ||
| * @return string|bool False if no data exists, inline styles if `$display` is true, | ||
| * @return string|bool False if no data exists, inline styles if `$display` is false, |
There was a problem hiding this comment.
This seems massive, that is the complete opposite of before, was this wrong all the way?
| * @param bool $display Optional. Whether to print the extra script | ||
| * instead of just returning it. Default true. | ||
| * @return bool|string|null Null if no data exists, extra scripts if `$display` is true, | ||
| * @return bool|string|null Null if no data exists, extra scripts if `$display` is false, |
There was a problem hiding this comment.
Same as below, this change seems massive, I can't believe that this slipped through and was wrong twice all the time.
There was a problem hiding this comment.
Double checked, these were wrong all the way since their introduction in WP4.5 nearly 10 years ago.
|
I am checking all core for similar errors in "display or return" booleans. If there is a significant number i'll create a new ticket and a new pr for that. |
|
The sweep is done, and it came back clean. Scope was every function and method in The three you found in Two more in that group looked wrong at first, but both are already handled. PHPStan agrees: on your branch with the repo config there is no So from my side there is nothing else to fix here. Disclosure: I used Claude Code (Opus 5) for the sweep and for drafting this comment. I reviewed the findings and ran the PHPStan verification in my own checkout. |
`WP_Styles::print_inline_style()`, `WP_Scripts::print_extra_script()` and the deprecated `WP_Scripts::print_scripts_l10n()` each document their return the wrong way round. All three say the markup comes back when `$display` is true, but the string is returned on the `! $display` branch and the printing branch returns `true`. The wording has read this way since r36744, so anyone consulting it to decide which argument to pass was told the opposite of what the code does. Swapping `true` for `false` in the three descriptions corrects that, and a conditional `@phpstan-return` pins the two behaviors apart, since the plain unions collapse the distinction. `print_inline_style()` now resolves to `bool` when printing and `string|false` when retrieving, rather than `string|bool` either way, and `print_extra_script()` to `true|null` and `string|null` rather than `bool|string|null`. The narrower retrieval types matter at the two internal call sites that pass `false` and then use the result as a string. `WP_Scripts::print_inline_script()` and `print_translations()` are left alone. Both print and then return the same value, so their existing `string|false` is accurate in either mode and there is nothing for a condition to separate. Developed as subset of #13359. Follow-up to r36744, r62178. Props apermo, westonruter. See #65817. git-svn-id: https://develop.svn.wordpress.org/trunk@63440 602fd350-edb4-49c9-b593-d223f7449a82
`WP_Styles::print_inline_style()`, `WP_Scripts::print_extra_script()` and the deprecated `WP_Scripts::print_scripts_l10n()` each document their return the wrong way round. All three say the markup comes back when `$display` is true, but the string is returned on the `! $display` branch and the printing branch returns `true`. The wording has read this way since r36744, so anyone consulting it to decide which argument to pass was told the opposite of what the code does. Swapping `true` for `false` in the three descriptions corrects that, and a conditional `@phpstan-return` pins the two behaviors apart, since the plain unions collapse the distinction. `print_inline_style()` now resolves to `bool` when printing and `string|false` when retrieving, rather than `string|bool` either way, and `print_extra_script()` to `true|null` and `string|null` rather than `bool|string|null`. The narrower retrieval types matter at the two internal call sites that pass `false` and then use the result as a string. `WP_Scripts::print_inline_script()` and `print_translations()` are left alone. Both print and then return the same value, so their existing `string|false` is accurate in either mode and there is nothing for a condition to separate. Developed as subset of WordPress/wordpress-develop#13359. Follow-up to r36744, r62178. Props apermo, westonruter. See #65817. Built from https://develop.svn.wordpress.org/trunk@63440 git-svn-id: http://core.svn.wordpress.org/trunk@62621 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Many template tag functions either print their result or return it, depending on a `display` or `echo` param. Their `@return` carried `void` in a union until r61766, r61768 and r62178 replaced it with `null`, on the premise that `void` cannot belong to a union type. That premise holds for PHP's native return types but not for PHPDoc, where PHPStan reads `void` in a union as "may not return at all". Restoring it is not enough on its own, though. PHPStan raises the "Result of function … (void) is used." error only when the resolved return type is exactly `void`, which a union never is, so the tags that kept `void` were getting no more out of it than the ones converted to `null`. What carries the distinction is a conditional `@phpstan-return`, resolving to plain `void` when the tag prints and to the type it returns otherwise. Thirty-one functions gain one, including one in the bundled Twenty Twenty theme, each with a single `void` branch and a nullable retrieval branch. The trailing `return null;` statements added in r63378 and r63379 are removed; they existed only to satisfy `return.missing`, which does not apply once `void` is in the union. Where the flag lives in an `$args` array the condition has to match every falsy spelling of it, since these tags variously default it to `true` or to `1`. A call whose flag PHPStan cannot see, such as a query string or an array built at runtime, does not resolve to plain `void`, so it is never reported. Four descriptions are corrected alongside: `next_posts()`, `previous_posts()` and `wp_register()` promised a link where an empty string is possible, and `wp_dropdown_languages()` prints and then returns the markup rather than choosing between the two. Tags returning a meaningful value on a path shared by both modes cannot resolve to plain `void` and are left alone, among them `single_month_title()`, `wp_list_categories()` and `wp_nav_menu()`. Developed as subset of #13359. Follow-up to r32568, r61766, r61768, r62178, r63378, r63379, r63440. Props marian1, westonruter, apermo. See #65817, #64704. git-svn-id: https://develop.svn.wordpress.org/trunk@63441 602fd350-edb4-49c9-b593-d223f7449a82
Many template tag functions either print their result or return it, depending on a `display` or `echo` param. Their `@return` carried `void` in a union until r61766, r61768 and r62178 replaced it with `null`, on the premise that `void` cannot belong to a union type. That premise holds for PHP's native return types but not for PHPDoc, where PHPStan reads `void` in a union as "may not return at all". Restoring it is not enough on its own, though. PHPStan raises the "Result of function … (void) is used." error only when the resolved return type is exactly `void`, which a union never is, so the tags that kept `void` were getting no more out of it than the ones converted to `null`. What carries the distinction is a conditional `@phpstan-return`, resolving to plain `void` when the tag prints and to the type it returns otherwise. Thirty-one functions gain one, including one in the bundled Twenty Twenty theme, each with a single `void` branch and a nullable retrieval branch. The trailing `return null;` statements added in r63378 and r63379 are removed; they existed only to satisfy `return.missing`, which does not apply once `void` is in the union. Where the flag lives in an `$args` array the condition has to match every falsy spelling of it, since these tags variously default it to `true` or to `1`. A call whose flag PHPStan cannot see, such as a query string or an array built at runtime, does not resolve to plain `void`, so it is never reported. Four descriptions are corrected alongside: `next_posts()`, `previous_posts()` and `wp_register()` promised a link where an empty string is possible, and `wp_dropdown_languages()` prints and then returns the markup rather than choosing between the two. Tags returning a meaningful value on a path shared by both modes cannot resolve to plain `void` and are left alone, among them `single_month_title()`, `wp_list_categories()` and `wp_nav_menu()`. Developed as subset of WordPress/wordpress-develop#13359. Follow-up to r32568, r61766, r61768, r62178, r63378, r63379, r63440. Props marian1, westonruter, apermo. See #65817, #64704. Built from https://develop.svn.wordpress.org/trunk@63441 git-svn-id: http://core.svn.wordpress.org/trunk@62622 1a063a9b-81f0-0310-95a4-ce76da25c4cd
|
I was only able to have a quick look.
|
|
🤖 Comment drafted initially by Claude Opus 5 @IanDelMar Thank you for the review. It seems several of your points were right, and five have been acted on in a follow-up PR: #13371 Responses in order.
|
| Call | Combined | Nested |
|---|---|---|
array( 'id' => '' ) |
void ✔ |
void ✔ |
array( 'name' => '' ) |
void ✔ |
void ✔ |
array( 'id' => 'x', 'name' => 'y' ) |
void ✘ |
string ✔ |
array( 'echo' => false ) |
void ✘ |
string ✔ |
array( 'selected' => 'de_DE' ) |
void ✘ |
string ✔ |
array() / no argument |
string ✔ |
string ✔ |
The two rows that look right in the combined column are coincidence — the empty array passes only by failing non-empty-array, and the two bail shapes pass only by being non-empty. Every realistic call, including all seven call sites in core, would have been typed void and reported. Nesting the two shapes is the only form PHPStan evaluates shape by shape, so that is what was committed:
* @phpstan-return (
* $args is array{ id: ''|'0', ... }
* ? void
* : ( $args is array{ name: ''|'0', ... } ? void : string )
* )
The falsy set was narrowed to ''|'0' rather than null|0|''|'0', since both arguments are documented @type string and those are the falsy strings — parallel to the sibling conditions enumerating false|0|''|'0' for a bool|int flag. Easy to widen if modeling undocumented argument types is preferred.
This also corrects a claim in the description above, which filed wp_dropdown_languages() under "not dual-mode at all — there is no argument for a condition to switch on". There is one; it is the bail condition rather than the display flag. That makes it the only function in the set whose conditional keys off a bail shape, so if the pattern is accepted, the other bail-only voids deserve the same sweep.
src/ was also swept for the same construct elsewhere — no other conditional in core unions array shapes, so nothing is carrying this bug.
@param true $deprecated_echo on trackback_url()
Unrelated, but I just noticed this:
@param true $deprecated_echoontrackback_url()would result in:Parameter #1 $deprecated_echo of function trackback_url expects true, false given.. In php-stubs/wordpress-stubs, this is used deliberately to signal that a deprecated argument was supplied. See: https://phpstan.org/r/2e2aa144-9b64-4815-bf72-2c343512462d
A genuinely useful idea, and the wordpress-stubs precedent is a good one. It does conflict with what is there now, though: declaring the parameter true while a conditional return switches on it produces a new error on the function itself.
Condition "true is true" in conditional return type is always true. [conditionalType.alwaysTrue]
So adopting it means removing the conditional. That turns out to cost less than it appears — a call passing false still has its return resolved from the argument's own type, so trackback_url( false ) still types as string while trackback_url() still resolves to void and is reported.
The larger consideration is that it reclassifies every existing trackback_url( false ) call as an argument-type error. That is the intent, but it is a policy decision about how deprecated arguments should be signaled across core rather than a detail of this change, so it seems better raised on its own ticket.
On workarounds versus design questions
General thought: I think the effort being put into improving code quality is very welcome.
However, I also think that some of the errors reported by PHPStan point to design questions and should prompt us to think about those questions, rather than trying to introduce workarounds solely to silence PHPStan. Once such workarounds land in core, the PHPStan error disappears and therefore no longer indicates that there may be an underlying problem. The issue has not necessarily been resolved; it may simply have been masked.
I don’t think there is anything wrong with deliberately ignoring some errors. That way, there is still an indication that something may deserve attention, without forcing the implementation or documentation into shapes that primarily exist to satisfy the analyser.
Good point. Yeah, there are definitely some design issues here. One example that stands out to me is that WP_Term_Query::get_terms() cannot have its return type narrowed since it takes no argument, but the WP_Term_Query::query() wrapper function does take an argument, and so it can have its return type conditionally narrowed.
Every conditional added here creates reports that did not exist before — a call consuming the result of a printing tag is now an error where previously the plain union made it silent. Nothing was ignored and no baseline entry was added. The one baseline removed, return.missing, went because void in the union genuinely licenses falling off the end, which is the accurate description of what those functions do rather than a way around the rule.
Where the underlying design question was reached rather than papered over, it was left alone and written down. The functions listed under Deliberately not changed are there precisely because the annotation cannot express what they do — single_month_title(), wp_list_categories(), wp_nav_menu() and twentytwenty_site_logo() each return a meaningful value on a path shared by both modes, and making them fit would have meant changing what callers receive. That is the dual-responsibility problem originally raised, and it is still open; an annotation was not treated as its resolution.
Where that leaves things: updates are ready for review in a follow-up PR: #13371
`WP_Styles::print_inline_style()`, `WP_Scripts::print_extra_script()` and the deprecated `WP_Scripts::print_scripts_l10n()` each document their return the wrong way round. All three say the markup comes back when `$display` is true, but the string is returned on the `! $display` branch and the printing branch returns `true`. The wording has read this way since r36744, so anyone consulting it to decide which argument to pass was told the opposite of what the code does. Swapping `true` for `false` in the three descriptions corrects that, and a conditional `@phpstan-return` pins the two behaviors apart, since the plain unions collapse the distinction. `print_inline_style()` now resolves to `bool` when printing and `string|false` when retrieving, rather than `string|bool` either way, and `print_extra_script()` to `true|null` and `string|null` rather than `bool|string|null`. The narrower retrieval types matter at the two internal call sites that pass `false` and then use the result as a string. `WP_Scripts::print_inline_script()` and `print_translations()` are left alone. Both print and then return the same value, so their existing `string|false` is accurate in either mode and there is nothing for a condition to separate. Developed as subset of WordPress#13359. Follow-up to r36744, r62178. Props apermo, westonruter. See #65817. git-svn-id: https://develop.svn.wordpress.org/trunk@63440 602fd350-edb4-49c9-b593-d223f7449a82
Many template tag functions either print their result or return it, depending on a `display` or `echo` param. Their `@return` carried `void` in a union until r61766, r61768 and r62178 replaced it with `null`, on the premise that `void` cannot belong to a union type. That premise holds for PHP's native return types but not for PHPDoc, where PHPStan reads `void` in a union as "may not return at all". Restoring it is not enough on its own, though. PHPStan raises the "Result of function … (void) is used." error only when the resolved return type is exactly `void`, which a union never is, so the tags that kept `void` were getting no more out of it than the ones converted to `null`. What carries the distinction is a conditional `@phpstan-return`, resolving to plain `void` when the tag prints and to the type it returns otherwise. Thirty-one functions gain one, including one in the bundled Twenty Twenty theme, each with a single `void` branch and a nullable retrieval branch. The trailing `return null;` statements added in r63378 and r63379 are removed; they existed only to satisfy `return.missing`, which does not apply once `void` is in the union. Where the flag lives in an `$args` array the condition has to match every falsy spelling of it, since these tags variously default it to `true` or to `1`. A call whose flag PHPStan cannot see, such as a query string or an array built at runtime, does not resolve to plain `void`, so it is never reported. Four descriptions are corrected alongside: `next_posts()`, `previous_posts()` and `wp_register()` promised a link where an empty string is possible, and `wp_dropdown_languages()` prints and then returns the markup rather than choosing between the two. Tags returning a meaningful value on a path shared by both modes cannot resolve to plain `void` and are left alone, among them `single_month_title()`, `wp_list_categories()` and `wp_nav_menu()`. Developed as subset of WordPress#13359. Follow-up to r32568, r61766, r61768, r62178, r63378, r63379, r63440. Props marian1, westonruter, apermo. See #65817, #64704. git-svn-id: https://develop.svn.wordpress.org/trunk@63441 602fd350-edb4-49c9-b593-d223f7449a82
|
I'm including it because the absence of a conditional return it would come up again when scanning for places where it is missing. The method remains deprecated, so it's not suddenly encouraged to be used.
My goal currently is to try to document the behavior of the functions as they exist today, and hopefully add static analysis hints to catch incorrect usage as much as possible. Refactoring to address the fundamental inability to have deterministic return types for a given input is something which should be done, but at a later stage. |
Yes, I just wanted to raise awareness of the broader implications of introducing workarounds merely to silence PHPStan or to avoid documenting behaviour that seems odd from a native type perspective. In the case of the functions discussed here, those workarounds have already been reverted. Thank you for taking this up. |
✅ Committed in:
Follow-up to r63379, addressing review feedback from @IanDelMar on #13082.
The point raised there was that the
voidunions on the functions taking a$displayparameter were less a type-system problem than a consequence of those functions having two responsibilities: depending on the argument they either print a result or return one. Replacingvoidwithnullremoved the union, but it also removed information: Understring|nullPHPStan treats the display-mode result as a legitimate value, so consuming a meaningless one is no longer reported.That is correct, and it turns out to understate the problem.
voidin a plain union conveys nothingPHPStan raises
Result of function … (void) is used.only when the resolved return type is exactlyvoid. A plain union never resolves to that, so@return string|voidand@return void|stringcarry no more information thanstring|nulldoes. Verified with two otherwise identical functions:Only the conditional annotation does any work. So this is not a matter of putting back what r61768 and r63379 took away — the functions that still carry
voidtoday were never getting anything from it either.The shape used throughout
Every conditional added here has one branch that is exactly
void, meaning the call has no value to give, and one branch that is the retrieval type, unioned withnullwhere the function can bail. Bails reachable in retrieval mode sayreturn null;, because in that mode the null is a value the caller observes; a barereturn;is kept only where it can never be reached by a caller expecting a value.The two
voids are not redundant. Thevoidbranch is what makes the display-mode call resolve to exactlyvoidand so be reported. Thenullin the other branch is what keepsstrlen( post_type_archive_title( '', false ) )reported as passing a possible null. Narrowing that branch to plainstringwould trade a real bug class for nothing.Changes
Eleven commits, each independently reviewable.
1. The nine functions from r63379.
wp_title(),single_post_title(),post_type_archive_title(),single_term_title(),the_date(),the_modified_date(),edit_term_link(),next_posts()andprevious_posts()go back tostring|voidand gain a conditional@phpstan-return. The trailingreturn null;statements added by r63379 are removed, sincevoidin the union licenses falling off the end.single_cat_title()andsingle_tag_title()delegate tosingle_term_title()and take the same annotation, which requires expanding their one-line body into an early return — returning the delegate's value unconditionally never returns void, and PHPStan reports thevoidas unused.2. Five more with the same shape, found by sweeping core for the pattern:
comment_class(),the_title(),wp_loginout(),wp_register()andwp_update_php_annotation(). The first four already documentedvoid|stringand so, per the above, were getting nothing for it.wp_update_php_annotation()needs a small body change: its trailingreturn null;is reached in both modes, so the missing-annotation path becomes an early bail instead. No behavior change.3. An unrelated docs bug found by the same sweep.
WP_Styles::print_inline_style(),WP_Scripts::print_extra_script()and the deprecatedWP_Scripts::print_scripts_l10n()document their return inverted: each says the markup comes back when$displayis true, but the string is returned on the! $displaybranch and the printing branch returnstrue. Corrected, and given conditional annotations as well — not for void detection, but for narrowing:print_inline_style( $h )string|boolboolprint_inline_style( $h, false )string|boolstring|falseprint_extra_script( $h )bool|string|nulltrue|nullprint_extra_script( $h, false )bool|string|nullstring|nullThis matters at the two internal call sites that pass
falseand then use the result as a string, wheretruewas previously considered possible. Happy to split this commit off into its own ticket if preferred.4. The tags taking the flag inside an
$argsarray.the_title_attribute(),get_search_form(),get_calendar()(whose flag isdisplay),wp_login_form(),wp_get_archives(),wp_list_pages(),wp_page_menu(),wp_list_comments(),wp_list_bookmarks(),wp_list_authors(),wp_list_users(),wp_tag_cloud()andpaginate_comments_links().These were nearly left out, on the grounds that
$argsaccepts a query string as well as an array, so anarray{echo: false}condition would resolve to thevoidbranch forwp_list_categories( 'echo=0&title_li=' )and report correct code as an error. Testing rather than reasoning about it showed two of the assumptions behind that to be wrong:array( 'echo' => false, 'aria_label' => 'a' )matchesarray{ echo: false, ... }, so extra keys are not a problem.$argsis neither the falsy-flag shape nor an array, the type stays a union and nothing is reported. All that gives up is the undecidable call styles — the barethe_title_attribute(), the empty array and an explicit truthy flag all still resolve tovoid.The flag also has to be matched as
false|0|''|'0'rather thanfalse, since these tags variously default it totrueor to1. Matching onlyfalsereportsarray( 'echo' => 0 )as void while it actually returns the markup.wp_tag_cloud()andpaginate_comments_links()answer toformatandtypeas well, either of which returns an array even while printing, so their conditions nest that dimension first.wp_list_users()also loses the trailingreturn null;r63378 gave it. Where$argsis documented as an array the third branch is always true and PHPStan says so, so those take the plain two-branch form.Narrowing
paginate_comments_links()tostring[]resolves three existing errors as a side effect, two of them in Twenty Twenty, where the result of a call passingecho => falseand notypewas still typed as possibly an array.wp_dropdown_languages()turns out not to belong to this group at all — it prints and then returns the markup regardless of the flag, so itsvoidis correct for the bail on a missingidorname, and only the description needed saying.5–7. Settling on one shape. Addressing @copilot's review, which spotted that some conditionals gave the retrieval branch as
string|nullwhile the@returnabove saidstring|void, so the two tags disagreed about the third outcome. Resolved in the direction described under The shape used throughout, rather than by addingnullto the public tag, which would have producedstring|null|void— not an idiom used elsewhere in core, and one that blurs the distinction this pull request restores.single_cat_title()andsingle_tag_title()keepnull, correctly: neither bails, and their retrieval path hands back the delegate's value.8.
trackback_url(), the last function in core with this shape. No bail, so the condition is the simple one. Its@paramalso said the argument was "Not used.", which is wrong — it is read twice, once to raise the 2.5.0 deprecation notice and again to choose between echoing and returning.9.
twentytwenty_site_description(), the same shape in a bundled theme. Also separable onto its own ticket if preferred.10. A regression this pull request introduced, caught by @copilot.
get_search_form()still honors the boolean$echoflag that r44956 replaced with$args, casting a non-array argument to bool and using it as the flag. The condition described only the array form, soget_search_form( false )resolved tovoidwhile at runtime it returns the markup.The guard that would have covered this was present earlier on the branch and was removed because PHPStan reported
$args is arrayasCondition … is always true. That report was the symptom rather than the problem:@param array $argsis what makes the non-array branch look unreachable, while a call passingfalseis still resolved against the argument's own type — so the branch that mattered went and the wrong one stayed. Declaring the legacy form in@phpstan-param array<string, mixed>|bool $argsmakes the two agree, and the condition then nests the boolean case:get_search_form( false )stringget_search_form( true )voidget_search_form()voidget_search_form( array( 'echo' => false ) )stringget_search_form( $someBool )string|nullThe last row is why the boolean case is nested rather than folded into the first branch: where the flag cannot be determined the type stays a union and nothing is reported.
The neighbors were checked for the same trap.
get_calendar()also accepts legacy positional arguments, but its first one setsinitialrather thandisplay, soget_calendar( false )prints and resolving it tovoidis already correct.wp_login_form()has no back-compat branch.11. Three descriptions that promise more than the function delivers, from a further @copilot pass.
next_posts()says it returns "the link URL for next posts page" when retrieving, but maps a missing link to an empty string, which is what a caller gets on the last page.previous_posts()has the identical shape, andwp_register()promises a registration or admin link while setting the value to an empty string when registration is disabled and the visitor is logged out, or when a logged-in user cannot reach the dashboard. The types were already right — an empty string is a string — so only the descriptions changed.Deliberately not changed
Sweeping all of
srcforvoidin a return union turns up 35 further symbols in core and the bundled themes. None of them can drop thevoid, and none of them can be given a conditional that resolves to it. Grouped by why:Dual-mode, but the printing branch cannot be
void. Each returns a meaningful value on a path shared by both modes, so that branch isX|voidand never resolves to plainvoid. Changing what those paths return would alter what callers receive, which is not worth doing for an annotation:single_month_title()falsewhen there is no valid title for the monthwp_list_categories()falsewhen the taxonomy does not existwp_nav_menu()falsewhen no menu is found, plus thefallback_cbresulttwentytwenty_site_logo()''when the site has no titleNot dual-mode at all — the
voidcovers only a bail, and there is no argument for a condition to switch on:wp_dropdown_languages(),twentytwenty_get_post_meta()andtwentytwentyfive_format_binding().WP_Widget::form()belongs here too: whether it returns anything depends on the subclass, not on an argument, which is what r59336 recorded when it added thevoid.Void on success, error on failure — 26 functions whose failure value is not predictable from the arguments and is legitimately consumed, as in
if ( false === get_header() ). Thevoidin the union is correct and no report is wanted:void|false—get_header(),get_footer(),get_sidebar(),get_template_part(),add_theme_support(),do_enclose(),do_trackbacks(),the_terms(),update_object_term_cache(),update_user_caches(),WP_Customize_Setting::save(),site_admin_notice(),parent_dropdown(),maintenance_nag(),update_nag(),wp_plugin_update_row(),wp_theme_update_row(),make_site_theme_from_default(),wp_dropdown_cats()void|WP_Errorand friends —register_importer(),_fix_attachment_links(),WP_Image_Editor_Imagick::thumbnail_image(),WP_Metadata_Lazyloader::queue_objects(),WP_Metadata_Lazyloader::reset_queue(),WP_XMLRPC_Server::_toggle_sticky(),WP_REST_Edit_Site_Export_Controller::export()Also unchanged are the functions that print and then return the value unconditionally, where the result is always meaningful and no
voidis involved:wp_nonce_field(),wp_referer_field(),wp_original_referer_field(),checked()/selected()/disabled()/wp_readonly()/readonly()and__checked_selected_helper(),menu_page_url(),_post_states(),_media_states(),timer_stop(),wp_popular_terms_checklist(),wp_nav_menu_disabled_check(),WP_Scripts::print_inline_script()andWP_Scripts::print_translations(). Deprecated functions of this shape are left alone as well:the_category_ID(),get_author_link(),get_category_rss_link(),get_author_rss_link(),get_most_active_blogs()andwp_get_links().Verification
voidin printing mode and is reported when consumed, and that none of them are reported in retrieval mode — includingecho => 0as well asecho => false, flags accompanied by other keys, andformat/typeof'array'. Query-string and dynamic$argsare reported in neither mode, nor isget_search_form()'s legacy boolean argument where its value cannot be determined. Retrieval-mode calls type exactly as they did before.paginate_comments_links()narrowing. Two reports restate a narrower expected type without changing meaning: the pre-existingreturn.typeonprint_extra_script()and onwp_list_comments(), both of which stem from amixedreaching the return.mixedin one run and to a shape in another. The comparisons above were repeated until stable, andphpstan-diff --changed --stagedis clean on every commit.tests/phpstan/baselines/return.missing.neonstays deleted —voidin the union is precisely what licenses falling off the end.Tests_General_,Tests_Link_,Tests_Date_,Tests_Post_,Tests_Comment_,Tests_Term_,Tests_Category_,Tests_User_,Tests_Functions_andTests_Dependencies_all pass.Trac ticket: https://core.trac.wordpress.org/ticket/65817
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Opus 5
Used for: Evaluating the review feedback, sweeping core for other instances of the pattern, and drafting the annotations and this description. The PHPStan and PHPUnit verification was run against the working tree, and the result has been reviewed and is owned by me.
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.