Repository navigation
Docs: Add refined string types to PHPStan return and parameter types - #13892
swissspidy wants to merge 22 commits into
Conversation
Adds `non-empty-string`, `non-falsy-string` and `lowercase-string` return types where the function always produces such a string, and narrows a few parameters whose function rejects an empty value: `wp_register_ability()`, `register_post_type()`, `add_shortcode()` and `wp_upload_bits()`. The map's `non-empty-string` on the `$hook_name` parameter of the hook functions and `non-falsy-string` on `register_rest_route()` are left out, since core itself passes plain strings to them. See #65817. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VieGtSQtUoVfHiGwTmkvtc
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @claude. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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.
Copilot review overview
🟡 Changes recommended
Several parameter and hook-return refinements do not match values handled by the implementations.
Review effort: Balanced
Findings: 3
Open (3)
What changed in this PR
Adds refined PHPStan string types to core DocBlocks without changing runtime behavior.
Changes:
- Narrows selected string parameters.
- Refines helper return types for non-empty, non-falsy, and lowercase strings.
- Improves static analysis across APIs, markup helpers, hashes, and regex generators.
| File | Description |
|---|---|
src/wp-includes/shortcodes.php |
Refines shortcode parameter and regex types. |
src/wp-includes/script-loader.php |
Refines script-tag output. |
src/wp-includes/rewrite.php |
Refines feed hook output. |
src/wp-includes/post.php |
Narrows post-type names. |
src/wp-includes/plugin.php |
Refines current hook names. |
src/wp-includes/pluggable.php |
Refines nonce and hash outputs. |
src/wp-includes/link-template.php |
Types internal hosts as lowercase. |
src/wp-includes/functions.php |
Refines utility parameters and returns. |
src/wp-includes/formatting.php |
Refines regex, slug, and path outputs. |
src/wp-includes/class-wp-widget.php |
Refines widget field identifiers. |
src/wp-includes/class-wp-dependencies.php |
Refines ETag output. |
src/wp-includes/class-wp-ajax-response.php |
Refines XML response output. |
src/wp-includes/blocks.php |
Refines pagination-arrow markup. |
src/wp-includes/block-template-utils.php |
Refines export-file paths. |
src/wp-includes/block-supports/elements.php |
Refines generated class names. |
src/wp-includes/abilities-api.php |
Narrows ability names. |
src/wp-admin/includes/widgets.php |
Refines body-class output. |
src/wp-admin/includes/template.php |
Refines submit-button markup. |
src/wp-admin/includes/plugin.php |
Refines plugin-page hook names. |
src/wp-admin/includes/ms.php |
Refines help-sidebar markup. |
src/wp-admin/includes/media.php |
Refines media form markup. |
src/wp-admin/includes/class-wp-comments-list-table.php |
Refines avatar markup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
🤖 Comment by Claude Opus 5.5 Props for this subset, gathered from the full history of Props for this PR (swissspidy for the split, westonruter for the review that shaped it): Generated by Claude Code |
Drop the non-empty-string return type from current_filter() and current_action(), since apply_filters() and do_action() do not reject an empty hook name. Use non-falsy-string for register_post_type(), which rejects '0' as well, and place @phpstan-param before @phpstan-return in wp_upload_bits(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BuoHZQvJMvPEYp4Sfjgvd
# Conflicts: # src/wp-includes/functions.php
PHPStan cannot see that requiring `version.php` assigns the static `$wp_version`, so the return value was inferred as `mixed`, which conflicts with the `non-falsy-string` return type. An inline `@var` after the `require` asserts the type. A `@global` tag would not work, since the function deliberately avoids the global. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The untyped static was inferred as `mixed`, which conflicts with the `non-falsy-string` return type. Annotating it as `non-falsy-string|null` lets PHPStan narrow it to `non-falsy-string` by the time it is returned. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PHPStan loses the `lowercase-string` type established by `strtolower()` once the title passes through `str_replace()` and `preg_replace()`, so the return value was inferred as `string`. At runtime only lowercase characters can remain after the `[^%a-z0-9 _-]` replacement, so an inline `@var` asserts the declared return type. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The untyped static `$internal_hosts` was inferred as `mixed`, which conflicts with the declared return type. Annotating the static fixes that, since `array_map( 'strtolower', … )` already yields lowercase strings. The result of `array_unique()` is also passed through `array_values()`, so that the function returns a `list` even when a filter adds a duplicate host. Callers only check membership with `in_array()`, so the reindexing does not affect them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PHPStan infers `non-falsy-string` from the `sprintf()` format, but does not know that the `%x` conversion only produces lowercase hex digits. The result is now assigned to a variable with an inline `@var`, so that it matches the declared `lowercase-string&non-falsy-string` return type. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The conditional return type of `path_is_absolute()` narrows its result but not its argument, so the early `return $path;` in `path_join()` was inferred as `string`, which conflicts with its `non-falsy-string` return type. Every path that returns true requires a path that is neither empty nor `'0'`, so `@phpstan-assert-if-true` narrows `$path` accordingly. The false branch is left unnarrowed, since relative paths like `'../'` also return false. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The key is documented as only containing lowercase alphanumeric characters, dashes, and underscores, and the function rejects empty names. This matches the type the branch gives `register_post_type()`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The name is passed through `sanitize_key()`, so the registered status is always lowercase, and every core call registers a lowercase literal. This matches the type the branch gives `register_post_type()`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`WP_Abilities_Registry::register()` validates the name against `/^[a-z0-9-]+\/[a-z0-9-]+$/`, so it now has the same `lowercase-string&non-falsy-string` type as `wp_register_ability()`. `wp_register_ability_category()` and `WP_Ability_Categories_Registry::register()` validate the slug against `/^[a-z0-9]+(?:-[a-z0-9]+)*$/`. That pattern accepts `'0'`, so the slug is narrowed to `lowercase-string&non-empty-string`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`WP_Block_Bindings_Registry::register()` rejects uppercase characters and requires a name matching `/^[a-z0-9-]+\/[a-z0-9-]+$/`, so it and `register_block_bindings_source()` now take a `lowercase-string&non-falsy-string`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`WP_Icon_Collections_Registry::register()` validates the slug against `/^[a-z0-9]([a-z0-9_-]*[a-z0-9])?$/`. That pattern accepts `'0'`, so the slug is narrowed to `lowercase-string&non-empty-string`. `WP_Icons_Registry::register()` requires a namespaced name whose collection is registered and whose icon name is lowercase, so the name is narrowed to `lowercase-string&non-falsy-string`. The wrapper functions get the same types. To keep the default icons registered by `_wp_register_default_icons()` consistent with this, the generated icon manifest is given an array shape with lowercase icon names and collection slugs. This also resolves the `mixed` errors previously reported for the manifest's label, collections, and keywords. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`WP_Connector_Registry::register()` validates the ID against `/^[a-z0-9_-]+$/`. That pattern accepts `'0'`, so the ID is narrowed to `lowercase-string&non-empty-string`. The default AI providers are registered with IDs that come from the AI Client, which PHPStan cannot see are lowercase. `ProviderMetadata` already rejects IDs that are not lowercase, so an inline `@var` asserts the type at that call site. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`WP_Block_Type_Registry::register()` rejects uppercase characters and requires a name matching `/^[a-z0-9-]+\/[a-z0-9-]+$/`, so a string name is narrowed to `lowercase-string&non-falsy-string`. `register_block_type()` keeps a plain `string` parameter, since it also accepts a path to block metadata. Once the path case has returned, the remaining string should be a block type name, which an inline `@var` asserts before it is passed to the registry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`WP_Block_Templates_Registry::register()` rejects uppercase characters and requires a name matching `/^[a-z0-9_\-]+\/\/[a-z0-9_\-]+$/`, so it and `register_block_template()` now take a `lowercase-string&non-falsy-string`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`add_menu_page()`, `add_submenu_page()`, and the `add_*_page()` helpers now take a `non-falsy-string` menu slug. The slug is not narrowed to lowercase, even though the documentation says it should be: established plugins register mixed-case slugs that appear in admin URLs, so they cannot be changed without breaking existing links. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The shape uses PHPStan-only types such as `lowercase-string` and `non-empty-list`, so it belongs in a `@phpstan-` prefixed tag, where other tools that read docblocks will not misinterpret it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This reverts the switch to `@phpstan-var`. Inline annotations on local variables are not part of the developer documentation, and core already uses plain `@var` with array shapes for them, as in `blocks.php`. The `@phpstan-` prefix is only needed in docblocks that document a symbol. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@westonruter please feel free to commit :) |
Adds `non-empty-string`, `non-falsy-string`, and `lowercase-string` return types where a function always produces such a string, mostly taken from [https://github.com/php-stubs/wordpress-stubs php-stubs/wordpress-stubs]. Where PHPStan cannot infer the refinement from the function body, such as from a static cache or a `sprintf()` call, the type is asserted inline so that the narrower types introduce no new errors. Registration functions now require a lowercase key where it is documented or validated as lowercase. This covers post types, post statuses, and taxonomies, as well as the names used by the abilities, block bindings, block types, block templates, connectors, and icons registries. Static analysis describes what callers should supply, so these follow the documented contract even where a function lowercases the key itself. Admin menu slugs are only narrowed to `non-falsy-string`: although they are documented as lowercase, established plugins register mixed-case slugs which appear in admin URLs and cannot be changed without breaking links. Script and style handles are left as plain strings for the same reason, since mixed-case handles are widespread and other code must pass them verbatim as dependencies. The only runtime change is in `wp_internal_hosts()`, which now re-indexes the deduplicated hosts with `array_values()` so that it returns a list. Developed in #13892. Follow-up to r55289, r64163. Props marian1, swissspidy, westonruter. See #65817. git-svn-id: https://develop.svn.wordpress.org/trunk@64235 602fd350-edb4-49c9-b593-d223f7449a82
Adds `non-empty-string`, `non-falsy-string`, and `lowercase-string` return types where a function always produces such a string, mostly taken from [https://github.com/php-stubs/wordpress-stubs php-stubs/wordpress-stubs]. Where PHPStan cannot infer the refinement from the function body, such as from a static cache or a `sprintf()` call, the type is asserted inline so that the narrower types introduce no new errors. Registration functions now require a lowercase key where it is documented or validated as lowercase. This covers post types, post statuses, and taxonomies, as well as the names used by the abilities, block bindings, block types, block templates, connectors, and icons registries. Static analysis describes what callers should supply, so these follow the documented contract even where a function lowercases the key itself. Admin menu slugs are only narrowed to `non-falsy-string`: although they are documented as lowercase, established plugins register mixed-case slugs which appear in admin URLs and cannot be changed without breaking links. Script and style handles are left as plain strings for the same reason, since mixed-case handles are widespread and other code must pass them verbatim as dependencies. The only runtime change is in `wp_internal_hosts()`, which now re-indexes the deduplicated hosts with `array_values()` so that it returns a list. Developed in WordPress/wordpress-develop#13892. Follow-up to r55289, r64163. Props marian1, swissspidy, westonruter. See #65817. Built from https://develop.svn.wordpress.org/trunk@64235 git-svn-id: http://core.svn.wordpress.org/trunk@63386 1a063a9b-81f0-0310-95a4-ce76da25c4cd
| * @param int $comment_id Comment ID. | ||
| * @return string Avatar with the user name. | ||
| * | ||
| * @phpstan-return non-falsy-string |
There was a problem hiding this comment.
it seems like we have been adding a lot of aspirational, but wrong type annotations lately, and I’m really concerned about the choices we’re making in response to PHPStan warnings.
is it not obvious by inspection that this can return a false string? maybe I’m wrong, but it seems like get_comment() can return null and get_avatar() can return false, which leads to string(1) " " which itself is falsy.
if this is the case, these more-frequent invalid type annotations only hide type errors and make them harder to find, and they taint the type checking even further by asserting that the type is not what the code says, but that there is some reason to trust us rather than the code.
at least this won’t case crashes, being the docblock annotation, but it can still cause headaches when debugging why a false appears where the type system claims it cannot
There was a problem hiding this comment.
Actually, " " is truthy: https://3v4l.org/Ft5fq#veol
Otherwise, if it had indeed been falsy, then an aspirational type would be wrong.
If there is any other example which is aspirational you see, I'm happy to revisit!
There was a problem hiding this comment.
the choices we’re making in response to PHPStan warnings.
Also, most of these narrowed types here are not to address any warnings. They are intended to improve correctness so that callers and callees have as specific type information available.
There was a problem hiding this comment.
thanks for refuting me on that @westonruter
mostly it’s with these dependent-type annotations like non-empty-string or non-empty-array — these seem frail, and by aspirational I mean it’s what we expect, but not necessarily what can occur.
I tend to skip over these without too much thought, and maybe every time I see them I’m wrong, so I’ll try and pay more attention, though I definitely cannot keep up with the pace of PHPStan changes.
There was a problem hiding this comment.
@dmsnell The hope is that you won't have to think about these. We can offload the thinking to PHPStan. If the method here is ever modified to do something like this instead:
diff --git a/src/wp-admin/includes/class-wp-comments-list-table.php b/src/wp-admin/includes/class-wp-comments-list-table.php
index 7294f89d1b..419427b6ce 100644
--- a/src/wp-admin/includes/class-wp-comments-list-table.php
+++ b/src/wp-admin/includes/class-wp-comments-list-table.php
@@ -68,7 +68,7 @@ class WP_Comments_List_Table extends WP_List_Table {
public function floated_admin_avatar( $name, $comment_id ) {
$comment = get_comment( $comment_id );
$avatar = get_avatar( $comment, 32, 'mystery' );
- return "$avatar $name";
+ return $avatar . $name;
}
/**Then PHPStan would raise an error here:
71 Method WP_Comments_List_Table::floated_admin_avatar() should return non-falsy-string but returns string.
🪪 return.type
at src/wp-admin/includes/class-wp-comments-list-table.php:71
In this way, I don't think they are frail. They are being checked for their truthfulness (and truthiness!)
The hope is that having these narrowed types will allow users of the method to avoid dead code. For example, if someone did this:
$avatar = $list_table->floated_admin_avatar( 'John Smith', 1 );
if ( $avatar ) {
echo "<div>$avatar</div>";
} else {
echo "<div>NO AVATAR!</div>"; // DEAD CODE!
}Then PHPStan can flag the if() condition as an if.alwaysTrue error.
There was a problem hiding this comment.
Then PHPStan would raise an error here…don’t think they are frail
if we’re overly conservative that seems fine, but we are seeming to be overly-optimistic in these situations.
it seems that either way, in this case with the comment and avatar, things are not changed, but some strings are falsey, like "0". and I think the abs_int() example is illustrative.
PHP and WP code generally has done a poor job of indicating the real possible types. I understand PRs like this exist to improve that situation.
in the example, I think the conflation of types inside floated_admin_avatar() is more of the problem than the if ( $avatar ) (though I would agree it’s also a problem). it would seem more robust to perform the checks within floated_admin_avatar() and return a positive signal of failure rather than an implicit behavior, e.g. string|null.
in this particular case I mis-identified the type as being wrong, so it’s not important to focus on it. that was my mistake.
✅ Committed in r64235 (f670e8a).
Split out of #13530, as proposed in this comment (subset 5).
non-empty-string,non-falsy-stringandlowercase-stringare added where the function always produces such a string. Examples are the admin markup helpers,trailingslashit(),path_join(),wp_get_wp_version(), the shortcode regexes,wp_json_encode(),wp_create_nonce()andwp_hash().wp_register_ability()$name,register_post_type()$post_type,add_shortcode()$tagandwp_upload_bits()$name.Changes from #13530
These follow the review analysis:
non-falsy-stringonregister_rest_route()'s$route_namespaceand$route. Every controller passes$this->namespace, which is typedstring, so this alone accounted for 88 new errors at level 10.non-empty-stringon the$hook_nameparameter ofapply_filters(),do_action()and the related functions. Core passes plain strings to them in several places, includingwp-cron.php. As the comment suggested, these parameter narrowings are better handled separately from the return types.Changes after review
non-empty-string|falseoncurrent_filter()andcurrent_action(). Sinceapply_filters()anddo_action()don't reject an empty hook name,''can be the current filter. This belongs with the$hook_namenarrowing above.register_post_type()'s$post_typeislowercase-string&non-falsy-string, since the function also rejects'0'.wp_upload_bits()keepsnon-empty-string, becausenon-falsy-stringwould flag the XML-RPC caller, which only guards against''.wp_upload_bits(),@phpstan-paramnow comes before@phpstan-return, as everywhere else in core.Effect on the analysis
composer run phpstanpasses with no baseline changes. At level 10, compared against trunk, 17 errors are fixed and 21 are introduced. Most of the 21 are existing errors reworded with the narrower type. The rest arereturn.typeerrors where PHPStan cannot infer the refinement from the function body, for example a static cache inget_html_split_regex()andsprintf()inwp_generate_uuid4(). (These counts were measured before the changes after review, which remove thecurrent_filter()error among the 21.)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.5
Used for: Splitting #13530 by tag kind, applying the corrections from the review analysis on #13530 and from review on this PR, checking each subset with
composer run phpstan(also at level 10, compared against trunk) andphpcs, and drafting the commit messages and this description. Reviewed 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.
🤖 Generated with Claude Code
https://claude.ai/code/session_015BuoHZQvJMvPEYp4Sfjgvd