Code Quality: Declare conditional return types where an argument selects the return type. - #13614
Open
westonruter wants to merge 15 commits into
Open
westonruter wants to merge 15 commits into
westonruter wants to merge 15 commits into
Conversation
The `get_comments()` function already declares a conditional `@phpstan-return` that narrows to `non-negative-int` for `count`, to `non-negative-int[]` for `fields => 'ids'`, and to `array<int, WP_Comment>` otherwise. That function is a thin wrapper that immediately delegates to `WP_Comment_Query::query()`, which declared only the unnarrowed union, so the narrowing was asserted at the wrapper rather than derived from the method that actually produces the value. Declare the same condition on `WP_Comment_Query::query()`. Callers of the method now get the same narrowing the function's callers already had, and the function's own conditional type follows from its return statement instead of standing on its own. `WP_Comment_Query::get_comments()` keeps the plain union. A PHPStan conditional return type can only branch on a parameter or a template type, and that method takes no parameters -- its behavior depends on `$this->query_vars`, which the PHPDoc type grammar cannot express. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…category getters. `get_post()` and `get_comment()` already declare a conditional `@phpstan-return` that resolves `$output` to the concrete shape it selects: an associative array for `ARRAY_A`, a list for `ARRAY_N`, and the object otherwise. The term and category getters take the same `$output` parameter but declared only the unnarrowed union, so every caller had to re-establish the type by hand. Declare the condition on `get_term()`, `get_term_by()`, `get_category()`, `get_category_by_path()`, and `get_tag()`, along with the `'OBJECT'|'ARRAY_A'|'ARRAY_N'` parameter type the condition branches on. The error union each function can return regardless of `$output` -- `WP_Error` and `null`, or `false` for `get_term_by()` -- is repeated in every branch, since those early returns happen before `$output` is consulted. Give `WP_Term::to_array()` the `array<string, mixed>` value type that `WP_Post::to_array()` and `WP_Comment::to_array()` already have. Without it the `ARRAY_A` and `ARRAY_N` branches degrade to an untyped array and the narrowing is lost at the point it matters most. Across the full tree this removes 53 static-analysis errors and introduces none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The deprecated `get_page_by_title()` hands `$output` straight to `get_post()`, which already declares a conditional `@phpstan-return`, but re-flattened the result to `WP_Post|array|null` on the way out. Mirror `get_post()`'s branches so the narrowing survives the call. `src/wp-includes/deprecated.php` is on the `excludePaths.analyse` list in `tests/phpstan/base.neon`, so nothing in the file is checked. The file is still scanned for signatures, though, which is what makes the annotation reach callers in plugins and themes -- the only consumers a deprecated function still has. That exclusion is also why this commit bypasses the pre-commit hook: with only an unanalysed file staged, PHPStan exits with "No files found to analyse" and the hook reads that as a failure. The change was verified separately -- all three branches resolve correctly at call sites, and PHPCS is clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`get_comments()` and `WP_Comment_Query::query()` narrow their return type on the `count` and `fields` query vars, and `WP_Term_Query::query()` does the same. The site and network queries take those same two query vars and shape their return value the same way, but declared only the unnarrowed union, so every caller had to widen back out by hand. Declare the condition on `get_sites()`, `WP_Site_Query::query()`, `get_networks()`, and `WP_Network_Query::query()`. The network side needed its types filled in first. `WP_Network_Query::query()`, `WP_Network_Query::get_networks()`, and `get_networks()` all declared a bare `array|int`, which says nothing about what the array holds, and the `networks_pre_query` filter documented its short-circuit value as `array|int|null`. Give them the `WP_Network[]|int[]|int` the site query has used all along; the prose in each of those docblocks already described exactly that. One consequence worth noting: with `get_sites()` resolving to `array<int, WP_Site>`, `wp_filter_oembed_result()` was caught handing `WP_Site::$blog_id` -- a numeric string, for historical compatibility -- straight to `switch_to_blog()`, which wants an int. The line immediately above it already casts for its comparison, so cast here too. Across the full tree this removes 56 static-analysis errors. The one it adds is the counterpart of an error `WP_Site_Query::get_sites()` already carries: both build their result with `array_map()` over `get_site()`/`get_network()`, which are nullable, so the mapped array is not provably free of nulls. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`get_users()` declared a bare `array`, so callers learned nothing about what it holds -- not even that it is a list, let alone that the default query returns `WP_User` objects. The shape is decided entirely by the `fields` query var, so express that as a condition. The branches follow `WP_User_Query::prepare_query()`: `'all'` and `'all_with_meta'` give `WP_User` objects, a lone `'ID'` gives user IDs, any other field name or list of field names gives the requested values, and anything unrecognized -- including `fields` being absent, which is the common case -- falls back to `WP_User` objects. Ordering matters here: the catch-all for named fields would otherwise swallow `'all'`, so the two object-returning values are matched first and the same object type is repeated in the final `else`. `WP_User_Query::get_results()` had to say more than `array` for this to hold at the call site, since `get_users()` returns its result directly. Give it `array<int, mixed>`, which is as precise as the method can honestly be: the value type depends on the same `fields` query var, and the method takes no parameter to condition on. That leaves one error behind on `get_results()`, replacing the missing-value-type one it had before. It reports accurately: the private `WP_User_Query::$results` is still declared as a bare `array`. Typing that property is the next step and a larger one -- it is assigned from the `users_pre_query` filter, from two different `wpdb` methods, and from the object cache, and it holds `null` between the filter call and the query, so closing this properly means reconciling all four. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Eight functions return an integer for one particular format string and a formatted string for everything else. Every one of them already said so in prose -- `current_time()` documents "Integer if `$type` is 'timestamp' or 'U', string otherwise" -- while declaring a flat `int|string` that callers had to re-narrow by hand. Declare the condition on `current_time()` and `mysql2date()` in functions.php, and on `get_the_date()`, `get_the_modified_date()`, `get_the_time()`, `get_post_time()`, `get_the_modified_time()`, and `get_post_modified_time()` in general-template.php. The `false` returned when there is no post, or when the date cannot be parsed, precedes the format check in every case, so it appears in both branches. Two of these carry their answer in the default value: `get_post_time()` and `get_post_modified_time()` default `$format` to 'U', so a bare call now resolves to `int|false` rather than the full union. Across the full tree this removes 13 static-analysis errors and introduces none. Most of them are call sites that were handing one of these values to something expecting a string -- `strtotime()`, `substr()`, `preg_match()`, `wp_handle_upload()` -- where the integer branch could never have reached. Two baseline patterns in `tests/phpstan/baselines/argument.type.neon` quote the old, wider types in their message text, so they stop matching once the types narrow. Update both to the text PHPStan now reports. They are the same two errors as before, still baselined; only the spelling of the types inside them changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`WP_Theme::get()` already resolves its return type from `$header`, giving `string[]` for Tags and a string for the other known headers. `WP_Theme::display()` wraps it, and its own docblock says "An array for Tags if `$markup` is false, string otherwise" -- but it declared a flat `string|array|false`, so everything `get()` had established was thrown away one call later. Declare the condition on both parameters: an array only when `$markup` is false and the header is Tags, a string in every other case, and false throughout for a header the theme does not have. `WP_Theme::translate_header()` had to say more than `string|array` first, since `display()` passes its value through on the way out. It is private, so it takes the PHPStan types directly: `string|string[]` in and out. Across the full tree this removes 57 static-analysis errors and introduces none. Nearly all of them are `display()` results being concatenated or passed to `sprintf()` -- in the themes list table, the theme editor, the upgrader skins, update-core.php -- where the array branch could only ever have arrived for Tags, and never at all once `$markup` was left at its default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three functions take a boolean that decides not how much work they do but what type comes back, and each already spelled the rule out in its `@return` description while declaring the flat union. `wp_insert_category()` and `wp_allow_comment()` both take `$wp_error`, and return a `WP_Error` only when it is true. Follow the form `wp_insert_post()` already uses for the same parameter, keyed on `false` so that omitting the argument -- which is how nearly every caller invokes them -- resolves to the narrow type rather than the union. `single_month_title()` takes `$display`, echoing and returning null when it is true and returning the title when it is false. Its siblings `single_cat_title()`, `single_tag_title()`, and `single_term_title()` are already annotated this way; this one was missed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`WP_Theme::display()` gained a conditional return type in the previous commit, but the two private methods it hands its value to were left saying only that the result is a string or an array. Both say something more specific. `sanitize_header()` is where the array comes from: it takes a string in every case and returns one back, except for Tags, which it explodes on commas. That is a condition on `$header`. Giving it one also supplies the value type its `@return` was missing, which is where the one error this removes came from. `translate_header()` never changes the shape it is handed. Every branch either returns `$value` untouched or replaces it with `translate()`, so a string in means a string out and an array in means an array out. That is a condition on `$value`, not on `$header` -- the Tags branch returns the value as-is when it is empty or the feature list is unavailable, so even Tags can come back as a string. The two together mean the shape is now stated at each step it passes through: `sanitize_header()` decides it from the header name, `get()` reports it, `translate_header()` preserves it, and `display()` resolves it for callers. Also drop the `@phpstan-param` and `@phpstan-return` added to `translate_header()` in the previous commit. `string[]` is ordinary PHPDoc, so the plain tags carry it -- PHPStan reports the same 87 errors for the file either way. Only the conditional genuinely needs a prefixed tag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both functions have three distinct return values selected by their arguments: a bool for whether anything at all is hooked when no callback is given, the callback's priority as an int when one is, and a bool again when a specific priority is also supplied. The docblock has described that in prose since 6.9.0 added `$priority`, while the declared type stayed `bool|int`, so callers had to re-establish which of the three they were holding. Declare the condition on `has_filter()`, `has_action()`, and the `WP_Hook::has_filter()` they both ultimately reach. This resolves eight errors across the tree, all of the same shape: a priority is read back out of `has_filter()` and handed straight to `add_filter()` or `remove_filter()`, which want an int. `do_blocks()` is the clearest case -- it takes the priority of `wpautop`, removes the filter, and re-adds a restore callback at `$priority + 1`. Guarding with `false !== $priority` used to leave `true|int`, so both the arithmetic and the hook calls were checked against a type that included a bool. The same pattern appears in `wp_filter_content_tags()`, `wp_common_block_scripts_and_styles()`, and `WP_Widget_Text`. PHPStan reports `return.unusedType` against `has_filter()` and `has_action()` anyway, claiming neither returns an int. That is a bug in PHPStan, reported as phpstan/phpstan#15268: when it combines the branches of a conditional return type for a call whose argument is not narrow enough to select one, it drops the int from an `int|false` branch. So each wrapper is handed a type that has already lost the int before it can return it. The fault depends on the PHP version PHPStan itself runs on, not on the analysed `phpVersion`: correct on PHP 8.2, wrong on 8.3, 8.4 and 8.5, identically across PHPStan 2.2.13 through 2.3.x-dev. Suppress it in `phpstan.neon.dist` rather than with `@phpstan-ignore` comments. An inline ignore has to match, and on PHP 8.2 there is nothing to match, so it becomes an `ignore.unmatchedIdentifier` error of its own -- which is non-ignorable, and would report in any editor configured against 8.2 while CI on a newer PHP stayed green. The config entry takes `reportUnmatched: false` and so is silent either way. Remove it, and its comment, once the upstream fix ships. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… `@return`. `_get_block_templates_files()` returns null for a `$template_type` that is neither 'wp_template' nor 'wp_template_part', and an array of template files for either of those. Both are the only outcomes, so the condition is exact. Giving it one also supplies the value type its `@return` was missing, which is what removes the five errors below. `wp_find_hierarchy_loop_tortoise_hare()` returns an array of the loop's members when `$_return_loop` is set and an arbitrary member's ID otherwise, both by way of the same `$_return_loop ? $return : $tortoise` expression, or false when no loop was found. Only the array case is worth stating, since the scalar is whatever the `$callback` returns and stays `mixed`; note the `false` in both. While there, say in the description that false means no loop, which was not written down anywhere. The element type on the first is `array<array-key, mixed>` rather than the `array<string, mixed>` these items actually are. The items come back from `_add_block_template_info()` and `_add_block_template_part_area_info()`, both of which declare a bare `array`, so the stricter type is correct but not checkable from here. Typing those two is its own change. Separately, `_get_block_template_file()` claimed to return "Array with template metadata if $template_type is one of 'wp_template' or 'wp_template_part', null otherwise". The second half is wrong: a matched `$template_type` also returns null when the theme has no file for `$slug`, which is the common case for any slug the theme does not define. Say both reasons. No conditional for this one, because with null reachable from either branch there is nothing left to distinguish. Across the full tree this removes six static-analysis errors and introduces none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rkaround. The conditional return types on `has_filter()`, `has_action()`, and `WP_Hook::has_filter()` tripped a PHPStan bug that dropped the `int` from their `int|false` branch, so `return.unusedType` claimed each wrapper never returns an int. Keeping the tree clean needed an `ignoreErrors` entry, reported upstream as phpstan/phpstan#15268. The bug turns out to depend on the order the union is written in, and on the branch it sits in: `int|false` in the else branch loses its `int`, while `false|int` in the same place, or `int|false` in the then branch, does not. The two spellings denote the same type and callers cannot tell them apart, so write the one that works and delete the suppression along with its comment. Verified with each of four PHP builds running PHPStan against `phpstan.neon.dist`: no errors on 8.2.33, 8.3.33, 8.4.25, or 8.5.9, where before this every version from 8.3 up needed the entry. Call sites are unchanged -- `has_filter( $hook )` still resolves to `bool`, `has_filter( $hook, $cb )` to `int|false`, and `has_filter( $hook, $cb, 10 )` to `bool` -- and the full analysis is unchanged at 27,760 errors, with none introduced and none resolved. The spelling comes from the PHPStan types in WordPress#13530, which writes `($callback is false ? bool : false|int)` and so never met this. That version predates the `$priority` parameter added in 6.9.0, however: it reports `false|int` for a call that passes a specific priority, where the function returns a bool. The three-way condition is kept here for that reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ion map. The function map behind the WordPress stubs has carried conditional return types for years, and WordPress#13530 proposes moving them into core's own docblocks. Take the conditional ones from that set here, so this branch covers the same ground rather than half of it. That PR is a draft and expected to keep moving; where the two disagree, settling it is its business rather than this branch's. Only the conditional `@phpstan-return` tags are taken. That PR also adds `@phpstan-pure`, `@phpstan-impure`, `@phpstan-assert-if-true` and a number of plain `@phpstan-param` types, none of which have anything to do with conditional returns; those are left to it. The exception is `@phpstan-template`, kept together with the `@phpstan-param T $x` that binds it, wherever a condition is written in terms of the template rather than of a parameter -- `maybe_serialize()`, `rest_sanitize_boolean()`, `wp_http_validate_url()`, `add_cssclass()`, `sanitize_sql_orderby()`, `translate_plural()` -- since without the declaration the type does not parse. Fifty functions across twenty-five files, all of the kinds this branch has been working through already: - A flag deciding whether failure arrives as a `WP_Error` or as `false`: `wp_schedule_single_event()`, `wp_schedule_event()`, `wp_reschedule_event()`, `wp_unschedule_event()`, `wp_clear_scheduled_hook()`, `wp_unschedule_hook()`, `wp_set_comment_status()`, `wp_update_comment()`, `wp_insert_link()`. - An argument selecting the shape of the result: `wp_list_categories()` and `wp_generate_tag_cloud()` on `echo` and `format`, `paginate_links()` on `type`, `get_categories()` and `get_tags()` on `fields`, `get_user_by()` on `$field`, `wp_debug_backtrace_summary()` on `$pretty`, `WP_Dependencies::query()` on `$status`. - A literal argument value the result follows from: `size_format()`, `bool_from_yn()`, `validate_file()`, `zeroise()`, `get_tag_regex()`, `maybe_serialize()`, `block_version()`, `taxonomy_exists()`, `tag_exists()`, `is_term()`, `sanitize_term_field()`, `wp_is_post_revision()`, `wp_unique_prefixed_id()`, `translate_plural()`. - Emptiness or the type of the input deciding what can come back at all: `wp_get_link_cats()`, `add_cssclass()`, `delete_plugins()`, `validate_plugin()`, `wp_extract_urls()`, `path_is_absolute()`, `wp_is_numeric_array()`, `wp_is_uuid()`, `sanitize_sql_orderby()`, `wp_http_validate_url()`, `has_shortcode()`, `is_wp_error()`, `rest_ensure_response()`, `rest_sanitize_boolean()`, `get_user()`, `get_the_permalink()`, `get_post_permalink()`, `addslashes_gpc()`, `get_block_wrapper_attributes()`. Formatted the way the rest of core's conditional types are written: spaces inside the parentheses and the array-shape braces, a blank docblock line separating the PHPStan tags from the standard ones, and the two that ran long broken across lines as the longer ones here already are. Twelve of the set overlap with work already on this branch and are left alone for now; each is compared on its own merits in what follows. Eight more did not apply cleanly, five because the function already carries a conditional type on trunk and two because an attribute sits between the docblock and the declaration; those are handled separately too. One baseline entry falls away: an `if.alwaysFalse` in the REST comments controller that a narrowed type makes reachable again. Verified against `phpstan.neon.dist` with PHPStan running on PHP 8.2.33 and 8.3.33: no errors on either, and PHPCS reports nothing on the added lines. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…` with it. Of the twelve conditional return types in WordPress#13530 that overlap work already on this branch, `get_bookmark()` is the one where that PR is plainly ahead, and it unblocks a function dropped earlier for want of exactly this. `get_bookmark()` was attempted here before and abandoned: `sanitize_bookmark()` declares `stdClass|array` and `$_bookmark` is `mixed` for much of the function, so a `stdClass` branch produced errors on the returns. The version in that PR gets around it by spelling the array branches `array<string, mixed>` and `array<int, mixed>`, whose union is plain `array` and therefore accepts whatever `sanitize_bookmark()` hands back. The earlier attempt here used `list<mixed>` for the `ARRAY_N` branch, which is more precise and breaks that union. Precision was the wrong trade: this removes two errors, one of them the missing value type on the `@return`. `get_link()` is the deprecated wrapper over `get_bookmark()` and was dropped for the same reason, so give it the same condition. Its `@return` also gains the `null` it has always been able to return. The other eleven overlaps stay as they are: - `mysql2date()`, `current_time()`, `single_month_title()` and `get_sites()` are the same type either way; the two branches converged on them independently. - `get_term()` and `get_term_by()` are written there with `array<string, string|int>` and `list<string|int>` rather than `mixed`. More precise and not checkable: `WP_Term::to_array()` returns `array<string, mixed>`, and tightening that runs into `get_object_vars()`, which PHPStan will not resolve to a shape. Adopting it costs two `return.type` errors for no gain. - `get_category()` is written there as an intersection of two conditionals, one on `$output` and one on `$category`, which is a technique worth knowing and narrows better. The `$category` half is wrong, though: it excludes `WP_Error|null` for any object, while a `stdClass` whose `filter` is set to anything but 'raw' reaches `WP_Term::get_instance()`, which is documented `WP_Term|WP_Error|false`. Nothing in core passes an object to `get_category()` anyway. - `get_category_by_path()` differs only in `array<int, mixed>` where this branch has the more precise `list<mixed>`, which verifies here. - `wp_insert_category()` is written there as `int<0, max>` and `int<1, max>|WP_Error`. Correct, and it does not verify: the term ID comes back from `wp_insert_term()` and `wp_update_term()` as a plain `int`, and that PR does not retype either. - `has_filter()` and `has_action()` are written there without the `$priority` parameter added in 6.9.0, so they report `false|int` for a call that passes a priority, where the function returns a bool. A `property.nonObject` baseline entry falls away, now that `get_bookmark()` reports an object rather than `mixed`. Verified against `phpstan.neon.dist` with PHPStan running on PHP 8.2.33 and 8.3.33: no errors on either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Eight of the conditional types in WordPress#13530 did not go in with the rest. Six do now, and the two that do not are recorded below. `WP_Theme::offsetExists()` and `WP_Theme::offsetGet()` answer for a fixed set of header names, so both condition on whether `$offset` is one of them. The set is declared once as a `@phpstan-type Theme_Key` on the class, which is what the two conditions are written in terms of; that PR calls it `ThemeKey`, but core's own aliases are `Data_Array`, `Endpoint_Arg`, `Header_Image_Data` and so on, so follow that. Array access resolves through the condition: `$theme['Bogus']` is now `null` rather than `mixed`. `isset( $theme['Name'] )` still does not, because PHPStan answers that from its own ArrayAccess handling rather than from `offsetExists()`, but a direct call does. `wp_unique_id()` and `wp_unique_id_from_values()` return a string whose shape follows the prefix: lowercase for a lowercase prefix, and numeric when there is no prefix at all. `WP_Translations::translate()` returns null only for a null singular. `get_permalink()` cannot fail when handed a `WP_Post` rather than an ID, so the `false` drops out for that case. `absint()` keeps the `non-negative-int` it already had. The version in that PR resolves literals exactly -- `absint( 5 )` to `5`, `absint( 'abc' )` to `0`, `absint( true )` to `1` -- but it costs more than it returns. It gives up the non-negative guarantee for any argument PHPStan cannot pin down, which is most of them, resolving `absint( $mixed )` to plain `int`; and the template it uses needs a `@phpstan-param T|scalar|array|resource|null`, which narrows a parameter that was `mixed` and so reports at every call site handing it something unknown. Measured across the whole tree that is 215 errors added against a handful resolved. Stating the guarantee unconditionally in one line is the better trade. Intersecting the two was tried and does not work either: the intersection stops the template resolving, so the literal precision is lost anyway. `_get_list_table()` keeps what it has too. It is already generic, taking `class-string<T>` and returning `T|false`, which holds for any class name. The version in that PR enumerates the seventeen core list tables and returns `new<T>`, which is more precise for those and says nothing about anything else. Verified against `phpstan.neon.dist` with PHPStan running on PHP 8.2.33 and 8.3.33: no errors on either, and no baseline needed regenerating. Co-Authored-By: Claude Opus 5 <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. |
Member
Author
|
cc @apermo |
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. |
apermo
reviewed
Sep 19, 2026
| * @param int $link_id Link ID to look up. | ||
| * @return int[] The IDs of the requested link's categories. | ||
| * | ||
| * @phpstan-return ( $link_id is empty ? array{ } : array<int, int<1, max>> ) |
There was a problem hiding this comment.
While intentionally correct, technically it can be wrong if someone messes with the database and sets a negative id in there manually.
But I m happy to ignore this technicality
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Declares PHPStan conditional return types on 89 functions and methods whose return type is selected by one of their arguments. All of them already described that selection in prose in their
@returntext; none of them said it in a way a static analyzer could use.For example,
get_term()has documented since 4.4.0 that$outputpicks between aWP_Term, an associative array and a numeric array, while declaring the flat union, so every caller had to re-establish which one it was holding:Why
Because the flat union is not just imprecise, it is unusable. A caller writing
get_term( $id, 'category', ARRAY_A )['name']is checked against a type that includesWP_Term, so the offset access is an error; a caller doing the opposite is checked against a type that includesarray, so the property access is an error. Neither caller is wrong. The type was.The effect is largest where a value is read back out of core and fed straight back in.
do_blocks()is the clearest case:has_filter()returnedbool|int, sofalse !== $prioritylefttrue|intand both hook calls, plus the arithmetic, were checked against a type including a bool. With the condition declared,$priorityisint|false, the guard narrows it toint, and eight call sites of that shape stop reporting, inblocks.php,media.php,script-loader.phpandWP_Widget_Text.What is here
Grouped by the kind of argument that does the selecting:
$outputselecting an object or an array form:get_term(),get_term_by(),get_category(),get_category_by_path(),get_tag(),get_bookmark(),get_link(),get_page_by_title(), andWP_Term::to_array()gains the value type the branches need.countandfieldsreshaping a query result:WP_Comment_Query::query(),get_sites(),WP_Site_Query::query(),get_networks(),WP_Network_Query::query(),get_users(),get_categories(),get_tags().current_time(),mysql2date(),get_the_date(),get_the_modified_date(),get_the_time(),get_post_time(),get_the_modified_time(),get_post_modified_time(). Each of these already said "Integer if$formatis 'U' or 'G', string otherwise" in prose.WP_Erroror asfalse: thewp_schedule_*andwp_unschedule_*family,wp_set_comment_status(),wp_update_comment(),wp_insert_link(),wp_insert_category(),wp_allow_comment().echoordisplayflag returning the markup rather than printing it:single_month_title(),wp_list_categories(),wp_generate_tag_cloud(),paginate_links().WP_Themeheader pipeline:sanitize_header()decides the shape from the header name,get()reports it,translate_header()preserves it, anddisplay()resolves it for callers. This one removes 57 errors on its own, nearly all of themdisplay()results being concatenated or passed tosprintf()in the themes list table, the theme editor, the upgrader skins andupdate-core.php.has_filter(),has_action(),WP_Hook::has_filter(),WP_Theme::offsetExists(),WP_Theme::offsetGet(),get_permalink(),get_user_by(),is_wp_error(),absint()'s neighbours inload.php, and about thirty more of the same shape.One non-docblock change:
wp_filter_oembed_result()was handingWP_Site::$blog_id, a numeric string for historical reasons, straight toswitch_to_blog(), which wants an int. The line immediately above it already casts for its comparison. Narrowingget_sites()is what surfaced it.Prior art
Conditional return types are not new to core. This follows a line of changesets that established the pattern and the conventions used here, starting from the PHPStan integration itself:
voidwith proper return types inwpdband related functions._load_script_textdomain_from_src().wpdbquery methods.term_exists().neverreturn types to functions that always terminate.wp_parse_list()et al.string[]input type inwp_parse_list()return.fields.$displayreturn docs.voidon the dual-mode template tag.wp_get_object_terms()when requesting a count.esc_sql()parameter and return types.Those left 103 functions with a conditional return type on trunk. This takes that to 192, and several of the annotations here are direct continuations:
get_comments()was given one in r62822 whileWP_Comment_Query::query(), the method it is a thin wrapper over, was not, so the narrowing was asserted at the wrapper rather than derived from the method that produces the value. That is the first commit in this branch.Relationship to #13530
#13530 was found late, with most of the work below already done. It proposes moving the same conditional types out of the WordPress stubs' function map and into core's docblocks, and it served two purposes here once discovered: the annotations not yet covered were adopted from it, and its versions of the ones already written were used as a cross-reference, which is how several of the decisions below got settled in each direction.
Only the conditional
@phpstan-returntags were taken, plus the@phpstan-templatedeclarations that some of those conditions are written in terms of; the@phpstan-pure,@phpstan-assert-if-trueand plain@phpstan-paramadditions in that PR are its own business. Twelve annotations overlap. Most are identical or equivalent, which is some reassurance about both; where they differ:get_bookmark()is taken from Docs: Bring the PHPStan types from the WordPress stubs' function map into core #13530 and is better than what was attempted here. Spelling the array branchesarray<string, mixed>andarray<int, mixed>gives a union of plainarray, which accepts whateversanitize_bookmark()returns. The attempt here used the more preciselist<mixed>, which breaks that and is why the function was originally left out.has_filter()andhas_action()are kept as written here, because Docs: Bring the PHPStan types from the WordPress stubs' function map into core #13530 predates the$priorityparameter added in 6.9.0 and reportsfalse|intfor a call that passes a priority, where the function returns a bool.absint()keeps its existingnon-negative-int. The version in Docs: Bring the PHPStan types from the WordPress stubs' function map into core #13530 resolves literals exactly but gives up the non-negative guarantee for any argument PHPStan cannot pin down, and its@phpstan-param T|scalar|array|resource|nullnarrows a parameter that wasmixed. Measured across the tree that is 215 errors added._get_list_table()keeps its existingclass-string<T>/T|false, which holds for any class, rather than an enumeration of the seventeen core list tables.An upstream PHPStan bug
The three
has_filter()annotations initially tripped phpstan/phpstan#15268, reported from this work: a conditional branch spelledint|falsesilently loses itsint, but only in the else position, only in that spelling, and only when PHPStan itself runs on PHP 8.3 or newer.return.unusedTypethen claims the function never returns an int. Nothing here depends on the fix: writing the unionfalse|intsidesteps it, which is what #13530 happens to do and what this branch now does too.Verification
phpstan.neon.distreports no errors at every one of the 15 commits, with PHPStan running on PHP 8.2.33 and on 8.3.33. Baselines are regenerated inside the commit that invalidates them, so no point in the history is left inconsistent. A narrowed return type rewrites the message of every error downstream of it, and a baseline entry quoting the old wording then stops matching.Measured at rule level 10, where the effect is visible rather than absorbed by the baselines: 27,962 errors to 27,600, none introduced. At the configured level 5 the tree is clean on both sides, and six previously-baselined errors are resolved, across
argument.type,if.alwaysFalse,isset.property,property.nonObjectandproperty.notFound.Every annotation was checked with
\PHPStan\dumpType()at call sites for each branch, including the defaults:get_post_time()defaults$formatto'U', so a bare call now resolves toint|falserather than the full union.Landing
This is one PR because it is one idea, but it is not expected to be one changeset. The fifteen commits are sequential and each one leaves
phpstan.neon.distclean on its own, which is the reason the baselines are regenerated inside the commit that invalidates them rather than in a single pass at the end. Any leading run of them can therefore be committed on its own, and the natural seams are the argument kinds listed above, with the import from #13530 as the last three.Deliberately not done
WP_Comment_Query::get_comments(),WP_Site_Query::get_sites(),WP_Network_Query::get_networks()andWP_List_Util::pluck()take no parameters; they read$this->query_varsor$this->output. A conditional return type can only branch on a parameter or a template, so these keep their unions and the wrappers that do take the arguments carry the condition instead.wp_list_pluck()andfilter_default_metadata()look like candidates and are not. The first documents a difference in which keys are used, not in the type; expressing it needs a template that core's untyped call sites cannot satisfy, and it produces eightUnable to resolve the template typeerrors. The second is only ever registered as a filter callback, and its fourreturn $value;passthroughs aremixed, so a condition on$singlewould be unverifiable and wrong for a caller passing a non-array value.get_results()onWP_User_Query, andsanitize_bookmark(), would each need internals reconciled before the types can tighten further. Noted in the relevant commits rather than worked around.Trac ticket: https://core.trac.wordpress.org/ticket/65817
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Surveying core for functions whose
@returnprose already describes an argument-dependent return type, drafting the annotations, and writing the commit messages and this description.Every annotation was verified by running PHPStan locally before being committed:
\PHPStan\dumpType()at call sites to confirm each branch resolves as intended, including the defaults, and a whole-tree comparison before and after to confirm nothing new was reported. Every measurement quoted above comes from those runs rather than from an estimate. Where an annotation could not be verified, or would have been inaccurate, it was left out deliberately and the reasoning recorded in the commit;absint(),wp_list_pluck()andfilter_default_metadata()under "Deliberately not done" are the notable cases.Reviewed and directed throughout, including the formatting conventions, the
Theme_Keyalias name, the decision to handle the upstream PHPStan bug in configuration rather than with inline suppression and then to drop that once the union spelling made it unnecessary, and the choice to regenerate the baselines inside each commit rather than once at the end.🤖 Generated with Claude Code
https://claude.ai/code/session_016Wgj2aw5gb2k4V5jzo57N9
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.