diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml new file mode 100644 index 0000000..7bf6ca5 --- /dev/null +++ b/.github/workflows/continuous-integration.yml @@ -0,0 +1,54 @@ +# Continuous Integration for stellarwp/coding-standards. +# +# 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: + 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@v7 + + - 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@v4 + + # 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/.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..6353aa3 100644 --- a/README.md +++ b/README.md @@ -75,3 +75,125 @@ You can follow [this guide](https://confluence.jetbrains.com/display/PhpStorm/PH */vendor/* ``` + +## Hook handler argument types + +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). + +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) | +| 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), 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 +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, 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. + +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 +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 +[`phpstan/extension-installer`](https://github.com/phpstan/extension-installer) +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: + +```neon +includes: + - vendor/stellarwp/coding-standards/StellarWP/PHPStan/extension.neon +``` + +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 new file mode 100644 index 0000000..46ab115 --- /dev/null +++ b/StellarWP/PHPStan/HookHandlerTypesRule.php @@ -0,0 +1,675 @@ +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 + */ + +namespace StellarWP\PHPStan; + +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; +use PhpParser\Node\Expr\StaticCall; +use PhpParser\Node\Name; +use PhpParser\Node\Scalar\String_; +use PHPStan\Analyser\Scope; +use PHPStan\Reflection\ParameterReflectionWithPhpDocs; +use PHPStan\Reflection\ParametersAcceptorSelector; +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 + */ +class HookHandlerTypesRule implements Rule { + + /** + * @var ReflectionProvider + */ + private $reflection_provider; + + /** + * 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 bool $allow_void_return_on_actions Allow `: void` on action handlers. + */ + public function __construct( ReflectionProvider $reflection_provider, bool $allow_void_return_on_actions = true ) { + $this->reflection_provider = $reflection_provider; + $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; + } + + /** + * 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. + 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. + * + * @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 ) { + 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 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 []; + } + + // 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; + } + + /** + * 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. + * + * @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 ) { + 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 []; + } + + // 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 []; + } + } + + $class_names = $this->collect_class_names( $scope->getType( $node->var ) ); + $is_filter = $method === 'add_filter'; + $line = $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; + } + + /** + * Dispatches to the appropriate resolver for the callback expression. + * + * @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 ) { + 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() ); + } + + // 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 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 + */ + private function check_container_callback( Node $callback, Scope $scope, string $hook, bool $is_filter ): array { + $args = $callback->getArgs(); + if ( count( $args ) < 2 ) { + return []; + } + + $class_names = $this->collect_class_names( $scope->getType( $args[0]->value ) ); + + $method_names = []; + foreach ( $scope->getType( $args[1]->value )->getConstantStrings() as $method_string ) { + $method_names[ $method_string->getValue() ] = true; + } + + return $this->check_class_methods( $class_names, array_keys( $method_names ), $hook, $is_filter, $callback->getStartLine() ); + } + + /** + * Checks an inline closure or arrow function using its declared native types. + * + * @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 + */ + private function check_closure_node( Node $node, string $hook, bool $is_filter ): array { + $errors = []; + + foreach ( $node->params as $param ) { + if ( $param->type === null ) { + continue; + } + + $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 ) { + $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. + * + * @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 ) { + return []; + } + + $method_value = $callback->items[1]->value; + if ( ! $method_value instanceof String_ ) { + return []; + } + + $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 ( $type->getObjectClassReflections() as $class_reflection ) { + $class_names[ $class_reflection->getName() ] = true; + } + + 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 ( $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; + } + + /** + * Checks a resolved class method's native parameter and return types. + * + * @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, '\\' ); + + 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[] = $this->param_error( + $is_filter, + $declaring . '::' . $method . '()', + $hook, + $this->reflection_type_to_string( $parameter->getType() ), + $parameter->getName(), + $line + ); + } + + 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. + * + * @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, '\\' ) ); + + 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[] = $this->param_error( + $is_filter, + $name . '()', + $hook, + $parameter->getNativeType()->describe( VerbosityLevel::typeOnly() ), + $parameter->getName(), + $line + ); + } + + $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. + * + * @param Type $type The native type to inspect. + * + * @return bool + */ + 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. + * + * @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 ): RuleError { + $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.', + $this->handler_prefix( $handler ), + $hook_type, + $hook, + $type, + $subject + ); + + return RuleErrorBuilder::message( $message )->identifier( 'stellarwp.hookHandlerParamType' )->line( $line )->build(); + } + + /** + * Builds a return-type violation error. + * + * @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 = '' ): RuleError { + $where = $this->handler_prefix( $handler ); + + if ( $is_filter ) { + $message = sprintf( + '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 + ); + } 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(); + } + + /** + * 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 + && $this->allow_void_return_on_actions + && strtolower( ltrim( $return_type, '?\\' ) ) === 'void'; + } + + /** + * Renders a PhpParser type node to a readable string (for messages and void + * 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 ) { + 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. + * + * @return string + */ + 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..728959c --- /dev/null +++ b/StellarWP/PHPStan/extension.neon @@ -0,0 +1,18 @@ +parameters: + stellarwpHookHandlerTypes: + # 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([ + allowVoidReturnOnActions: bool() + ]) + +services: + - + class: StellarWP\PHPStan\HookHandlerTypesRule + arguments: + 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..d1a3a7a --- /dev/null +++ b/StellarWP/Sniffs/Hooks/HookHandlerTypesSniff.php @@ -0,0 +1,764 @@ +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 + */ + +namespace StellarWP\Sniffs\Hooks; + +use PHP_CodeSniffer\Files\File; +use PHP_CodeSniffer\Sniffs\Sniff; +use PHP_CodeSniffer\Util\Tokens; + +/** + * HookHandlerTypesSniff class. + */ +class HookHandlerTypesSniff implements Sniff { + + /** + * Hook-registration functions mapped to whether they register a filter. + * + * @var array + */ + private const HOOK_FUNCTIONS = [ + 'add_filter' => true, + '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. + * + * 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. + * + * $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 + */ + 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'] ); + + if ( ! isset( self::HOOK_FUNCTIONS[ $content ] ) ) { + return; + } + + // 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 ) { + 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; + } + + $is_wrapper = true; + } elseif ( in_array( $tokens[ $prev ]['code'], [ T_DOUBLE_COLON, T_FUNCTION, T_NEW ], 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 ( $args === [] ) { + return; + } + + // 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 ) { + $phpcs_file->addWarning( + 'Unable to determine the hook name statically; the handler type restriction could not be verified.', + $args[0]['start'], + 'DynamicHookName' + ); + } + + return; + } + + $is_filter = self::HOOK_FUNCTIONS[ $content ]; + + 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 handler: left to the PHPStan rule. + return; + } + + // 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 ); + } + + /** + * 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. + * + * @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 = []; + + $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, self::ARG_OPEN_BRACKETS, true ) ) { + $depth++; + continue; + } + + if ( in_array( $code, self::ARG_CLOSE_BRACKETS, 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; + } + + /** + * 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; + } + + if ( $this->function_name_matches( $phpcs_file, $ptr, $name_lc ) ) { + return $ptr; + } + } + + 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. + * + * @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']; + + // 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 $first === $element['end'] + && strtolower( $tokens[ $first ]['content'] ) === '$this'; + } + + if ( $code === T_CLASS_C ) { + return $first === $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; + } + + if ( $this->function_name_matches( $phpcs_file, $i, $method_lc ) ) { + return $i; + } + } + + return null; + } + + /** + * 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, bool $fixable ): void { + $hook_type = $is_filter ? 'filter' : 'action'; + $params = $phpcs_file->getMethodParameters( $func_ptr ); + + foreach ( $params as $param ) { + if ( empty( $param['type_hint'] ) ) { + continue; + } + + $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 ); + } + } + + $props = $phpcs_file->getMethodProperties( $func_ptr ); + if ( empty( $props['return_type'] ) ) { + return; + } + + $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 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.'; + } 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', $data ); + + 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..1c41701 --- /dev/null +++ b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc @@ -0,0 +1,77 @@ +add_action( 'save_post', 'wrapper_action' ); + $this->add_filter( 'the_title', 'wrapper_filter' ); + } + + public function filter_method( string $content ): string { + return $content; + } + + public function action_method( int $post_id, WP_Post $post ): void {} + + public function filter_context_method( bool $has_access, int $post_id, int $user_id ): bool { + return $has_access; + } + + public function action_typed( int $id ): void {} + + public function ref_action( \WP_Error $error ) {} + + 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 { + return $length; +} diff --git a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed new file mode 100644 index 0000000..ff9bfb0 --- /dev/null +++ b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.inc.fixed @@ -0,0 +1,77 @@ +add_action( 'save_post', 'wrapper_action' ); + $this->add_filter( 'the_title', 'wrapper_filter' ); + } + + public function filter_method( $content ) { + return $content; + } + + public function action_method( $post_id, $post ): void {} + + public function filter_context_method( $has_access, $post_id, $user_id ) { + return $has_access; + } + + public function action_typed( $id ): void {} + + public function ref_action( $error ) {} + + public static function static_filter( $title ) { + return $title; + } + + public function wrapper_action( int $id ): void {} + + public function wrapper_filter( string $title ): string { + return $title; + } +} + +function global_filter( $length ) { + return $length; +} diff --git a/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.php b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.php new file mode 100644 index 0000000..80446ae --- /dev/null +++ b/StellarWP/Tests/Hooks/HookHandlerTypesUnitTest.php @@ -0,0 +1,47 @@ + + */ + protected function getErrorList() { + return [ + 19 => 1, // init action closure: param; void return allowed. + 22 => 3, // login_redirect filter closure: two params + return. + 27 => 3, // pre_get_posts action closure: nullable, by-ref, variadic params. + 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. + ]; + } + + /** + * 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..d806e9e --- /dev/null +++ b/StellarWP/Tests/PHPStan/HookHandlerTypesRuleTest.php @@ -0,0 +1,105 @@ + + */ +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(), true ); + } + + public function testRule(): void { + // 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 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.'; + $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' ], + [ + // 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 ], + + // 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 ], + + // 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 ], + + // 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 ], + + // Action closure (void return allowed). + [ 'Handler for action "init" must not declare the native type "int" on parameter $x; ' . $param, 28 ], + + // 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 ], + + // 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::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 ], + + // 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 ], + + // 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, 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/handlers.php b/StellarWP/Tests/PHPStan/data/handlers.php new file mode 100644 index 0000000..7e5e61e --- /dev/null +++ b/StellarWP/Tests/PHPStan/data/handlers.php @@ -0,0 +1,70 @@ +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' ) ); + +// 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' ); + +// Hook name that type inference narrows to two constant strings: each resolved +// 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' ] ); +} diff --git a/composer.json b/composer.json index af6d760..0587e68 100644 --- a/composer.json +++ b/composer.json @@ -31,25 +31,34 @@ }, "require-dev": { "dealerdirect/phpcodesniffer-composer-installer": "*", - "phpcompatibility/php-compatibility": "^9" + "phpcompatibility/php-compatibility": "^9", + "phpunit/phpunit": "^9.6", + "phpstan/phpstan": "^1.12" }, "suggest": { - "dealerdirect/phpcodesniffer-composer-installer": "^0.7 || This Composer plugin will sort out the PHPCS 'installed_paths' automatically." + "dealerdirect/phpcodesniffer-composer-installer": "^0.7 || This Composer plugin will sort out the PHPCS 'installed_paths' automatically.", + "phpstan/phpstan": "Required only to use the StellarWP PHPStan rules." + }, + "autoload": { + "psr-4": { + "StellarWP\\": "StellarWP/" + } + }, + "extra": { + "phpstan": { + "includes": [ + "StellarWP/PHPStan/extension.neon" + ] + } }, "scripts": { "install-codestandards": "Dealerdirect\\Composer\\Plugin\\Installers\\PHPCodeSniffer\\Plugin::run", - "ruleset": "bin/ruleset-tests", - "lint": [ - "bin/php-lint", - "bin/xml-lint" - ], + "ruleset": "vendor/bin/phpcs --standard=StellarWP -e", "phpcs": "bin/phpcs", - "phpunit": "bin/unit-tests", + "phpunit": "vendor/bin/phpunit --testdox", "test": [ - "@lint", "@ruleset", - "@phpunit", - "@phpcs" + "@phpunit" ] }, "minimum-stability": "dev", diff --git a/phpunit-bootstrap.php b/phpunit-bootstrap.php new file mode 100644 index 0000000..5613ffc --- /dev/null +++ b/phpunit-bootstrap.php @@ -0,0 +1,37 @@ +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 + + +