Opened 8 hours ago
Last modified 3 hours ago
#65376 assigned defect (bug)
Build/Test Tools: Statically verify that hook documentation and arguments stay in sync
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 7.1 | Priority: | normal |
| Severity: | normal | Version: | |
| Component: | Build/Test Tools | Keywords: | has-patch has-unit-tests |
| Focuses: | docs | Cc: |
Description (last modified by )
WordPress hooks rely on conventions that are currently only enforced by human review, which means they drift out of sync over time.
The problem
Every apply_filters() and do_action() call is expected to be preceded by a docblock documenting the hook, or by a reference comment pointing at the canonical docblock when the same hook fires in more than one place, for example:
/** This filter is documented in <file> */
Nothing verifies any of this automatically:
- Reference comments are not checked. A
This filter is documented in <file>comment can point at a file that no longer exists, or at a file that does not actually document that hook. These references have to be manually traced to confirm they are still accurate, and they silently rot when hooks are moved or renamed. - Argument counts are not checked. A hook's docblock lists the arguments it passes, and WordPress forwards those arguments straight to each callback. When a call site passes fewer arguments than documented, a callback registered for the documented count fails with an
ArgumentCountError(or aTypeErrorfor a typed parameter) at runtime. When it passes more, the extra argument is silently dropped and the documentation is misleading. Today the only way to catch either is to manually compare every call site against the docblock. - Undocumented hooks slip through. A new
apply_filters()/do_action()with no preceding docblock or reference is not flagged, so gaps accumulate. - Documentation itself is unverified. Malformed
@paramtags (e.g. missing the variable name) and stale references are invisible until someone reads the file closely.
I realized the need for this when reviewing a PR for #65367, as the file reference was incorrect but I had to manually check it for accuracy.
In regards to argument counts no being checked, consider the following code:
<?php add_filter( 'https_local_ssl_verify', static function ( bool $ssl_verify, string $url ): bool { if ( str_contains( wp_parse_url( $url, PHP_URL_HOST ), 'local' ) ) { $ssl_verify = false; } return $ssl_verify; }, 10, 2 );
The filter callback is expecting two parameters, $ssl_verify and $url, according to the filter documentation:
/** * Filters whether SSL should be verified for local HTTP API requests. * * @since 2.8.0 * @since 5.1.0 The `$url` parameter was added. * * @param bool|string $ssl_verify Boolean to control whether to verify the SSL connection * or path to an SSL certificate. * @param string $url The request URL. */
Nevertheless, the above filter code currently causes a fatal error when loading Site Health:
Uncaught Error: Too few arguments to function {closure}(), 1 passed in /var/www/src/wp-includes/class-wp-hook.php on line 344 and exactly 2 expected
This is because the filter is applied in WP_Site_Health::get_test_rest_availability() without that second parameter:
<?php /** This filter is documented in wp-includes/class-wp-http-streams.php */ $sslverify = apply_filters( 'https_local_ssl_verify', false );
Since the $url parameter was added in 5.1.0, apparently this argument was not also added to all other instances of the https_local_ssl_verify filter being applied, or actually in this case the filter was added in 5.3.0 via r46231 and this parameter was missed.
Because these checks are manual, mistakes happen readily: there are hooks fired with the wrong number of arguments across taxonomy, multisite, XML-RPC, plugin, menu, and REST API code, references pointing at an obsolete file, malformed @param tags, and bundled-theme calls that did not pass the documented arguments.
Separately, the value returned from apply_filters() is typed as mixed, even though the hook's docblock already declares the type of the value being filtered. This discards type information that is sitting right there in the documentation and weakens static analysis of any code that consumes a filtered value.
Proposed solution
Teach PHPStan to read the existing hook docblocks so these conventions are verified automatically rather than by hand:
- Validate that every hook invocation is documented, that "documented elsewhere" references resolve to a real docblock for that hook, and that each call passes the documented number of arguments.
- Use the documented
@paramtype as the return type ofapply_filters().
Then fix the issues this surfaces in core.
As with how r62292 implemented support in a bundled PHPStan extension for the @global tag in WordPress conventions, I suggest the above be implemented as bundled PHPStan extensions to start with. The great work of @szepeviktor in phpstan-wordpress can serve as a starting point. However, once landed in core, they should then be submitted upstream for adding to szepeviktor/phpstan-wordpress so they can more easily be reused by both core and Gutenberg (once gutenberg#66598 is completed).
Change History (9)
This ticket was mentioned in PR #12022 on WordPress/wordpress-develop by @westonruter.
7 hours ago
#3
- Keywords has-patch has-unit-tests added
Teaches PHPStan to read WordPress's existing hook docblocks so that hook documentation, "documented elsewhere" references, and the arguments passed to hooks are verified automatically instead of by manual review — then fixes the issues this surfaces in core.
See the Trac ticket for the full problem framing. In short:
- The value returned from
apply_filters()was typed asmixed, discarding the type already declared in the hook's docblock. /** This filter is documented in <file> */reference comments were never verified, so they could point at a missing file or a file that no longer documents the hook.- Nothing checked that a hook is fired with the number of arguments its docblock describes. Passing too few risks an
ArgumentCountError(or aTypeErrorfor a typed parameter) in a callback registered for the documented count; passing too many silently drops the extra argument and misleads the documentation. - Undocumented hooks and malformed
@paramtags went unflagged.
## What's in this PR
Tooling (in tests/phpstan/, wired in via base.neon so it applies to both the level‑0 phpstan.neon.dist and the local level‑10 config):
- A dynamic return-type extension that types
apply_filters()from the first documented@param. Adapted from `szepeviktor/phpstan-wordpress`. - Resolution of the "documented elsewhere" convention, including dynamic hook names (e.g.
{$type}_template_hierarchy). - A rule requiring every hook invocation to be documented (inline or via a valid reference).
- A rule requiring each invocation to pass the documented number of arguments.
- File-scope
@globaldocblock resolution inGlobalDocBlockVisitor.
Core fixes surfaced by the tooling:
- Hooks fired with fewer arguments than documented:
edit_terms/edited_terms/edit_term_taxonomy/edited_term_taxonomy/term_id_filter,https_local_ssl_verify,wp_update_nav_menu,wp_creating_autosave,install_plugins_upload,activate_{$plugin}, the networkhandle_network_bulk_actions-{$screen}screens, and the XML‑RPCpre_option_enable_xmlrpc/option_enable_xmlrpcshim. - Hooks fired with more arguments than documented:
the_contentindo_trackbacks()andthe_excerptinWP_REST_Revisions_Controller. - Documentation fixes: malformed
@paramtags and a stale reference comment in bundled themes, andthe_permalinkcalls now passing the documentedget_post()argument.
## Notes for reviewers
- The argument-count fixes change which arguments callbacks receive, so they're worth reviewing against existing hook consumers. Where a call site had no meaningful value for a documented parameter (e.g. term-count callbacks, network-wide bulk screens), an empty array or
0is passed to satisfy the signature. - The Gutenberg-generated
src/wp-includes/buildtree is excluded from analysis; the corresponding fix lives in the Gutenbergwp-buildpage templates and is handled separately. - The full analysis is clean except for one pre-existing, unrelated error (
_mb_ord()incompat.php).
## Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 4.8
Used for: Drafting the PHPStan extension and rules, locating and fixing the hook issues across core, and ecosystem impact research. All changes were reviewed and directed by me.
@westonruter commented on PR #12022:
7 hours ago
#4
cc @szepeviktor @apermo
@szepe.viktor commented on PR #12022:
7 hours ago
#5
What an ocean of bugs revealed!
@westonruter commented on PR #12022:
4 hours ago
#6
## Level 10 error impact (trunk vs. this branch)
I ran the full level-10 analysis (phpstan.neon) on trunk and on this branch and compared the results by error identifier. Total: 36,241 → 33,034 file errors — a net reduction of 3,207 (−8.8%).
Only identifiers whose count changed are shown below; all others were identical.
| Error identifier | Before | After | Δ |
| --- | ---: | ---: | ---: |
argument.type | 12,279 | 11,276 | −1,003 |
return.type | 1,483 | 765 | −718 |
offsetAccess.nonOffsetAccessible | 4,839 | 4,171 | −668 |
property.nonObject | 3,752 | 3,571 | −181 |
binaryOp.invalid | 1,305 | 1,161 | −144 |
encapsedStringPart.nonString | 715 | 572 | −143 |
echo.nonString | 311 | 205 | −106 |
foreach.nonIterable | 578 | 507 | −71 |
method.nonObject | 662 | 613 | −49 |
assign.propertyType | 277 | 237 | −40 |
offsetAccess.invalidOffset | 733 | 699 | −34 |
cast.int | 830 | 801 | −29 |
missingType.return | 2,340 | 2,315 | −25 |
assignOp.invalid | 151 | 135 | −16 |
phpDoc.parseError | 5 | 0 | −5 |
cast.string | 89 | 84 | −5 |
parameterByRef.type | 15 | 12 | −3 |
property.nameNotString | 27 | 26 | −1 |
offsetAccess.nonArray | 7 | 6 | −1 |
missingType.iterableValue | 3,377 | 3,376 | −1 |
array.invalidKey | 18 | 17 | −1 |
function.alreadyNarrowedType | 17 | 18 | +1 |
offsetAccess.notFound | 120 | 122 | +2 |
variable.undefined | 540 | 547 | +7 |
return.unusedType | 9 | 16 | +7 |
property.notFound | 757 | 777 | +20 |
| Total (all identifiers) | 36,241 | 33,034 | −3,207 |
### Summary
- The reduction is disproportionately large relative to the code change because the
apply_filters()return-type extension is the dominant lever: typing filtered values (instead ofmixed) cascades through every downstream consumer. The three biggest drops —argument.type(−1,003),return.type(−718), andoffsetAccess.nonOffsetAccessible(−668) — are exactly this: once a filtered value has a real type, PHPStan stops flagging it as passed to typed params, returned from typed functions, or indexed as a possible non-array. - The small regressions (+37 across five identifiers, e.g.
property.notFound+20,variable.undefined+7,return.unusedType+7) are the flip side of more precise typing: giving a value a concrete type occasionally exposes a more-specific complaint thatmixedpreviously masked. These are precision gains rather than new defects, and are far outweighed by the ~3,244 genuine reductions. - The new rules (
wordpress.hookDocMissing,wordpress.hookParamCountMismatch, and the "documented in" reference checks) contribute 0 errors in the "after" run — every violation they surfaced is fixed in this branch — so they add enforcement going forward without leaving standing errors.phpDoc.parseErroralso drops to 0 from the malformed@paramdocblock fixes.
Methodology: both runs used the same local phpstan.neon (level 10, treatPhpDocTypesAsCertain: false); the only difference is the branch's code and the registered rules. The "before" run is trunk (where the new rule files and registrations don't exist, so wordpress.* identifiers are absent by construction).
_(Analysis performed and posted by Claude, on behalf of @westonruter.)_
@westonruter commented on PR #12022:
3 hours ago
#7
I don't intend to commit this all in one go. The PHPStan extensions would be in one commit, and then the other fixes would be committed generally on a per-component basis.
@westonruter commented on PR #12022:
3 hours ago
#8
Or rather, the other fixes would be committed first, and _then_ the PHPStan extensions would be committed.
@westonruter commented on PR #12022:
3 hours ago
#9
@justlevine You may want to review this as well.
PR to fix hook documentation in Gutenberg: https://github.com/WordPress/gutenberg/pull/78826