feat: hook handler argument types (sniff + PHPStan rule)#28
Draft
d4mation wants to merge 17 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.
d4mation
added
code review
- 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
- Removing native types moves type-safety responsibility into the handler - Emphasise checking the type (is_string/is_scalar/is_numeric) before use rather than a blind cast, which can fatal on an object or mangle an array - For filters, return the value unchanged when it is not a type the handler can process
- Handle `$container->callback( Class::class, 'method' )` (lucatume/di52 and StellarWP containers) as a hook callback and check the resolved method's native types - Only acts when the arguments resolve to a real class method, so an unrelated ->callback() is ignored - Verified on real sfwd-lms Providers, which register hooks pervasively this way
- Handle `$receiver->add_action( 'tag', 'method' )` / `->add_filter( ... )` wrappers (e.g. memberdash's MS_Hooker), where the second argument names a method on the receiver resolved to [ $receiver, 'method' ] - Register the rule on CallLike so it also sees method calls, not just function calls - Only acts when the named method exists on the receiver, so unrelated methods named add_action()/add_filter() are ignored
- The sniff now handles $this->add_action( 'tag', 'method' ) / ->add_filter( ... ) wrapper calls (e.g. memberdash's MS_Hooker), resolving 'method' to a method on the enclosing class and auto-fixing it - Only $this-> wrappers are handled (same-file, fixable); other receivers are left to the PHPStan rule - Verified on real memberdash code (MS_Controller_SiteHealth::add_site_health_info)
Confirmed nikic/php-parser 5.x parses fixtures under PHP 7.4 without the token-emulation fatal that PHPStan 1.x's php-parser 4.x hits. Staying on PHPStan 1.x because consuming projects are largely still on 1.x.
The $this->add_action( 'tag', 'method' ) wrapper form (memberdash's MS_Hooker) is resolved via a name-match heuristic, which is safe enough to report on but not to auto-fix: a coincidental method match would have its native types stripped by phpcbf. The sniff now raises a non-fixable error for the wrapper path so it still fails CI while leaving the code untouched; direct callbacks remain auto-fixable. Documented the behavior and the phpcs:ignore escape hatch in the README. Also fixes the self-contradictory "...other than void." message that fired on a void action return when allowVoidReturnOnActions is false (both the sniff and the PHPStan rule), and corrects a stale comment about the hook name being used only for messaging.
Fill in the missing @param/@return tags on every method of the PHPStan rule, replace leading-backslash FQCNs (\PHPStan\Rules\RuleError, \ReflectionType, \ReflectionNamedType) with top-of-file use imports, and give the two error-builder helpers a native : RuleError return type. Also narrow the sniff's Slevomat suppression: process()'s $stack_ptr cannot take a native type because the Sniff interface declares that parameter untyped, but the previous docblock @phpcs:disable had no matching enable and so silenced MissingNativeTypeHint for the rest of the file. Replace it with a scoped, documented phpcs:ignore on the signature line.
Extract shared helpers to remove duplicated logic: - Rule: collect_class_names(), check_class_methods(), handler_prefix() - Sniff: function_name_matches(); hoist bracket token sets to consts Report each inferred constant hook name on its own error line instead of naming only the first when a hook argument narrows to several strings. Drop the never-used default on the sniff's check_handler_types() $fixable argument and document the warn_on_dynamic_hook_names sniff property.
d4mation
commented
Jul 24, 2026
Comment on lines
+34
to
+41
| // This skip is a limitation of the RuleTestCase harness only, on the PHP | ||
| // 7.4 runtime: PHPStan 1.x bundles nikic/php-parser 4.x, whose token | ||
| // emulation fatals when parsing fixtures under PHP 7.4. PHPStan 2.x bundles | ||
| // php-parser 5.x, which fixes this - but the projects that consume this | ||
| // rule are largely still on PHPStan 1.x, so we cannot move to 2.x yet. The | ||
| // rule itself is PHP 7.4-compatible and runs correctly under a normal | ||
| // `phpstan analyse` on 7.4. | ||
| if ( PHP_VERSION_ID < 80000 ) { |
Author
There was a problem hiding this comment.
If we want to add nikic/php-parser: ^5 as a direct dependency of this project it does get around this issue. But it may not be worth potential future confusion I think.
d4mation
commented
Jul 24, 2026
| handler's types could not be verified by the sniff. Leave it off to stay quiet | ||
| about the cross-file and dynamic cases the PHPStan rule already covers. | ||
|
|
||
| #### Wrapper handlers are reported but not auto-fixed |
Author
There was a problem hiding this comment.
This whole slice of the code was based on MemberDash's implementation, but Give has its own version of this idea which wouldn't be immediately compatible with this code: https://github.com/impress-org/givewp/blob/150cf56a1af2e4f58941a3f910d960170ae40340/src/Helpers/Hooks.php
Maybe it isn't worth trying to account for cases like this here? 🤔
Analysing a rand() call made PHPStan lex a function stub whose contents trip php-parser 4.x's enum token emulation on the PHP 8.0 test runtime (a TypeError; 8.1+ has native enums, 7.4 already self-skips). Produce the two-constant-string hook-name union with a phpdoc literal-string union instead, so the fixture pulls in no stub.
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
1 participant
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),'function_name'globals, and$this->add_action( 'tag', 'method' )wrapper calls (memberdash'sMS_Hooker) — 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).Wrapper handlers are reported but not auto-fixed. The
$this->add_action( 'tag', 'method' )form is resolved with a name-match heuristic (the second argument names a method on the enclosing class, falling back to the hook name when it is absent or empty). That is safe enough to report on but not to auto-fix: in rare cases the matched method may not actually be a hook handler (e.g. a class that defines its own unrelatedadd_action()/add_filter()method alongside a coincidentally-named method), and stripping native types is destructive. So the sniff raises the violation without a fix — it still fails CI, butphpcbfleaves it alone. Remove the types by hand, or silence a false positive at the handler with// phpcs:ignore StellarWP.Hooks.HookHandlerTypes.NativeParameterType. Direct callbacks (closures,[ $this, 'method' ], global functions) remain auto-fixable.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,$container->callback( Class::class, 'method' )container callbacks, and$receiver->add_action( 'tag', 'method' )wrapper methods) 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 (the wrapper handlers appear as errors but are intentionally left unfixed in the baseline).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.