From ec4104ceaca71e1032115defb40af269d1f1e9d5 Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Thu, 23 Jul 2026 15:29:22 -0400 Subject: [PATCH 01/17] feat: add hook handler argument types checks - 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. --- .gitignore | 4 +- README.md | 75 ++ StellarWP/PHPStan/HookHandlerTypesRule.php | 425 +++++++++++ StellarWP/PHPStan/extension.neon | 22 + .../Sniffs/Hooks/HookHandlerTypesSniff.php | 713 ++++++++++++++++++ .../Tests/Hooks/HookHandlerTypesUnitTest.inc | 77 ++ .../Hooks/HookHandlerTypesUnitTest.inc.fixed | 76 ++ .../Tests/Hooks/HookHandlerTypesUnitTest.php | 45 ++ .../PHPStan/HookHandlerTypesRuleTest.php | 104 +++ StellarWP/Tests/PHPStan/data/handlers.php | 35 + .../Tests/PHPStan/data/hook-handler-types.php | 51 ++ composer.json | 31 +- phpunit-bootstrap.php | 37 + phpunit.xml.dist | 14 + 14 files changed, 1697 insertions(+), 12 deletions(-) create mode 100644 StellarWP/PHPStan/HookHandlerTypesRule.php create mode 100644 StellarWP/PHPStan/extension.neon create mode 100644 StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php create mode 100644 StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc create mode 100644 StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed create mode 100644 StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.php create mode 100644 StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php create mode 100644 StellarWP/Tests/PHPStan/data/handlers.php create mode 100644 StellarWP/Tests/PHPStan/data/hook-handler-types.php create mode 100644 phpunit-bootstrap.php create mode 100644 phpunit.xml.dist diff --git a/.gitignore b/.gitignore index 40b4eac..95c8906 100644 --- a/.gitignore +++ b/.gitignore @@ -3,4 +3,6 @@ vendor/ phpcs.xml .phpcs.xml composer.lock -.DS_Store \ No newline at end of file +.DS_Store +.phpunit.result.cache +.phpunit.cache/ \ No newline at end of file diff --git a/README.md b/README.md index db47114..0ba1ad3 100644 --- a/README.md +++ b/README.md @@ -75,3 +75,78 @@ You can follow [this guide](https://confluence.jetbrains.com/display/PhpStorm/PH */vendor/* ``` + +## Hook handler argument types + +WordPress passes hook arguments with no type guarantees and reassigns or discards +filter return values however it likes. Declaring a **native parameter type** or a +**native return type** on a handler attached to a hook you do not own (a WP core +hook, or one defined by another plugin or theme) can therefore cause a runtime +**fatal** when the value received - or the value the filter chain expects back - +does not match the declared type. The safe posture is to be type-less on those +handlers. + +This standard enforces that rule with two complementary tools. "First-party" is +determined solely by prefixes you configure, which should mirror your project's +`WordPress.NamingConventions.PrefixAllGlobals` value. Any hook whose name does +**not** start with a configured prefix is treated as WP core / third-party and its +handler must be type-less. + +| | PHPCS sniff (`StellarWP.Hooks.HookHandlerTypes`) | PHPStan rule | +|---|---|---| +| Runs in | phpcs (fast, in-editor) | phpstan (whole codebase, never diff-limited) | +| Covers | same-file handlers only (inline closures/arrows, `[ $this, 'method' ]` / `[ self::class, 'method' ]`, and `'function_name'`) with literal hook names | all callback forms across files (via reflection) and hook names that type inference narrows to constant string(s) | +| Auto-fix | yes (`phpcbf` strips the offending native types) | no (report-only; the message names the exact handler) | + +Run both: the sniff gives instant, auto-fixable feedback for the common +same-file case, while the PHPStan rule is the authoritative gate that catches +handlers whose declaration lives in a different file from the `add_filter()` / +`add_action()` call - the case a diff-limited phpcs run can miss. + +### PHPCS sniff + +The sniff is part of the `StellarWP` standard. Configure the `prefixes` property +(and, optionally, `allow_void_return_on_actions`, default `true`). With no +prefixes configured the sniff does nothing. + +```xml + + + + + + + + +``` + +### PHPStan rule + +The rule ships as an auto-discovered PHPStan extension. Projects using +[`phpstan/extension-installer`](https://github.com/phpstan/extension-installer) +get it registered automatically - no `includes:` entry needed. You must still set +the `prefixes` parameter in your `phpstan.neon` (an empty list makes the rule a +no-op): + +```neon +parameters: + stellarwpHookHandlerTypes: + prefixes: + - my_project + - mp_ + # allowVoidReturnOnActions: true # optional, default true +``` + +If you are not using `extension-installer`, include the extension manually: + +```neon +includes: + - vendor/stellarwp/coding-standards/StellarWP/PHPStan/extension.neon +``` + +> The `prefixes` value now lives in up to three places - `PrefixAllGlobals` in +> `phpcs.xml`, the sniff's `prefixes`, and the rule's `prefixes` in `phpstan.neon`. +> There is no shared source both tools can read, so keep them in sync. + +When adopting this in a project with existing violations, regenerate your PHPStan +baseline to grandfather them, then burn them down over time. diff --git a/StellarWP/PHPStan/HookHandlerTypesRule.php b/StellarWP/PHPStan/HookHandlerTypesRule.php new file mode 100644 index 0000000..1eeee7b --- /dev/null +++ b/StellarWP/PHPStan/HookHandlerTypesRule.php @@ -0,0 +1,425 @@ + + */ +class HookHandlerTypesRule implements Rule { + + /** + * @var ReflectionProvider + */ + private $reflection_provider; + + /** + * Prefixes that identify first-party hooks. Mirror the project's + * WordPress.NamingConventions.PrefixAllGlobals "prefixes" value. + * + * @var string[] + */ + private $prefixes; + + /** + * Whether a native `void` return type is acceptable on action handlers. + * + * @var bool + */ + private $allow_void_return_on_actions; + + /** + * @param ReflectionProvider $reflection_provider Provided by PHPStan. + * @param string[] $prefixes First-party hook prefixes. + * @param bool $allow_void_return_on_actions Allow `: void` on action handlers. + */ + public function __construct( ReflectionProvider $reflection_provider, array $prefixes, bool $allow_void_return_on_actions = true ) { + $this->reflection_provider = $reflection_provider; + $this->prefixes = $prefixes; + $this->allow_void_return_on_actions = $allow_void_return_on_actions; + } + + public function getNodeType(): string { + return FuncCall::class; + } + + /** + * @param FuncCall $node + * + * @return array + */ + public function processNode( Node $node, Scope $scope ): array { + if ( $this->prefixes === [] || ! $node->name instanceof Name ) { + return []; + } + + $function = strtolower( $node->name->toString() ); + if ( $function !== 'add_filter' && $function !== 'add_action' ) { + return []; + } + + $args = $node->getArgs(); + if ( count( $args ) < 2 ) { + return []; + } + + // Resolve the hook name(s) via type inference. This covers literals as + // well as any expression narrowing to constant string(s). + $constant_strings = $scope->getType( $args[0]->value )->getConstantStrings(); + if ( $constant_strings === [] ) { + return []; + } + + $foreign_hook = null; + foreach ( $constant_strings as $constant_string ) { + if ( ! $this->is_first_party( $constant_string->getValue() ) ) { + $foreign_hook = $constant_string->getValue(); + break; + } + } + + // Every possible hook name is first-party: nothing to enforce. + if ( $foreign_hook === null ) { + return []; + } + + return $this->check_callback( $args[1]->value, $scope, $foreign_hook, $function === 'add_filter' ); + } + + /** + * Dispatches to the appropriate resolver for the callback expression. + * + * @return array + */ + private function check_callback( Node $callback, Scope $scope, string $hook, bool $is_filter ): array { + if ( $callback instanceof Closure || $callback instanceof ArrowFunction ) { + return $this->check_closure_node( $callback, $hook, $is_filter ); + } + + if ( $callback instanceof Array_ ) { + return $this->check_array_callback( $callback, $scope, $hook, $is_filter ); + } + + if ( $callback instanceof String_ ) { + if ( strpos( $callback->value, '::' ) !== false ) { + list( $class, $method ) = explode( '::', $callback->value, 2 ); + + return $this->check_class_method( $class, $method, $hook, $is_filter, $callback->getStartLine() ); + } + + return $this->check_global_function( $callback->value, $scope, $hook, $is_filter, $callback->getStartLine() ); + } + + // Unresolvable callbacks (variables, first-class callables) are skipped. + return []; + } + + /** + * Checks an inline closure or arrow function using its declared native types. + * + * @param Closure|ArrowFunction $node + * + * @return array + */ + private function check_closure_node( Node $node, string $hook, bool $is_filter ): array { + $errors = []; + + foreach ( $node->params as $param ) { + if ( $param->type === null ) { + continue; + } + + $subject = $param->var instanceof Node\Expr\Variable && is_string( $param->var->name ) + ? 'parameter $' . $param->var->name + : 'a parameter'; + + $errors[] = RuleErrorBuilder::message( + sprintf( + 'Handler for non-first-party hook "%s" must not declare the native type "%s" on %s; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', + $hook, + $this->type_node_to_string( $param->type ), + $subject + ) + )->identifier( 'stellarwp.hookHandlerParamType' )->line( $param->getStartLine() )->build(); + } + + if ( $node->returnType !== null ) { + $return_type = $this->type_node_to_string( $node->returnType ); + + if ( ! $this->is_void_allowed( $is_filter, $return_type ) ) { + $errors[] = $this->return_type_error( $hook, $is_filter, $return_type, $node->returnType->getStartLine() ); + } + } + + return $errors; + } + + /** + * Resolves an array callback ([ $this, 'method' ], [ Foo::class, 'method' ], + * [ 'Foo', 'method' ]) to the handler class(es) and checks the method. + * + * @return array + */ + private function check_array_callback( Array_ $callback, Scope $scope, string $hook, bool $is_filter ): array { + if ( count( $callback->items ) < 2 || $callback->items[0] === null || $callback->items[1] === null ) { + return []; + } + + $method_value = $callback->items[1]->value; + if ( ! $method_value instanceof String_ ) { + return []; + } + + $method = $method_value->value; + $subject_type = $scope->getType( $callback->items[0]->value ); + + $class_names = []; + foreach ( $subject_type->getObjectClassReflections() as $class_reflection ) { + $class_names[ $class_reflection->getName() ] = true; + } + foreach ( $subject_type->getConstantStrings() as $constant_string ) { + $class_names[ $constant_string->getValue() ] = true; + } + + $errors = []; + foreach ( array_keys( $class_names ) as $class_name ) { + $errors = array_merge( + $errors, + $this->check_class_method( $class_name, $method, $hook, $is_filter, $callback->getStartLine() ) + ); + } + + return $errors; + } + + /** + * Checks a resolved class method's native parameter and return types. + * + * @return array + */ + private function check_class_method( string $class_name, string $method, string $hook, bool $is_filter, int $line ): array { + $class_name = ltrim( $class_name, '\\' ); + + if ( ! $this->reflection_provider->hasClass( $class_name ) ) { + return []; + } + + $native = $this->reflection_provider->getClass( $class_name )->getNativeReflection(); + if ( ! $native->hasMethod( $method ) ) { + return []; + } + + $reflection_method = $native->getMethod( $method ); + $declaring = $reflection_method->getDeclaringClass()->getName(); + $errors = []; + + foreach ( $reflection_method->getParameters() as $parameter ) { + if ( ! $parameter->hasType() ) { + continue; + } + + $errors[] = RuleErrorBuilder::message( + sprintf( + 'Handler %s::%s() for non-first-party hook "%s" must not declare the native type "%s" on parameter $%s; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', + $declaring, + $method, + $hook, + $this->reflection_type_to_string( $parameter->getType() ), + $parameter->getName() + ) + )->identifier( 'stellarwp.hookHandlerParamType' )->line( $line )->build(); + } + + if ( $reflection_method->hasReturnType() ) { + $return_type = $this->reflection_type_to_string( $reflection_method->getReturnType() ); + + if ( ! $this->is_void_allowed( $is_filter, $return_type ) ) { + $errors[] = $this->return_type_error( $hook, $is_filter, $return_type, $line, $declaring . '::' . $method . '()' ); + } + } + + return $errors; + } + + /** + * Checks a global-function-name callback ('my_function') using PHPStan's + * static reflection, which knows project functions without loading them. + * + * @return array + */ + private function check_global_function( string $name, Scope $scope, string $hook, bool $is_filter, int $line ): array { + $function_name = new Name( ltrim( $name, '\\' ) ); + + if ( ! $this->reflection_provider->hasFunction( $function_name, $scope ) ) { + return []; + } + + $variant = ParametersAcceptorSelector::selectSingle( + $this->reflection_provider->getFunction( $function_name, $scope )->getVariants() + ); + + if ( ! $variant instanceof ParametersAcceptorWithPhpDocs ) { + return []; + } + + $errors = []; + + foreach ( $variant->getParameters() as $parameter ) { + if ( ! $parameter instanceof ParameterReflectionWithPhpDocs || ! $this->has_native_type( $parameter->getNativeType() ) ) { + continue; + } + + $errors[] = RuleErrorBuilder::message( + sprintf( + 'Handler %s() for non-first-party hook "%s" must not declare the native type "%s" on parameter $%s; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', + $name, + $hook, + $parameter->getNativeType()->describe( VerbosityLevel::typeOnly() ), + $parameter->getName() + ) + )->identifier( 'stellarwp.hookHandlerParamType' )->line( $line )->build(); + } + + $native_return = $variant->getNativeReturnType(); + + if ( $this->has_native_type( $native_return ) ) { + $return_type = $native_return->describe( VerbosityLevel::typeOnly() ); + + if ( ! $this->is_void_allowed( $is_filter, $return_type ) ) { + $errors[] = $this->return_type_error( $hook, $is_filter, $return_type, $line, $name . '()' ); + } + } + + return $errors; + } + + /** + * Whether a native type is actually declared. An undeclared type is an + * implicit `mixed`; an explicit `mixed` counts as a declared native type, + * matching \ReflectionParameter::hasType() semantics used for methods. + */ + private function has_native_type( Type $type ): bool { + return ! ( $type instanceof MixedType && ! $type->isExplicitMixed() ); + } + + /** + * Builds a return-type violation error. + * + * @return \PHPStan\Rules\RuleError + */ + private function return_type_error( string $hook, bool $is_filter, string $return_type, int $line, string $handler = '' ) { + $where = $handler === '' ? '' : $handler . ' '; + + if ( $is_filter ) { + $message = sprintf( + 'Handler %sfor non-first-party filter "%s" must not declare a native return type ("%s"); filter return values are not type-guaranteed and a native return type can cause a fatal error.', + $where, + $hook, + $return_type + ); + } else { + $message = sprintf( + 'Handler %sfor non-first-party action "%s" must not declare a native return type ("%s") other than void.', + $where, + $hook, + $return_type + ); + } + + return RuleErrorBuilder::message( $message )->identifier( 'stellarwp.hookHandlerReturnType' )->line( $line )->build(); + } + + /** + * Whether a native `void` return type is acceptable in this context. + */ + private function is_void_allowed( bool $is_filter, string $return_type ): bool { + return ! $is_filter + && $this->allow_void_return_on_actions + && strtolower( ltrim( $return_type, '?\\' ) ) === 'void'; + } + + /** + * Whether a hook name belongs to this project by prefix. + */ + private function is_first_party( string $hook_name ): bool { + foreach ( $this->prefixes as $prefix ) { + if ( $prefix !== '' && stripos( $hook_name, $prefix ) === 0 ) { + return true; + } + } + + return false; + } + + /** + * Renders a PhpParser type node to a readable string (for messages and void + * detection). + * + * @param Node $type A parameter or return type node. + */ + private function type_node_to_string( Node $type ): string { + if ( $type instanceof Node\Identifier || $type instanceof Node\Name ) { + return $type->toString(); + } + + if ( $type instanceof Node\NullableType ) { + return '?' . $this->type_node_to_string( $type->type ); + } + + if ( $type instanceof Node\UnionType || $type instanceof Node\IntersectionType ) { + $separator = $type instanceof Node\UnionType ? '|' : '&'; + $parts = []; + + foreach ( $type->types as $inner ) { + $parts[] = $this->type_node_to_string( $inner ); + } + + return implode( $separator, $parts ); + } + + return 'mixed'; + } + + /** + * Renders a native ReflectionType to a readable string. + * + * @param \ReflectionType|null $type The reflection type. + */ + private function reflection_type_to_string( ?\ReflectionType $type ): string { + if ( $type instanceof \ReflectionNamedType ) { + return ( $type->allowsNull() && strtolower( $type->getName() ) !== 'null' ? '?' : '' ) . $type->getName(); + } + + return $type === null ? 'mixed' : (string) $type; + } +} diff --git a/StellarWP/PHPStan/extension.neon b/StellarWP/PHPStan/extension.neon new file mode 100644 index 0000000..1e58556 --- /dev/null +++ b/StellarWP/PHPStan/extension.neon @@ -0,0 +1,22 @@ +parameters: + # Prefixes that identify first-party hooks. Mirror the project's + # WordPress.NamingConventions.PrefixAllGlobals "prefixes" value. When empty, + # the rule does nothing (a safe default that also makes "unconfigured" obvious). + stellarwpHookHandlerTypes: + prefixes: [] + allowVoidReturnOnActions: true + +parametersSchema: + stellarwpHookHandlerTypes: structure([ + prefixes: listOf(string()) + allowVoidReturnOnActions: bool() + ]) + +services: + - + class: StellarWP\PHPStan\HookHandlerTypesRule + arguments: + prefixes: %stellarwpHookHandlerTypes.prefixes% + allow_void_return_on_actions: %stellarwpHookHandlerTypes.allowVoidReturnOnActions% + tags: + - phpstan.rules.rule diff --git a/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php b/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php new file mode 100644 index 0000000..4c9a140 --- /dev/null +++ b/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php @@ -0,0 +1,713 @@ + + */ + private const HOOK_FUNCTIONS = [ + 'add_filter' => true, + 'add_action' => false, + ]; + + /** + * Prefixes that identify first-party hooks. + * + * Mirror the project's WordPress.NamingConventions.PrefixAllGlobals + * "prefixes" value. When empty, the sniff does nothing. + * + * @var string[] + */ + public $prefixes = []; + + /** + * Whether a native `void` return type is acceptable on action handlers. + * + * Actions always return void, so `: void` is harmless on an action handler. + * Filters can never carry a native return type regardless of this setting. + * + * @var bool + */ + public $allow_void_return_on_actions = true; + + /** + * Whether to warn when the hook name cannot be resolved to a literal string. + * + * @var bool + */ + public $warn_on_dynamic_hook_names = false; + + /** + * Returns an array of tokens this test wants to listen for. + * + * @return array + */ + public function register(): array { + return [ T_STRING ]; + } + + /** + * Processes this test, when one of its tokens is encountered. + * + * @param File $phpcs_file The file being scanned. + * @param int $stack_ptr The position of the current token in the stack. + * + * @return void + * + * @phpcs:disable SlevomatCodingStandard.TypeHints.ParameterTypeHint.MissingNativeTypeHint + */ + public function process( File $phpcs_file, $stack_ptr ): void { + // Nothing to do until the project tells us which prefixes are first-party. + if ( empty( $this->prefixes ) ) { + return; + } + + $tokens = $phpcs_file->getTokens(); + $content = strtolower( $tokens[ $stack_ptr ]['content'] ); + + if ( ! isset( self::HOOK_FUNCTIONS[ $content ] ) ) { + return; + } + + // Ensure this is a direct function call, not a method call or a definition. + $prev = $phpcs_file->findPrevious( Tokens::$emptyTokens, $stack_ptr - 1, null, true ); + if ( $prev !== false ) { + $skip_before = [ + T_OBJECT_OPERATOR, + T_NULLSAFE_OBJECT_OPERATOR, + T_DOUBLE_COLON, + T_FUNCTION, + T_NEW, + ]; + + if ( in_array( $tokens[ $prev ]['code'], $skip_before, true ) ) { + return; + } + } + + $open_paren = $phpcs_file->findNext( Tokens::$emptyTokens, $stack_ptr + 1, null, true ); + if ( + $open_paren === false + || $tokens[ $open_paren ]['code'] !== T_OPEN_PARENTHESIS + || ! isset( $tokens[ $open_paren ]['parenthesis_closer'] ) + ) { + return; + } + + $close_paren = $tokens[ $open_paren ]['parenthesis_closer']; + + $args = $this->split_arguments( $phpcs_file, $open_paren, $close_paren ); + if ( count( $args ) < 2 ) { + return; + } + + // Argument 1: the hook name. + $hook_name = $this->get_string_argument( $phpcs_file, $args[0] ); + if ( $hook_name === null ) { + if ( $this->warn_on_dynamic_hook_names ) { + $phpcs_file->addWarning( + 'Unable to determine the hook name statically; the handler type restriction for non-first-party hooks could not be verified.', + $args[0]['start'], + 'DynamicHookName' + ); + } + + return; + } + + if ( $this->is_first_party( $hook_name ) ) { + return; + } + + // Argument 2: the callback. Resolve to a function/closure/method token. + $func_ptr = $this->resolve_handler( $phpcs_file, $args[1], $stack_ptr ); + if ( $func_ptr === null ) { + // Cross-file, global-function, or dynamic callback: left to the PHPStan rule. + return; + } + + $this->check_handler_types( $phpcs_file, $func_ptr, $hook_name, self::HOOK_FUNCTIONS[ $content ] ); + } + + /** + * Splits the contents of a parenthesis or bracket pair into top-level, + * comma-separated argument token ranges. + * + * @param File $phpcs_file The file being scanned. + * @param int $opener The opening parenthesis/bracket position. + * @param int $closer The matching closing position. + * + * @return array + */ + private function split_arguments( File $phpcs_file, int $opener, int $closer ): array { + $tokens = $phpcs_file->getTokens(); + $args = []; + + $open_codes = [ + T_OPEN_PARENTHESIS, + T_OPEN_SHORT_ARRAY, + T_OPEN_SQUARE_BRACKET, + T_OPEN_CURLY_BRACKET, + ]; + + $close_codes = [ + T_CLOSE_PARENTHESIS, + T_CLOSE_SHORT_ARRAY, + T_CLOSE_SQUARE_BRACKET, + T_CLOSE_CURLY_BRACKET, + ]; + + $start = $phpcs_file->findNext( Tokens::$emptyTokens, $opener + 1, $closer, true ); + if ( $start === false ) { + return $args; + } + + $depth = 0; + $current_start = $start; + + for ( $i = $start; $i < $closer; $i++ ) { + $code = $tokens[ $i ]['code']; + + if ( in_array( $code, $open_codes, true ) ) { + $depth++; + continue; + } + + if ( in_array( $code, $close_codes, true ) ) { + $depth--; + continue; + } + + if ( $depth === 0 && $code === T_COMMA ) { + $end = $phpcs_file->findPrevious( Tokens::$emptyTokens, $i - 1, $current_start, true ); + if ( $end !== false ) { + $args[] = [ + 'start' => $current_start, + 'end' => $end, + ]; + } + + $next = $phpcs_file->findNext( Tokens::$emptyTokens, $i + 1, $closer, true ); + if ( $next === false ) { + return $args; + } + + $current_start = $next; + } + } + + $end = $phpcs_file->findPrevious( Tokens::$emptyTokens, $closer - 1, $current_start, true ); + if ( $end !== false ) { + $args[] = [ + 'start' => $current_start, + 'end' => $end, + ]; + } + + return $args; + } + + /** + * Returns the literal string value of an argument, or null when the argument + * is not a single constant string (e.g. a variable or concatenation). + * + * @param File $phpcs_file The file being scanned. + * @param array{start: int, end: int} $arg The argument token range. + * + * @return string|null + */ + private function get_string_argument( File $phpcs_file, array $arg ): ?string { + $tokens = $phpcs_file->getTokens(); + + if ( $arg['start'] !== $arg['end'] ) { + return null; + } + + if ( $tokens[ $arg['start'] ]['code'] !== T_CONSTANT_ENCAPSED_STRING ) { + return null; + } + + return $this->strip_quotes( $tokens[ $arg['start'] ]['content'] ); + } + + /** + * Removes matching surrounding single or double quotes from a raw string token. + * + * @param string $raw The raw token content. + * + * @return string + */ + private function strip_quotes( string $raw ): string { + if ( strlen( $raw ) >= 2 ) { + $first = $raw[0]; + $last = $raw[ strlen( $raw ) - 1 ]; + + if ( ( $first === "'" || $first === '"' ) && $first === $last ) { + return substr( $raw, 1, -1 ); + } + } + + return $raw; + } + + /** + * Determines whether a hook name belongs to this project by prefix. + * + * @param string $hook_name The resolved hook name. + * + * @return bool + */ + private function is_first_party( string $hook_name ): bool { + foreach ( $this->prefixes as $prefix ) { + $prefix = (string) $prefix; + + if ( $prefix === '' ) { + continue; + } + + if ( stripos( $hook_name, $prefix ) === 0 ) { + return true; + } + } + + return false; + } + + /** + * Resolves a callback argument to the token of the function, closure, or + * arrow function whose declared types should be checked. Returns null when + * the callback cannot be resolved within this file. + * + * @param File $phpcs_file The file being scanned. + * @param array{start: int, end: int} $arg The callback argument range. + * @param int $stack_ptr The hook-call token position. + * + * @return int|null + */ + private function resolve_handler( File $phpcs_file, array $arg, int $stack_ptr ): ?int { + $tokens = $phpcs_file->getTokens(); + $ptr = $arg['start']; + + // Allow a leading `static` for static closures/arrow functions. + if ( $tokens[ $ptr ]['code'] === T_STATIC ) { + $ptr = $phpcs_file->findNext( Tokens::$emptyTokens, $ptr + 1, $arg['end'] + 1, true ); + if ( $ptr === false ) { + return null; + } + } + + $code = $tokens[ $ptr ]['code']; + + if ( $code === T_CLOSURE || $code === T_FN ) { + return $ptr; + } + + if ( $code === T_OPEN_SHORT_ARRAY || $code === T_ARRAY ) { + return $this->resolve_array_callback( $phpcs_file, $ptr, $stack_ptr ); + } + + // A plain 'function_name' string: resolve a same-file global function. + // Namespaced names and 'Class::method' strings are left to the PHPStan rule. + if ( $code === T_CONSTANT_ENCAPSED_STRING && $ptr === $arg['end'] ) { + $value = $this->strip_quotes( $tokens[ $ptr ]['content'] ); + + if ( $value === '' || strpos( $value, '::' ) !== false || strpos( $value, '\\' ) !== false ) { + return null; + } + + return $this->find_global_function( $phpcs_file, $value ); + } + + return null; + } + + /** + * Finds a global (file-scope) function declaration by name. Returns the + * T_FUNCTION token position, or null when not found in this file. + * + * @param File $phpcs_file The file being scanned. + * @param string $name The function name to find. + * + * @return int|null + */ + private function find_global_function( File $phpcs_file, string $name ): ?int { + $tokens = $phpcs_file->getTokens(); + $name_lc = strtolower( $name ); + $ptr = 0; + + while ( ( $ptr = $phpcs_file->findNext( T_FUNCTION, $ptr + 1 ) ) !== false ) { + // Only file-scope functions - skip methods and nested functions. + if ( ! empty( $tokens[ $ptr ]['conditions'] ) ) { + continue; + } + + $name_ptr = $phpcs_file->findNext( Tokens::$emptyTokens, $ptr + 1, null, true ); + if ( $name_ptr !== false && $tokens[ $name_ptr ]['code'] === T_BITWISE_AND ) { + $name_ptr = $phpcs_file->findNext( Tokens::$emptyTokens, $name_ptr + 1, null, true ); + } + + if ( + $name_ptr !== false + && $tokens[ $name_ptr ]['code'] === T_STRING + && strtolower( $tokens[ $name_ptr ]['content'] ) === $name_lc + ) { + return $ptr; + } + } + + return null; + } + + /** + * Resolves an array callback ([ $this, 'method' ] or array( self::class, + * 'method' )) to a same-file method declaration token, or null. + * + * @param File $phpcs_file The file being scanned. + * @param int $array_ptr The array opener token position. + * @param int $stack_ptr The hook-call token position. + * + * @return int|null + */ + private function resolve_array_callback( File $phpcs_file, int $array_ptr, int $stack_ptr ): ?int { + $tokens = $phpcs_file->getTokens(); + + if ( $tokens[ $array_ptr ]['code'] === T_OPEN_SHORT_ARRAY ) { + $opener = $array_ptr; + $closer = $tokens[ $array_ptr ]['bracket_closer']; + } else { + $opener = $phpcs_file->findNext( T_OPEN_PARENTHESIS, $array_ptr + 1 ); + if ( $opener === false || ! isset( $tokens[ $opener ]['parenthesis_closer'] ) ) { + return null; + } + + $closer = $tokens[ $opener ]['parenthesis_closer']; + } + + $elements = $this->split_arguments( $phpcs_file, $opener, $closer ); + if ( count( $elements ) < 2 ) { + return null; + } + + if ( ! $this->is_current_class_reference( $phpcs_file, $elements[0] ) ) { + return null; + } + + $method = $this->get_string_argument( $phpcs_file, $elements[1] ); + if ( $method === null ) { + return null; + } + + return $this->find_class_method( $phpcs_file, $stack_ptr, $method ); + } + + /** + * Determines whether a callback array's first element refers to the current + * class instance or name ($this, self::class, static::class, __CLASS__). + * + * @param File $phpcs_file The file being scanned. + * @param array{start: int, end: int} $element The element token range. + * + * @return bool + */ + private function is_current_class_reference( File $phpcs_file, array $element ): bool { + $tokens = $phpcs_file->getTokens(); + $first = $element['start']; + $code = $tokens[ $first ]['code']; + + if ( $code === T_VARIABLE ) { + return $element['start'] === $element['end'] + && strtolower( $tokens[ $first ]['content'] ) === '$this'; + } + + if ( $code === T_CLASS_C ) { + return $element['start'] === $element['end']; + } + + // self::class / static::class. PHPCS tokenizes these as T_SELF / T_STATIC; + // the T_STRING fallback covers tokenizer/version differences. + $name = strtolower( $tokens[ $first ]['content'] ); + if ( + $code === T_SELF + || $code === T_STATIC + || ( $code === T_STRING && ( $name === 'self' || $name === 'static' ) ) + ) { + $next = $phpcs_file->findNext( Tokens::$emptyTokens, $first + 1, $element['end'] + 1, true ); + + return $next !== false && $tokens[ $next ]['code'] === T_DOUBLE_COLON; + } + + return false; + } + + /** + * Finds a method declaration by name within the class/trait enclosing the + * hook call. Returns the T_FUNCTION token position, or null when not found. + * + * @param File $phpcs_file The file being scanned. + * @param int $stack_ptr The hook-call token position. + * @param string $method The method name to find. + * + * @return int|null + */ + private function find_class_method( File $phpcs_file, int $stack_ptr, string $method ): ?int { + $tokens = $phpcs_file->getTokens(); + + if ( empty( $tokens[ $stack_ptr ]['conditions'] ) ) { + return null; + } + + $class_scopes = [ T_CLASS, T_TRAIT, T_ANON_CLASS ]; + $class_ptr = null; + + foreach ( array_reverse( $tokens[ $stack_ptr ]['conditions'], true ) as $ptr => $code ) { + if ( in_array( $code, $class_scopes, true ) ) { + $class_ptr = $ptr; + break; + } + } + + if ( + $class_ptr === null + || ! isset( $tokens[ $class_ptr ]['scope_opener'], $tokens[ $class_ptr ]['scope_closer'] ) + ) { + return null; + } + + $start = $tokens[ $class_ptr ]['scope_opener']; + $end = $tokens[ $class_ptr ]['scope_closer']; + $method_lc = strtolower( $method ); + + for ( $i = $start + 1; $i < $end; $i++ ) { + if ( $tokens[ $i ]['code'] !== T_FUNCTION ) { + continue; + } + + // Only direct methods of this class - skip functions nested in methods. + $conditions = $tokens[ $i ]['conditions']; + if ( empty( $conditions ) ) { + continue; + } + + end( $conditions ); + if ( key( $conditions ) !== $class_ptr ) { + continue; + } + + $name_ptr = $phpcs_file->findNext( Tokens::$emptyTokens, $i + 1, null, true ); + if ( $name_ptr !== false && $tokens[ $name_ptr ]['code'] === T_BITWISE_AND ) { + $name_ptr = $phpcs_file->findNext( Tokens::$emptyTokens, $name_ptr + 1, null, true ); + } + + if ( + $name_ptr !== false + && $tokens[ $name_ptr ]['code'] === T_STRING + && strtolower( $tokens[ $name_ptr ]['content'] ) === $method_lc + ) { + return $i; + } + } + + return null; + } + + /** + * Checks a resolved handler for disallowed native parameter and return types + * and fixes them by stripping only the offending type declarations. + * + * @param File $phpcs_file The file being scanned. + * @param int $func_ptr The function/closure/arrow token position. + * @param string $hook_name The resolved hook name (for messaging). + * @param bool $is_filter Whether the hook is a filter. + * + * @return void + */ + private function check_handler_types( File $phpcs_file, int $func_ptr, string $hook_name, bool $is_filter ): void { + $params = $phpcs_file->getMethodParameters( $func_ptr ); + + foreach ( $params as $param ) { + if ( empty( $param['type_hint'] ) ) { + continue; + } + + $fix = $phpcs_file->addFixableError( + 'Handler for non-first-party hook "%s" must not declare the native type "%s" on parameter %s; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', + $param['type_hint_token'], + 'NativeParameterType', + [ $hook_name, $param['type_hint'], $param['name'] ] + ); + + if ( $fix === true ) { + $this->remove_parameter_type( $phpcs_file, $param ); + } + } + + $props = $phpcs_file->getMethodProperties( $func_ptr ); + if ( empty( $props['return_type'] ) ) { + return; + } + + $return_type = $props['return_type']; + $return_type_lc = strtolower( ltrim( $return_type, '?\\' ) ); + + if ( ! $is_filter && $this->allow_void_return_on_actions && $return_type_lc === 'void' ) { + return; + } + + if ( $is_filter ) { + $message = 'Handler for non-first-party filter "%s" must not declare a native return type ("%s"); filter return values are not type-guaranteed and a native return type can cause a fatal error.'; + } else { + $message = 'Handler for non-first-party action "%s" must not declare a native return type ("%s") other than void.'; + } + + $fix = $phpcs_file->addFixableError( + $message, + $props['return_type_token'], + 'NativeReturnType', + [ $hook_name, $return_type ] + ); + + if ( $fix === true ) { + $this->remove_return_type( $phpcs_file, $func_ptr, $props ); + } + } + + /** + * Strips a parameter's native type declaration, leaving the variable (and + * any reference/variadic markers, attributes, default, and line breaks) + * untouched. + * + * @param File $phpcs_file The file being scanned. + * @param array $param A single entry from File::getMethodParameters(). + * + * @return void + */ + private function remove_parameter_type( File $phpcs_file, array $param ): void { + $tokens = $phpcs_file->getTokens(); + $fixer = $phpcs_file->fixer; + + $start = $param['type_hint_token']; + $end = $param['type_hint_end_token']; + + // Include a leading nullable `?` when it sits just before the type. + if ( ! empty( $param['nullable_type'] ) ) { + $before = $phpcs_file->findPrevious( Tokens::$emptyTokens, $start - 1, null, true ); + if ( $before !== false && $tokens[ $before ]['code'] === T_NULLABLE ) { + $start = $before; + } + } + + $fixer->beginChangeset(); + + for ( $i = $start; $i <= $end; $i++ ) { + $fixer->replaceToken( $i, '' ); + } + + // Remove exactly one following whitespace token (the space before the + // variable/&/...), but only when it does not span a line break. + $after = $end + 1; + if ( + isset( $tokens[ $after ] ) + && $tokens[ $after ]['code'] === T_WHITESPACE + && strpos( $tokens[ $after ]['content'], "\n" ) === false + ) { + $fixer->replaceToken( $after, '' ); + } + + $fixer->endChangeset(); + } + + /** + * Strips a native return type declaration (the colon through the type), + * leaving whatever whitespace joined the type to the body untouched so the + * brace/arrow position is preserved. + * + * @param File $phpcs_file The file being scanned. + * @param int $func_ptr The function/closure/arrow token position. + * @param array $props The result of File::getMethodProperties(). + * + * @return void + */ + private function remove_return_type( File $phpcs_file, int $func_ptr, array $props ): void { + $tokens = $phpcs_file->getTokens(); + $fixer = $phpcs_file->fixer; + + $end = $props['return_type_end_token']; + + // The return-type colon lives between the parameter list close-paren and + // the type. Bound the search at the close-paren to avoid stray colons. + $boundary = $func_ptr; + if ( + isset( $tokens[ $func_ptr ]['parenthesis_opener'] ) + && isset( $tokens[ $tokens[ $func_ptr ]['parenthesis_opener'] ]['parenthesis_closer'] ) + ) { + $boundary = $tokens[ $tokens[ $func_ptr ]['parenthesis_opener'] ]['parenthesis_closer']; + } + + $colon = $phpcs_file->findPrevious( T_COLON, $props['return_type_token'] - 1, $boundary ); + $start = ( $colon !== false ) ? $colon : $props['return_type_token']; + + // When the return type begins on a line of its own (a line break sits + // between the parameter list close-paren and the colon), also remove that + // leading break and indentation so no blank or indent-only line is left + // behind. Inline return types have no such whitespace and are untouched. + $ws_start = $start; + $saw_newline = false; + for ( $p = $start - 1; isset( $tokens[ $p ] ) && $tokens[ $p ]['code'] === T_WHITESPACE; $p-- ) { + if ( strpos( $tokens[ $p ]['content'], "\n" ) !== false ) { + $saw_newline = true; + } + + $ws_start = $p; + } + + if ( $saw_newline ) { + $start = $ws_start; + } + + $fixer->beginChangeset(); + + for ( $i = $start; $i <= $end; $i++ ) { + $fixer->replaceToken( $i, '' ); + } + + $fixer->endChangeset(); + } +} diff --git a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc new file mode 100644 index 0000000..40573af --- /dev/null +++ b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc @@ -0,0 +1,77 @@ +post_title; + } ); + } + + public function filter_content( string $content ): string { + return $content; + } + + public function typed_action( int $id ): void {} + + public function on_save( int $post_id, WP_Post $post ): void {} + + public static function static_filter( string $title ): string { + return $title; + } +} + +function global_excerpt_handler( string $excerpt ): string { + return $excerpt; +} + +function typeless_excerpt_handler( $excerpt ) { + return $excerpt; +} diff --git a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed new file mode 100644 index 0000000..f16e796 --- /dev/null +++ b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed @@ -0,0 +1,76 @@ +post_title; + } ); + } + + public function filter_content( $content ) { + return $content; + } + + public function typed_action( int $id ): void {} + + public function on_save( $post_id, $post ): void {} + + public static function static_filter( $title ) { + return $title; + } +} + +function global_excerpt_handler( $excerpt ) { + return $excerpt; +} + +function typeless_excerpt_handler( $excerpt ) { + return $excerpt; +} diff --git a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.php b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.php new file mode 100644 index 0000000..4ab2f30 --- /dev/null +++ b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.php @@ -0,0 +1,45 @@ + + */ + protected function getErrorList() { + return [ + 12 => 1, // Closure on 'init': native param type (void return is allowed). + 15 => 2, // Closure on 'body_class': native param type + native return type. + 48 => 3, // Closure on 'pre_get_posts': nullable, by-reference, variadic params. + 51 => 1, // Closure on 'the_title': native param type. + 52 => 1, // Closure on 'the_title': native return type (on its own line). + 58 => 2, // filter_content(): param + return (via the 'the_content' binding). + 64 => 2, // on_save(): two native param types (via the 'save_post' binding). + 66 => 2, // static_filter(): param + return (via the 'wp_title' self::class binding). + 71 => 2, // global_excerpt_handler(): param + return (via the same-file 'excerpt_length' binding). + ]; + } + + /** + * Returns the lines where warnings should occur, keyed by line number. + * + * @return array + */ + protected function getWarningList() { + return []; + } +} diff --git a/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php new file mode 100644 index 0000000..7563c7f --- /dev/null +++ b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php @@ -0,0 +1,104 @@ + + */ +class HookHandlerTypesRuleTest extends RuleTestCase { + + /** + * Loads the cross-file handler classes so the rule can reflect them, exactly + * as they would be autoloadable in a real analysed project. + */ + public static function setUpBeforeClass(): void { + parent::setUpBeforeClass(); + + require_once __DIR__ . '/data/handlers.php'; + } + + protected function getRule(): Rule { + return new HookHandlerTypesRule( + $this->createReflectionProvider(), + [ 'learndash', 'ld_', 'sfwd' ], + true + ); + } + + public function testRule(): void { + // This skip is a limitation of the RuleTestCase harness only: PHPStan's + // bundled php-parser hits a token-emulation bug when parsing fixtures on + // the PHP 7.4 runtime. The rule itself is PHP 7.4-compatible and runs + // correctly under a normal `phpstan analyse` on 7.4. + if ( PHP_VERSION_ID < 80000 ) { + $this->markTestSkipped( 'Skipped on the PHP 7.4 runtime: PHPStan\'s RuleTestCase php-parser emulation is unreliable here. The rule works on 7.4 under a normal phpstan analyse.' ); + } + + $this->analyse( + [ __DIR__ . '/data/hook-handler-types.php' ], + [ + [ + 'Handler Hook_Test_Handlers::filter_content() for non-first-party hook "the_content" must not declare the native type "string" on parameter $content; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', + 13, + ], + [ + 'Handler Hook_Test_Handlers::filter_content() for non-first-party filter "the_content" must not declare a native return type ("string"); filter return values are not type-guaranteed and a native return type can cause a fatal error.', + 13, + ], + [ + 'Handler Hook_Test_Handlers::on_save() for non-first-party hook "save_post" must not declare the native type "int" on parameter $post_id; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', + 16, + ], + [ + 'Handler Hook_Test_Handlers::on_save() for non-first-party hook "save_post" must not declare the native type "WP_Post" on parameter $post; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', + 16, + ], + [ + 'Handler Hook_Test_Handlers::static_filter() for non-first-party hook "wp_title" must not declare the native type "string" on parameter $title; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', + 19, + ], + [ + 'Handler Hook_Test_Handlers::static_filter() for non-first-party filter "wp_title" must not declare a native return type ("string"); filter return values are not type-guaranteed and a native return type can cause a fatal error.', + 19, + ], + [ + 'Handler for non-first-party hook "init" must not declare the native type "int" on parameter $x; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', + 28, + ], + [ + 'Handler for non-first-party hook "body_class" must not declare the native type "array" on parameter $classes; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', + 31, + ], + [ + 'Handler for non-first-party filter "body_class" must not declare a native return type ("array"); filter return values are not type-guaranteed and a native return type can cause a fatal error.', + 31, + ], + [ + 'Handler Hook_Test_Handlers::filter_content() for non-first-party hook "the_content" must not declare the native type "string" on parameter $content; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', + 38, + ], + [ + 'Handler Hook_Test_Handlers::filter_content() for non-first-party filter "the_content" must not declare a native return type ("string"); filter return values are not type-guaranteed and a native return type can cause a fatal error.', + 38, + ], + [ + 'Handler typed_global_handler() for non-first-party hook "excerpt_length" must not declare the native type "string" on parameter $value; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', + 42, + ], + [ + 'Handler typed_global_handler() for non-first-party filter "excerpt_length" must not declare a native return type ("string"); filter return values are not type-guaranteed and a native return type can cause a fatal error.', + 42, + ], + ] + ); + } +} diff --git a/StellarWP/Tests/PHPStan/data/handlers.php b/StellarWP/Tests/PHPStan/data/handlers.php new file mode 100644 index 0000000..3ec013a --- /dev/null +++ b/StellarWP/Tests/PHPStan/data/handlers.php @@ -0,0 +1,35 @@ +getFilename(), -12 ) !== 'UnitTest.php' ) { + continue; + } + + $relative = substr( $file->getPathname(), strlen( $tests_dir ), -4 ); + $class = 'StellarWP\\Tests\\' . str_replace( DIRECTORY_SEPARATOR, '\\', $relative ); + + $GLOBALS['PHP_CODESNIFFER_STANDARD_DIRS'][ $class ] = $standard_dir; + $GLOBALS['PHP_CODESNIFFER_TEST_DIRS'][ $class ] = $tests_dir; +} diff --git a/phpunit.xml.dist b/phpunit.xml.dist new file mode 100644 index 0000000..0f84a5a --- /dev/null +++ b/phpunit.xml.dist @@ -0,0 +1,14 @@ + + + + + StellarWP/Tests + + + From b9f414a18ab170b9f51700b994c21c52f0ec5eac Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Thu, 23 Jul 2026 20:29:50 -0400 Subject: [PATCH 02/17] fix: enforce type-less filter handlers; resolve &$this callbacks - 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 --- README.md | 15 ++- StellarWP/PHPStan/HookHandlerTypesRule.php | 126 +++++++++++------- .../Sniffs/Hooks/HookHandlerTypesSniff.php | 52 ++++++-- .../Tests/Hooks/HookHandlerTypesUnitTest.inc | 87 ++++++------ .../Hooks/HookHandlerTypesUnitTest.inc.fixed | 86 ++++++------ .../Tests/Hooks/HookHandlerTypesUnitTest.php | 18 +-- .../PHPStan/HookHandlerTypesRuleTest.php | 89 +++++-------- StellarWP/Tests/PHPStan/data/handlers.php | 22 +-- .../Tests/PHPStan/data/hook-handler-types.php | 48 +++---- 9 files changed, 296 insertions(+), 247 deletions(-) diff --git a/README.md b/README.md index 0ba1ad3..95f2f30 100644 --- a/README.md +++ b/README.md @@ -88,9 +88,18 @@ handlers. This standard enforces that rule with two complementary tools. "First-party" is determined solely by prefixes you configure, which should mirror your project's -`WordPress.NamingConventions.PrefixAllGlobals` value. Any hook whose name does -**not** start with a configured prefix is treated as WP core / third-party and its -handler must be type-less. +`WordPress.NamingConventions.PrefixAllGlobals` value. Enforcement follows the hook +type: + +- **Filters (any prefix, first-party or not):** the handler must be fully + type-less - no native type on **any** parameter and no native return type. A + filter can be dispatched with arguments of unexpected types even by first-party + code (LearnDash core calls `apply_filters( 'sfwd_lms_has_access', true, 28, null )`, + so a native `int $user_id` throws a `TypeError`), and its return value flows + through code you do not control. +- **Non-first-party actions** (WP core / third-party): no native parameter types; + a `void` return type is allowed. +- **First-party actions:** unrestricted. | | PHPCS sniff (`StellarWP.Hooks.HookHandlerTypes`) | PHPStan rule | |---|---|---| diff --git a/StellarWP/PHPStan/HookHandlerTypesRule.php b/StellarWP/PHPStan/HookHandlerTypesRule.php index 1eeee7b..27e1d1a 100644 --- a/StellarWP/PHPStan/HookHandlerTypesRule.php +++ b/StellarWP/PHPStan/HookHandlerTypesRule.php @@ -1,14 +1,25 @@ getValue(); + $all_first_party = true; foreach ( $constant_strings as $constant_string ) { if ( ! $this->is_first_party( $constant_string->getValue() ) ) { - $foreign_hook = $constant_string->getValue(); + $hook = $constant_string->getValue(); + $all_first_party = false; break; } } - // Every possible hook name is first-party: nothing to enforce. - if ( $foreign_hook === null ) { + // First-party actions are unrestricted. Everything else is checked: any + // filter (a first-party filter still cannot type its arguments or return - + // the value is shaped by other code, and even core dispatches unexpected + // types), and non-first-party actions. + if ( $all_first_party && ! $is_filter ) { return []; } - return $this->check_callback( $args[1]->value, $scope, $foreign_hook, $function === 'add_filter' ); + return $this->check_callback( $args[1]->value, $scope, $hook, $is_filter ); } /** @@ -159,18 +177,9 @@ private function check_closure_node( Node $node, string $hook, bool $is_filter ) continue; } - $subject = $param->var instanceof Node\Expr\Variable && is_string( $param->var->name ) - ? 'parameter $' . $param->var->name - : 'a parameter'; - - $errors[] = RuleErrorBuilder::message( - sprintf( - 'Handler for non-first-party hook "%s" must not declare the native type "%s" on %s; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', - $hook, - $this->type_node_to_string( $param->type ), - $subject - ) - )->identifier( 'stellarwp.hookHandlerParamType' )->line( $param->getStartLine() )->build(); + $name = $param->var instanceof Node\Expr\Variable && is_string( $param->var->name ) ? $param->var->name : ''; + + $errors[] = $this->param_error( $is_filter, '', $hook, $this->type_node_to_string( $param->type ), $name, $param->getStartLine() ); } if ( $node->returnType !== null ) { @@ -248,16 +257,14 @@ private function check_class_method( string $class_name, string $method, string continue; } - $errors[] = RuleErrorBuilder::message( - sprintf( - 'Handler %s::%s() for non-first-party hook "%s" must not declare the native type "%s" on parameter $%s; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', - $declaring, - $method, - $hook, - $this->reflection_type_to_string( $parameter->getType() ), - $parameter->getName() - ) - )->identifier( 'stellarwp.hookHandlerParamType' )->line( $line )->build(); + $errors[] = $this->param_error( + $is_filter, + $declaring . '::' . $method . '()', + $hook, + $this->reflection_type_to_string( $parameter->getType() ), + $parameter->getName(), + $line + ); } if ( $reflection_method->hasReturnType() ) { @@ -299,15 +306,14 @@ private function check_global_function( string $name, Scope $scope, string $hook continue; } - $errors[] = RuleErrorBuilder::message( - sprintf( - 'Handler %s() for non-first-party hook "%s" must not declare the native type "%s" on parameter $%s; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', - $name, - $hook, - $parameter->getNativeType()->describe( VerbosityLevel::typeOnly() ), - $parameter->getName() - ) - )->identifier( 'stellarwp.hookHandlerParamType' )->line( $line )->build(); + $errors[] = $this->param_error( + $is_filter, + $name . '()', + $hook, + $parameter->getNativeType()->describe( VerbosityLevel::typeOnly() ), + $parameter->getName(), + $line + ); } $native_return = $variant->getNativeReturnType(); @@ -332,6 +338,36 @@ private function has_native_type( Type $type ): bool { return ! ( $type instanceof MixedType && ! $type->isExplicitMixed() ); } + /** + * Builds a parameter-type violation error. + * + * @return \PHPStan\Rules\RuleError + */ + private function param_error( bool $is_filter, string $handler, string $hook, string $type, string $param_name, int $line ) { + $where = $handler === '' ? '' : $handler . ' '; + $subject = $param_name === '' ? 'a parameter' : 'parameter $' . $param_name; + + if ( $is_filter ) { + $message = sprintf( + 'Handler %sfor filter "%s" must not declare the native type "%s" on %s; a filter can be dispatched with arguments of unexpected types (including null), so a native type can cause a fatal error.', + $where, + $hook, + $type, + $subject + ); + } else { + $message = sprintf( + 'Handler %sfor non-first-party action "%s" must not declare the native type "%s" on %s; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', + $where, + $hook, + $type, + $subject + ); + } + + return RuleErrorBuilder::message( $message )->identifier( 'stellarwp.hookHandlerParamType' )->line( $line )->build(); + } + /** * Builds a return-type violation error. * @@ -342,7 +378,7 @@ private function return_type_error( string $hook, bool $is_filter, string $retur if ( $is_filter ) { $message = sprintf( - 'Handler %sfor non-first-party filter "%s" must not declare a native return type ("%s"); filter return values are not type-guaranteed and a native return type can cause a fatal error.', + 'Handler %sfor filter "%s" must not declare a native return type ("%s"); filter return values are not type-guaranteed and a native return type can cause a fatal error.', $where, $hook, $return_type diff --git a/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php b/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php index 4c9a140..806937a 100644 --- a/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php +++ b/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php @@ -12,9 +12,18 @@ * handlers. * * "First-party" is determined solely by the configured prefixes, which should - * mirror the project's WordPress.NamingConventions.PrefixAllGlobals value. Any - * hook whose name does not start with a configured prefix is treated as WP core - * or third-party and its handler must be type-less. + * mirror the project's WordPress.NamingConventions.PrefixAllGlobals value. The + * enforcement follows the hook type: + * + * - Filters (any prefix, first-party or not): the handler must be fully + * type-less - no native type on ANY parameter and no native return type. A + * filter can be dispatched with arguments of unexpected types even by + * first-party code (e.g. LearnDash core calls apply_filters( 'sfwd_lms_has_access', + * true, 28, null ), so a native `int $user_id` fatals), and its return value + * flows through code we do not control. + * - Non-first-party actions (WP core / third-party): no native parameter types; + * a void return type is allowed. + * - First-party actions: unrestricted. * * This sniff analyses at the call site (add_action()/add_filter()) and can only * resolve handlers that live in the same file: inline closures and arrow @@ -153,7 +162,13 @@ public function process( File $phpcs_file, $stack_ptr ): void { return; } - if ( $this->is_first_party( $hook_name ) ) { + $is_filter = self::HOOK_FUNCTIONS[ $content ]; + $first_party = $this->is_first_party( $hook_name ); + + // First-party actions may be typed freely. First-party filters still must + // have a type-less first argument and no native return type: a filter's + // value is shaped by other code (plugins/themes) we do not control. + if ( $first_party && ! $is_filter ) { return; } @@ -164,7 +179,7 @@ public function process( File $phpcs_file, $stack_ptr ): void { return; } - $this->check_handler_types( $phpcs_file, $func_ptr, $hook_name, self::HOOK_FUNCTIONS[ $content ] ); + $this->check_handler_types( $phpcs_file, $func_ptr, $hook_name, $is_filter ); } /** @@ -450,15 +465,24 @@ private function resolve_array_callback( File $phpcs_file, int $array_ptr, int $ private function is_current_class_reference( File $phpcs_file, array $element ): bool { $tokens = $phpcs_file->getTokens(); $first = $element['start']; - $code = $tokens[ $first ]['code']; + + // Skip a leading reference operator, e.g. the old `[ &$this, 'method' ]` idiom. + if ( $tokens[ $first ]['code'] === T_BITWISE_AND ) { + $first = $phpcs_file->findNext( Tokens::$emptyTokens, $first + 1, $element['end'] + 1, true ); + if ( $first === false ) { + return false; + } + } + + $code = $tokens[ $first ]['code']; if ( $code === T_VARIABLE ) { - return $element['start'] === $element['end'] + return $first === $element['end'] && strtolower( $tokens[ $first ]['content'] ) === '$this'; } if ( $code === T_CLASS_C ) { - return $element['start'] === $element['end']; + return $first === $element['end']; } // self::class / static::class. PHPCS tokenizes these as T_SELF / T_STATIC; @@ -567,8 +591,14 @@ private function check_handler_types( File $phpcs_file, int $func_ptr, string $h continue; } + if ( $is_filter ) { + $message = 'Handler for filter "%s" must not declare the native type "%s" on parameter %s; a filter can be dispatched with arguments of unexpected types (including null), so a native type can cause a fatal error.'; + } else { + $message = 'Handler for non-first-party action "%s" must not declare the native type "%s" on parameter %s; WordPress does not guarantee hook argument types and a native type can cause a fatal error.'; + } + $fix = $phpcs_file->addFixableError( - 'Handler for non-first-party hook "%s" must not declare the native type "%s" on parameter %s; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', + $message, $param['type_hint_token'], 'NativeParameterType', [ $hook_name, $param['type_hint'], $param['name'] ] @@ -587,12 +617,14 @@ private function check_handler_types( File $phpcs_file, int $func_ptr, string $h $return_type = $props['return_type']; $return_type_lc = strtolower( ltrim( $return_type, '?\\' ) ); + // Actions always return void, so a void return type is acceptable. if ( ! $is_filter && $this->allow_void_return_on_actions && $return_type_lc === 'void' ) { return; } if ( $is_filter ) { - $message = 'Handler for non-first-party filter "%s" must not declare a native return type ("%s"); filter return values are not type-guaranteed and a native return type can cause a fatal error.'; + // Filters can never carry a native return type, first-party or not. + $message = 'Handler for filter "%s" must not declare a native return type ("%s"); filter return values are not type-guaranteed and a native return type can cause a fatal error.'; } else { $message = 'Handler for non-first-party action "%s" must not declare a native return type ("%s") other than void.'; } diff --git a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc index 40573af..d081e5c 100644 --- a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc +++ b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc @@ -5,73 +5,66 @@ class Example { public function register() { - // Non-first-party filter, same-file method handler (param + return type). - add_filter( 'the_content', [ $this, 'filter_content' ] ); + // Non-first-party filter: all params + return must be type-less. + add_filter( 'the_content', [ $this, 'np_filter' ] ); - // Non-first-party action, closure with param type and void return (void ok). - add_action( 'init', function ( int $x ): void {} ); - - // Non-first-party filter, closure with a return type (not allowed). - add_filter( 'body_class', function ( array $classes ): array { - return $classes; - } ); - - // First-party filter (matches prefix) - no error. - add_filter( 'learndash_something', [ $this, 'filter_content' ] ); + // Non-first-party action: params must be type-less; void return is allowed. + add_action( 'save_post', [ $this, 'np_action' ] ); - // First-party action - no error even with types. - add_action( 'ld_after_save', [ $this, 'typed_action' ] ); + // First-party filter: all params + return must be type-less, including the + // context arguments (core can dispatch e.g. a null user id). + add_filter( 'learndash_has_access', [ $this, 'fp_filter' ] ); - // Non-first-party action handler with param types (method). - add_action( 'save_post', [ $this, 'on_save' ] ); + // First-party action: unrestricted - no errors. + add_action( 'learndash_after_save', [ $this, 'fp_action' ] ); - // Non-literal hook name - the sniff skips this (the PHPStan rule catches it). - $hook = 'the_content'; - add_filter( $hook, [ $this, 'filter_content' ] ); + // Non-first-party action, closure: param flagged, void return allowed. + add_action( 'init', function ( int $x ): void {} ); - // self::class reference to a static method. - add_filter( 'wp_title', [ self::class, 'static_filter' ] ); + // First-party filter, closure: all params + return flagged. + add_filter( 'ld_valid', function ( bool $valid, int $id ): bool { + return $valid; + } ); - // Same-file global-function handler with types - resolved and fixed here. - add_filter( 'excerpt_length', 'global_excerpt_handler' ); + // Non-first-party action closure with nullable / by-reference / variadic. + add_action( 'pre_get_posts', function ( ?WP_Query $q, int &$ref, string ...$rest ) {} ); - // Same-file global-function handler that is already type-less - no error. - add_filter( 'get_the_excerpt', 'typeless_excerpt_handler' ); + // self::class static handler on a first-party filter. + add_filter( 'sfwd_lms_has_access', [ self::class, 'fp_static_filter' ] ); - // First-party hook with a typed global handler - no error. - add_filter( 'learndash_excerpt', 'global_excerpt_handler' ); + // Old-style [ &$this, 'method' ] reference on a non-first-party action. + add_action( 'wp_mail_failed', [ &$this, 'np_ref_action' ] ); - // Cross-file global-function handler (not defined here) - left to PHPStan. - add_filter( 'wp_footer', 'some_cross_file_handler' ); + // Same-file global-function handler on a non-first-party filter. + add_filter( 'excerpt_length', 'np_global_filter' ); - // Non-first-party action, nullable + by-reference + variadic params. - add_action( 'pre_get_posts', function ( ?WP_Query $q, int &$ref, string ...$rest ) {} ); + // Dynamic hook name - skipped by the sniff (the PHPStan rule catches it). + $hook = 'the_content'; + add_filter( $hook, [ $this, 'np_filter' ] ); - // Non-first-party filter with the return type on its own line. - add_filter( 'the_title', function ( \WP_Post $post ) - : string - { - return $post->post_title; - } ); + // Cross-file callback - left to the PHPStan rule. + add_filter( 'wp_title', 'some_cross_file_handler' ); } - public function filter_content( string $content ): string { + public function np_filter( string $content ): string { return $content; } - public function typed_action( int $id ): void {} + public function np_action( int $post_id, WP_Post $post ): void {} + + public function fp_filter( bool $has_access, int $post_id, int $user_id ): bool { + return $has_access; + } + + public function fp_action( int $id ): void {} - public function on_save( int $post_id, WP_Post $post ): void {} + public function np_ref_action( \WP_Error $error ) {} - public static function static_filter( string $title ): string { + public static function fp_static_filter( string $title ): string { return $title; } } -function global_excerpt_handler( string $excerpt ): string { - return $excerpt; -} - -function typeless_excerpt_handler( $excerpt ) { - return $excerpt; +function np_global_filter( string $length ): string { + return $length; } diff --git a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed index f16e796..c1670f8 100644 --- a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed +++ b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed @@ -5,72 +5,66 @@ class Example { public function register() { - // Non-first-party filter, same-file method handler (param + return type). - add_filter( 'the_content', [ $this, 'filter_content' ] ); + // Non-first-party filter: all params + return must be type-less. + add_filter( 'the_content', [ $this, 'np_filter' ] ); - // Non-first-party action, closure with param type and void return (void ok). - add_action( 'init', function ( $x ): void {} ); - - // Non-first-party filter, closure with a return type (not allowed). - add_filter( 'body_class', function ( $classes ) { - return $classes; - } ); - - // First-party filter (matches prefix) - no error. - add_filter( 'learndash_something', [ $this, 'filter_content' ] ); + // Non-first-party action: params must be type-less; void return is allowed. + add_action( 'save_post', [ $this, 'np_action' ] ); - // First-party action - no error even with types. - add_action( 'ld_after_save', [ $this, 'typed_action' ] ); + // First-party filter: all params + return must be type-less, including the + // context arguments (core can dispatch e.g. a null user id). + add_filter( 'learndash_has_access', [ $this, 'fp_filter' ] ); - // Non-first-party action handler with param types (method). - add_action( 'save_post', [ $this, 'on_save' ] ); + // First-party action: unrestricted - no errors. + add_action( 'learndash_after_save', [ $this, 'fp_action' ] ); - // Non-literal hook name - the sniff skips this (the PHPStan rule catches it). - $hook = 'the_content'; - add_filter( $hook, [ $this, 'filter_content' ] ); + // Non-first-party action, closure: param flagged, void return allowed. + add_action( 'init', function ( $x ): void {} ); - // self::class reference to a static method. - add_filter( 'wp_title', [ self::class, 'static_filter' ] ); + // First-party filter, closure: all params + return flagged. + add_filter( 'ld_valid', function ( $valid, $id ) { + return $valid; + } ); - // Same-file global-function handler with types - resolved and fixed here. - add_filter( 'excerpt_length', 'global_excerpt_handler' ); + // Non-first-party action closure with nullable / by-reference / variadic. + add_action( 'pre_get_posts', function ( $q, &$ref, ...$rest ) {} ); - // Same-file global-function handler that is already type-less - no error. - add_filter( 'get_the_excerpt', 'typeless_excerpt_handler' ); + // self::class static handler on a first-party filter. + add_filter( 'sfwd_lms_has_access', [ self::class, 'fp_static_filter' ] ); - // First-party hook with a typed global handler - no error. - add_filter( 'learndash_excerpt', 'global_excerpt_handler' ); + // Old-style [ &$this, 'method' ] reference on a non-first-party action. + add_action( 'wp_mail_failed', [ &$this, 'np_ref_action' ] ); - // Cross-file global-function handler (not defined here) - left to PHPStan. - add_filter( 'wp_footer', 'some_cross_file_handler' ); + // Same-file global-function handler on a non-first-party filter. + add_filter( 'excerpt_length', 'np_global_filter' ); - // Non-first-party action, nullable + by-reference + variadic params. - add_action( 'pre_get_posts', function ( $q, &$ref, ...$rest ) {} ); + // Dynamic hook name - skipped by the sniff (the PHPStan rule catches it). + $hook = 'the_content'; + add_filter( $hook, [ $this, 'np_filter' ] ); - // Non-first-party filter with the return type on its own line. - add_filter( 'the_title', function ( $post ) - { - return $post->post_title; - } ); + // Cross-file callback - left to the PHPStan rule. + add_filter( 'wp_title', 'some_cross_file_handler' ); } - public function filter_content( $content ) { + public function np_filter( $content ) { return $content; } - public function typed_action( int $id ): void {} + public function np_action( $post_id, $post ): void {} + + public function fp_filter( $has_access, $post_id, $user_id ) { + return $has_access; + } + + public function fp_action( int $id ): void {} - public function on_save( $post_id, $post ): void {} + public function np_ref_action( $error ) {} - public static function static_filter( $title ) { + public static function fp_static_filter( $title ) { return $title; } } -function global_excerpt_handler( $excerpt ) { - return $excerpt; -} - -function typeless_excerpt_handler( $excerpt ) { - return $excerpt; +function np_global_filter( $length ) { + return $length; } diff --git a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.php b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.php index 4ab2f30..dd325ae 100644 --- a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.php +++ b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.php @@ -22,15 +22,15 @@ class HookHandlerTypesUnitTest extends AbstractSniffUnitTest { */ protected function getErrorList() { return [ - 12 => 1, // Closure on 'init': native param type (void return is allowed). - 15 => 2, // Closure on 'body_class': native param type + native return type. - 48 => 3, // Closure on 'pre_get_posts': nullable, by-reference, variadic params. - 51 => 1, // Closure on 'the_title': native param type. - 52 => 1, // Closure on 'the_title': native return type (on its own line). - 58 => 2, // filter_content(): param + return (via the 'the_content' binding). - 64 => 2, // on_save(): two native param types (via the 'save_post' binding). - 66 => 2, // static_filter(): param + return (via the 'wp_title' self::class binding). - 71 => 2, // global_excerpt_handler(): param + return (via the same-file 'excerpt_length' binding). + 22 => 1, // init closure (non-first-party action): param; void return allowed. + 25 => 3, // ld_valid closure (first-party filter): two params + return. + 30 => 3, // pre_get_posts closure (non-first-party action): nullable, by-ref, variadic. + 49 => 2, // np_filter() (non-first-party filter): param + return. + 53 => 2, // np_action() (non-first-party action): two params; void return allowed. + 55 => 4, // fp_filter() (first-party filter): all three params + return. + 61 => 1, // np_ref_action() (non-first-party action, via [ &$this, ... ]): param. + 63 => 2, // fp_static_filter() (first-party filter, via self::class): param + return. + 68 => 2, // np_global_filter() (non-first-party filter, global function): param + return. ]; } diff --git a/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php index 7563c7f..2edf217 100644 --- a/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php +++ b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php @@ -43,61 +43,46 @@ public function testRule(): void { $this->markTestSkipped( 'Skipped on the PHP 7.4 runtime: PHPStan\'s RuleTestCase php-parser emulation is unreliable here. The rule works on 7.4 under a normal phpstan analyse.' ); } + $filter_param = 'a filter can be dispatched with arguments of unexpected types (including null), so a native type can cause a fatal error.'; + $filter_return = 'filter return values are not type-guaranteed and a native return type can cause a fatal error.'; + $action_param = 'WordPress does not guarantee hook argument types and a native type can cause a fatal error.'; + $this->analyse( [ __DIR__ . '/data/hook-handler-types.php' ], [ - [ - 'Handler Hook_Test_Handlers::filter_content() for non-first-party hook "the_content" must not declare the native type "string" on parameter $content; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', - 13, - ], - [ - 'Handler Hook_Test_Handlers::filter_content() for non-first-party filter "the_content" must not declare a native return type ("string"); filter return values are not type-guaranteed and a native return type can cause a fatal error.', - 13, - ], - [ - 'Handler Hook_Test_Handlers::on_save() for non-first-party hook "save_post" must not declare the native type "int" on parameter $post_id; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', - 16, - ], - [ - 'Handler Hook_Test_Handlers::on_save() for non-first-party hook "save_post" must not declare the native type "WP_Post" on parameter $post; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', - 16, - ], - [ - 'Handler Hook_Test_Handlers::static_filter() for non-first-party hook "wp_title" must not declare the native type "string" on parameter $title; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', - 19, - ], - [ - 'Handler Hook_Test_Handlers::static_filter() for non-first-party filter "wp_title" must not declare a native return type ("string"); filter return values are not type-guaranteed and a native return type can cause a fatal error.', - 19, - ], - [ - 'Handler for non-first-party hook "init" must not declare the native type "int" on parameter $x; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', - 28, - ], - [ - 'Handler for non-first-party hook "body_class" must not declare the native type "array" on parameter $classes; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', - 31, - ], - [ - 'Handler for non-first-party filter "body_class" must not declare a native return type ("array"); filter return values are not type-guaranteed and a native return type can cause a fatal error.', - 31, - ], - [ - 'Handler Hook_Test_Handlers::filter_content() for non-first-party hook "the_content" must not declare the native type "string" on parameter $content; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', - 38, - ], - [ - 'Handler Hook_Test_Handlers::filter_content() for non-first-party filter "the_content" must not declare a native return type ("string"); filter return values are not type-guaranteed and a native return type can cause a fatal error.', - 38, - ], - [ - 'Handler typed_global_handler() for non-first-party hook "excerpt_length" must not declare the native type "string" on parameter $value; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', - 42, - ], - [ - 'Handler typed_global_handler() for non-first-party filter "excerpt_length" must not declare a native return type ("string"); filter return values are not type-guaranteed and a native return type can cause a fatal error.', - 42, - ], + // Non-first-party filter, cross-file method. + [ 'Handler Hook_Test_Handlers::np_filter() for filter "the_content" must not declare the native type "string" on parameter $content; ' . $filter_param, 13 ], + [ 'Handler Hook_Test_Handlers::np_filter() for filter "the_content" must not declare a native return type ("string"); ' . $filter_return, 13 ], + + // Non-first-party action, cross-file method (void return allowed). + [ 'Handler Hook_Test_Handlers::np_action() for non-first-party action "save_post" must not declare the native type "int" on parameter $post_id; ' . $action_param, 16 ], + [ 'Handler Hook_Test_Handlers::np_action() for non-first-party action "save_post" must not declare the native type "WP_Post" on parameter $post; ' . $action_param, 16 ], + + // First-party filter, cross-file method: every param + return. + [ 'Handler Hook_Test_Handlers::fp_filter() for filter "learndash_has_access" must not declare the native type "bool" on parameter $has_access; ' . $filter_param, 20 ], + [ 'Handler Hook_Test_Handlers::fp_filter() for filter "learndash_has_access" must not declare the native type "int" on parameter $post_id; ' . $filter_param, 20 ], + [ 'Handler Hook_Test_Handlers::fp_filter() for filter "learndash_has_access" must not declare the native type "int" on parameter $user_id; ' . $filter_param, 20 ], + [ 'Handler Hook_Test_Handlers::fp_filter() for filter "learndash_has_access" must not declare a native return type ("bool"); ' . $filter_return, 20 ], + + // First-party filter, string class reference to a static method. + [ 'Handler Hook_Test_Handlers::fp_static_filter() for filter "sfwd_lms_has_access" must not declare the native type "string" on parameter $title; ' . $filter_param, 26 ], + [ 'Handler Hook_Test_Handlers::fp_static_filter() for filter "sfwd_lms_has_access" must not declare a native return type ("string"); ' . $filter_return, 26 ], + + // Non-first-party action, closure (void return allowed). + [ 'Handler for non-first-party action "init" must not declare the native type "int" on parameter $x; ' . $action_param, 29 ], + + // First-party filter, closure: both params + return. + [ 'Handler for filter "ld_valid" must not declare the native type "bool" on parameter $valid; ' . $filter_param, 32 ], + [ 'Handler for filter "ld_valid" must not declare the native type "int" on parameter $id; ' . $filter_param, 32 ], + [ 'Handler for filter "ld_valid" must not declare a native return type ("bool"); ' . $filter_return, 32 ], + + // Non-first-party filter, global function. + [ 'Handler np_global_filter() for filter "excerpt_length" must not declare the native type "string" on parameter $length; ' . $filter_param, 37 ], + [ 'Handler np_global_filter() for filter "excerpt_length" must not declare a native return type ("string"); ' . $filter_return, 37 ], + + // Non-literal hook name resolved by inference. + [ 'Handler Hook_Test_Handlers::np_filter() for filter "the_content" must not declare the native type "string" on parameter $content; ' . $filter_param, 44 ], + [ 'Handler Hook_Test_Handlers::np_filter() for filter "the_content" must not declare a native return type ("string"); ' . $filter_return, 44 ], ] ); } diff --git a/StellarWP/Tests/PHPStan/data/handlers.php b/StellarWP/Tests/PHPStan/data/handlers.php index 3ec013a..a0bbf70 100644 --- a/StellarWP/Tests/PHPStan/data/handlers.php +++ b/StellarWP/Tests/PHPStan/data/handlers.php @@ -13,23 +13,27 @@ class WP_Query {} class Hook_Test_Handlers { - public function filter_content( string $content ): string { + public function np_filter( string $content ): string { return $content; } - public function on_save( int $post_id, WP_Post $post ): void {} + public function np_action( int $post_id, WP_Post $post ): void {} - public static function static_filter( string $title ): string { - return $title; + public function fp_filter( bool $has_access, int $post_id, int $user_id ): bool { + return $has_access; } - public function typed_ld_handler( int $id ): void {} + public function fp_action( int $id ): void {} + + public static function fp_static_filter( string $title ): string { + return $title; + } } -function typed_global_handler( string $value ): string { - return $value; +function np_global_filter( string $length ): string { + return $length; } -function typeless_global_handler( $value ) { - return $value; +function typeless_global_filter( $length ) { + return $length; } diff --git a/StellarWP/Tests/PHPStan/data/hook-handler-types.php b/StellarWP/Tests/PHPStan/data/hook-handler-types.php index 8cbe490..a823724 100644 --- a/StellarWP/Tests/PHPStan/data/hook-handler-types.php +++ b/StellarWP/Tests/PHPStan/data/hook-handler-types.php @@ -9,43 +9,39 @@ $obj = new Hook_Test_Handlers(); -// Cross-file method handler (declared in handlers.php): param + return. -add_filter( 'the_content', [ $obj, 'filter_content' ] ); +// Non-first-party filter, cross-file method: param + return. +add_filter( 'the_content', [ $obj, 'np_filter' ] ); -// Cross-file action handler: two param types; void return is allowed. -add_action( 'save_post', [ $obj, 'on_save' ] ); +// Non-first-party action, cross-file method: two params; void return allowed. +add_action( 'save_post', [ $obj, 'np_action' ] ); -// String class reference to a cross-file static method: param + return. -add_filter( 'wp_title', [ 'Hook_Test_Handlers', 'static_filter' ] ); +// First-party filter, cross-file method: all three params + return (a context +// argument such as the user id can be dispatched as null by core). +add_filter( 'learndash_has_access', [ $obj, 'fp_filter' ] ); -// First-party filter (matches prefix) - no error. -add_filter( 'learndash_something', [ $obj, 'filter_content' ] ); +// First-party action, cross-file method: unrestricted - no error. +add_action( 'learndash_after_save', [ $obj, 'fp_action' ] ); -// First-party action - no error even with types. -add_action( 'ld_after_save', [ $obj, 'typed_ld_handler' ] ); +// First-party filter, string class reference to a static method: param + return. +add_filter( 'sfwd_lms_has_access', [ 'Hook_Test_Handlers', 'fp_static_filter' ] ); -// Inline closure on a non-first-party action: param type; void return allowed. +// Non-first-party action, closure: param; void return allowed. add_action( 'init', function ( int $x ): void {} ); -// Inline closure on a non-first-party filter: param + return type. -add_filter( 'body_class', function ( array $classes ): array { - return $classes; +// First-party filter, closure: two params + return. +add_filter( 'ld_valid', function ( bool $valid, int $id ): bool { + return $valid; } ); -// Non-literal hook name that type inference narrows to 'the_content' - the case -// the sniff cannot see. Resolves to a non-first-party hook: param + return. -$hook = 'the_content'; -add_filter( $hook, [ $obj, 'filter_content' ] ); - -// Global-function-name callback with native types - resolved via PHPStan's -// static function reflection: param + return. -add_filter( 'excerpt_length', 'typed_global_handler' ); +// Non-first-party filter, global function: param + return. +add_filter( 'excerpt_length', 'np_global_filter' ); -// Global-function-name callback that is already type-less - no error. -add_filter( 'get_the_excerpt', 'typeless_global_handler' ); +// Already type-less global function - no error. +add_filter( 'get_the_excerpt', 'typeless_global_filter' ); -// First-party hook with a typed global handler - no error. -add_filter( 'learndash_excerpt', 'typed_global_handler' ); +// Non-literal hook name resolved by inference to a non-first-party filter. +$hook = 'the_content'; +add_filter( $hook, [ $obj, 'np_filter' ] ); // Unknown/undefined global function - skipped. add_filter( 'wp_footer', 'some_undefined_handler' ); From a34080715bfedeedfbe92cd715f812092c54d0aa Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Thu, 23 Jul 2026 21:57:57 -0400 Subject: [PATCH 03/17] refactor: drop prefix config; forbid native param types on all handlers - 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 --- README.md | 76 +++++-------- StellarWP/PHPStan/HookHandlerTypesRule.php | 106 +++++------------- StellarWP/PHPStan/extension.neon | 8 +- .../Sniffs/Hooks/HookHandlerTypesSniff.php | 101 ++++------------- .../Tests/Hooks/HookHandlerTypesUnitTest.inc | 57 +++++----- .../Hooks/HookHandlerTypesUnitTest.inc.fixed | 57 +++++----- .../Tests/Hooks/HookHandlerTypesUnitTest.php | 22 ++-- .../PHPStan/HookHandlerTypesRuleTest.php | 64 +++++------ StellarWP/Tests/PHPStan/data/handlers.php | 12 +- .../Tests/PHPStan/data/hook-handler-types.php | 35 +++--- 10 files changed, 195 insertions(+), 343 deletions(-) diff --git a/README.md b/README.md index 95f2f30..65f8d7e 100644 --- a/README.md +++ b/README.md @@ -78,28 +78,18 @@ You can follow [this guide](https://confluence.jetbrains.com/display/PhpStorm/PH ## Hook handler argument types -WordPress passes hook arguments with no type guarantees and reassigns or discards -filter return values however it likes. Declaring a **native parameter type** or a -**native return type** on a handler attached to a hook you do not own (a WP core -hook, or one defined by another plugin or theme) can therefore cause a runtime -**fatal** when the value received - or the value the filter chain expects back - -does not match the declared type. The safe posture is to be type-less on those -handlers. - -This standard enforces that rule with two complementary tools. "First-party" is -determined solely by prefixes you configure, which should mirror your project's -`WordPress.NamingConventions.PrefixAllGlobals` value. Enforcement follows the hook -type: - -- **Filters (any prefix, first-party or not):** the handler must be fully - type-less - no native type on **any** parameter and no native return type. A - filter can be dispatched with arguments of unexpected types even by first-party - code (LearnDash core calls `apply_filters( 'sfwd_lms_has_access', true, 28, null )`, - so a native `int $user_id` throws a `TypeError`), and its return value flows - through code you do not control. -- **Non-first-party actions** (WP core / third-party): no native parameter types; - a `void` return type is allowed. -- **First-party actions:** unrestricted. +A hook's argument and return types are never guaranteed. Relying on documentation +that turns out to be inaccurate can lead to the wrong native type, and any third +party can dispatch one of your hooks via `apply_filters()` / `do_action()` with +arguments of a different type. Either way, a value that does not match a +**native parameter type** or a **native return type** declared on the handler +causes a runtime **fatal**. This standard enforces that with two complementary +tools, following the hook type: + +- **Filter handlers:** no native type on **any** parameter and no native return + type. +- **Action handlers:** no native type on **any** parameter; a native `void` + return type is allowed (actions always return void). | | PHPCS sniff (`StellarWP.Hooks.HookHandlerTypes`) | PHPStan rule | |---|---|---| @@ -114,37 +104,22 @@ handlers whose declaration lives in a different file from the `add_filter()` / ### PHPCS sniff -The sniff is part of the `StellarWP` standard. Configure the `prefixes` property -(and, optionally, `allow_void_return_on_actions`, default `true`). With no -prefixes configured the sniff does nothing. +The sniff is part of the `StellarWP` standard, so referencing it needs no +configuration. Enable just this sniff with: ```xml - - - - - - - - + ``` +An optional `allow_void_return_on_actions` property (default `true`) controls +whether a native `void` return type is permitted on action handlers. + ### PHPStan rule The rule ships as an auto-discovered PHPStan extension. Projects using [`phpstan/extension-installer`](https://github.com/phpstan/extension-installer) -get it registered automatically - no `includes:` entry needed. You must still set -the `prefixes` parameter in your `phpstan.neon` (an empty list makes the rule a -no-op): - -```neon -parameters: - stellarwpHookHandlerTypes: - prefixes: - - my_project - - mp_ - # allowVoidReturnOnActions: true # optional, default true -``` +get it registered automatically with **no configuration** - no `includes:` entry +and no parameters required. If you are not using `extension-installer`, include the extension manually: @@ -153,9 +128,14 @@ includes: - vendor/stellarwp/coding-standards/StellarWP/PHPStan/extension.neon ``` -> The `prefixes` value now lives in up to three places - `PrefixAllGlobals` in -> `phpcs.xml`, the sniff's `prefixes`, and the rule's `prefixes` in `phpstan.neon`. -> There is no shared source both tools can read, so keep them in sync. +The optional `allowVoidReturnOnActions` parameter (default `true`) mirrors the +sniff property: + +```neon +parameters: + stellarwpHookHandlerTypes: + allowVoidReturnOnActions: true +``` When adopting this in a project with existing violations, regenerate your PHPStan baseline to grandfather them, then burn them down over time. diff --git a/StellarWP/PHPStan/HookHandlerTypesRule.php b/StellarWP/PHPStan/HookHandlerTypesRule.php index 27e1d1a..b99ff5a 100644 --- a/StellarWP/PHPStan/HookHandlerTypesRule.php +++ b/StellarWP/PHPStan/HookHandlerTypesRule.php @@ -1,18 +1,16 @@ reflection_provider = $reflection_provider; - $this->prefixes = $prefixes; $this->allow_void_return_on_actions = $allow_void_return_on_actions; } @@ -90,7 +78,7 @@ public function getNodeType(): string { * @return array */ public function processNode( Node $node, Scope $scope ): array { - if ( $this->prefixes === [] || ! $node->name instanceof Name ) { + if ( ! $node->name instanceof Name ) { return []; } @@ -104,34 +92,15 @@ public function processNode( Node $node, Scope $scope ): array { return []; } - // Resolve the hook name(s) via type inference. This covers literals as - // well as any expression narrowing to constant string(s). + // Resolve the hook name via type inference (used only to name the hook in + // the message). This covers literals as well as any expression narrowing + // to constant string(s). $constant_strings = $scope->getType( $args[0]->value )->getConstantStrings(); if ( $constant_strings === [] ) { return []; } - $is_filter = $function === 'add_filter'; - - $hook = $constant_strings[0]->getValue(); - $all_first_party = true; - foreach ( $constant_strings as $constant_string ) { - if ( ! $this->is_first_party( $constant_string->getValue() ) ) { - $hook = $constant_string->getValue(); - $all_first_party = false; - break; - } - } - - // First-party actions are unrestricted. Everything else is checked: any - // filter (a first-party filter still cannot type its arguments or return - - // the value is shaped by other code, and even core dispatches unexpected - // types), and non-first-party actions. - if ( $all_first_party && ! $is_filter ) { - return []; - } - - return $this->check_callback( $args[1]->value, $scope, $hook, $is_filter ); + return $this->check_callback( $args[1]->value, $scope, $constant_strings[0]->getValue(), $function === 'add_filter' ); } /** @@ -344,26 +313,18 @@ private function has_native_type( Type $type ): bool { * @return \PHPStan\Rules\RuleError */ private function param_error( bool $is_filter, string $handler, string $hook, string $type, string $param_name, int $line ) { - $where = $handler === '' ? '' : $handler . ' '; - $subject = $param_name === '' ? 'a parameter' : 'parameter $' . $param_name; - - if ( $is_filter ) { - $message = sprintf( - 'Handler %sfor filter "%s" must not declare the native type "%s" on %s; a filter can be dispatched with arguments of unexpected types (including null), so a native type can cause a fatal error.', - $where, - $hook, - $type, - $subject - ); - } else { - $message = sprintf( - 'Handler %sfor non-first-party action "%s" must not declare the native type "%s" on %s; WordPress does not guarantee hook argument types and a native type can cause a fatal error.', - $where, - $hook, - $type, - $subject - ); - } + $where = $handler === '' ? '' : $handler . ' '; + $subject = $param_name === '' ? 'a parameter' : 'parameter $' . $param_name; + $hook_type = $is_filter ? 'filter' : 'action'; + + $message = sprintf( + 'Handler %sfor %s "%s" must not declare the native type "%s" on %s; hook arguments are not type-guaranteed (a hook can be dispatched with unexpected types, including null), so a native type can cause a fatal error.', + $where, + $hook_type, + $hook, + $type, + $subject + ); return RuleErrorBuilder::message( $message )->identifier( 'stellarwp.hookHandlerParamType' )->line( $line )->build(); } @@ -385,7 +346,7 @@ private function return_type_error( string $hook, bool $is_filter, string $retur ); } else { $message = sprintf( - 'Handler %sfor non-first-party action "%s" must not declare a native return type ("%s") other than void.', + 'Handler %sfor action "%s" must not declare a native return type ("%s") other than void.', $where, $hook, $return_type @@ -404,19 +365,6 @@ private function is_void_allowed( bool $is_filter, string $return_type ): bool { && strtolower( ltrim( $return_type, '?\\' ) ) === 'void'; } - /** - * Whether a hook name belongs to this project by prefix. - */ - private function is_first_party( string $hook_name ): bool { - foreach ( $this->prefixes as $prefix ) { - if ( $prefix !== '' && stripos( $hook_name, $prefix ) === 0 ) { - return true; - } - } - - return false; - } - /** * Renders a PhpParser type node to a readable string (for messages and void * detection). diff --git a/StellarWP/PHPStan/extension.neon b/StellarWP/PHPStan/extension.neon index 1e58556..728959c 100644 --- a/StellarWP/PHPStan/extension.neon +++ b/StellarWP/PHPStan/extension.neon @@ -1,14 +1,11 @@ parameters: - # Prefixes that identify first-party hooks. Mirror the project's - # WordPress.NamingConventions.PrefixAllGlobals "prefixes" value. When empty, - # the rule does nothing (a safe default that also makes "unconfigured" obvious). stellarwpHookHandlerTypes: - prefixes: [] + # Actions always return void, so a native `void` return type is allowed on + # action handlers. Set to false to forbid it too. allowVoidReturnOnActions: true parametersSchema: stellarwpHookHandlerTypes: structure([ - prefixes: listOf(string()) allowVoidReturnOnActions: bool() ]) @@ -16,7 +13,6 @@ services: - class: StellarWP\PHPStan\HookHandlerTypesRule arguments: - prefixes: %stellarwpHookHandlerTypes.prefixes% allow_void_return_on_actions: %stellarwpHookHandlerTypes.allowVoidReturnOnActions% tags: - phpstan.rules.rule diff --git a/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php b/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php index 806937a..8ef10bc 100644 --- a/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php +++ b/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php @@ -1,29 +1,18 @@ false, ]; - /** - * Prefixes that identify first-party hooks. - * - * Mirror the project's WordPress.NamingConventions.PrefixAllGlobals - * "prefixes" value. When empty, the sniff does nothing. - * - * @var string[] - */ - public $prefixes = []; - /** * Whether a native `void` return type is acceptable on action handlers. * @@ -104,11 +83,6 @@ public function register(): array { * @phpcs:disable SlevomatCodingStandard.TypeHints.ParameterTypeHint.MissingNativeTypeHint */ public function process( File $phpcs_file, $stack_ptr ): void { - // Nothing to do until the project tells us which prefixes are first-party. - if ( empty( $this->prefixes ) ) { - return; - } - $tokens = $phpcs_file->getTokens(); $content = strtolower( $tokens[ $stack_ptr ]['content'] ); @@ -148,12 +122,12 @@ public function process( File $phpcs_file, $stack_ptr ): void { return; } - // Argument 1: the hook name. + // Argument 1: the hook name (used only to name the hook in the message). $hook_name = $this->get_string_argument( $phpcs_file, $args[0] ); if ( $hook_name === null ) { if ( $this->warn_on_dynamic_hook_names ) { $phpcs_file->addWarning( - 'Unable to determine the hook name statically; the handler type restriction for non-first-party hooks could not be verified.', + 'Unable to determine the hook name statically; the handler type restriction could not be verified.', $args[0]['start'], 'DynamicHookName' ); @@ -162,15 +136,7 @@ public function process( File $phpcs_file, $stack_ptr ): void { return; } - $is_filter = self::HOOK_FUNCTIONS[ $content ]; - $first_party = $this->is_first_party( $hook_name ); - - // First-party actions may be typed freely. First-party filters still must - // have a type-less first argument and no native return type: a filter's - // value is shaped by other code (plugins/themes) we do not control. - if ( $first_party && ! $is_filter ) { - return; - } + $is_filter = self::HOOK_FUNCTIONS[ $content ]; // Argument 2: the callback. Resolve to a function/closure/method token. $func_ptr = $this->resolve_handler( $phpcs_file, $args[1], $stack_ptr ); @@ -303,29 +269,6 @@ private function strip_quotes( string $raw ): string { return $raw; } - /** - * Determines whether a hook name belongs to this project by prefix. - * - * @param string $hook_name The resolved hook name. - * - * @return bool - */ - private function is_first_party( string $hook_name ): bool { - foreach ( $this->prefixes as $prefix ) { - $prefix = (string) $prefix; - - if ( $prefix === '' ) { - continue; - } - - if ( stripos( $hook_name, $prefix ) === 0 ) { - return true; - } - } - - return false; - } - /** * Resolves a callback argument to the token of the function, closure, or * arrow function whose declared types should be checked. Returns null when @@ -584,24 +527,19 @@ private function find_class_method( File $phpcs_file, int $stack_ptr, string $me * @return void */ private function check_handler_types( File $phpcs_file, int $func_ptr, string $hook_name, bool $is_filter ): void { - $params = $phpcs_file->getMethodParameters( $func_ptr ); + $hook_type = $is_filter ? 'filter' : 'action'; + $params = $phpcs_file->getMethodParameters( $func_ptr ); foreach ( $params as $param ) { if ( empty( $param['type_hint'] ) ) { continue; } - if ( $is_filter ) { - $message = 'Handler for filter "%s" must not declare the native type "%s" on parameter %s; a filter can be dispatched with arguments of unexpected types (including null), so a native type can cause a fatal error.'; - } else { - $message = 'Handler for non-first-party action "%s" must not declare the native type "%s" on parameter %s; WordPress does not guarantee hook argument types and a native type can cause a fatal error.'; - } - $fix = $phpcs_file->addFixableError( - $message, + 'Handler for %s "%s" must not declare the native type "%s" on parameter %s; hook arguments are not type-guaranteed (a hook can be dispatched with unexpected types, including null), so a native type can cause a fatal error.', $param['type_hint_token'], 'NativeParameterType', - [ $hook_name, $param['type_hint'], $param['name'] ] + [ $hook_type, $hook_name, $param['type_hint'], $param['name'] ] ); if ( $fix === true ) { @@ -623,10 +561,9 @@ private function check_handler_types( File $phpcs_file, int $func_ptr, string $h } if ( $is_filter ) { - // Filters can never carry a native return type, first-party or not. $message = 'Handler for filter "%s" must not declare a native return type ("%s"); filter return values are not type-guaranteed and a native return type can cause a fatal error.'; } else { - $message = 'Handler for non-first-party action "%s" must not declare a native return type ("%s") other than void.'; + $message = 'Handler for action "%s" must not declare a native return type ("%s") other than void.'; } $fix = $phpcs_file->addFixableError( diff --git a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc index d081e5c..c908a31 100644 --- a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc +++ b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc @@ -1,70 +1,67 @@ 1, // init closure (non-first-party action): param; void return allowed. - 25 => 3, // ld_valid closure (first-party filter): two params + return. - 30 => 3, // pre_get_posts closure (non-first-party action): nullable, by-ref, variadic. - 49 => 2, // np_filter() (non-first-party filter): param + return. - 53 => 2, // np_action() (non-first-party action): two params; void return allowed. - 55 => 4, // fp_filter() (first-party filter): all three params + return. - 61 => 1, // np_ref_action() (non-first-party action, via [ &$this, ... ]): param. - 63 => 2, // fp_static_filter() (first-party filter, via self::class): param + return. - 68 => 2, // np_global_filter() (non-first-party filter, global function): param + return. + 19 => 1, // init action closure: param; void return allowed. + 22 => 3, // ld_valid filter closure: two params + return. + 27 => 3, // pre_get_posts action closure: nullable, by-ref, variadic params. + 46 => 2, // filter_method(): param + return. + 50 => 2, // action_method(): two params; void return allowed. + 52 => 4, // filter_context_method(): all three params + return. + 56 => 1, // action_typed(): the native param (an action still forbids param types). + 58 => 1, // ref_action() (via [ &$this, ... ]): param. + 60 => 2, // static_filter() (via self::class): param + return. + 65 => 2, // global_filter() (global function): param + return. ]; } diff --git a/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php index 2edf217..c53bce3 100644 --- a/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php +++ b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php @@ -27,11 +27,7 @@ public static function setUpBeforeClass(): void { } protected function getRule(): Rule { - return new HookHandlerTypesRule( - $this->createReflectionProvider(), - [ 'learndash', 'ld_', 'sfwd' ], - true - ); + return new HookHandlerTypesRule( $this->createReflectionProvider(), true ); } public function testRule(): void { @@ -43,46 +39,48 @@ public function testRule(): void { $this->markTestSkipped( 'Skipped on the PHP 7.4 runtime: PHPStan\'s RuleTestCase php-parser emulation is unreliable here. The rule works on 7.4 under a normal phpstan analyse.' ); } - $filter_param = 'a filter can be dispatched with arguments of unexpected types (including null), so a native type can cause a fatal error.'; - $filter_return = 'filter return values are not type-guaranteed and a native return type can cause a fatal error.'; - $action_param = 'WordPress does not guarantee hook argument types and a native type can cause a fatal error.'; + $param = 'hook arguments are not type-guaranteed (a hook can be dispatched with unexpected types, including null), so a native type can cause a fatal error.'; + $return = 'filter return values are not type-guaranteed and a native return type can cause a fatal error.'; $this->analyse( [ __DIR__ . '/data/hook-handler-types.php' ], [ - // Non-first-party filter, cross-file method. - [ 'Handler Hook_Test_Handlers::np_filter() for filter "the_content" must not declare the native type "string" on parameter $content; ' . $filter_param, 13 ], - [ 'Handler Hook_Test_Handlers::np_filter() for filter "the_content" must not declare a native return type ("string"); ' . $filter_return, 13 ], + // Filter, cross-file method: param + return. + [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_content" must not declare the native type "string" on parameter $content; ' . $param, 13 ], + [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_content" must not declare a native return type ("string"); ' . $return, 13 ], + + // Action, cross-file method: two params (void return allowed). + [ 'Handler Hook_Test_Handlers::action_method() for action "save_post" must not declare the native type "int" on parameter $post_id; ' . $param, 16 ], + [ 'Handler Hook_Test_Handlers::action_method() for action "save_post" must not declare the native type "WP_Post" on parameter $post; ' . $param, 16 ], - // Non-first-party action, cross-file method (void return allowed). - [ 'Handler Hook_Test_Handlers::np_action() for non-first-party action "save_post" must not declare the native type "int" on parameter $post_id; ' . $action_param, 16 ], - [ 'Handler Hook_Test_Handlers::np_action() for non-first-party action "save_post" must not declare the native type "WP_Post" on parameter $post; ' . $action_param, 16 ], + // Filter with several context arguments: every param + return. + [ 'Handler Hook_Test_Handlers::filter_context_method() for filter "user_has_cap" must not declare the native type "bool" on parameter $has_access; ' . $param, 19 ], + [ 'Handler Hook_Test_Handlers::filter_context_method() for filter "user_has_cap" must not declare the native type "int" on parameter $post_id; ' . $param, 19 ], + [ 'Handler Hook_Test_Handlers::filter_context_method() for filter "user_has_cap" must not declare the native type "int" on parameter $user_id; ' . $param, 19 ], + [ 'Handler Hook_Test_Handlers::filter_context_method() for filter "user_has_cap" must not declare a native return type ("bool"); ' . $return, 19 ], - // First-party filter, cross-file method: every param + return. - [ 'Handler Hook_Test_Handlers::fp_filter() for filter "learndash_has_access" must not declare the native type "bool" on parameter $has_access; ' . $filter_param, 20 ], - [ 'Handler Hook_Test_Handlers::fp_filter() for filter "learndash_has_access" must not declare the native type "int" on parameter $post_id; ' . $filter_param, 20 ], - [ 'Handler Hook_Test_Handlers::fp_filter() for filter "learndash_has_access" must not declare the native type "int" on parameter $user_id; ' . $filter_param, 20 ], - [ 'Handler Hook_Test_Handlers::fp_filter() for filter "learndash_has_access" must not declare a native return type ("bool"); ' . $filter_return, 20 ], + // Action with a typed param (void return allowed). + [ 'Handler Hook_Test_Handlers::action_typed() for action "transition_post_status" must not declare the native type "int" on parameter $id; ' . $param, 22 ], - // First-party filter, string class reference to a static method. - [ 'Handler Hook_Test_Handlers::fp_static_filter() for filter "sfwd_lms_has_access" must not declare the native type "string" on parameter $title; ' . $filter_param, 26 ], - [ 'Handler Hook_Test_Handlers::fp_static_filter() for filter "sfwd_lms_has_access" must not declare a native return type ("string"); ' . $filter_return, 26 ], + // Filter, string class reference to a static method. + [ 'Handler Hook_Test_Handlers::static_filter() for filter "wp_title" must not declare the native type "string" on parameter $title; ' . $param, 25 ], + [ 'Handler Hook_Test_Handlers::static_filter() for filter "wp_title" must not declare a native return type ("string"); ' . $return, 25 ], - // Non-first-party action, closure (void return allowed). - [ 'Handler for non-first-party action "init" must not declare the native type "int" on parameter $x; ' . $action_param, 29 ], + // Action closure (void return allowed). + [ 'Handler for action "init" must not declare the native type "int" on parameter $x; ' . $param, 28 ], - // First-party filter, closure: both params + return. - [ 'Handler for filter "ld_valid" must not declare the native type "bool" on parameter $valid; ' . $filter_param, 32 ], - [ 'Handler for filter "ld_valid" must not declare the native type "int" on parameter $id; ' . $filter_param, 32 ], - [ 'Handler for filter "ld_valid" must not declare a native return type ("bool"); ' . $filter_return, 32 ], + // Filter closure: both params + return. + [ 'Handler for filter "login_redirect" must not declare the native type "bool" on parameter $valid; ' . $param, 31 ], + [ 'Handler for filter "login_redirect" must not declare the native type "int" on parameter $id; ' . $param, 31 ], + [ 'Handler for filter "login_redirect" must not declare a native return type ("bool"); ' . $return, 31 ], - // Non-first-party filter, global function. - [ 'Handler np_global_filter() for filter "excerpt_length" must not declare the native type "string" on parameter $length; ' . $filter_param, 37 ], - [ 'Handler np_global_filter() for filter "excerpt_length" must not declare a native return type ("string"); ' . $filter_return, 37 ], + // Filter, global function. + [ 'Handler global_filter() for filter "excerpt_length" must not declare the native type "string" on parameter $length; ' . $param, 36 ], + [ 'Handler global_filter() for filter "excerpt_length" must not declare a native return type ("string"); ' . $return, 36 ], // Non-literal hook name resolved by inference. - [ 'Handler Hook_Test_Handlers::np_filter() for filter "the_content" must not declare the native type "string" on parameter $content; ' . $filter_param, 44 ], - [ 'Handler Hook_Test_Handlers::np_filter() for filter "the_content" must not declare a native return type ("string"); ' . $filter_return, 44 ], + [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_content" must not declare the native type "string" on parameter $content; ' . $param, 43 ], + [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_content" must not declare a native return type ("string"); ' . $return, 43 ], ] ); } diff --git a/StellarWP/Tests/PHPStan/data/handlers.php b/StellarWP/Tests/PHPStan/data/handlers.php index a0bbf70..06724f8 100644 --- a/StellarWP/Tests/PHPStan/data/handlers.php +++ b/StellarWP/Tests/PHPStan/data/handlers.php @@ -13,24 +13,24 @@ class WP_Query {} class Hook_Test_Handlers { - public function np_filter( string $content ): string { + public function filter_method( string $content ): string { return $content; } - public function np_action( int $post_id, WP_Post $post ): void {} + public function action_method( int $post_id, WP_Post $post ): void {} - public function fp_filter( bool $has_access, int $post_id, int $user_id ): bool { + public function filter_context_method( bool $has_access, int $post_id, int $user_id ): bool { return $has_access; } - public function fp_action( int $id ): void {} + public function action_typed( int $id ): void {} - public static function fp_static_filter( string $title ): string { + public static function static_filter( string $title ): string { return $title; } } -function np_global_filter( string $length ): string { +function global_filter( string $length ): string { return $length; } diff --git a/StellarWP/Tests/PHPStan/data/hook-handler-types.php b/StellarWP/Tests/PHPStan/data/hook-handler-types.php index a823724..0c73c97 100644 --- a/StellarWP/Tests/PHPStan/data/hook-handler-types.php +++ b/StellarWP/Tests/PHPStan/data/hook-handler-types.php @@ -9,39 +9,38 @@ $obj = new Hook_Test_Handlers(); -// Non-first-party filter, cross-file method: param + return. -add_filter( 'the_content', [ $obj, 'np_filter' ] ); +// Filter, cross-file method: param + return. +add_filter( 'the_content', [ $obj, 'filter_method' ] ); -// Non-first-party action, cross-file method: two params; void return allowed. -add_action( 'save_post', [ $obj, 'np_action' ] ); +// Action, cross-file method: two params; void return allowed. +add_action( 'save_post', [ $obj, 'action_method' ] ); -// First-party filter, cross-file method: all three params + return (a context -// argument such as the user id can be dispatched as null by core). -add_filter( 'learndash_has_access', [ $obj, 'fp_filter' ] ); +// Filter with several context arguments, cross-file method: all params + return. +add_filter( 'user_has_cap', [ $obj, 'filter_context_method' ] ); -// First-party action, cross-file method: unrestricted - no error. -add_action( 'learndash_after_save', [ $obj, 'fp_action' ] ); +// Action with a typed param, cross-file method: the param is flagged (void ok). +add_action( 'transition_post_status', [ $obj, 'action_typed' ] ); -// First-party filter, string class reference to a static method: param + return. -add_filter( 'sfwd_lms_has_access', [ 'Hook_Test_Handlers', 'fp_static_filter' ] ); +// Filter, string class reference to a static method: param + return. +add_filter( 'wp_title', [ 'Hook_Test_Handlers', 'static_filter' ] ); -// Non-first-party action, closure: param; void return allowed. +// Action closure: param; void return allowed. add_action( 'init', function ( int $x ): void {} ); -// First-party filter, closure: two params + return. -add_filter( 'ld_valid', function ( bool $valid, int $id ): bool { +// Filter closure: two params + return. +add_filter( 'login_redirect', function ( bool $valid, int $id ): bool { return $valid; } ); -// Non-first-party filter, global function: param + return. -add_filter( 'excerpt_length', 'np_global_filter' ); +// Filter, global function: param + return. +add_filter( 'excerpt_length', 'global_filter' ); // Already type-less global function - no error. add_filter( 'get_the_excerpt', 'typeless_global_filter' ); -// Non-literal hook name resolved by inference to a non-first-party filter. +// Non-literal hook name resolved by inference to a filter. $hook = 'the_content'; -add_filter( $hook, [ $obj, 'np_filter' ] ); +add_filter( $hook, [ $obj, 'filter_method' ] ); // Unknown/undefined global function - skipped. add_filter( 'wp_footer', 'some_undefined_handler' ); From 99d47e8a6f5313c0b416df47f59a2ec7cfe77415 Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Fri, 24 Jul 2026 08:12:26 -0400 Subject: [PATCH 04/17] docs: advise validating/casting hook arguments inside the callback - 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 --- README.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/README.md b/README.md index 65f8d7e..9554caf 100644 --- a/README.md +++ b/README.md @@ -91,6 +91,35 @@ tools, following the hook type: - **Action handlers:** no native type on **any** parameter; a native `void` return type is allowed (actions always return void). +Removing the native types does **not** remove the need for type safety - it moves +that responsibility into the callback. Native types gave you a guarantee the +handler could rely on; since that guarantee is not safe here, the handler has to +provide it itself. Validate the incoming arguments (and, for filters, the value +you return) at the top of the handler before using them. + +Check the type before you use it - do not simply cast. A blind cast is not safe: +`$value` could be an object or array, and `(string) $value` would mangle it or +fatal. Use a scalar/type check (`is_string()`, `is_numeric()`, `is_scalar()`, ...) +and handle the unexpected case. For a filter, return the value unchanged when it +is not something you can handle, so the chain is preserved: + +```php +// Do not rely on the signature to guarantee the types: +// public function filter_the_value( string $value, int $post_id ): string +public function filter_the_value( $value, $post_id ) { + // A cast alone is unsafe - $value could be an object or array. + if ( ! is_string( $value ) ) { + return $value; // not something we handle: pass it through unchanged. + } + + $post_id = is_numeric( $post_id ) ? (int) $post_id : 0; + + // ... $value is a string and $post_id is an int now. + + return $value; +} +``` + | | PHPCS sniff (`StellarWP.Hooks.HookHandlerTypes`) | PHPStan rule | |---|---|---| | Runs in | phpcs (fast, in-editor) | phpstan (whole codebase, never diff-limited) | From cc7bd303066412783fd059d84bdc00d8e1887229 Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Fri, 24 Jul 2026 08:33:58 -0400 Subject: [PATCH 05/17] feat: resolve container callbacks in the PHPStan rule - 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 --- StellarWP/PHPStan/HookHandlerTypesRule.php | 57 ++++++++++++++++++- .../PHPStan/HookHandlerTypesRuleTest.php | 4 ++ StellarWP/Tests/PHPStan/data/handlers.php | 11 ++++ .../Tests/PHPStan/data/hook-handler-types.php | 5 ++ 4 files changed, 75 insertions(+), 2 deletions(-) diff --git a/StellarWP/PHPStan/HookHandlerTypesRule.php b/StellarWP/PHPStan/HookHandlerTypesRule.php index b99ff5a..cceeb6c 100644 --- a/StellarWP/PHPStan/HookHandlerTypesRule.php +++ b/StellarWP/PHPStan/HookHandlerTypesRule.php @@ -16,8 +16,9 @@ * entire codebase (never diff-limited) and resolves callbacks and hook names * across files, it catches the cases the sniff cannot: a handler whose * declaration lives in a different file from the add_action()/add_filter() call, - * and hook names that are not literal strings but which type inference can narrow - * to constant string(s). + * hook names that are not literal strings but which type inference can narrow to + * constant string(s), and container callbacks + * (`$container->callback( Class::class, 'method' )`). * * @package StellarWP\CodingStandards */ @@ -29,6 +30,8 @@ use PhpParser\Node\Expr\Array_; use PhpParser\Node\Expr\Closure; use PhpParser\Node\Expr\FuncCall; +use PhpParser\Node\Expr\MethodCall; +use PhpParser\Node\Expr\StaticCall; use PhpParser\Node\Name; use PhpParser\Node\Scalar\String_; use PHPStan\Analyser\Scope; @@ -127,10 +130,60 @@ private function check_callback( Node $callback, Scope $scope, string $hook, boo return $this->check_global_function( $callback->value, $scope, $hook, $is_filter, $callback->getStartLine() ); } + // Container callback: `$container->callback( Some_Class::class, 'method' )` + // (lucatume/di52 and StellarWP containers), which returns a callable that + // invokes Some_Class::method. + if ( + ( $callback instanceof MethodCall || $callback instanceof StaticCall ) + && $callback->name instanceof Node\Identifier + && strtolower( $callback->name->name ) === 'callback' + ) { + return $this->check_container_callback( $callback, $scope, $hook, $is_filter ); + } + // Unresolvable callbacks (variables, first-class callables) are skipped. return []; } + /** + * Checks a container callback - `$container->callback( Class::class, 'method' )` + * - by resolving the class/method arguments and inspecting that method. Only + * acts when the arguments actually resolve to a real class method, so an + * unrelated `->callback()` is harmlessly ignored. + * + * @param MethodCall|StaticCall $callback + * + * @return array + */ + private function check_container_callback( Node $callback, Scope $scope, string $hook, bool $is_filter ): array { + $args = $callback->getArgs(); + if ( count( $args ) < 2 ) { + return []; + } + + $class_type = $scope->getType( $args[0]->value ); + + $class_names = []; + foreach ( $class_type->getObjectClassReflections() as $class_reflection ) { + $class_names[ $class_reflection->getName() ] = true; + } + foreach ( $class_type->getConstantStrings() as $constant_string ) { + $class_names[ $constant_string->getValue() ] = true; + } + + $errors = []; + foreach ( $scope->getType( $args[1]->value )->getConstantStrings() as $method_string ) { + foreach ( array_keys( $class_names ) as $class_name ) { + $errors = array_merge( + $errors, + $this->check_class_method( $class_name, $method_string->getValue(), $hook, $is_filter, $callback->getStartLine() ) + ); + } + } + + return $errors; + } + /** * Checks an inline closure or arrow function using its declared native types. * diff --git a/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php index c53bce3..7fd0903 100644 --- a/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php +++ b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php @@ -81,6 +81,10 @@ public function testRule(): void { // Non-literal hook name resolved by inference. [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_content" must not declare the native type "string" on parameter $content; ' . $param, 43 ], [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_content" must not declare a native return type ("string"); ' . $return, 43 ], + + // Container callback: $container->callback( Class::class, 'method' ). + [ 'Handler Hook_Test_Handlers::container_handler() for filter "render_block" must not declare the native type "int" on parameter $value; ' . $param, 48 ], + [ 'Handler Hook_Test_Handlers::container_handler() for filter "render_block" must not declare a native return type ("int"); ' . $return, 48 ], ] ); } diff --git a/StellarWP/Tests/PHPStan/data/handlers.php b/StellarWP/Tests/PHPStan/data/handlers.php index 06724f8..ea9da49 100644 --- a/StellarWP/Tests/PHPStan/data/handlers.php +++ b/StellarWP/Tests/PHPStan/data/handlers.php @@ -28,6 +28,17 @@ public function action_typed( int $id ): void {} public static function static_filter( string $title ): string { return $title; } + + public function container_handler( int $value ): int { + return $value; + } +} + +class Fake_Container { + + public function callback( $id, $method ) { + return static function () {}; + } } function global_filter( string $length ): string { diff --git a/StellarWP/Tests/PHPStan/data/hook-handler-types.php b/StellarWP/Tests/PHPStan/data/hook-handler-types.php index 0c73c97..63b7cf1 100644 --- a/StellarWP/Tests/PHPStan/data/hook-handler-types.php +++ b/StellarWP/Tests/PHPStan/data/hook-handler-types.php @@ -42,5 +42,10 @@ $hook = 'the_content'; add_filter( $hook, [ $obj, 'filter_method' ] ); +// Container callback ($container->callback( Class::class, 'method' )): resolves +// to the target method - param + return. +$container = new Fake_Container(); +add_filter( 'render_block', $container->callback( Hook_Test_Handlers::class, 'container_handler' ) ); + // Unknown/undefined global function - skipped. add_filter( 'wp_footer', 'some_undefined_handler' ); From 47b4c85fc56c7fbbe2f39bbf52ea5b13f793aa04 Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Fri, 24 Jul 2026 08:34:35 -0400 Subject: [PATCH 06/17] docs: note container-callback support in the capability table --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 9554caf..f0a25db 100644 --- a/README.md +++ b/README.md @@ -123,7 +123,7 @@ public function filter_the_value( $value, $post_id ) { | | PHPCS sniff (`StellarWP.Hooks.HookHandlerTypes`) | PHPStan rule | |---|---|---| | Runs in | phpcs (fast, in-editor) | phpstan (whole codebase, never diff-limited) | -| Covers | same-file handlers only (inline closures/arrows, `[ $this, 'method' ]` / `[ self::class, 'method' ]`, and `'function_name'`) with literal hook names | all callback forms across files (via reflection) and hook names that type inference narrows to constant string(s) | +| Covers | same-file handlers only (inline closures/arrows, `[ $this, 'method' ]` / `[ self::class, 'method' ]`, and `'function_name'`) with literal hook names | all callback forms across files (via reflection), including `$container->callback( Class::class, 'method' )`, and hook names that type inference narrows to constant string(s) | | Auto-fix | yes (`phpcbf` strips the offending native types) | no (report-only; the message names the exact handler) | Run both: the sniff gives instant, auto-fixable feedback for the common From 4f514874c3c5d298c661931c18ef85d0f5771237 Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Fri, 24 Jul 2026 08:47:29 -0400 Subject: [PATCH 07/17] feat: resolve hook-registration wrapper methods in the PHPStan rule - 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 --- README.md | 2 +- StellarWP/PHPStan/HookHandlerTypesRule.php | 102 ++++++++++++++++-- .../PHPStan/HookHandlerTypesRuleTest.php | 5 + StellarWP/Tests/PHPStan/data/handlers.php | 20 ++++ .../Tests/PHPStan/data/hook-handler-types.php | 6 ++ 5 files changed, 128 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index f0a25db..1f6ce11 100644 --- a/README.md +++ b/README.md @@ -123,7 +123,7 @@ public function filter_the_value( $value, $post_id ) { | | PHPCS sniff (`StellarWP.Hooks.HookHandlerTypes`) | PHPStan rule | |---|---|---| | Runs in | phpcs (fast, in-editor) | phpstan (whole codebase, never diff-limited) | -| Covers | same-file handlers only (inline closures/arrows, `[ $this, 'method' ]` / `[ self::class, 'method' ]`, and `'function_name'`) with literal hook names | all callback forms across files (via reflection), including `$container->callback( Class::class, 'method' )`, and hook names that type inference narrows to constant string(s) | +| Covers | same-file handlers only (inline closures/arrows, `[ $this, 'method' ]` / `[ self::class, 'method' ]`, and `'function_name'`) with literal hook names | all callback forms across files (via reflection), including `$container->callback( Class::class, 'method' )` and wrapper methods like `$this->add_action( 'tag', 'method' )`, plus hook names that type inference narrows to constant string(s) | | Auto-fix | yes (`phpcbf` strips the offending native types) | no (report-only; the message names the exact handler) | Run both: the sniff gives instant, auto-fixable feedback for the common diff --git a/StellarWP/PHPStan/HookHandlerTypesRule.php b/StellarWP/PHPStan/HookHandlerTypesRule.php index cceeb6c..53af393 100644 --- a/StellarWP/PHPStan/HookHandlerTypesRule.php +++ b/StellarWP/PHPStan/HookHandlerTypesRule.php @@ -17,8 +17,10 @@ * across files, it catches the cases the sniff cannot: a handler whose * declaration lives in a different file from the add_action()/add_filter() call, * hook names that are not literal strings but which type inference can narrow to - * constant string(s), and container callbacks - * (`$container->callback( Class::class, 'method' )`). + * constant string(s), container callbacks + * (`$container->callback( Class::class, 'method' )`), and hook-registration + * wrapper methods (`$receiver->add_action( 'tag', 'method' )`, where the second + * argument is a method on the receiver). * * @package StellarWP\CodingStandards */ @@ -28,6 +30,7 @@ use PhpParser\Node; use PhpParser\Node\Expr\ArrowFunction; use PhpParser\Node\Expr\Array_; +use PhpParser\Node\Expr\CallLike; use PhpParser\Node\Expr\Closure; use PhpParser\Node\Expr\FuncCall; use PhpParser\Node\Expr\MethodCall; @@ -46,7 +49,7 @@ use PHPStan\Type\VerbosityLevel; /** - * @implements Rule + * @implements Rule */ class HookHandlerTypesRule implements Rule { @@ -72,15 +75,35 @@ public function __construct( ReflectionProvider $reflection_provider, bool $allo } public function getNodeType(): string { - return FuncCall::class; + return CallLike::class; } /** - * @param FuncCall $node - * * @return array */ public function processNode( Node $node, Scope $scope ): array { + // WordPress add_action()/add_filter() calls. + if ( $node instanceof FuncCall ) { + return $this->process_wp_call( $node, $scope ); + } + + // Wrapper methods that forward to add_action()/add_filter(), where the + // second argument is the name of a method on the receiver, resolved to + // [ $receiver, 'method' ] (e.g. memberdash's MS_Hooker: + // $this->add_action( 'tag', 'method' )). + if ( $node instanceof MethodCall ) { + return $this->process_wrapper_call( $node, $scope ); + } + + return []; + } + + /** + * Handles a WordPress add_action()/add_filter() function call. + * + * @return array + */ + private function process_wp_call( FuncCall $node, Scope $scope ): array { if ( ! $node->name instanceof Name ) { return []; } @@ -106,6 +129,73 @@ public function processNode( Node $node, Scope $scope ): array { return $this->check_callback( $args[1]->value, $scope, $constant_strings[0]->getValue(), $function === 'add_filter' ); } + /** + * Handles a hook-registration wrapper method: `$receiver->add_action( 'tag', + * 'method' )` / `->add_filter( ... )` whose second argument is the name of a + * method on the receiver (the handler is [ $receiver, 'method' ]; when the + * second argument is absent or empty the hook name is used as the method + * name). Only acts when that method actually exists on the receiver, so an + * unrelated method named add_action()/add_filter() is ignored. + * + * @return array + */ + private function process_wrapper_call( MethodCall $node, Scope $scope ): array { + if ( ! $node->name instanceof Node\Identifier ) { + return []; + } + + $method = strtolower( $node->name->name ); + if ( $method !== 'add_action' && $method !== 'add_filter' ) { + return []; + } + + $args = $node->getArgs(); + if ( $args === [] ) { + return []; + } + + $hook_strings = $scope->getType( $args[0]->value )->getConstantStrings(); + if ( $hook_strings === [] ) { + return []; + } + $hook = $hook_strings[0]->getValue(); + + // The handler method name is the second argument, falling back to the hook + // name when it is absent or an empty string. A dynamic second argument + // cannot be resolved. + $method_names = []; + if ( isset( $args[1] ) ) { + $method_strings = $scope->getType( $args[1]->value )->getConstantStrings(); + if ( $method_strings === [] ) { + return []; + } + + foreach ( $method_strings as $method_string ) { + $method_names[ $method_string->getValue() === '' ? $hook : $method_string->getValue() ] = true; + } + } else { + $method_names[ $hook ] = true; + } + + $class_names = []; + foreach ( $scope->getType( $node->var )->getObjectClassReflections() as $class_reflection ) { + $class_names[ $class_reflection->getName() ] = true; + } + + $is_filter = $method === 'add_filter'; + $errors = []; + foreach ( array_keys( $method_names ) as $method_name ) { + foreach ( array_keys( $class_names ) as $class_name ) { + $errors = array_merge( + $errors, + $this->check_class_method( $class_name, $method_name, $hook, $is_filter, $node->getStartLine() ) + ); + } + } + + return $errors; + } + /** * Dispatches to the appropriate resolver for the callback expression. * diff --git a/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php index 7fd0903..11a5a28 100644 --- a/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php +++ b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php @@ -85,6 +85,11 @@ public function testRule(): void { // Container callback: $container->callback( Class::class, 'method' ). [ 'Handler Hook_Test_Handlers::container_handler() for filter "render_block" must not declare the native type "int" on parameter $value; ' . $param, 48 ], [ 'Handler Hook_Test_Handlers::container_handler() for filter "render_block" must not declare a native return type ("int"); ' . $return, 48 ], + + // Wrapper method: $receiver->add_action( 'tag', 'method' ). + [ 'Handler Wrapper_Subclass::on_save() for action "save_post" must not declare the native type "int" on parameter $post_id; ' . $param, 53 ], + [ 'Handler Wrapper_Subclass::filter_it() for filter "the_content" must not declare the native type "string" on parameter $content; ' . $param, 54 ], + [ 'Handler Wrapper_Subclass::filter_it() for filter "the_content" must not declare a native return type ("string"); ' . $return, 54 ], ] ); } diff --git a/StellarWP/Tests/PHPStan/data/handlers.php b/StellarWP/Tests/PHPStan/data/handlers.php index ea9da49..7e5e61e 100644 --- a/StellarWP/Tests/PHPStan/data/handlers.php +++ b/StellarWP/Tests/PHPStan/data/handlers.php @@ -41,6 +41,26 @@ public function callback( $id, $method ) { } } +class Fake_Registrar { + + public function add_action( $tag, $method = '', $priority = 10, $accepted_args = 1 ) { + add_action( $tag, array( $this, '' === $method ? $tag : $method ), $priority, $accepted_args ); + } + + public function add_filter( $tag, $method = '', $priority = 10, $accepted_args = 1 ) { + add_filter( $tag, array( $this, '' === $method ? $tag : $method ), $priority, $accepted_args ); + } +} + +class Wrapper_Subclass extends Fake_Registrar { + + public function on_save( int $post_id ): void {} + + public function filter_it( string $content ): string { + return $content; + } +} + function global_filter( string $length ): string { return $length; } diff --git a/StellarWP/Tests/PHPStan/data/hook-handler-types.php b/StellarWP/Tests/PHPStan/data/hook-handler-types.php index 63b7cf1..a608c22 100644 --- a/StellarWP/Tests/PHPStan/data/hook-handler-types.php +++ b/StellarWP/Tests/PHPStan/data/hook-handler-types.php @@ -47,5 +47,11 @@ $container = new Fake_Container(); add_filter( 'render_block', $container->callback( Hook_Test_Handlers::class, 'container_handler' ) ); +// Hook-registration wrapper methods ($receiver->add_action( 'tag', 'method' )) +// where the second argument is a method name on the receiver. +$registrar = new Wrapper_Subclass(); +$registrar->add_action( 'save_post', 'on_save' ); +$registrar->add_filter( 'the_content', 'filter_it' ); + // Unknown/undefined global function - skipped. add_filter( 'wp_footer', 'some_undefined_handler' ); From 6cbe5785b2bb2b1382a9f22351510ae580a9d9fd Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Fri, 24 Jul 2026 08:57:38 -0400 Subject: [PATCH 08/17] feat: resolve $this->add_action/add_filter wrappers in the sniff - 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) --- README.md | 2 +- .../Sniffs/Hooks/HookHandlerTypesSniff.php | 94 ++++++++++++++----- .../Tests/Hooks/HookHandlerTypesUnitTest.inc | 10 ++ .../Hooks/HookHandlerTypesUnitTest.inc.fixed | 10 ++ .../Tests/Hooks/HookHandlerTypesUnitTest.php | 18 ++-- 5 files changed, 103 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index 1f6ce11..6e29207 100644 --- a/README.md +++ b/README.md @@ -123,7 +123,7 @@ public function filter_the_value( $value, $post_id ) { | | PHPCS sniff (`StellarWP.Hooks.HookHandlerTypes`) | PHPStan rule | |---|---|---| | Runs in | phpcs (fast, in-editor) | phpstan (whole codebase, never diff-limited) | -| Covers | same-file handlers only (inline closures/arrows, `[ $this, 'method' ]` / `[ self::class, 'method' ]`, and `'function_name'`) with literal hook names | all callback forms across files (via reflection), including `$container->callback( Class::class, 'method' )` and wrapper methods like `$this->add_action( 'tag', 'method' )`, plus hook names that type inference narrows to constant string(s) | +| Covers | same-file handlers only (inline closures/arrows, `[ $this, 'method' ]` / `[ self::class, 'method' ]`, `'function_name'`, and `$this->add_action( 'tag', 'method' )` wrappers) with literal hook names | all callback forms across files (via reflection), including `$container->callback( Class::class, 'method' )` and wrapper methods like `$this->add_action( 'tag', 'method' )`, plus hook names that type inference narrows to constant string(s) | | Auto-fix | yes (`phpcbf` strips the offending native types) | no (report-only; the message names the exact handler) | Run both: the sniff gives instant, auto-fixable feedback for the common diff --git a/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php b/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php index 8ef10bc..b3e599e 100644 --- a/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php +++ b/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php @@ -14,13 +14,15 @@ * - Action handlers: no native type on ANY parameter; a native `void` return * type is allowed (actions always return void). * - * This sniff analyses at the call site (add_action()/add_filter()) and can only - * resolve handlers that live in the same file: inline closures and arrow - * functions, same-file [ $this, 'method' ] / [ self::class, 'method' ] references, - * and same-file 'function_name' global-function handlers. It also only understands - * literal hook names. Cross-file callbacks and non-literal hook names are left to - * the companion PHPStan rule, which resolves callbacks across files via reflection - * and resolves hook names via type inference (constant-string types). + * This sniff analyses at the call site and can only resolve handlers that live in + * the same file: inline closures and arrow functions, same-file [ $this, 'method' ] + * / [ self::class, 'method' ] references, same-file 'function_name' global-function + * handlers, and $this->add_action( 'tag', 'method' ) wrapper calls whose handler + * method is on the enclosing class (e.g. memberdash's MS_Hooker). It also only + * understands literal hook names. Cross-file callbacks and non-literal hook names + * are left to the companion PHPStan rule, which resolves callbacks across files + * via reflection and resolves hook names via type inference (constant-string + * types). * * @package StellarWP\CodingStandards */ @@ -90,18 +92,28 @@ public function process( File $phpcs_file, $stack_ptr ): void { return; } - // Ensure this is a direct function call, not a method call or a definition. - $prev = $phpcs_file->findPrevious( Tokens::$emptyTokens, $stack_ptr - 1, null, true ); + // Determine the call form. A global add_action()/add_filter() call takes a + // callback as its second argument. A $this->add_action( 'tag', 'method' ) + // wrapper (e.g. memberdash's MS_Hooker) takes a method name that resolves + // to a method on the enclosing class. Other forms (another object, ::, or a + // definition) are not handled here. + $prev = $phpcs_file->findPrevious( Tokens::$emptyTokens, $stack_ptr - 1, null, true ); + $is_wrapper = false; + if ( $prev !== false ) { - $skip_before = [ - T_OBJECT_OPERATOR, - T_NULLSAFE_OBJECT_OPERATOR, - T_DOUBLE_COLON, - T_FUNCTION, - T_NEW, - ]; + if ( in_array( $tokens[ $prev ]['code'], [ T_OBJECT_OPERATOR, T_NULLSAFE_OBJECT_OPERATOR ], true ) ) { + // Only $this-> wrappers can be resolved (and fixed) within this file. + $receiver = $phpcs_file->findPrevious( Tokens::$emptyTokens, $prev - 1, null, true ); + if ( + $receiver === false + || $tokens[ $receiver ]['code'] !== T_VARIABLE + || strtolower( $tokens[ $receiver ]['content'] ) !== '$this' + ) { + return; + } - if ( in_array( $tokens[ $prev ]['code'], $skip_before, true ) ) { + $is_wrapper = true; + } elseif ( in_array( $tokens[ $prev ]['code'], [ T_DOUBLE_COLON, T_FUNCTION, T_NEW ], true ) ) { return; } } @@ -116,9 +128,9 @@ public function process( File $phpcs_file, $stack_ptr ): void { } $close_paren = $tokens[ $open_paren ]['parenthesis_closer']; + $args = $this->split_arguments( $phpcs_file, $open_paren, $close_paren ); - $args = $this->split_arguments( $phpcs_file, $open_paren, $close_paren ); - if ( count( $args ) < 2 ) { + if ( $args === [] ) { return; } @@ -138,16 +150,54 @@ public function process( File $phpcs_file, $stack_ptr ): void { $is_filter = self::HOOK_FUNCTIONS[ $content ]; - // Argument 2: the callback. Resolve to a function/closure/method token. - $func_ptr = $this->resolve_handler( $phpcs_file, $args[1], $stack_ptr ); + if ( $is_wrapper ) { + $func_ptr = $this->resolve_wrapper_handler( $phpcs_file, $args, $hook_name, $stack_ptr ); + } elseif ( isset( $args[1] ) ) { + // Argument 2: the callback. Resolve to a function/closure/method token. + $func_ptr = $this->resolve_handler( $phpcs_file, $args[1], $stack_ptr ); + } else { + return; + } + if ( $func_ptr === null ) { - // Cross-file, global-function, or dynamic callback: left to the PHPStan rule. + // Cross-file, global-function, or dynamic handler: left to the PHPStan rule. return; } $this->check_handler_types( $phpcs_file, $func_ptr, $hook_name, $is_filter ); } + /** + * Resolves the handler method for a $this->add_action( 'tag', 'method' ) + * wrapper call. The method name is the second argument, falling back to the + * hook name when it is absent or an empty string (per MS_Hooker). Returns the + * method token in the enclosing class, or null when it cannot be resolved. + * + * @param File $phpcs_file The file being scanned. + * @param array $args The call arguments. + * @param string $hook_name The resolved hook name. + * @param int $stack_ptr The call token position. + * + * @return int|null + */ + private function resolve_wrapper_handler( File $phpcs_file, array $args, string $hook_name, int $stack_ptr ): ?int { + $method = $hook_name; + + if ( isset( $args[1] ) ) { + $argument = $this->get_string_argument( $phpcs_file, $args[1] ); + if ( $argument === null ) { + // Dynamic method name - cannot be resolved within this file. + return null; + } + + if ( $argument !== '' ) { + $method = $argument; + } + } + + return $this->find_class_method( $phpcs_file, $stack_ptr, $method ); + } + /** * Splits the contents of a parenthesis or bracket pair into top-level, * comma-separated argument token ranges. diff --git a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc index c908a31..1c41701 100644 --- a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc +++ b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc @@ -41,6 +41,10 @@ class Example { // Cross-file callback - left to the PHPStan rule. add_filter( 'get_the_excerpt', 'some_cross_file_handler' ); + + // Wrapper method calls on $this: the handler is a method on this class. + $this->add_action( 'save_post', 'wrapper_action' ); + $this->add_filter( 'the_title', 'wrapper_filter' ); } public function filter_method( string $content ): string { @@ -60,6 +64,12 @@ class Example { public static function static_filter( string $title ): string { return $title; } + + public function wrapper_action( int $id ): void {} + + public function wrapper_filter( string $title ): string { + return $title; + } } function global_filter( string $length ): string { diff --git a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed index 301053b..9a7c700 100644 --- a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed +++ b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed @@ -41,6 +41,10 @@ class Example { // Cross-file callback - left to the PHPStan rule. add_filter( 'get_the_excerpt', 'some_cross_file_handler' ); + + // Wrapper method calls on $this: the handler is a method on this class. + $this->add_action( 'save_post', 'wrapper_action' ); + $this->add_filter( 'the_title', 'wrapper_filter' ); } public function filter_method( $content ) { @@ -60,6 +64,12 @@ class Example { public static function static_filter( $title ) { return $title; } + + public function wrapper_action( $id ): void {} + + public function wrapper_filter( $title ) { + return $title; + } } function global_filter( $length ) { diff --git a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.php b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.php index 3c956aa..80446ae 100644 --- a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.php +++ b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.php @@ -22,15 +22,17 @@ class HookHandlerTypesUnitTest extends AbstractSniffUnitTest { protected function getErrorList() { return [ 19 => 1, // init action closure: param; void return allowed. - 22 => 3, // ld_valid filter closure: two params + return. + 22 => 3, // login_redirect filter closure: two params + return. 27 => 3, // pre_get_posts action closure: nullable, by-ref, variadic params. - 46 => 2, // filter_method(): param + return. - 50 => 2, // action_method(): two params; void return allowed. - 52 => 4, // filter_context_method(): all three params + return. - 56 => 1, // action_typed(): the native param (an action still forbids param types). - 58 => 1, // ref_action() (via [ &$this, ... ]): param. - 60 => 2, // static_filter() (via self::class): param + return. - 65 => 2, // global_filter() (global function): param + return. + 50 => 2, // filter_method(): param + return. + 54 => 2, // action_method(): two params; void return allowed. + 56 => 4, // filter_context_method(): all three params + return. + 60 => 1, // action_typed(): the native param (an action still forbids param types). + 62 => 1, // ref_action() (via [ &$this, ... ]): param. + 64 => 2, // static_filter() (via self::class): param + return. + 68 => 1, // wrapper_action() (via $this->add_action( 'save_post', 'wrapper_action' )): param. + 70 => 2, // wrapper_filter() (via $this->add_filter( 'the_title', 'wrapper_filter' )): param + return. + 75 => 2, // global_filter() (global function): param + return. ]; } From ed358ca97e17b15db96e8fd217d3c1ea8e4d6df4 Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Fri, 24 Jul 2026 09:01:26 -0400 Subject: [PATCH 09/17] ci: add GitHub Actions workflow running composer test across PHP 7.4-8.4 --- .github/workflows/continuous-integration.yml | 51 ++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 .github/workflows/continuous-integration.yml diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml new file mode 100644 index 0000000..f55e431 --- /dev/null +++ b/.github/workflows/continuous-integration.yml @@ -0,0 +1,51 @@ +# Continuous Integration for stellarwp/coding-standards. +# +# Exercises the standard's tests on every push to main and every pull request: +# validates composer.json, confirms the StellarWP ruleset loads, and runs the +# PHPUnit suite (the HookHandlerTypes sniff and PHPStan rule tests). +name: CI + +on: + push: + branches: + - main + pull_request: + +jobs: + test: + name: Test (PHP ${{ matrix.php-version }}) + runs-on: ubuntu-latest + + strategy: + fail-fast: false + matrix: + # PHPUnit 9.6 and PHPStan 1.x support this range. The PHPStan rule test + # skips on 7.4 (a php-parser emulation quirk in the test harness only); + # the sniff test runs on every version. + php-version: + - '7.4' + - '8.0' + - '8.1' + - '8.2' + - '8.3' + - '8.4' + + steps: + - name: Check out the code + uses: actions/checkout@v4 + + - name: Configure PHP ${{ matrix.php-version }} + uses: shivammathur/setup-php@v2 + with: + php-version: ${{ matrix.php-version }} + extensions: mbstring + coverage: none + + - name: Validate composer.json + run: composer validate --no-check-lock + + - name: Install dependencies + uses: ramsey/composer-install@v3 + + - name: Run tests (ruleset explain + PHPUnit) + run: composer test From 0d990ed6647179d2661395c2beb4b86cff5364e8 Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Fri, 24 Jul 2026 09:03:18 -0400 Subject: [PATCH 10/17] ci: allow manual workflow_dispatch runs --- .github/workflows/continuous-integration.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index f55e431..d8dc54f 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -10,6 +10,7 @@ on: branches: - main pull_request: + workflow_dispatch: jobs: test: From 6cd789a09889ab0cc48af63b86c48926ae6128d9 Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Fri, 24 Jul 2026 09:10:55 -0400 Subject: [PATCH 11/17] ci: run PHPUnit suite directly with --testdox; drop workflow_dispatch --- .github/workflows/continuous-integration.yml | 14 ++++++++------ composer.json | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index d8dc54f..a42e24b 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -1,8 +1,8 @@ # Continuous Integration for stellarwp/coding-standards. # -# Exercises the standard's tests on every push to main and every pull request: -# validates composer.json, confirms the StellarWP ruleset loads, and runs the -# PHPUnit suite (the HookHandlerTypes sniff and PHPStan rule tests). +# On every push to main and every pull request: validates composer.json and runs +# the PHPUnit suite (the HookHandlerTypes sniff and PHPStan rule tests) across a +# range of PHP versions. name: CI on: @@ -10,7 +10,6 @@ on: branches: - main pull_request: - workflow_dispatch: jobs: test: @@ -48,5 +47,8 @@ jobs: - name: Install dependencies uses: ramsey/composer-install@v3 - - name: Run tests (ruleset explain + PHPUnit) - run: composer test + # Runs the sniff (AbstractSniffUnitTest) and PHPStan rule (RuleTestCase) + # tests. The sniff test constructs the full StellarWP ruleset, so a broken + # ruleset fails here too. --testdox lists each test and its result. + - name: Run the test suite + run: composer phpunit diff --git a/composer.json b/composer.json index 44fc913..0587e68 100644 --- a/composer.json +++ b/composer.json @@ -55,7 +55,7 @@ "install-codestandards": "Dealerdirect\\Composer\\Plugin\\Installers\\PHPCodeSniffer\\Plugin::run", "ruleset": "vendor/bin/phpcs --standard=StellarWP -e", "phpcs": "bin/phpcs", - "phpunit": "vendor/bin/phpunit", + "phpunit": "vendor/bin/phpunit --testdox", "test": [ "@ruleset", "@phpunit" From 2268a84da373f889e4d3f5d084de1c852b927607 Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Fri, 24 Jul 2026 09:17:00 -0400 Subject: [PATCH 12/17] ci: bump checkout to v7 and composer-install to v4 (Node 24) --- .github/workflows/continuous-integration.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index a42e24b..7bf6ca5 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -32,7 +32,7 @@ jobs: steps: - name: Check out the code - uses: actions/checkout@v4 + uses: actions/checkout@v7 - name: Configure PHP ${{ matrix.php-version }} uses: shivammathur/setup-php@v2 @@ -45,7 +45,7 @@ jobs: run: composer validate --no-check-lock - name: Install dependencies - uses: ramsey/composer-install@v3 + uses: ramsey/composer-install@v4 # Runs the sniff (AbstractSniffUnitTest) and PHPStan rule (RuleTestCase) # tests. The sniff test constructs the full StellarWP ruleset, so a broken From 0d28d13f9b2f15191c83efc7544f7e091c86221b Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Fri, 24 Jul 2026 09:27:53 -0400 Subject: [PATCH 13/17] docs: note PHPStan 2.x (php-parser 5.x) would remove the 7.4 test skip 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. --- .../Tests/PHPStan/HookHandlerTypesRuleTest.php | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php index 11a5a28..d596f6c 100644 --- a/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php +++ b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php @@ -31,12 +31,15 @@ protected function getRule(): Rule { } public function testRule(): void { - // This skip is a limitation of the RuleTestCase harness only: PHPStan's - // bundled php-parser hits a token-emulation bug when parsing fixtures on - // the PHP 7.4 runtime. The rule itself is PHP 7.4-compatible and runs - // correctly under a normal `phpstan analyse` on 7.4. + // 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 ) { - $this->markTestSkipped( 'Skipped on the PHP 7.4 runtime: PHPStan\'s RuleTestCase php-parser emulation is unreliable here. The rule works on 7.4 under a normal phpstan analyse.' ); + $this->markTestSkipped( 'Skipped on the PHP 7.4 runtime: PHPStan 1.x\'s bundled php-parser 4.x has a token-emulation bug here. PHPStan 2.x (php-parser 5.x) fixes it, but consumers are still on PHPStan 1.x. The rule works on 7.4 under a normal phpstan analyse.' ); } $param = 'hook arguments are not type-guaranteed (a hook can be dispatched with unexpected types, including null), so a native type can cause a fatal error.'; From b04547e57eade4b3f363779ed5bff9ed15d54c2b Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Fri, 24 Jul 2026 09:53:55 -0400 Subject: [PATCH 14/17] fix: report but do not auto-fix wrapper-resolved hook handlers 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. --- README.md | 26 +++++++++- StellarWP/PHPStan/HookHandlerTypesRule.php | 9 +++- .../Sniffs/Hooks/HookHandlerTypesSniff.php | 51 ++++++++++++------- .../Hooks/HookHandlerTypesUnitTest.inc.fixed | 4 +- 4 files changed, 68 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 6e29207..01ee4d5 100644 --- a/README.md +++ b/README.md @@ -124,7 +124,7 @@ public function filter_the_value( $value, $post_id ) { |---|---|---| | Runs in | phpcs (fast, in-editor) | phpstan (whole codebase, never diff-limited) | | Covers | same-file handlers only (inline closures/arrows, `[ $this, 'method' ]` / `[ self::class, 'method' ]`, `'function_name'`, and `$this->add_action( 'tag', 'method' )` wrappers) with literal hook names | all callback forms across files (via reflection), including `$container->callback( Class::class, 'method' )` and wrapper methods like `$this->add_action( 'tag', 'method' )`, plus hook names that type inference narrows to constant string(s) | -| Auto-fix | yes (`phpcbf` strips the offending native types) | no (report-only; the message names the exact handler) | +| Auto-fix | yes (`phpcbf` strips the offending native types), except `$this->add_action()` wrapper handlers, which are reported but not auto-fixed (see below) | no (report-only; the message names the exact handler) | Run both: the sniff gives instant, auto-fixable feedback for the common same-file case, while the PHPStan rule is the authoritative gate that catches @@ -143,6 +143,30 @@ configuration. Enable just this sniff with: An optional `allow_void_return_on_actions` property (default `true`) controls whether a native `void` return type is permitted on action handlers. +#### Wrapper handlers are reported but not auto-fixed + +Handlers registered through a `$this->add_action( 'tag', 'method' )` wrapper (for +example memberdash's `MS_Hooker`, where the second argument names a method on the +enclosing class and falls back to the hook name when it is absent or empty) are +resolved with a name-match heuristic: the sniff finds the enclosing-class method +whose name matches that argument. That heuristic is safe enough to **report** on, +but not to **auto-fix** - in rare cases the matched method may not actually be a +hook handler (for instance a class that defines its own unrelated +`add_action()` / `add_filter()` method alongside a coincidentally-named method), +and stripping native types is destructive. + +So for the wrapper form the sniff raises the violation without a fix: it still +fails CI, but `phpcbf` will not touch it. Remove the native types by hand, or, if +it is a false positive, silence it at the handler with an ignore annotation: + +```php +// phpcs:ignore StellarWP.Hooks.HookHandlerTypes.NativeParameterType +public function my_handler( int $post_id ): void {} +``` + +Direct callbacks (`[ $this, 'method' ]`, closures, arrow functions, same-file +global functions) are unambiguous and remain auto-fixable as usual. + ### PHPStan rule The rule ships as an auto-discovered PHPStan extension. Projects using diff --git a/StellarWP/PHPStan/HookHandlerTypesRule.php b/StellarWP/PHPStan/HookHandlerTypesRule.php index 53af393..9d70245 100644 --- a/StellarWP/PHPStan/HookHandlerTypesRule.php +++ b/StellarWP/PHPStan/HookHandlerTypesRule.php @@ -487,13 +487,20 @@ private function return_type_error( string $hook, bool $is_filter, string $retur $hook, $return_type ); - } else { + } elseif ( $this->allow_void_return_on_actions ) { $message = sprintf( 'Handler %sfor action "%s" must not declare a native return type ("%s") other than void.', $where, $hook, $return_type ); + } else { + $message = sprintf( + 'Handler %sfor action "%s" must not declare a native return type ("%s").', + $where, + $hook, + $return_type + ); } return RuleErrorBuilder::message( $message )->identifier( 'stellarwp.hookHandlerReturnType' )->line( $line )->build(); diff --git a/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php b/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php index b3e599e..fe31b2a 100644 --- a/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php +++ b/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php @@ -134,7 +134,8 @@ public function process( File $phpcs_file, $stack_ptr ): void { return; } - // Argument 1: the hook name (used only to name the hook in the message). + // Argument 1: the hook name. Used to name the hook in the message, and on + // the wrapper path it also serves as the fallback handler method name. $hook_name = $this->get_string_argument( $phpcs_file, $args[0] ); if ( $hook_name === null ) { if ( $this->warn_on_dynamic_hook_names ) { @@ -164,7 +165,11 @@ public function process( File $phpcs_file, $stack_ptr ): void { return; } - $this->check_handler_types( $phpcs_file, $func_ptr, $hook_name, $is_filter ); + // Wrapper handlers ($this->add_action( 'tag', 'method' )) are resolved via + // a name-match heuristic, so they are reported but never auto-fixed - + // stripping types off a coincidental match would be destructive. Direct + // callbacks are unambiguous and stay auto-fixable. + $this->check_handler_types( $phpcs_file, $func_ptr, $hook_name, $is_filter, ! $is_wrapper ); } /** @@ -566,17 +571,19 @@ private function find_class_method( File $phpcs_file, int $stack_ptr, string $me } /** - * Checks a resolved handler for disallowed native parameter and return types - * and fixes them by stripping only the offending type declarations. + * Checks a resolved handler for disallowed native parameter and return types. + * When $fixable is true the offending type declarations are stripped in place; + * otherwise the violation is reported without an auto-fix. * * @param File $phpcs_file The file being scanned. * @param int $func_ptr The function/closure/arrow token position. * @param string $hook_name The resolved hook name (for messaging). * @param bool $is_filter Whether the hook is a filter. + * @param bool $fixable Whether the violation may be auto-fixed. * * @return void */ - private function check_handler_types( File $phpcs_file, int $func_ptr, string $hook_name, bool $is_filter ): void { + private function check_handler_types( File $phpcs_file, int $func_ptr, string $hook_name, bool $is_filter, bool $fixable = true ): void { $hook_type = $is_filter ? 'filter' : 'action'; $params = $phpcs_file->getMethodParameters( $func_ptr ); @@ -585,12 +592,15 @@ private function check_handler_types( File $phpcs_file, int $func_ptr, string $h continue; } - $fix = $phpcs_file->addFixableError( - 'Handler for %s "%s" must not declare the native type "%s" on parameter %s; hook arguments are not type-guaranteed (a hook can be dispatched with unexpected types, including null), so a native type can cause a fatal error.', - $param['type_hint_token'], - 'NativeParameterType', - [ $hook_type, $hook_name, $param['type_hint'], $param['name'] ] - ); + $message = 'Handler for %s "%s" must not declare the native type "%s" on parameter %s; hook arguments are not type-guaranteed (a hook can be dispatched with unexpected types, including null), so a native type can cause a fatal error.'; + $data = [ $hook_type, $hook_name, $param['type_hint'], $param['name'] ]; + + if ( ! $fixable ) { + $phpcs_file->addError( $message, $param['type_hint_token'], 'NativeParameterType', $data ); + continue; + } + + $fix = $phpcs_file->addFixableError( $message, $param['type_hint_token'], 'NativeParameterType', $data ); if ( $fix === true ) { $this->remove_parameter_type( $phpcs_file, $param ); @@ -612,16 +622,21 @@ private function check_handler_types( File $phpcs_file, int $func_ptr, string $h if ( $is_filter ) { $message = 'Handler for filter "%s" must not declare a native return type ("%s"); filter return values are not type-guaranteed and a native return type can cause a fatal error.'; - } else { + } elseif ( $this->allow_void_return_on_actions ) { $message = 'Handler for action "%s" must not declare a native return type ("%s") other than void.'; + } else { + $message = 'Handler for action "%s" must not declare a native return type ("%s").'; + } + + $data = [ $hook_name, $return_type ]; + + if ( ! $fixable ) { + $phpcs_file->addError( $message, $props['return_type_token'], 'NativeReturnType', $data ); + + return; } - $fix = $phpcs_file->addFixableError( - $message, - $props['return_type_token'], - 'NativeReturnType', - [ $hook_name, $return_type ] - ); + $fix = $phpcs_file->addFixableError( $message, $props['return_type_token'], 'NativeReturnType', $data ); if ( $fix === true ) { $this->remove_return_type( $phpcs_file, $func_ptr, $props ); diff --git a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed index 9a7c700..ff9bfb0 100644 --- a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed +++ b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed @@ -65,9 +65,9 @@ class Example { return $title; } - public function wrapper_action( $id ): void {} + public function wrapper_action( int $id ): void {} - public function wrapper_filter( $title ) { + public function wrapper_filter( string $title ): string { return $title; } } From 28b21f570a1b2c898e8110c27c392193917a68e4 Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Fri, 24 Jul 2026 10:14:54 -0400 Subject: [PATCH 15/17] docs: complete param/return docblocks and scope the phpcs suppression 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. --- StellarWP/PHPStan/HookHandlerTypesRule.php | 110 +++++++++++++++--- .../Sniffs/Hooks/HookHandlerTypesSniff.php | 8 +- 2 files changed, 96 insertions(+), 22 deletions(-) diff --git a/StellarWP/PHPStan/HookHandlerTypesRule.php b/StellarWP/PHPStan/HookHandlerTypesRule.php index 9d70245..67f0647 100644 --- a/StellarWP/PHPStan/HookHandlerTypesRule.php +++ b/StellarWP/PHPStan/HookHandlerTypesRule.php @@ -43,10 +43,13 @@ use PHPStan\Reflection\ParametersAcceptorWithPhpDocs; use PHPStan\Reflection\ReflectionProvider; use PHPStan\Rules\Rule; +use PHPStan\Rules\RuleError; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Type\MixedType; use PHPStan\Type\Type; use PHPStan\Type\VerbosityLevel; +use ReflectionNamedType; +use ReflectionType; /** * @implements Rule @@ -74,12 +77,22 @@ public function __construct( ReflectionProvider $reflection_provider, bool $allo $this->allow_void_return_on_actions = $allow_void_return_on_actions; } + /** + * The node type this rule listens for. + * + * @return string + */ public function getNodeType(): string { return CallLike::class; } /** - * @return array + * Dispatches a call node to the WordPress-call or wrapper-call handler. + * + * @param Node $node The call node being analysed. + * @param Scope $scope The current analysis scope. + * + * @return array */ public function processNode( Node $node, Scope $scope ): array { // WordPress add_action()/add_filter() calls. @@ -101,7 +114,10 @@ public function processNode( Node $node, Scope $scope ): array { /** * Handles a WordPress add_action()/add_filter() function call. * - * @return array + * @param FuncCall $node The function-call node. + * @param Scope $scope The current analysis scope. + * + * @return array */ private function process_wp_call( FuncCall $node, Scope $scope ): array { if ( ! $node->name instanceof Name ) { @@ -137,7 +153,10 @@ private function process_wp_call( FuncCall $node, Scope $scope ): array { * name). Only acts when that method actually exists on the receiver, so an * unrelated method named add_action()/add_filter() is ignored. * - * @return array + * @param MethodCall $node The method-call node. + * @param Scope $scope The current analysis scope. + * + * @return array */ private function process_wrapper_call( MethodCall $node, Scope $scope ): array { if ( ! $node->name instanceof Node\Identifier ) { @@ -199,7 +218,12 @@ private function process_wrapper_call( MethodCall $node, Scope $scope ): array { /** * Dispatches to the appropriate resolver for the callback expression. * - * @return array + * @param Node $callback The callback argument node. + * @param Scope $scope The current analysis scope. + * @param string $hook The resolved hook name. + * @param bool $is_filter Whether the hook is a filter. + * + * @return array */ private function check_callback( Node $callback, Scope $scope, string $hook, bool $is_filter ): array { if ( $callback instanceof Closure || $callback instanceof ArrowFunction ) { @@ -241,9 +265,12 @@ private function check_callback( Node $callback, Scope $scope, string $hook, boo * acts when the arguments actually resolve to a real class method, so an * unrelated `->callback()` is harmlessly ignored. * - * @param MethodCall|StaticCall $callback + * @param MethodCall|StaticCall $callback The container `callback()` call node. + * @param Scope $scope The current analysis scope. + * @param string $hook The resolved hook name. + * @param bool $is_filter Whether the hook is a filter. * - * @return array + * @return array */ private function check_container_callback( Node $callback, Scope $scope, string $hook, bool $is_filter ): array { $args = $callback->getArgs(); @@ -277,9 +304,11 @@ private function check_container_callback( Node $callback, Scope $scope, string /** * Checks an inline closure or arrow function using its declared native types. * - * @param Closure|ArrowFunction $node + * @param Closure|ArrowFunction $node The closure or arrow-function node. + * @param string $hook The resolved hook name. + * @param bool $is_filter Whether the hook is a filter. * - * @return array + * @return array */ private function check_closure_node( Node $node, string $hook, bool $is_filter ): array { $errors = []; @@ -309,7 +338,12 @@ private function check_closure_node( Node $node, string $hook, bool $is_filter ) * Resolves an array callback ([ $this, 'method' ], [ Foo::class, 'method' ], * [ 'Foo', 'method' ]) to the handler class(es) and checks the method. * - * @return array + * @param Array_ $callback The array-callback node. + * @param Scope $scope The current analysis scope. + * @param string $hook The resolved hook name. + * @param bool $is_filter Whether the hook is a filter. + * + * @return array */ private function check_array_callback( Array_ $callback, Scope $scope, string $hook, bool $is_filter ): array { if ( count( $callback->items ) < 2 || $callback->items[0] === null || $callback->items[1] === null ) { @@ -346,7 +380,13 @@ private function check_array_callback( Array_ $callback, Scope $scope, string $h /** * Checks a resolved class method's native parameter and return types. * - * @return array + * @param string $class_name The handler class name. + * @param string $method The handler method name. + * @param string $hook The resolved hook name. + * @param bool $is_filter Whether the hook is a filter. + * @param int $line The line to report the violation on. + * + * @return array */ private function check_class_method( string $class_name, string $method, string $hook, bool $is_filter, int $line ): array { $class_name = ltrim( $class_name, '\\' ); @@ -394,7 +434,13 @@ private function check_class_method( string $class_name, string $method, string * Checks a global-function-name callback ('my_function') using PHPStan's * static reflection, which knows project functions without loading them. * - * @return array + * @param string $name The global function name. + * @param Scope $scope The current analysis scope. + * @param string $hook The resolved hook name. + * @param bool $is_filter Whether the hook is a filter. + * @param int $line The line to report the violation on. + * + * @return array */ private function check_global_function( string $name, Scope $scope, string $hook, bool $is_filter, int $line ): array { $function_name = new Name( ltrim( $name, '\\' ) ); @@ -444,7 +490,11 @@ private function check_global_function( string $name, Scope $scope, string $hook /** * Whether a native type is actually declared. An undeclared type is an * implicit `mixed`; an explicit `mixed` counts as a declared native type, - * matching \ReflectionParameter::hasType() semantics used for methods. + * matching ReflectionParameter::hasType() semantics used for methods. + * + * @param Type $type The native type to inspect. + * + * @return bool */ private function has_native_type( Type $type ): bool { return ! ( $type instanceof MixedType && ! $type->isExplicitMixed() ); @@ -453,9 +503,16 @@ private function has_native_type( Type $type ): bool { /** * Builds a parameter-type violation error. * - * @return \PHPStan\Rules\RuleError + * @param bool $is_filter Whether the hook is a filter. + * @param string $handler The handler label (e.g. `Foo::bar()`), or '' for closures. + * @param string $hook The resolved hook name. + * @param string $type The offending native type. + * @param string $param_name The parameter name, or '' when unknown. + * @param int $line The line to report the violation on. + * + * @return RuleError */ - private function param_error( bool $is_filter, string $handler, string $hook, string $type, string $param_name, int $line ) { + private function param_error( bool $is_filter, string $handler, string $hook, string $type, string $param_name, int $line ): RuleError { $where = $handler === '' ? '' : $handler . ' '; $subject = $param_name === '' ? 'a parameter' : 'parameter $' . $param_name; $hook_type = $is_filter ? 'filter' : 'action'; @@ -475,9 +532,15 @@ private function param_error( bool $is_filter, string $handler, string $hook, st /** * Builds a return-type violation error. * - * @return \PHPStan\Rules\RuleError + * @param string $hook The resolved hook name. + * @param bool $is_filter Whether the hook is a filter. + * @param string $return_type The offending native return type. + * @param int $line The line to report the violation on. + * @param string $handler The handler label (e.g. `Foo::bar()`), or '' for closures. + * + * @return RuleError */ - private function return_type_error( string $hook, bool $is_filter, string $return_type, int $line, string $handler = '' ) { + private function return_type_error( string $hook, bool $is_filter, string $return_type, int $line, string $handler = '' ): RuleError { $where = $handler === '' ? '' : $handler . ' '; if ( $is_filter ) { @@ -508,6 +571,11 @@ private function return_type_error( string $hook, bool $is_filter, string $retur /** * Whether a native `void` return type is acceptable in this context. + * + * @param bool $is_filter Whether the hook is a filter. + * @param string $return_type The declared native return type. + * + * @return bool */ private function is_void_allowed( bool $is_filter, string $return_type ): bool { return ! $is_filter @@ -520,6 +588,8 @@ private function is_void_allowed( bool $is_filter, string $return_type ): bool { * detection). * * @param Node $type A parameter or return type node. + * + * @return string */ private function type_node_to_string( Node $type ): string { if ( $type instanceof Node\Identifier || $type instanceof Node\Name ) { @@ -547,10 +617,12 @@ private function type_node_to_string( Node $type ): string { /** * Renders a native ReflectionType to a readable string. * - * @param \ReflectionType|null $type The reflection type. + * @param ReflectionType|null $type The reflection type. + * + * @return string */ - private function reflection_type_to_string( ?\ReflectionType $type ): string { - if ( $type instanceof \ReflectionNamedType ) { + private function reflection_type_to_string( ?ReflectionType $type ): string { + if ( $type instanceof ReflectionNamedType ) { return ( $type->allowsNull() && strtolower( $type->getName() ) !== 'null' ? '?' : '' ) . $type->getName(); } diff --git a/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php b/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php index fe31b2a..fe8df36 100644 --- a/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php +++ b/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php @@ -77,14 +77,16 @@ public function register(): array { /** * Processes this test, when one of its tokens is encountered. * + * $stack_ptr carries no native type hint on purpose: the PHP_CodeSniffer + * Sniff interface declares process() with an untyped second parameter, and + * PHP would fatal on an incompatible declaration if a type were added here. + * * @param File $phpcs_file The file being scanned. * @param int $stack_ptr The position of the current token in the stack. * * @return void - * - * @phpcs:disable SlevomatCodingStandard.TypeHints.ParameterTypeHint.MissingNativeTypeHint */ - public function process( File $phpcs_file, $stack_ptr ): void { + public function process( File $phpcs_file, $stack_ptr ): void { // phpcs:ignore SlevomatCodingStandard.TypeHints.ParameterTypeHint.MissingNativeTypeHint -- The Sniff interface forbids a native type on this parameter; see the note above. $tokens = $phpcs_file->getTokens(); $content = strtolower( $tokens[ $stack_ptr ]['content'] ); From 99a94af0075bb95fb0d3375f37c4e671ff759798 Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Fri, 24 Jul 2026 12:54:43 -0400 Subject: [PATCH 16/17] refactor: dedupe hook-handler resolution and report each hook name 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. --- README.md | 5 + StellarWP/PHPStan/HookHandlerTypesRule.php | 150 +++++++++++------- .../Sniffs/Hooks/HookHandlerTypesSniff.php | 89 ++++++----- .../PHPStan/HookHandlerTypesRuleTest.php | 6 + .../Tests/PHPStan/data/hook-handler-types.php | 5 + 5 files changed, 165 insertions(+), 90 deletions(-) diff --git a/README.md b/README.md index 01ee4d5..6353aa3 100644 --- a/README.md +++ b/README.md @@ -143,6 +143,11 @@ configuration. Enable just this sniff with: An optional `allow_void_return_on_actions` property (default `true`) controls whether a native `void` return type is permitted on action handlers. +A second optional property, `warn_on_dynamic_hook_names` (default `false`), +emits a warning when a hook name cannot be resolved to a literal string, so the +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 Handlers registered through a `$this->add_action( 'tag', 'method' )` wrapper (for diff --git a/StellarWP/PHPStan/HookHandlerTypesRule.php b/StellarWP/PHPStan/HookHandlerTypesRule.php index 67f0647..46ab115 100644 --- a/StellarWP/PHPStan/HookHandlerTypesRule.php +++ b/StellarWP/PHPStan/HookHandlerTypesRule.php @@ -142,7 +142,20 @@ private function process_wp_call( FuncCall $node, Scope $scope ): array { return []; } - return $this->check_callback( $args[1]->value, $scope, $constant_strings[0]->getValue(), $function === 'add_filter' ); + // A hook argument usually narrows to a single constant string, but type + // inference can yield several (e.g. a value that is one of two constants). + // Report each resolved hook name on its own error line so every + // registration the call stands in for is named individually. + $is_filter = $function === 'add_filter'; + $errors = []; + foreach ( $constant_strings as $constant_string ) { + $errors = array_merge( + $errors, + $this->check_callback( $args[1]->value, $scope, $constant_string->getValue(), $is_filter ) + ); + } + + return $errors; } /** @@ -177,39 +190,39 @@ private function process_wrapper_call( MethodCall $node, Scope $scope ): array { if ( $hook_strings === [] ) { return []; } - $hook = $hook_strings[0]->getValue(); - // The handler method name is the second argument, falling back to the hook - // name when it is absent or an empty string. A dynamic second argument - // cannot be resolved. - $method_names = []; + // The handler method name is the second argument. A dynamic second argument + // cannot be resolved; an absent or empty one falls back to the hook name. + $method_strings = null; if ( isset( $args[1] ) ) { $method_strings = $scope->getType( $args[1]->value )->getConstantStrings(); if ( $method_strings === [] ) { return []; } - - foreach ( $method_strings as $method_string ) { - $method_names[ $method_string->getValue() === '' ? $hook : $method_string->getValue() ] = true; - } - } else { - $method_names[ $hook ] = true; } - $class_names = []; - foreach ( $scope->getType( $node->var )->getObjectClassReflections() as $class_reflection ) { - $class_names[ $class_reflection->getName() ] = true; - } + $class_names = $this->collect_class_names( $scope->getType( $node->var ) ); + $is_filter = $method === 'add_filter'; + $line = $node->getStartLine(); - $is_filter = $method === 'add_filter'; - $errors = []; - foreach ( array_keys( $method_names ) as $method_name ) { - foreach ( array_keys( $class_names ) as $class_name ) { - $errors = array_merge( - $errors, - $this->check_class_method( $class_name, $method_name, $hook, $is_filter, $node->getStartLine() ) - ); + // Report each resolved hook name on its own error line (see process_wp_call()). + $errors = []; + foreach ( $hook_strings as $hook_string ) { + $hook = $hook_string->getValue(); + $method_names = []; + + if ( $method_strings === null ) { + $method_names[ $hook ] = true; + } else { + foreach ( $method_strings as $method_string ) { + $method_names[ $method_string->getValue() === '' ? $hook : $method_string->getValue() ] = true; + } } + + $errors = array_merge( + $errors, + $this->check_class_methods( $class_names, array_keys( $method_names ), $hook, $is_filter, $line ) + ); } return $errors; @@ -278,27 +291,14 @@ private function check_container_callback( Node $callback, Scope $scope, string return []; } - $class_type = $scope->getType( $args[0]->value ); + $class_names = $this->collect_class_names( $scope->getType( $args[0]->value ) ); - $class_names = []; - foreach ( $class_type->getObjectClassReflections() as $class_reflection ) { - $class_names[ $class_reflection->getName() ] = true; - } - foreach ( $class_type->getConstantStrings() as $constant_string ) { - $class_names[ $constant_string->getValue() ] = true; - } - - $errors = []; + $method_names = []; foreach ( $scope->getType( $args[1]->value )->getConstantStrings() as $method_string ) { - foreach ( array_keys( $class_names ) as $class_name ) { - $errors = array_merge( - $errors, - $this->check_class_method( $class_name, $method_string->getValue(), $hook, $is_filter, $callback->getStartLine() ) - ); - } + $method_names[ $method_string->getValue() ] = true; } - return $errors; + return $this->check_class_methods( $class_names, array_keys( $method_names ), $hook, $is_filter, $callback->getStartLine() ); } /** @@ -355,23 +355,56 @@ private function check_array_callback( Array_ $callback, Scope $scope, string $h return []; } - $method = $method_value->value; - $subject_type = $scope->getType( $callback->items[0]->value ); + $class_names = $this->collect_class_names( $scope->getType( $callback->items[0]->value ) ); + + return $this->check_class_methods( $class_names, [ $method_value->value ], $hook, $is_filter, $callback->getStartLine() ); + } + /** + * Collects candidate handler class names from a type: both resolved object + * classes (e.g. `$this` / an instance) and constant strings (e.g. + * `Foo::class` / `'Foo'`), keyed for uniqueness. + * + * @param Type $type The type to inspect. + * + * @return array + */ + private function collect_class_names( Type $type ): array { $class_names = []; - foreach ( $subject_type->getObjectClassReflections() as $class_reflection ) { + + foreach ( $type->getObjectClassReflections() as $class_reflection ) { $class_names[ $class_reflection->getName() ] = true; } - foreach ( $subject_type->getConstantStrings() as $constant_string ) { + + foreach ( $type->getConstantStrings() as $constant_string ) { $class_names[ $constant_string->getValue() ] = true; } + return $class_names; + } + + /** + * Checks every (class, method) pair for disallowed native types, collecting + * the resulting errors. + * + * @param array $class_names Candidate handler class names (keyed set). + * @param array $method_names Candidate handler method names. + * @param string $hook The formatted hook label (for messaging). + * @param bool $is_filter Whether the hook is a filter. + * @param int $line The line to report the violation on. + * + * @return array + */ + private function check_class_methods( array $class_names, array $method_names, string $hook, bool $is_filter, int $line ): array { $errors = []; - foreach ( array_keys( $class_names ) as $class_name ) { - $errors = array_merge( - $errors, - $this->check_class_method( $class_name, $method, $hook, $is_filter, $callback->getStartLine() ) - ); + + foreach ( $method_names as $method_name ) { + foreach ( array_keys( $class_names ) as $class_name ) { + $errors = array_merge( + $errors, + $this->check_class_method( $class_name, $method_name, $hook, $is_filter, $line ) + ); + } } return $errors; @@ -500,6 +533,18 @@ private function has_native_type( Type $type ): bool { return ! ( $type instanceof MixedType && ! $type->isExplicitMixed() ); } + /** + * Builds the leading handler segment of a message ("Foo::bar() "), or an + * empty string for anonymous handlers (closures/arrow functions). + * + * @param string $handler The handler label (e.g. `Foo::bar()`), or '' for closures. + * + * @return string + */ + private function handler_prefix( string $handler ): string { + return $handler === '' ? '' : $handler . ' '; + } + /** * Builds a parameter-type violation error. * @@ -513,13 +558,12 @@ private function has_native_type( Type $type ): bool { * @return RuleError */ private function param_error( bool $is_filter, string $handler, string $hook, string $type, string $param_name, int $line ): RuleError { - $where = $handler === '' ? '' : $handler . ' '; $subject = $param_name === '' ? 'a parameter' : 'parameter $' . $param_name; $hook_type = $is_filter ? 'filter' : 'action'; $message = sprintf( 'Handler %sfor %s "%s" must not declare the native type "%s" on %s; hook arguments are not type-guaranteed (a hook can be dispatched with unexpected types, including null), so a native type can cause a fatal error.', - $where, + $this->handler_prefix( $handler ), $hook_type, $hook, $type, @@ -541,7 +585,7 @@ private function param_error( bool $is_filter, string $handler, string $hook, st * @return RuleError */ private function return_type_error( string $hook, bool $is_filter, string $return_type, int $line, string $handler = '' ): RuleError { - $where = $handler === '' ? '' : $handler . ' '; + $where = $this->handler_prefix( $handler ); if ( $is_filter ) { $message = sprintf( diff --git a/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php b/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php index fe8df36..d1a3a7a 100644 --- a/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php +++ b/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php @@ -48,6 +48,30 @@ class HookHandlerTypesSniff implements Sniff { 'add_action' => false, ]; + /** + * Bracket-opening tokens that increase nesting depth when splitting arguments. + * + * @var array + */ + private const ARG_OPEN_BRACKETS = [ + T_OPEN_PARENTHESIS, + T_OPEN_SHORT_ARRAY, + T_OPEN_SQUARE_BRACKET, + T_OPEN_CURLY_BRACKET, + ]; + + /** + * Bracket-closing tokens that decrease nesting depth when splitting arguments. + * + * @var array + */ + private const ARG_CLOSE_BRACKETS = [ + T_CLOSE_PARENTHESIS, + T_CLOSE_SHORT_ARRAY, + T_CLOSE_SQUARE_BRACKET, + T_CLOSE_CURLY_BRACKET, + ]; + /** * Whether a native `void` return type is acceptable on action handlers. * @@ -219,20 +243,6 @@ private function split_arguments( File $phpcs_file, int $opener, int $closer ): $tokens = $phpcs_file->getTokens(); $args = []; - $open_codes = [ - T_OPEN_PARENTHESIS, - T_OPEN_SHORT_ARRAY, - T_OPEN_SQUARE_BRACKET, - T_OPEN_CURLY_BRACKET, - ]; - - $close_codes = [ - T_CLOSE_PARENTHESIS, - T_CLOSE_SHORT_ARRAY, - T_CLOSE_SQUARE_BRACKET, - T_CLOSE_CURLY_BRACKET, - ]; - $start = $phpcs_file->findNext( Tokens::$emptyTokens, $opener + 1, $closer, true ); if ( $start === false ) { return $args; @@ -244,12 +254,12 @@ private function split_arguments( File $phpcs_file, int $opener, int $closer ): for ( $i = $start; $i < $closer; $i++ ) { $code = $tokens[ $i ]['code']; - if ( in_array( $code, $open_codes, true ) ) { + if ( in_array( $code, self::ARG_OPEN_BRACKETS, true ) ) { $depth++; continue; } - if ( in_array( $code, $close_codes, true ) ) { + if ( in_array( $code, self::ARG_CLOSE_BRACKETS, true ) ) { $depth--; continue; } @@ -394,16 +404,7 @@ private function find_global_function( File $phpcs_file, string $name ): ?int { continue; } - $name_ptr = $phpcs_file->findNext( Tokens::$emptyTokens, $ptr + 1, null, true ); - if ( $name_ptr !== false && $tokens[ $name_ptr ]['code'] === T_BITWISE_AND ) { - $name_ptr = $phpcs_file->findNext( Tokens::$emptyTokens, $name_ptr + 1, null, true ); - } - - if ( - $name_ptr !== false - && $tokens[ $name_ptr ]['code'] === T_STRING - && strtolower( $tokens[ $name_ptr ]['content'] ) === $name_lc - ) { + if ( $this->function_name_matches( $phpcs_file, $ptr, $name_lc ) ) { return $ptr; } } @@ -411,6 +412,29 @@ private function find_global_function( File $phpcs_file, string $name ): ?int { return null; } + /** + * Whether the function/method declaration at $func_ptr is named $name_lc, + * skipping a leading reference operator (e.g. `function &name()`). + * + * @param File $phpcs_file The file being scanned. + * @param int $func_ptr The T_FUNCTION token position. + * @param string $name_lc The lowercase name to match. + * + * @return bool + */ + private function function_name_matches( File $phpcs_file, int $func_ptr, string $name_lc ): bool { + $tokens = $phpcs_file->getTokens(); + $name_ptr = $phpcs_file->findNext( Tokens::$emptyTokens, $func_ptr + 1, null, true ); + + if ( $name_ptr !== false && $tokens[ $name_ptr ]['code'] === T_BITWISE_AND ) { + $name_ptr = $phpcs_file->findNext( Tokens::$emptyTokens, $name_ptr + 1, null, true ); + } + + return $name_ptr !== false + && $tokens[ $name_ptr ]['code'] === T_STRING + && strtolower( $tokens[ $name_ptr ]['content'] ) === $name_lc; + } + /** * Resolves an array callback ([ $this, 'method' ] or array( self::class, * 'method' )) to a same-file method declaration token, or null. @@ -555,16 +579,7 @@ private function find_class_method( File $phpcs_file, int $stack_ptr, string $me continue; } - $name_ptr = $phpcs_file->findNext( Tokens::$emptyTokens, $i + 1, null, true ); - if ( $name_ptr !== false && $tokens[ $name_ptr ]['code'] === T_BITWISE_AND ) { - $name_ptr = $phpcs_file->findNext( Tokens::$emptyTokens, $name_ptr + 1, null, true ); - } - - if ( - $name_ptr !== false - && $tokens[ $name_ptr ]['code'] === T_STRING - && strtolower( $tokens[ $name_ptr ]['content'] ) === $method_lc - ) { + if ( $this->function_name_matches( $phpcs_file, $i, $method_lc ) ) { return $i; } } @@ -585,7 +600,7 @@ private function find_class_method( File $phpcs_file, int $stack_ptr, string $me * * @return void */ - private function check_handler_types( File $phpcs_file, int $func_ptr, string $hook_name, bool $is_filter, bool $fixable = true ): void { + private function check_handler_types( File $phpcs_file, int $func_ptr, string $hook_name, bool $is_filter, bool $fixable ): void { $hook_type = $is_filter ? 'filter' : 'action'; $params = $phpcs_file->getMethodParameters( $func_ptr ); diff --git a/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php index d596f6c..af98b40 100644 --- a/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php +++ b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php @@ -93,6 +93,12 @@ public function testRule(): void { [ 'Handler Wrapper_Subclass::on_save() for action "save_post" must not declare the native type "int" on parameter $post_id; ' . $param, 53 ], [ 'Handler Wrapper_Subclass::filter_it() for filter "the_content" must not declare the native type "string" on parameter $content; ' . $param, 54 ], [ 'Handler Wrapper_Subclass::filter_it() for filter "the_content" must not declare a native return type ("string"); ' . $return, 54 ], + + // Hook name narrowing to two constant strings: one error per name. + [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_content" must not declare the native type "string" on parameter $content; ' . $param, 62 ], + [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_content" must not declare a native return type ("string"); ' . $return, 62 ], + [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_title" must not declare the native type "string" on parameter $content; ' . $param, 62 ], + [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_title" must not declare a native return type ("string"); ' . $return, 62 ], ] ); } diff --git a/StellarWP/Tests/PHPStan/data/hook-handler-types.php b/StellarWP/Tests/PHPStan/data/hook-handler-types.php index a608c22..036a1e4 100644 --- a/StellarWP/Tests/PHPStan/data/hook-handler-types.php +++ b/StellarWP/Tests/PHPStan/data/hook-handler-types.php @@ -55,3 +55,8 @@ // Unknown/undefined global function - skipped. add_filter( 'wp_footer', 'some_undefined_handler' ); + +// Hook name that type inference narrows to two constant strings: each resolved +// name is reported on its own error line. +$which = rand( 0, 1 ) === 1 ? 'the_content' : 'the_title'; +add_filter( $which, [ $obj, 'filter_method' ] ); From d1d674f2a5d8955696ef9074e8564e3067f5c18e Mon Sep 17 00:00:00 2001 From: Eric Defore Date: Fri, 24 Jul 2026 15:06:58 -0400 Subject: [PATCH 17/17] test: avoid stdlib call in multi-hook fixture (fixes PHP 8.0 CI) 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. --- .../Tests/PHPStan/HookHandlerTypesRuleTest.php | 8 ++++---- StellarWP/Tests/PHPStan/data/hook-handler-types.php | 13 ++++++++++--- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php index af98b40..d806e9e 100644 --- a/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php +++ b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php @@ -95,10 +95,10 @@ public function testRule(): void { [ 'Handler Wrapper_Subclass::filter_it() for filter "the_content" must not declare a native return type ("string"); ' . $return, 54 ], // Hook name narrowing to two constant strings: one error per name. - [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_content" must not declare the native type "string" on parameter $content; ' . $param, 62 ], - [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_content" must not declare a native return type ("string"); ' . $return, 62 ], - [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_title" must not declare the native type "string" on parameter $content; ' . $param, 62 ], - [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_title" must not declare a native return type ("string"); ' . $return, 62 ], + [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_content" must not declare the native type "string" on parameter $content; ' . $param, 68 ], + [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_content" must not declare a native return type ("string"); ' . $return, 68 ], + [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_title" must not declare the native type "string" on parameter $content; ' . $param, 68 ], + [ 'Handler Hook_Test_Handlers::filter_method() for filter "the_title" must not declare a native return type ("string"); ' . $return, 68 ], ] ); } diff --git a/StellarWP/Tests/PHPStan/data/hook-handler-types.php b/StellarWP/Tests/PHPStan/data/hook-handler-types.php index 036a1e4..2344794 100644 --- a/StellarWP/Tests/PHPStan/data/hook-handler-types.php +++ b/StellarWP/Tests/PHPStan/data/hook-handler-types.php @@ -57,6 +57,13 @@ add_filter( 'wp_footer', 'some_undefined_handler' ); // Hook name that type inference narrows to two constant strings: each resolved -// name is reported on its own error line. -$which = rand( 0, 1 ) === 1 ? 'the_content' : 'the_title'; -add_filter( $which, [ $obj, 'filter_method' ] ); +// name is reported on its own error line. A phpdoc literal-string union is used +// (rather than a stdlib call like rand()) so analysing this fixture pulls in no +// function stub - a stub can trip php-parser's enum emulation on the PHP 8.0 +// test runtime. +/** + * @param 'the_content'|'the_title' $which + */ +function register_conditional_hook( string $which ): void { + add_filter( $which, [ Hook_Test_Handlers::class, 'filter_method' ] ); +}