Repository navigation
Docs: Add PHPStan type assertions - #13896
swissspidy wants to merge 8 commits into
Conversation
Adds `@phpstan-assert-if-true` to functions that check their argument, such as `taxonomy_exists()`, `shortcode_exists()` and `wp_is_uuid()`, and `@phpstan-pure` and `@phpstan-impure` to functions whose result depends only on their arguments, or does not. `addslashes_gpc()` is not marked as pure, since it calls `_deprecated_function()`. 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. 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. Props Bot commandsYou can trigger Props Bot actions by commenting on this PR:
|
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
🟢 Approval recommended
The annotations accurately reflect the reviewed function behavior and introduce no runtime changes.
Review effort: Balanced
Findings: None
What changed in this PR
Adds PHPStan annotations to improve type narrowing and correctly model deterministic and state-dependent functions, without runtime changes.
Changes:
- Adds conditional type assertions to validation helpers.
- Marks deterministic helpers as pure.
- Marks random, time-dependent, and stateful functions as impure.
| File | Description |
|---|---|
src/wp-includes/user.php |
Marks password reset key generation impure. |
src/wp-includes/taxonomy.php |
Adds taxonomy-name type assertion. |
src/wp-includes/shortcodes.php |
Adds shortcode assertions and purity metadata. |
src/wp-includes/rest-api.php |
Marks boolean sanitization pure. |
src/wp-includes/pluggable.php |
Marks mail, nonce, password, and random functions impure. |
src/wp-includes/option.php |
Marks transient retrieval impure. |
src/wp-includes/functions.php |
Adds assertions and purity metadata to general helpers. |
src/wp-includes/formatting.php |
Marks deterministic formatting helpers pure. |
src/wp-admin/includes/menu.php |
Marks CSS-class composition pure. |
src/wp-admin/includes/bookmark.php |
Marks request-dependent link defaults impure. |
💡 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 I couldn't find a WordPress.org profile for @PeterSmith8nss. That GitHub login is listed as a placeholder until the username is confirmed. Props for this PR (swissspidy for the split, westonruter for the review that shaped it): Generated by Claude Code |
Anyone who does not want PHPStan to remember potentially impure return values can set It would be a good idea to agree on an approach before adding |
…ns-purity # Conflicts: # src/wp-includes/formatting.php # src/wp-includes/functions.php # src/wp-includes/shortcodes.php
Drop `@phpstan-impure` until there is agreement on which functions core should mark as impure, given how much of WordPress depends on globals and the database. Drop `@phpstan-pure` too, since with the default `rememberPossiblyImpureFunctionValues: true` it does not change whether PHPStan remembers a function's return value. See #65817. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T3XYNUvB9sQNDXA14uJQeH
swissspidy
changed the title
|
I do not understand the decision to remove the |
|
I wanna add those in a separate PR Edit: see below. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A false return means the path is relative, not that it is falsy, so the non-equality `@phpstan-assert-if-true` wrongly narrowed an already non-falsy-string `$path` to `never` in the false branch. The `=` form narrows only the true branch, matching the assertions added in this PR. The tag is also moved above `@phpstan-return` for consistency. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
In PCRE, `$` also matches before a final newline, so a UUID followed by "\n" passed validation. Anchoring with `\z` matches only at the very end of the string, which also makes the `lowercase-string&non-falsy-string` assertion exact. Tests cover leading and trailing characters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This reverts commit 517e1d9. The stricter validation is a runtime behavior change for REST `format: uuid`, application password `app_id`, and Customizer changeset UUIDs, so it belongs in its own changeset rather than in this docs-only PR. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Functions that validate their argument now tell PHPStan what a `true` result implies about it, so callers no longer need redundant checks after, for example, `taxonomy_exists()` or `wp_is_uuid()`. Each assertion uses the equality form (`=Type`), which narrows only when the function returns `true`. The plain form also asserts the negation on a `false` return, which is wrong for these functions: an unregistered taxonomy or a relative path is still a non-falsy string. For that reason the existing assertion on `path_is_absolute()` is switched to the equality form as well, since an argument already known to be a non-falsy string was otherwise narrowed to `never` when the path was relative, making that branch look unreachable. The annotations are adopted from the function map of the php-stubs/wordpress-stubs package. Developed in #13896. Follow-up to r63941, r64235. Props swissspidy, marian1, szepeviktor, westonruter. See #65817. git-svn-id: https://develop.svn.wordpress.org/trunk@64795 602fd350-edb4-49c9-b593-d223f7449a82
Functions that validate their argument now tell PHPStan what a `true` result implies about it, so callers no longer need redundant checks after, for example, `taxonomy_exists()` or `wp_is_uuid()`. Each assertion uses the equality form (`=Type`), which narrows only when the function returns `true`. The plain form also asserts the negation on a `false` return, which is wrong for these functions: an unregistered taxonomy or a relative path is still a non-falsy string. For that reason the existing assertion on `path_is_absolute()` is switched to the equality form as well, since an argument already known to be a non-falsy string was otherwise narrowed to `never` when the path was relative, making that branch look unreachable. The annotations are adopted from the function map of the php-stubs/wordpress-stubs package. Developed in WordPress/wordpress-develop#13896. Follow-up to r63941, r64235. Props swissspidy, marian1, szepeviktor, westonruter. See #65817. Built from https://develop.svn.wordpress.org/trunk@64795 git-svn-id: http://core.svn.wordpress.org/trunk@63945 1a063a9b-81f0-0310-95a4-ce76da25c4cd
✅ Committed in r64795 (9b095dd).
Split out of #13530, as proposed in this comment (subset 9).
@phpstan-assert-if-trueis added towp_is_stream(),wp_is_uuid(),shortcode_exists(),has_shortcode()andtaxonomy_exists().path_is_absolute()'s existing assertion is changed to the equality form,=non-falsy-string.Changes from #13530
These follow the review analysis:
wp_is_uuid()asserts=lowercase-string&non-falsy-stringwithout theTUuid of stringtemplate, which rejected themixedits callers pass.Changes after review
@phpstan-impureannotations are removed for now. Much of WordPress depends on globals and the database, so nearly every function could be marked impure. Core should first agree on which functions to mark, for example only deliberately random ones such aswp_rand()and ones especially likely to change between calls such asget_transient(), as php-stubs does.@phpstan-pureannotations are removed as well. With the defaultrememberPossiblyImpureFunctionValues: true, PHPStan already remembers return values, so@phpstan-purechanges nothing (see Remembering and forgetting returned values).path_is_absolute()gained@phpstan-assert-if-true non-falsy-string $pathin trunk (r64235), so this PR no longer adds it. That form is not an equality assertion, so PHPStan also applied its negation when the function returnsfalse. A relative path likefoo/baris still a non-falsy string, yet an argument already typednon-falsy-stringwas narrowed toneverin the false branch, making it look unreachable. The=prefix narrows only the true branch, as with the other assertions here.Effect on the analysis
composer run phpstanpasses with no baseline changes.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 the result with
composer run phpstanand 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_01T3XYNUvB9sQNDXA14uJQeH