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 5 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.
| * @return string|void Update PHP page annotation if available and `$display` is false, | ||
| * nothing otherwise. |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5
A genuine inconsistency was identified here, and it has been addressed in f0e1c8c — though in the opposite direction to the one suggested.
The null in the conditional originated from the missing-annotation bail, which is a bare return;. In PHPDoc terms that is void, not a returned null, so the accurate resolution was to say void in the conditional rather than to add null to the @return. Both tags now read string|void.
Adding null to the public tag would have produced string|null|void, which is not an idiom used elsewhere in core and would blur the void/null distinction this pull request exists to restore.
Nothing is lost for consumers. A void occupying only one branch of a conditional resolves to null at the call site, so wp_update_php_annotation( '', '', false ) is still typed string|null. This was confirmed with \PHPStan\dumpType() before and after the change.
| * @param string $prefix Optional. What to display before the title. | ||
| * @param bool $display Optional. Whether to display or retrieve title. Default true. | ||
| * @return string|null Title when retrieving. | ||
| * @return string|void Title when retrieving. |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5
A genuine inconsistency was identified here, and it has been addressed in f0e1c8c for all five locations — though in the opposite direction to the one suggested.
The null in the conditional originated from the failure bails, every one of which is a bare return;. In PHPDoc terms that is void, not a returned null, so the accurate resolution was to say void in the conditional rather than to add null to the @return. Both tags now read string|void.
This also settles an inconsistency within the pull request itself: the echo argument tags added later already used string|void for exactly this shape, because PHPStan reported the null on get_calendar() as never returned. The five functions here had been left in the earlier spelling.
Adding null to the public tag would instead have produced string|null|void, which is not an idiom used elsewhere in core and would blur the void/null distinction this pull request exists to restore.
Nothing is lost for consumers. A void occupying only one branch of a conditional resolves to null at the call site, so single_post_title( '', false ) and its siblings are still typed string|null. This was confirmed with \PHPStan\dumpType() before and after the change.
The underlying documentation concern was fair, however: with the null gone from the type, the failure case needed stating in prose. The descriptions on single_post_title(), single_cat_title(), single_tag_title() and single_term_title() have been extended from "Title when retrieving." to "Title when retrieving, nothing when displaying or on failure.", matching what post_type_archive_title() already said.
| * @param int|WP_Term|null $term Optional. Term ID or object. If null, the queried object will be inspected. Default null. | ||
| * @param bool $display Optional. Whether or not to echo the return. Default true. | ||
| * @return string|null HTML content. | ||
| * @return string|void HTML content. |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5
A genuine inconsistency was identified here, and it has been addressed in f0e1c8c — though in the opposite direction to the one suggested.
The null in the conditional originated from the two bails, for a missing term and for a user without the capability to edit it. Both are a bare return;, which in PHPDoc terms is void rather than a returned null, so the accurate resolution was to say void in the conditional rather than to add null to the @return. Both tags now read string|void.
Adding null to the public tag would have produced string|null|void, which is not an idiom used elsewhere in core and would blur the void/null distinction this pull request exists to restore.
Nothing is lost for consumers. A void occupying only one branch of a conditional resolves to null at the call site, so edit_term_link( $link, '', '', $tag, false ) — the form used internally by edit_tag_link() — is still typed string|null. This was confirmed with \PHPStan\dumpType() before and after the change.
The underlying documentation concern was fair, however. The description here was a bare "HTML content.", which said nothing about either the printing mode or the bails, and it now reads "HTML content when retrieving, nothing when displaying, on failure, or without the capability to edit the term."
| * @return string|void Current post title if `$display` is false, nothing otherwise | ||
| * or when the title is empty. |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5
A genuine inconsistency was identified here, and it has been addressed in f0e1c8c — though in the opposite direction to the one suggested.
The null in the conditional originated from the empty-title bail, which is a bare return;. In PHPDoc terms that is void, not a returned null, so the accurate resolution was to say void in the conditional rather than to add null to the @return. Both tags now read string|void.
Adding null to the public tag would have produced string|null|void, which is not an idiom used elsewhere in core and would blur the void/null distinction this pull request exists to restore.
Nothing is lost for consumers. A void occupying only one branch of a conditional resolves to null at the call site, so the_title( '', '', false ) is still typed string|null. This was confirmed with \PHPStan\dumpType() before and after the change.
The empty-title case was already carried in the description, which reads "Current post title if $display is false, nothing otherwise or when the title is empty."
…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>
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. A dozen or so functions that still carry
voidtoday were never getting anything from it either.Changes
Four 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.Four of these bail before reaching the display branch, so their retrieval-mode type is
string|nullrather thanstring, and those bails go back to the barereturn;they had before r61768. The conditional return type is what now distinguishes them: nothing when printing, null when retrieving.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 it moves to an early barereturn;on the missing-annotation path. 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.Deliberately not changed
Functions that print and then return the value unconditionally, where the result is always meaningful:
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(). AlsoWP_Scripts::print_inline_script()andprint_translations(), whose existingstring|falseis accurate in either mode.Functions returning something meaningful while printing:
single_month_title()returnsfalseon failure before the display branch, so it cannot resolve to plainvoid.wp_list_categories()andwp_nav_menu(), for the same reason assingle_month_title(): each returns a meaningfulfalse— on a missing taxonomy and a missing menu respectively — on a path shared by both modes, so the printing branch isfalse|voidrather thanvoidand cannot be reported.Deprecated functions:
the_category_ID(),get_author_link(),get_category_rss_link(),get_author_rss_link(),get_most_active_blogs(),wp_get_links().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.Verification
paginate_comments_links()narrowing. Two reports restate a narrower expected type without changing meaning: the pre-existingreturn.typeonprint_extra_script(), which stems fromWP_Dependencies::get_data()returning mixed, and the pre-existingargument.typeon_navigation_markup().function.voidwhen consumed in display mode, and that none of them do 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.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_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.