feat: hook handler argument types (sniff + PHPStan rule)#28
Draft
d4mation wants to merge 3 commits into
Draft
Conversation
- Add StellarWP.Hooks.HookHandlerTypes phpcs sniff (auto-fixable) that forbids native parameter/return types on handlers of hooks whose name does not match a configured prefix - Add companion PHPStan rule (auto-discovered extension) for whole-codebase, cross-file coverage and type-inferred hook names - Add PHPUnit sniff (AbstractSniffUnitTest) and rule (RuleTestCase) tests plus phpunit harness and wired composer test scripts Native types on WP core / third-party hook handlers can cause runtime fatals; these checks enforce type-less handlers for hooks we do not own.
- Filters (any prefix, first-party or not) must now be fully type-less on every parameter and the return type, not just non-first-party hooks - A real WPML fatal (TypeError: filter_has_access(): Argument #3 $user_id must be of type int, null given) showed that even first-party filter context arguments are unsafe: core dispatches apply_filters( 'sfwd_lms_has_access', true, 28, null ) - First-party actions remain unrestricted; non-first-party actions unchanged (params type-less, void return allowed) - Sniff now resolves the old-style [ &$this, 'method' ] callback idiom - Update sniff + PHPStan rule fixtures/tests and the README enforcement matrix
- Action handlers now also forbid native types on every parameter (a native void return type is still allowed); filters already forbid all param and return types - Hook argument/return types are never guaranteed regardless of who defines the hook, so the first-party prefix concept is removed entirely - Remove the sniff `prefixes` property, the rule `prefixes` parameter, and all is_first_party logic; the rule now auto-registers with zero configuration - Update fixtures, tests, extension.neon and the README
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
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
Adds two complementary checks that forbid native parameter and return types on WordPress hook handlers, where the types are not guaranteed.
A hook's argument and return types are never guaranteed: inaccurate documentation can lead to the wrong native type, and any third party can dispatch one of our hooks via
apply_filters()/do_action()with arguments of a different type. Either way, a value that does not match a native type on the handler causes a runtime fatal. Enforcement follows the hook type:voidreturn type is allowed (actions always return void).There is no prefix or first-party concept — the check applies to every hook, and needs zero configuration.
1. PHPCS sniff —
StellarWP.Hooks.HookHandlerTypes(auto-fixable)Analyses at the
add_action()/add_filter()call site and resolves same-file handlers — inline closures/arrows,[ $this, 'method' ]/[ self::class, 'method' ](including the old[ &$this, 'method' ]idiom), and'function_name'globals — then strips only the offending native types (verified to never reformat: nullable/union/intersection/DNF/FQN/variadic/by-ref/attribute forms and multiline layouts, idempotent). Enable with<rule ref="StellarWP.Hooks.HookHandlerTypes"/>. Optionalallow_void_return_on_actionsproperty (defaulttrue).2. PHPStan rule (auto-discovered extension)
Whole-codebase, never diff-limited. Resolves all callback forms across files via reflection (methods,
Class::method, global functions, closures) and resolves hook names via type inference (so a$hook = '...'variable is caught). Report-only; the message names the exact handler. Auto-registers viaphpstan/extension-installerwith no configuration; optionalallowVoidReturnOnActionsparameter (defaulttrue).The two are complementary: the sniff gives instant, auto-fixable feedback for the same-file case; the PHPStan rule is the authoritative gate that catches handlers whose declaration lives in a different file from the hook call — the case a diff-limited phpcs run misses.
Tests
AbstractSniffUnitTest(sniff) — asserts exact error lines + comparesphpcbfoutput to a.inc.fixedbaseline.RuleTestCase(PHPStan rule) — asserts exact messages/lines.composer testruns ruleset-explain + both suites. Green on PHP 7.4 (the rule test skips due to a php-parser emulation quirk on the 7.4 test runtime; the rule itself runs fine on 7.4) and 8.3.Real-world validation
Installed into
learndash-notifications(zero config). Both tools flag a real WPML fatal that had shipped and been hotfixed separately:Each tool reports 16 findings across 5 files, including all 8 in
WPML.php.Notes
phpunit/phpunit,phpstan/phpstan. Newautoload(StellarWP\psr-4),extra.phpstan.includes, and a workingcomposer test.Draft: opened for review before merge.