From f5408288277ac3c84d6cd16c49f460306b110974 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Wed, 18 May 2022 20:12:15 +0200 Subject: [PATCH 01/55] Setup the hook callback rule and test. --- extension.neon | 1 + src/HookCallbackRule.php | 66 ++++++++++++++++++++++++++++++++++ tests/HookCallbackRuleTest.php | 43 ++++++++++++++++++++++ tests/data/hook-callback.php | 7 ++++ 4 files changed, 117 insertions(+) create mode 100644 src/HookCallbackRule.php create mode 100644 tests/HookCallbackRuleTest.php create mode 100644 tests/data/hook-callback.php diff --git a/extension.neon b/extension.neon index 372cd70..d78c7d7 100644 --- a/extension.neon +++ b/extension.neon @@ -80,6 +80,7 @@ services: tags: - phpstan.parser.richParserNodeVisitor rules: + - SzepeViktor\PHPStan\WordPress\HookCallbackRule - SzepeViktor\PHPStan\WordPress\HookDocsRule - SzepeViktor\PHPStan\WordPress\IsWpErrorRule parameters: diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php new file mode 100644 index 0000000..7d79687 --- /dev/null +++ b/src/HookCallbackRule.php @@ -0,0 +1,66 @@ + + */ +class HookCallbackRule implements \PHPStan\Rules\Rule +{ + private const SUPPORTED_FUNCTIONS = [ + 'add_filter', + 'add_action', + ]; + + /** @var \PHPStan\Rules\RuleLevelHelper */ + protected $ruleLevelHelper; + + /** @var \PhpParser\Node\Expr\FuncCall */ + protected $currentNode; + + /** @var \PHPStan\Analyser\Scope */ + protected $currentScope; + + public function __construct( + RuleLevelHelper $ruleLevelHelper + ) { + $this->ruleLevelHelper = $ruleLevelHelper; + } + + public function getNodeType(): string + { + return FuncCall::class; + } + + /** + * @param \PhpParser\Node\Expr\FuncCall $node + * @param \PHPStan\Analyser\Scope $scope + * @return array + */ + public function processNode(Node $node, Scope $scope): array + { + $name = $node->name; + + if (!($name instanceof \PhpParser\Node\Name)) { + return []; + } + + if (!in_array($name->toString(), self::SUPPORTED_FUNCTIONS, true)) { + return []; + } + + // @TODO everything. + return []; + } +} diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php new file mode 100644 index 0000000..5b354ff --- /dev/null +++ b/tests/HookCallbackRuleTest.php @@ -0,0 +1,43 @@ + + */ +class HookCallbackRuleTest extends \PHPStan\Testing\RuleTestCase +{ + protected function getRule(): \PHPStan\Rules\Rule + { + /** @var \PHPStan\Rules\RuleLevelHelper */ + $ruleLevelHelper = self::getContainer()->getByType(RuleLevelHelper::class); + + // getRule() method needs to return an instance of the tested rule + return new HookCallbackRule($ruleLevelHelper); + } + + // phpcs:ignore NeutronStandard.Functions.LongFunction.LongFunction + public function testRule(): void + { + // first argument: path to the example file that contains some errors that should be reported by HookCallbackRule + // second argument: an array of expected errors, + // each error consists of the asserted error message, and the asserted error file line + $this->analyse( + [ + __DIR__ . '/data/hook-callback.php', + ], + [ + ] + ); + } + + public static function getAdditionalConfigFiles(): array + { + return [dirname(__DIR__) . '/vendor/szepeviktor/phpstan-wordpress/extension.neon']; + } +} diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php new file mode 100644 index 0000000..91baa33 --- /dev/null +++ b/tests/data/hook-callback.php @@ -0,0 +1,7 @@ + Date: Wed, 18 May 2022 20:12:49 +0200 Subject: [PATCH 02/55] Add the wp-hooks library. --- composer.json | 1 + 1 file changed, 1 insertion(+) diff --git a/composer.json b/composer.json index 8d026e4..7d6a5ee 100644 --- a/composer.json +++ b/composer.json @@ -12,6 +12,7 @@ ], "require": { "php": "^7.2 || ^8.0", + "johnbillion/wp-hooks": "^0.8.0", "php-stubs/wordpress-stubs": "^4.7 || ^5.0", "phpstan/phpstan": "^1.6", "symfony/polyfill-php73": "^1.12.0" From 689306ae365bf9215925190d52f87fa35698de1c Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Wed, 18 May 2022 20:51:24 +0200 Subject: [PATCH 03/55] Start adding tests. --- tests/HookCallbackRuleTest.php | 24 ++++++++++++++++ tests/data/hook-callback.php | 50 ++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index 5b354ff..cc2bbb3 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -32,6 +32,30 @@ public function testRule(): void __DIR__ . '/data/hook-callback.php', ], [ + [ + 'Filter callback has no return value.', + 14, + ], + [ + 'Filter callback has no return value.', + 17, + ], + [ + 'Filter callback has no return value.', + 18, + ], + [ + 'Callback expects 1 argument, $accepted_args is set to 0.', + 21, + ], + [ + 'Callback expects 1 argument, $accepted_args is set to 2.', + 24, + ], + [ + 'Callback expects 2 arguments, $accepted_args is set to 1.', + 24, + ], ] ); } diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 91baa33..a8a4324 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -5,3 +5,53 @@ namespace SzepeViktor\PHPStan\WordPress\Tests; // phpcs:disable Squiz.NamingConventions.ValidFunctionName.NotCamelCaps,Squiz.NamingConventions.ValidVariableName.NotCamelCaps,Squiz.NamingConventions.ValidVariableName.MemberNotCamelCaps + +/** + * Incorrect usage: + */ + +// Not a core filter, but callback is missing a return value +add_filter('not_a_core_filter', function() {}); + +// Core filter, callback is missing a return value +add_filter('post_class', function() {}); +add_filter('post_class', function(array $classes) {}); + +// Not a core filter, accepted args are incorrect +add_filter('not_a_core_filter', function($value) { + return 123; +}, 10, 0); +add_filter('not_a_core_filter', function($value) { + return 123; +}, 10, 2); +add_filter('not_a_core_filter', function($value1, $value2) { + return 123; +}); + +// Core filter, callback is missing a return value +add_filter('post_class', function() {}); +add_filter('post_class', function(array $classes) {}); + +/** + * Correct usage: + */ + +// Not a core filter, but callback is ok +add_filter('not_a_core_filter', function() { + return 123; +}, 10, 0); +add_filter('not_a_core_filter', function() { + // We're allowing 0 parameters when `$accepted_args` is default value. + // This might change in the future to get more strict. + return 123; +}); +add_filter('not_a_core_filter', function($value) { + return 123; +}); +add_filter('not_a_core_filter', function($value) { + return 123; +}, 10, 1); +add_filter('not_a_core_filter', function($value1, $value2) { + return 123; +}, 10, 2); +add_filter('not_a_core_filter', '__return_false'); From 55e8e069be541d47b9a76c68b1c592ba919d8ea3 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Wed, 18 May 2022 23:34:34 +0100 Subject: [PATCH 04/55] Preparing more tests. --- tests/HookCallbackRuleTest.php | 2 +- tests/data/hook-callback.php | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index cc2bbb3..f6f1ae5 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -54,7 +54,7 @@ public function testRule(): void ], [ 'Callback expects 2 arguments, $accepted_args is set to 1.', - 24, + 27, ], ] ); diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index a8a4324..f4112a6 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -4,6 +4,9 @@ namespace SzepeViktor\PHPStan\WordPress\Tests; +use function add_filter; +use function add_action; + // phpcs:disable Squiz.NamingConventions.ValidFunctionName.NotCamelCaps,Squiz.NamingConventions.ValidVariableName.NotCamelCaps,Squiz.NamingConventions.ValidVariableName.MemberNotCamelCaps /** @@ -32,6 +35,27 @@ add_filter('post_class', function() {}); add_filter('post_class', function(array $classes) {}); +/** + * Incorrect usage that's handled by PHPStan: + * + * These are here to ensure the rule doesn't trigger unwanted errors. + */ + +// Too few parameters: +add_filter('post_class'); +add_filter(); + +// Invalid callback: +add_filter('post_class','i_do_not_exist'); + +// Invalid parameters: +add_filter('post_class', function() { + return 123; +}, false); +add_filter('post_class', function() { + return 123; +}, 10, false); + /** * Correct usage: */ From 6af5fcc204c397e31c4f3e1554d97a301193affc Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Wed, 18 May 2022 23:35:13 +0100 Subject: [PATCH 05/55] If we don't have enough arguments, bail out and let PHPStan handle the error. --- src/HookCallbackRule.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 7d79687..ae3c762 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -60,7 +60,13 @@ public function processNode(Node $node, Scope $scope): array return []; } - // @TODO everything. + $args = $node->getArgs(); + + // If we don't have enough arguments, bail out and let PHPStan handle the error: + if (count($args) < 2) { + return []; + } + return []; } } From c551b5cc794b70d708349cb417c3329254e9f41c Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Wed, 18 May 2022 23:35:26 +0100 Subject: [PATCH 06/55] If the callback is not valid, bail out and let PHPStan handle the error: --- src/HookCallbackRule.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index ae3c762..cdadd0a 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -67,6 +67,22 @@ public function processNode(Node $node, Scope $scope): array return []; } + list( + $hook_name, + $callback + ) = $args; + + $valid_callback = $this->ruleLevelHelper->accepts( + new CallableType(), + $scope->getType($callback->value), + $scope->isDeclareStrictTypes() + ); + + // If the callback is not valid, bail out and let PHPStan handle the error: + if ($valid_callback === false) { + return []; + } + return []; } } From e54097d26bf612ca395dc2970ef1384f906d86b0 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Wed, 1 Jun 2022 17:53:43 +0200 Subject: [PATCH 07/55] Simplify this. --- src/HookCallbackRule.php | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index cdadd0a..92090bc 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -72,14 +72,10 @@ public function processNode(Node $node, Scope $scope): array $callback ) = $args; - $valid_callback = $this->ruleLevelHelper->accepts( - new CallableType(), - $scope->getType($callback->value), - $scope->isDeclareStrictTypes() - ); + $callback = $scope->getType($callback->value); // If the callback is not valid, bail out and let PHPStan handle the error: - if ($valid_callback === false) { + if ($callback->isCallable()->no()) { return []; } From 50388cb65e5c6f80061e433be9d71c14ccf54a76 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Wed, 1 Jun 2022 17:54:08 +0200 Subject: [PATCH 08/55] More valid test cases. --- tests/data/hook-callback.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index f4112a6..67d63ee 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -78,4 +78,18 @@ add_filter('not_a_core_filter', function($value1, $value2) { return 123; }, 10, 2); + +// Various callback types add_filter('not_a_core_filter', '__return_false'); +add_filter('not_a_core_filter', __NAMESPACE__ . '\\filter_callback'); +add_filter('not_a_core_filter', new TestInvokable(), 10, 2); + +function filter_callback() { + return 123; +} + +class TestInvokable { + public function __invoke($one, $two) { + return 123; + } +} From b3d590a01ff6d2d1c1572bbdedf2c2bae127df69 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Wed, 1 Jun 2022 19:17:27 +0200 Subject: [PATCH 09/55] Update some test line numbers. --- tests/HookCallbackRuleTest.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index f6f1ae5..8fd4539 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -34,27 +34,27 @@ public function testRule(): void [ [ 'Filter callback has no return value.', - 14, + 17, ], [ 'Filter callback has no return value.', - 17, + 20, ], [ 'Filter callback has no return value.', - 18, + 21, ], [ 'Callback expects 1 argument, $accepted_args is set to 0.', - 21, + 24, ], [ 'Callback expects 1 argument, $accepted_args is set to 2.', - 24, + 27, ], [ 'Callback expects 2 arguments, $accepted_args is set to 1.', - 27, + 30, ], ] ); From 66e40997750029758a10b61ee1eded39b29bc4cb Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Wed, 1 Jun 2022 19:18:09 +0200 Subject: [PATCH 10/55] First pass at detecting mismatching accepted and expected argument counts. --- src/HookCallbackRule.php | 53 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 92090bc..9c2cc65 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -11,7 +11,10 @@ use PhpParser\Node; use PhpParser\Node\Expr\FuncCall; use PHPStan\Analyser\Scope; +use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Rules\RuleLevelHelper; +use PHPStan\Type\Constant\ConstantIntegerType; +use PHPStan\Type\Constant\ConstantStringType; /** * @implements \PHPStan\Rules\Rule<\PhpParser\Node\Expr\FuncCall> @@ -68,17 +71,59 @@ public function processNode(Node $node, Scope $scope): array } list( - $hook_name, - $callback + $hookNameArg, + $callbackArg ) = $args; - $callback = $scope->getType($callback->value); + $hookNameType = $scope->getType($hookNameArg->value); + $hookNameValue = null; + + if ($hookNameType instanceof ConstantStringType) { + $hookNameValue = $hookNameType->getValue(); + } + + $callbackType = $scope->getType($callbackArg->value); // If the callback is not valid, bail out and let PHPStan handle the error: - if ($callback->isCallable()->no()) { + if ($callbackType->isCallable()->no()) { return []; } + $acceptedArgs = 1; + + if (isset($args[3])) { + $acceptedArgs = null; + $argumentType = $scope->getType($args[3]->value); + + if ($argumentType instanceof ConstantIntegerType) { + $acceptedArgs = $argumentType->getValue(); + } + } + + if ($acceptedArgs !== null) { + $callbackAcceptor = $callbackType->getCallableParametersAcceptors($scope)[0]; + + $callbackParameters = $callbackAcceptor->getParameters(); + $expectedArgs = count($callbackParameters); + + if ($expectedArgs !== $acceptedArgs && ($expectedArgs !== 0 && $acceptedArgs !== 1)) { + $message = (1 === $expectedArgs) + ? 'Callback expects %1$d argument, $accepted_args is set to %2$d.' + : 'Callback expects %1$d arguments, $accepted_args is set to %2$d.' + ; + + return [ + RuleErrorBuilder::message( + sprintf( + $message, + $expectedArgs, + $acceptedArgs + ) + )->build() + ]; + } + } + return []; } } From b7f4a01728b41d953b7440a576507bd1225f0f2a Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Wed, 1 Jun 2022 19:35:22 +0200 Subject: [PATCH 11/55] Remove some accidental duplicates. --- tests/data/hook-callback.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 67d63ee..4bd6590 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -31,10 +31,6 @@ return 123; }); -// Core filter, callback is missing a return value -add_filter('post_class', function() {}); -add_filter('post_class', function(array $classes) {}); - /** * Incorrect usage that's handled by PHPStan: * From e6da76c4948e73a576dae149cca50ff875578acf Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Wed, 1 Jun 2022 20:26:41 +0200 Subject: [PATCH 12/55] Fix some logic. --- src/HookCallbackRule.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 9c2cc65..64a1770 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -106,7 +106,7 @@ public function processNode(Node $node, Scope $scope): array $callbackParameters = $callbackAcceptor->getParameters(); $expectedArgs = count($callbackParameters); - if ($expectedArgs !== $acceptedArgs && ($expectedArgs !== 0 && $acceptedArgs !== 1)) { + if ($expectedArgs !== $acceptedArgs && !($expectedArgs === 0 && $acceptedArgs === 1)) { $message = (1 === $expectedArgs) ? 'Callback expects %1$d argument, $accepted_args is set to %2$d.' : 'Callback expects %1$d arguments, $accepted_args is set to %2$d.' From 3fdb3318ff6bef2c20b8aa8146c64826d0f2698d Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Wed, 1 Jun 2022 21:37:00 +0200 Subject: [PATCH 13/55] Let's split this up before it gets out of hand. --- src/HookCallbackException.php | 14 ++++++++ src/HookCallbackRule.php | 63 ++++++++++++++++++++++------------- 2 files changed, 53 insertions(+), 24 deletions(-) create mode 100644 src/HookCallbackException.php diff --git a/src/HookCallbackException.php b/src/HookCallbackException.php new file mode 100644 index 0000000..77c4131 --- /dev/null +++ b/src/HookCallbackException.php @@ -0,0 +1,14 @@ +name; + $this->currentNode = $node; + $this->currentScope = $scope; if (!($name instanceof \PhpParser\Node\Name)) { return []; @@ -89,41 +91,54 @@ public function processNode(Node $node, Scope $scope): array return []; } + try { + $this->validateParamCount($args); + } catch (\SzepeViktor\PHPStan\WordPress\HookCallbackException $e) { + return [RuleErrorBuilder::message($e->getMessage())->build()]; + } + + return []; + } + + protected function validateParamCount(array $args): void + { + $callbackType = $this->currentScope->getType($args[1]->value); $acceptedArgs = 1; if (isset($args[3])) { $acceptedArgs = null; - $argumentType = $scope->getType($args[3]->value); + $argumentType = $this->currentScope->getType($args[3]->value); if ($argumentType instanceof ConstantIntegerType) { $acceptedArgs = $argumentType->getValue(); } } - if ($acceptedArgs !== null) { - $callbackAcceptor = $callbackType->getCallableParametersAcceptors($scope)[0]; - - $callbackParameters = $callbackAcceptor->getParameters(); - $expectedArgs = count($callbackParameters); - - if ($expectedArgs !== $acceptedArgs && !($expectedArgs === 0 && $acceptedArgs === 1)) { - $message = (1 === $expectedArgs) - ? 'Callback expects %1$d argument, $accepted_args is set to %2$d.' - : 'Callback expects %1$d arguments, $accepted_args is set to %2$d.' - ; - - return [ - RuleErrorBuilder::message( - sprintf( - $message, - $expectedArgs, - $acceptedArgs - ) - )->build() - ]; - } + if ($acceptedArgs === null) { + return; } - return []; + $callbackAcceptor = $callbackType->getCallableParametersAcceptors($this->currentScope)[0]; + $callbackParameters = $callbackAcceptor->getParameters(); + $expectedArgs = count($callbackParameters); + + if ($expectedArgs === $acceptedArgs) { + return; + } + + if ($expectedArgs === 0 && $acceptedArgs === 1) { + return; + } + + $message = (1 === $expectedArgs) + ? 'Callback expects %1$d argument, $accepted_args is set to %2$d.' + : 'Callback expects %1$d arguments, $accepted_args is set to %2$d.' + ; + + throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException(sprintf( + $message, + $expectedArgs, + $acceptedArgs + )); } } From 65c16ed76fa1739348be602e208a166efd4d5beb Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Wed, 1 Jun 2022 21:39:17 +0200 Subject: [PATCH 14/55] Reduce this down. --- src/HookCallbackRule.php | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index e5707ba..24e7ed1 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -72,19 +72,14 @@ public function processNode(Node $node, Scope $scope): array return []; } - list( - $hookNameArg, - $callbackArg - ) = $args; - - $hookNameType = $scope->getType($hookNameArg->value); + $hookNameType = $scope->getType($args[0]->value); $hookNameValue = null; if ($hookNameType instanceof ConstantStringType) { $hookNameValue = $hookNameType->getValue(); } - $callbackType = $scope->getType($callbackArg->value); + $callbackType = $scope->getType($args[1]->value); // If the callback is not valid, bail out and let PHPStan handle the error: if ($callbackType->isCallable()->no()) { From 9e60e0bbfb43b57d5a87e93259ad6b14db3dda48 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Wed, 1 Jun 2022 21:46:03 +0200 Subject: [PATCH 15/55] More tidying up. --- src/HookCallbackRule.php | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 24e7ed1..5db64e4 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -9,12 +9,14 @@ namespace SzepeViktor\PHPStan\WordPress; use PhpParser\Node; +use PhpParser\Node\Arg; use PhpParser\Node\Expr\FuncCall; use PHPStan\Analyser\Scope; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Rules\RuleLevelHelper; use PHPStan\Type\Constant\ConstantIntegerType; use PHPStan\Type\Constant\ConstantStringType; +use PHPStan\Type\Type; /** * @implements \PHPStan\Rules\Rule<\PhpParser\Node\Expr\FuncCall> @@ -87,7 +89,7 @@ public function processNode(Node $node, Scope $scope): array } try { - $this->validateParamCount($args); + $this->validateParamCount($callbackType, $args[3] ?? null); } catch (\SzepeViktor\PHPStan\WordPress\HookCallbackException $e) { return [RuleErrorBuilder::message($e->getMessage())->build()]; } @@ -95,14 +97,13 @@ public function processNode(Node $node, Scope $scope): array return []; } - protected function validateParamCount(array $args): void + protected function validateParamCount(Type $callbackType, ?Arg $arg): void { - $callbackType = $this->currentScope->getType($args[1]->value); $acceptedArgs = 1; - if (isset($args[3])) { + if (isset($arg)) { $acceptedArgs = null; - $argumentType = $this->currentScope->getType($args[3]->value); + $argumentType = $this->currentScope->getType($arg->value); if ($argumentType instanceof ConstantIntegerType) { $acceptedArgs = $argumentType->getValue(); From ebe7f9574c72a5ff0cf56cd0f70a1d9aa7431fc5 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Wed, 1 Jun 2022 21:53:38 +0200 Subject: [PATCH 16/55] We're going to be needing this in multiple methods soon. --- src/HookCallbackRule.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 5db64e4..f64a7be 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -12,6 +12,7 @@ use PhpParser\Node\Arg; use PhpParser\Node\Expr\FuncCall; use PHPStan\Analyser\Scope; +use PHPStan\Reflection\ParametersAcceptor; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Rules\RuleLevelHelper; use PHPStan\Type\Constant\ConstantIntegerType; @@ -88,8 +89,10 @@ public function processNode(Node $node, Scope $scope): array return []; } + $callbackAcceptor = $callbackType->getCallableParametersAcceptors($scope)[0]; + try { - $this->validateParamCount($callbackType, $args[3] ?? null); + $this->validateParamCount($callbackAcceptor, $args[3] ?? null); } catch (\SzepeViktor\PHPStan\WordPress\HookCallbackException $e) { return [RuleErrorBuilder::message($e->getMessage())->build()]; } @@ -97,7 +100,7 @@ public function processNode(Node $node, Scope $scope): array return []; } - protected function validateParamCount(Type $callbackType, ?Arg $arg): void + protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg $arg): void { $acceptedArgs = 1; @@ -114,7 +117,6 @@ protected function validateParamCount(Type $callbackType, ?Arg $arg): void return; } - $callbackAcceptor = $callbackType->getCallableParametersAcceptors($this->currentScope)[0]; $callbackParameters = $callbackAcceptor->getParameters(); $expectedArgs = count($callbackParameters); From 8fa8ec036667501f730f718a9141a067ee35bda5 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Thu, 2 Jun 2022 17:29:22 +0200 Subject: [PATCH 17/55] Prep for callback return type checking, not yet functional. --- src/HookCallbackRule.php | 36 ++++++++++++++++++++++++++++++++++ tests/HookCallbackRuleTest.php | 14 ++++++++++--- tests/data/hook-callback.php | 23 ++++++++++++++++++---- 3 files changed, 66 insertions(+), 7 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index f64a7be..b2fd549 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -11,13 +11,17 @@ use PhpParser\Node; use PhpParser\Node\Arg; use PhpParser\Node\Expr\FuncCall; +use PhpParser\Node\Name; use PHPStan\Analyser\Scope; use PHPStan\Reflection\ParametersAcceptor; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Rules\RuleLevelHelper; use PHPStan\Type\Constant\ConstantIntegerType; use PHPStan\Type\Constant\ConstantStringType; +use PHPStan\Type\MixedType; use PHPStan\Type\Type; +use PHPStan\Type\VerbosityLevel; +use PHPStan\Type\VoidType; /** * @implements \PHPStan\Rules\Rule<\PhpParser\Node\Expr\FuncCall> @@ -90,9 +94,15 @@ public function processNode(Node $node, Scope $scope): array } $callbackAcceptor = $callbackType->getCallableParametersAcceptors($scope)[0]; + $acceptingType = new MixedType(); + + if ('add_action' === $name->toString()) { + $acceptingType = new VoidType(); + } try { $this->validateParamCount($callbackAcceptor, $args[3] ?? null); + $this->validateReturnType($callbackAcceptor, $acceptingType); } catch (\SzepeViktor\PHPStan\WordPress\HookCallbackException $e) { return [RuleErrorBuilder::message($e->getMessage())->build()]; } @@ -139,4 +149,30 @@ protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg $acceptedArgs )); } + + protected function validateReturnType(ParametersAcceptor $callbackAcceptor, Type $acceptingType): void + { + $acceptedType = $callbackAcceptor->getReturnType(); + $accepted = $this->ruleLevelHelper->accepts( + $acceptingType, + $acceptedType, + true + ); + $acceptingVerbosityLevel = VerbosityLevel::getRecommendedLevelByType($acceptingType); + $acceptedVerbosityLevel = VerbosityLevel::getRecommendedLevelByType($acceptedType); + + if (! $accepted) { + $message = sprintf( + 'Callback should return %1$s but returns %2$s.', + $acceptingType->describe($acceptingVerbosityLevel), + $acceptedType->describe($acceptedVerbosityLevel) + ); + + if (! (new VoidType())->accepts($acceptedType, true)->no()) { + $message = 'Filter callback return statement is missing.'; + } + + throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException($message); + } + } } diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index 8fd4539..4aa93af 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -33,15 +33,15 @@ public function testRule(): void ], [ [ - 'Filter callback has no return value.', + 'Filter callback return statement is missing.', 17, ], [ - 'Filter callback has no return value.', + 'Filter callback return statement is missing.', 20, ], [ - 'Filter callback has no return value.', + 'Filter callback return statement is missing.', 21, ], [ @@ -56,6 +56,14 @@ public function testRule(): void 'Callback expects 2 arguments, $accepted_args is set to 1.', 30, ], + [ + 'Filter callback return statement is missing.', + 35, + ], + [ + 'Action callback must return void.', + 42, + ], ] ); } diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 4bd6590..bd865d9 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -13,14 +13,14 @@ * Incorrect usage: */ -// Not a core filter, but callback is missing a return value +// But callback is missing a return value add_filter('not_a_core_filter', function() {}); -// Core filter, callback is missing a return value +// Callback is missing a return value add_filter('post_class', function() {}); add_filter('post_class', function(array $classes) {}); -// Not a core filter, accepted args are incorrect +// Accepted args are incorrect add_filter('not_a_core_filter', function($value) { return 123; }, 10, 0); @@ -31,6 +31,18 @@ return 123; }); +// Callback may miss a return value +add_filter('post_class', function($class) { + if ($class) { + return []; + } +}); + +// Action callback must return void +add_action('hello', function() { + return true; +}); + /** * Incorrect usage that's handled by PHPStan: * @@ -44,6 +56,9 @@ // Invalid callback: add_filter('post_class','i_do_not_exist'); +// Unknown callback: +add_filter('post_class',$_GET['callback']); + // Invalid parameters: add_filter('post_class', function() { return 123; @@ -56,7 +71,7 @@ * Correct usage: */ -// Not a core filter, but callback is ok +// But callback is ok add_filter('not_a_core_filter', function() { return 123; }, 10, 0); From 1bf7e6103bf9267cd408a6920326fff21214bac8 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Thu, 2 Jun 2022 17:34:09 +0200 Subject: [PATCH 18/55] Split action return type assertion into its own method. --- src/HookCallbackRule.php | 27 +++++++++++++++++++++------ tests/HookCallbackRuleTest.php | 4 ++++ tests/data/hook-callback.php | 12 ++++++++++++ 3 files changed, 37 insertions(+), 6 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index b2fd549..ca08f5c 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -94,15 +94,13 @@ public function processNode(Node $node, Scope $scope): array } $callbackAcceptor = $callbackType->getCallableParametersAcceptors($scope)[0]; - $acceptingType = new MixedType(); - - if ('add_action' === $name->toString()) { - $acceptingType = new VoidType(); - } try { $this->validateParamCount($callbackAcceptor, $args[3] ?? null); - $this->validateReturnType($callbackAcceptor, $acceptingType); + + if ('add_action' === $name->toString()) { + $this->validateActionReturnType($callbackAcceptor); + } } catch (\SzepeViktor\PHPStan\WordPress\HookCallbackException $e) { return [RuleErrorBuilder::message($e->getMessage())->build()]; } @@ -150,6 +148,23 @@ protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg )); } + protected function validateActionReturnType(ParametersAcceptor $callbackAcceptor): void + { + $acceptingType = new VoidType(); + $acceptedType = $callbackAcceptor->getReturnType(); + $accepted = $this->ruleLevelHelper->accepts( + $acceptingType, + $acceptedType, + true + ); + + if (! $accepted) { + $message = 'Action callback must return void.'; + + throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException($message); + } + } + protected function validateReturnType(ParametersAcceptor $callbackAcceptor, Type $acceptingType): void { $acceptedType = $callbackAcceptor->getReturnType(); diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index 4aa93af..4cc3def 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -64,6 +64,10 @@ public function testRule(): void 'Action callback must return void.', 42, ], + [ + 'Action callback must return void.', + 45, + ], ] ); } diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index bd865d9..8d1451c 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -42,6 +42,11 @@ add_action('hello', function() { return true; }); +add_action('hello', function($value) { + if ($value) { + return true; + } +}); /** * Incorrect usage that's handled by PHPStan: @@ -90,6 +95,13 @@ return 123; }, 10, 2); +// Action callbacks must return void +add_action('hello', function() { + return; +}); +add_action('hello', function() { +}); + // Various callback types add_filter('not_a_core_filter', '__return_false'); add_filter('not_a_core_filter', __NAMESPACE__ . '\\filter_callback'); From c1a6579b42d7ef66838b4a90e32a5fb64b524c14 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Thu, 2 Jun 2022 17:40:20 +0200 Subject: [PATCH 19/55] Bring this message more inline with those used by PHPStan. --- src/HookCallbackRule.php | 7 ++++++- tests/HookCallbackRuleTest.php | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index ca08f5c..482497c 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -159,7 +159,12 @@ protected function validateActionReturnType(ParametersAcceptor $callbackAcceptor ); if (! $accepted) { - $message = 'Action callback must return void.'; + $acceptedVerbosityLevel = VerbosityLevel::getRecommendedLevelByType($acceptedType); + + $message = sprintf( + 'Action callback returns %s but should not return anything.', + $acceptedType->describe($acceptedVerbosityLevel) + ); throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException($message); } diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index 4cc3def..5e358d0 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -61,11 +61,11 @@ public function testRule(): void 35, ], [ - 'Action callback must return void.', + 'Action callback returns true but should not return anything.', 42, ], [ - 'Action callback must return void.', + 'Action callback returns true but should not return anything.', 45, ], ] From 3eaaea453a61691dba6e63428a116fa5ab74f59b Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Thu, 2 Jun 2022 18:39:17 +0200 Subject: [PATCH 20/55] Add detection for filter callbacks with no return value. --- src/HookCallbackRule.php | 19 +++++++++++++++++-- tests/data/hook-callback.php | 15 ++++++++++++--- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 482497c..11bc48e 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -14,6 +14,7 @@ use PhpParser\Node\Name; use PHPStan\Analyser\Scope; use PHPStan\Reflection\ParametersAcceptor; +use PHPStan\Reflection\ParametersAcceptorSelector; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Rules\RuleLevelHelper; use PHPStan\Type\Constant\ConstantIntegerType; @@ -93,13 +94,15 @@ public function processNode(Node $node, Scope $scope): array return []; } - $callbackAcceptor = $callbackType->getCallableParametersAcceptors($scope)[0]; + $callbackAcceptor = ParametersAcceptorSelector::selectSingle($callbackType->getCallableParametersAcceptors($scope)); try { $this->validateParamCount($callbackAcceptor, $args[3] ?? null); if ('add_action' === $name->toString()) { $this->validateActionReturnType($callbackAcceptor); + } else { + $this->validateUnknownFilterReturnType($callbackAcceptor); } } catch (\SzepeViktor\PHPStan\WordPress\HookCallbackException $e) { return [RuleErrorBuilder::message($e->getMessage())->build()]; @@ -170,7 +173,19 @@ protected function validateActionReturnType(ParametersAcceptor $callbackAcceptor } } - protected function validateReturnType(ParametersAcceptor $callbackAcceptor, Type $acceptingType): void + protected function validateUnknownFilterReturnType(ParametersAcceptor $callbackAcceptor): void + { + $returnType = $callbackAcceptor->getReturnType(); + $isVoidSuperType = $returnType->isSuperTypeOf(new VoidType()); + + if ($isVoidSuperType->yes()) { + $message = 'Filter callback return statement is missing.'; + + throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException($message); + } + } + + protected function validateKnownFilterReturnType(ParametersAcceptor $callbackAcceptor, Type $acceptingType): void { $acceptedType = $callbackAcceptor->getReturnType(); $accepted = $this->ruleLevelHelper->accepts( diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 8d1451c..6c395bb 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -13,10 +13,10 @@ * Incorrect usage: */ -// But callback is missing a return value +// Filter callback is missing a return value add_filter('not_a_core_filter', function() {}); -// Callback is missing a return value +// Filter callback is missing a return value add_filter('post_class', function() {}); add_filter('post_class', function(array $classes) {}); @@ -31,7 +31,7 @@ return 123; }); -// Callback may miss a return value +// Filter callback may miss a return value add_filter('post_class', function($class) { if ($class) { return []; @@ -102,6 +102,15 @@ add_action('hello', function() { }); +// Filter callback may exit, unfortunately +add_filter('post_class', function(array $classes) { + if ($classes) { + exit('Goodbye'); + } + + return $classes; +}); + // Various callback types add_filter('not_a_core_filter', '__return_false'); add_filter('not_a_core_filter', __NAMESPACE__ . '\\filter_callback'); From becc7a915f927fd1a0f0d35e8f174dc1bf1255de Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 23 Jul 2022 01:08:25 +0100 Subject: [PATCH 21/55] Don't need this just yet. --- composer.json | 1 - 1 file changed, 1 deletion(-) diff --git a/composer.json b/composer.json index 07daf29..bd2e0f3 100644 --- a/composer.json +++ b/composer.json @@ -12,7 +12,6 @@ ], "require": { "php": "^7.2 || ^8.0", - "johnbillion/wp-hooks": "^0.8.0", "php-stubs/wordpress-stubs": "^4.7 || ^5.0 || ^6.0", "phpstan/phpstan": "^1.6", "symfony/polyfill-php73": "^1.12.0" From 1303e83be892c58561423310bf69d9f39f0eabfb Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 23 Jul 2022 01:12:34 +0100 Subject: [PATCH 22/55] Use early exit to reduce code nesting. --- src/HookCallbackRule.php | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 11bc48e..ba65139 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -196,18 +196,20 @@ protected function validateKnownFilterReturnType(ParametersAcceptor $callbackAcc $acceptingVerbosityLevel = VerbosityLevel::getRecommendedLevelByType($acceptingType); $acceptedVerbosityLevel = VerbosityLevel::getRecommendedLevelByType($acceptedType); - if (! $accepted) { - $message = sprintf( - 'Callback should return %1$s but returns %2$s.', - $acceptingType->describe($acceptingVerbosityLevel), - $acceptedType->describe($acceptedVerbosityLevel) - ); + if ($accepted) { + return; + } - if (! (new VoidType())->accepts($acceptedType, true)->no()) { - $message = 'Filter callback return statement is missing.'; - } + $message = sprintf( + 'Callback should return %1$s but returns %2$s.', + $acceptingType->describe($acceptingVerbosityLevel), + $acceptedType->describe($acceptedVerbosityLevel) + ); - throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException($message); + if (! (new VoidType())->accepts($acceptedType, true)->no()) { + $message = 'Filter callback return statement is missing.'; } + + throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException($message); } } From 73fb4f73ddbf4c02b35d018dfe79689760c6c878 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 23 Jul 2022 01:12:46 +0100 Subject: [PATCH 23/55] Remove unused imports. --- src/HookCallbackRule.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index ba65139..35bde25 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -11,7 +11,6 @@ use PhpParser\Node; use PhpParser\Node\Arg; use PhpParser\Node\Expr\FuncCall; -use PhpParser\Node\Name; use PHPStan\Analyser\Scope; use PHPStan\Reflection\ParametersAcceptor; use PHPStan\Reflection\ParametersAcceptorSelector; @@ -19,7 +18,6 @@ use PHPStan\Rules\RuleLevelHelper; use PHPStan\Type\Constant\ConstantIntegerType; use PHPStan\Type\Constant\ConstantStringType; -use PHPStan\Type\MixedType; use PHPStan\Type\Type; use PHPStan\Type\VerbosityLevel; use PHPStan\Type\VoidType; From a9991e4dff4de957fed7779db2aad703d80cf93b Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 23 Jul 2022 01:14:39 +0100 Subject: [PATCH 24/55] Yoda comparisons are disallowed. --- src/HookCallbackRule.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 35bde25..c3226a6 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -97,7 +97,7 @@ public function processNode(Node $node, Scope $scope): array try { $this->validateParamCount($callbackAcceptor, $args[3] ?? null); - if ('add_action' === $name->toString()) { + if ($name->toString() === 'add_action') { $this->validateActionReturnType($callbackAcceptor); } else { $this->validateUnknownFilterReturnType($callbackAcceptor); @@ -137,7 +137,7 @@ protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg return; } - $message = (1 === $expectedArgs) + $message = ($expectedArgs === 1) ? 'Callback expects %1$d argument, $accepted_args is set to %2$d.' : 'Callback expects %1$d arguments, $accepted_args is set to %2$d.' ; From 7a394486c3991122d9a0f0e476bc4bc36b0ee430 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 23 Jul 2022 01:15:55 +0100 Subject: [PATCH 25/55] Coding standards. --- src/HookCallbackRule.php | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index c3226a6..bdec9ed 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -139,14 +139,15 @@ protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg $message = ($expectedArgs === 1) ? 'Callback expects %1$d argument, $accepted_args is set to %2$d.' - : 'Callback expects %1$d arguments, $accepted_args is set to %2$d.' - ; - - throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException(sprintf( - $message, - $expectedArgs, - $acceptedArgs - )); + : 'Callback expects %1$d arguments, $accepted_args is set to %2$d.'; + + throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException( + sprintf( + $message, + $expectedArgs, + $acceptedArgs + ) + ); } protected function validateActionReturnType(ParametersAcceptor $callbackAcceptor): void From 4f2a64fc5224d8e85da07ee04a202a49d1424b2b Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 23 Jul 2022 01:16:01 +0100 Subject: [PATCH 26/55] Remove unused code. --- src/HookCallbackRule.php | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index bdec9ed..4d58c22 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -17,7 +17,6 @@ use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Rules\RuleLevelHelper; use PHPStan\Type\Constant\ConstantIntegerType; -use PHPStan\Type\Constant\ConstantStringType; use PHPStan\Type\Type; use PHPStan\Type\VerbosityLevel; use PHPStan\Type\VoidType; @@ -78,13 +77,6 @@ public function processNode(Node $node, Scope $scope): array return []; } - $hookNameType = $scope->getType($args[0]->value); - $hookNameValue = null; - - if ($hookNameType instanceof ConstantStringType) { - $hookNameValue = $hookNameType->getValue(); - } - $callbackType = $scope->getType($args[1]->value); // If the callback is not valid, bail out and let PHPStan handle the error: From 38e7c35084c187f0dc807bb938a544dd7e92c8c7 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 23 Jul 2022 18:41:53 +0100 Subject: [PATCH 27/55] Use early return to reduce code nesting. --- src/HookCallbackRule.php | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 4d58c22..5ef685d 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -152,16 +152,18 @@ protected function validateActionReturnType(ParametersAcceptor $callbackAcceptor true ); - if (! $accepted) { - $acceptedVerbosityLevel = VerbosityLevel::getRecommendedLevelByType($acceptedType); + if ($accepted) { + return; + } - $message = sprintf( - 'Action callback returns %s but should not return anything.', - $acceptedType->describe($acceptedVerbosityLevel) - ); + $acceptedVerbosityLevel = VerbosityLevel::getRecommendedLevelByType($acceptedType); - throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException($message); - } + $message = sprintf( + 'Action callback returns %s but should not return anything.', + $acceptedType->describe($acceptedVerbosityLevel) + ); + + throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException($message); } protected function validateUnknownFilterReturnType(ParametersAcceptor $callbackAcceptor): void From 05de0efd64c4582de8600db4339c74d1289d9079 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 23 Jul 2022 18:42:23 +0100 Subject: [PATCH 28/55] Remove more unused code. --- src/HookCallbackRule.php | 29 ----------------------------- 1 file changed, 29 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 5ef685d..9317abe 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -17,7 +17,6 @@ use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Rules\RuleLevelHelper; use PHPStan\Type\Constant\ConstantIntegerType; -use PHPStan\Type\Type; use PHPStan\Type\VerbosityLevel; use PHPStan\Type\VoidType; @@ -177,32 +176,4 @@ protected function validateUnknownFilterReturnType(ParametersAcceptor $callbackA throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException($message); } } - - protected function validateKnownFilterReturnType(ParametersAcceptor $callbackAcceptor, Type $acceptingType): void - { - $acceptedType = $callbackAcceptor->getReturnType(); - $accepted = $this->ruleLevelHelper->accepts( - $acceptingType, - $acceptedType, - true - ); - $acceptingVerbosityLevel = VerbosityLevel::getRecommendedLevelByType($acceptingType); - $acceptedVerbosityLevel = VerbosityLevel::getRecommendedLevelByType($acceptedType); - - if ($accepted) { - return; - } - - $message = sprintf( - 'Callback should return %1$s but returns %2$s.', - $acceptingType->describe($acceptingVerbosityLevel), - $acceptedType->describe($acceptedVerbosityLevel) - ); - - if (! (new VoidType())->accepts($acceptedType, true)->no()) { - $message = 'Filter callback return statement is missing.'; - } - - throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException($message); - } } From 723748cdad29294c8f20221999a6c77f32e5ca19 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 23 Jul 2022 18:44:52 +0100 Subject: [PATCH 29/55] Renaming. --- src/HookCallbackRule.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 9317abe..54b3019 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -91,7 +91,7 @@ public function processNode(Node $node, Scope $scope): array if ($name->toString() === 'add_action') { $this->validateActionReturnType($callbackAcceptor); } else { - $this->validateUnknownFilterReturnType($callbackAcceptor); + $this->validateFilterReturnType($callbackAcceptor); } } catch (\SzepeViktor\PHPStan\WordPress\HookCallbackException $e) { return [RuleErrorBuilder::message($e->getMessage())->build()]; @@ -165,7 +165,7 @@ protected function validateActionReturnType(ParametersAcceptor $callbackAcceptor throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException($message); } - protected function validateUnknownFilterReturnType(ParametersAcceptor $callbackAcceptor): void + protected function validateFilterReturnType(ParametersAcceptor $callbackAcceptor): void { $returnType = $callbackAcceptor->getReturnType(); $isVoidSuperType = $returnType->isSuperTypeOf(new VoidType()); From e4fb1e7748b021bf52bd460041fb5ccf6a75716e Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 23 Jul 2022 18:44:58 +0100 Subject: [PATCH 30/55] Use early return to reduce code nesting. --- src/HookCallbackRule.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 54b3019..dcdbea8 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -170,10 +170,12 @@ protected function validateFilterReturnType(ParametersAcceptor $callbackAcceptor $returnType = $callbackAcceptor->getReturnType(); $isVoidSuperType = $returnType->isSuperTypeOf(new VoidType()); - if ($isVoidSuperType->yes()) { - $message = 'Filter callback return statement is missing.'; - - throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException($message); + if (! $isVoidSuperType->no()) { + return; } + + $message = 'Filter callback return statement is missing.'; + + throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException($message); } } From 75d103ab1fdad1a94b499fd7852c1159f91bc529 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 23 Jul 2022 19:06:27 +0100 Subject: [PATCH 31/55] Tidying up. --- src/HookCallbackRule.php | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index dcdbea8..b4e13c6 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -117,8 +117,7 @@ protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg return; } - $callbackParameters = $callbackAcceptor->getParameters(); - $expectedArgs = count($callbackParameters); + $expectedArgs = count($callbackAcceptor->getParameters()); if ($expectedArgs === $acceptedArgs) { return; @@ -143,10 +142,9 @@ protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg protected function validateActionReturnType(ParametersAcceptor $callbackAcceptor): void { - $acceptingType = new VoidType(); $acceptedType = $callbackAcceptor->getReturnType(); $accepted = $this->ruleLevelHelper->accepts( - $acceptingType, + new VoidType(), $acceptedType, true ); @@ -155,14 +153,12 @@ protected function validateActionReturnType(ParametersAcceptor $callbackAcceptor return; } - $acceptedVerbosityLevel = VerbosityLevel::getRecommendedLevelByType($acceptedType); - - $message = sprintf( - 'Action callback returns %s but should not return anything.', - $acceptedType->describe($acceptedVerbosityLevel) + throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException( + sprintf( + 'Action callback returns %s but should not return anything.', + $acceptedType->describe(VerbosityLevel::getRecommendedLevelByType($acceptedType)) + ) ); - - throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException($message); } protected function validateFilterReturnType(ParametersAcceptor $callbackAcceptor): void @@ -174,8 +170,8 @@ protected function validateFilterReturnType(ParametersAcceptor $callbackAcceptor return; } - $message = 'Filter callback return statement is missing.'; - - throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException($message); + throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException( + 'Filter callback return statement is missing.' + ); } } From 4bab338f11c2407b33fb101e908a4cc0177aeacc Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 23 Jul 2022 19:17:25 +0100 Subject: [PATCH 32/55] Correct logic introduced during refactoring. --- src/HookCallbackRule.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index b4e13c6..cec1801 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -166,7 +166,7 @@ protected function validateFilterReturnType(ParametersAcceptor $callbackAcceptor $returnType = $callbackAcceptor->getReturnType(); $isVoidSuperType = $returnType->isSuperTypeOf(new VoidType()); - if (! $isVoidSuperType->no()) { + if (! $isVoidSuperType->yes()) { return; } From 96fcbe91a0ae5cfa1f31433dac702c42acc672e8 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 23 Jul 2022 19:23:23 +0100 Subject: [PATCH 33/55] Exclude "maybe" callables. --- src/HookCallbackRule.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index cec1801..c4271e0 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -79,7 +79,7 @@ public function processNode(Node $node, Scope $scope): array $callbackType = $scope->getType($args[1]->value); // If the callback is not valid, bail out and let PHPStan handle the error: - if ($callbackType->isCallable()->no()) { + if (! $callbackType->isCallable()->yes()) { return []; } From f4c856a1e4e4db046268d48023a3c34f7bf41c69 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 23 Jul 2022 19:23:46 +0100 Subject: [PATCH 34/55] More handling for parameter counts. --- src/HookCallbackRule.php | 8 +++++++- tests/HookCallbackRuleTest.php | 14 +++++++++++--- tests/data/hook-callback.php | 12 ++++++++++++ 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index c4271e0..d8c189f 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -117,7 +117,13 @@ protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg return; } - $expectedArgs = count($callbackAcceptor->getParameters()); + $requiredParameters = array_filter( + $callbackAcceptor->getParameters(), + function(\PHPStan\Reflection\ParameterReflection $parameter): bool { + return ! $parameter->isOptional(); + } + ); + $expectedArgs = count($requiredParameters); if ($expectedArgs === $acceptedArgs) { return; diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index 5e358d0..1c085b6 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -56,17 +56,25 @@ public function testRule(): void 'Callback expects 2 arguments, $accepted_args is set to 1.', 30, ], + [ + 'Callback expects 2 arguments, $accepted_args is set to 1.', + 33, + ], + [ + 'Callback expects 2 arguments, $accepted_args is set to 4.', + 36, + ], [ 'Filter callback return statement is missing.', - 35, + 41, ], [ 'Action callback returns true but should not return anything.', - 42, + 48, ], [ 'Action callback returns true but should not return anything.', - 45, + 51, ], ] ); diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 6c395bb..6998988 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -30,6 +30,12 @@ add_filter('not_a_core_filter', function($value1, $value2) { return 123; }); +add_filter('not_a_core_filter', function($value1, $value2, $value3 = null) { + return 123; +}); +add_filter('not_a_core_filter', function($value1, $value2, $value3 = null) { + return 123; +}, 10, 4); // Filter callback may miss a return value add_filter('post_class', function($class) { @@ -94,6 +100,12 @@ add_filter('not_a_core_filter', function($value1, $value2) { return 123; }, 10, 2); +add_filter('not_a_core_filter', function($value1, $value2, $value3 = null) { + return 123; +}, 10, 2); +add_filter('not_a_core_filter', function($value1, $value2, $value3 = null) { + return 123; +}, 10, 3); // Action callbacks must return void add_action('hello', function() { From 8335226bddc6d8c24848d02766e872c4f01db56c Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sun, 24 Jul 2022 15:04:05 +0100 Subject: [PATCH 35/55] More handling for optional parameters in hook callbacks. --- src/HookCallbackRule.php | 38 ++++++++++++++++++++++------------ tests/HookCallbackRuleTest.php | 20 +++++++++++------- tests/data/hook-callback.php | 14 ++++++++++++- 3 files changed, 50 insertions(+), 22 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index d8c189f..bd2a3b9 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -102,46 +102,58 @@ public function processNode(Node $node, Scope $scope): array protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg $arg): void { - $acceptedArgs = 1; + $acceptedArgsParam = 1; if (isset($arg)) { - $acceptedArgs = null; + $acceptedArgsParam = null; $argumentType = $this->currentScope->getType($arg->value); if ($argumentType instanceof ConstantIntegerType) { - $acceptedArgs = $argumentType->getValue(); + $acceptedArgsParam = $argumentType->getValue(); } } - if ($acceptedArgs === null) { + if ($acceptedArgsParam === null) { return; } + $allParameters = $callbackAcceptor->getParameters(); $requiredParameters = array_filter( - $callbackAcceptor->getParameters(), + $allParameters, function(\PHPStan\Reflection\ParameterReflection $parameter): bool { return ! $parameter->isOptional(); } ); - $expectedArgs = count($requiredParameters); + $expectedArgs = count($allParameters); + $expectedRequiredArgs = count($requiredParameters); - if ($expectedArgs === $acceptedArgs) { + if (($acceptedArgsParam >= $expectedRequiredArgs) && ($acceptedArgsParam <= $expectedArgs)) { return; } - if ($expectedArgs === 0 && $acceptedArgs === 1) { + if ($expectedArgs === 0 && $acceptedArgsParam === 1) { return; } - $message = ($expectedArgs === 1) - ? 'Callback expects %1$d argument, $accepted_args is set to %2$d.' - : 'Callback expects %1$d arguments, $accepted_args is set to %2$d.'; + $expectedParametersMessage = $expectedArgs; + + if ($expectedArgs !== $expectedRequiredArgs) { + $expectedParametersMessage = sprintf( + '%1$d-%2$d', + $expectedRequiredArgs, + $expectedArgs + ); + } + + $message = ($expectedParametersMessage === 1) + ? 'Callback expects %1$d parameter, $accepted_args is set to %2$d.' + : 'Callback expects %1$s parameters, $accepted_args is set to %2$d.'; throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException( sprintf( $message, - $expectedArgs, - $acceptedArgs + $expectedParametersMessage, + $acceptedArgsParam ) ); } diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index 1c085b6..569227a 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -45,36 +45,40 @@ public function testRule(): void 21, ], [ - 'Callback expects 1 argument, $accepted_args is set to 0.', + 'Callback expects 1 parameter, $accepted_args is set to 0.', 24, ], [ - 'Callback expects 1 argument, $accepted_args is set to 2.', + 'Callback expects 1 parameter, $accepted_args is set to 2.', 27, ], [ - 'Callback expects 2 arguments, $accepted_args is set to 1.', + 'Callback expects 0-1 parameters, $accepted_args is set to 2.', 30, ], [ - 'Callback expects 2 arguments, $accepted_args is set to 1.', + 'Callback expects 2 parameters, $accepted_args is set to 1.', 33, ], [ - 'Callback expects 2 arguments, $accepted_args is set to 4.', + 'Callback expects 2-4 parameters, $accepted_args is set to 1.', 36, ], + [ + 'Callback expects 2-3 parameters, $accepted_args is set to 4.', + 39, + ], [ 'Filter callback return statement is missing.', - 41, + 44, ], [ 'Action callback returns true but should not return anything.', - 48, + 51, ], [ 'Action callback returns true but should not return anything.', - 51, + 54, ], ] ); diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 6998988..6995a42 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -27,10 +27,13 @@ add_filter('not_a_core_filter', function($value) { return 123; }, 10, 2); +add_filter('not_a_core_filter', function($value = null) { + return 123; +}, 10, 2); add_filter('not_a_core_filter', function($value1, $value2) { return 123; }); -add_filter('not_a_core_filter', function($value1, $value2, $value3 = null) { +add_filter('not_a_core_filter', function($value1, $value2, $value3 = null, $value4 = null) { return 123; }); add_filter('not_a_core_filter', function($value1, $value2, $value3 = null) { @@ -106,6 +109,15 @@ add_filter('not_a_core_filter', function($value1, $value2, $value3 = null) { return 123; }, 10, 3); +add_filter('not_a_core_filter', function($value = null) { + return 123; +}); +add_filter('not_a_core_filter', function($value = null) { + return 123; +}, 10, 0); +add_filter('not_a_core_filter', function($value = null) { + return 123; +}, 10, 1); // Action callbacks must return void add_action('hello', function() { From 609864460742450fcb2a028723604dcfcbc6dd15 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sun, 24 Jul 2022 15:09:18 +0100 Subject: [PATCH 36/55] Make the tests more manageable. --- tests/HookCallbackRuleTest.php | 20 ++++++++++---------- tests/data/hook-callback.php | 25 +++++++++++++++++++------ 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index 569227a..5ae245c 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -42,43 +42,43 @@ public function testRule(): void ], [ 'Filter callback return statement is missing.', - 21, + 23, ], [ 'Callback expects 1 parameter, $accepted_args is set to 0.', - 24, + 26, ], [ 'Callback expects 1 parameter, $accepted_args is set to 2.', - 27, + 31, ], [ 'Callback expects 0-1 parameters, $accepted_args is set to 2.', - 30, + 36, ], [ 'Callback expects 2 parameters, $accepted_args is set to 1.', - 33, + 41, ], [ 'Callback expects 2-4 parameters, $accepted_args is set to 1.', - 36, + 46, ], [ 'Callback expects 2-3 parameters, $accepted_args is set to 4.', - 39, + 51, ], [ 'Filter callback return statement is missing.', - 44, + 56, ], [ 'Action callback returns true but should not return anything.', - 51, + 63, ], [ 'Action callback returns true but should not return anything.', - 54, + 68, ], ] ); diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 6995a42..da0dbcc 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -13,44 +13,58 @@ * Incorrect usage: */ -// Filter callback is missing a return value +// Filter callback return statement is missing. add_filter('not_a_core_filter', function() {}); -// Filter callback is missing a return value +// Filter callback return statement is missing. add_filter('post_class', function() {}); + +// Filter callback return statement is missing. add_filter('post_class', function(array $classes) {}); -// Accepted args are incorrect +// Callback expects 1 parameter, $accepted_args is set to 0. add_filter('not_a_core_filter', function($value) { return 123; }, 10, 0); + +// Callback expects 1 parameter, $accepted_args is set to 2. add_filter('not_a_core_filter', function($value) { return 123; }, 10, 2); + +// Callback expects 0-1 parameters, $accepted_args is set to 2. add_filter('not_a_core_filter', function($value = null) { return 123; }, 10, 2); + +// Callback expects 2 parameters, $accepted_args is set to 1. add_filter('not_a_core_filter', function($value1, $value2) { return 123; }); + +// Callback expects 2-4 parameter, $accepted_args is set to 1. add_filter('not_a_core_filter', function($value1, $value2, $value3 = null, $value4 = null) { return 123; }); + +// Callback expects 2-3 parameter, $accepted_args is set to 4. add_filter('not_a_core_filter', function($value1, $value2, $value3 = null) { return 123; }, 10, 4); -// Filter callback may miss a return value +// Filter callback return statement is missing. add_filter('post_class', function($class) { if ($class) { return []; } }); -// Action callback must return void +// Action callback returns true but should not return anything. add_action('hello', function() { return true; }); + +// Action callback returns true but should not return anything. add_action('hello', function($value) { if ($value) { return true; @@ -85,7 +99,6 @@ * Correct usage: */ -// But callback is ok add_filter('not_a_core_filter', function() { return 123; }, 10, 0); From 85e1fdd985678a7c53a62a65e1869c94a8d49683 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sun, 24 Jul 2022 23:21:38 +0100 Subject: [PATCH 37/55] Tests for more callback types. --- tests/HookCallbackRuleTest.php | 12 ++++++++++++ tests/data/hook-callback.php | 11 +++++++++++ 2 files changed, 23 insertions(+) diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index 5ae245c..58ec8f4 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -80,6 +80,18 @@ public function testRule(): void 'Action callback returns true but should not return anything.', 68, ], + [ + 'Filter callback return statement is missing.', + 77, + ], + [ + 'Action callback returns false but should not return anything.', + 80, + ], + [ + 'Action callback returns 123 but should not return anything.', + 83, + ], ] ); } diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index da0dbcc..ac02fba 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -71,6 +71,17 @@ } }); +function no_return_value( $value ) {} + +// Filter callback return statement is missing. +add_filter('not_a_core_filter', __NAMESPACE__ . '\\no_return_value'); + +// Action callback returns false but should not return anything. +add_action('hello', '__return_false'); + +// Action callback returns 123 but should not return anything. +add_action('hello', new TestInvokable(), 10, 2); + /** * Incorrect usage that's handled by PHPStan: * From 13eff30f9b31da70dfa3b04d3126df97ce0cccad Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sun, 24 Jul 2022 23:28:59 +0100 Subject: [PATCH 38/55] Tidying up. --- src/HookCallbackRule.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index bd2a3b9..f57645c 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -71,14 +71,14 @@ public function processNode(Node $node, Scope $scope): array $args = $node->getArgs(); - // If we don't have enough arguments, bail out and let PHPStan handle the error: + // If we don't have enough arguments, let PHPStan handle the error: if (count($args) < 2) { return []; } $callbackType = $scope->getType($args[1]->value); - // If the callback is not valid, bail out and let PHPStan handle the error: + // If the callback is not valid, let PHPStan handle the error: if (! $callbackType->isCallable()->yes()) { return []; } @@ -120,7 +120,7 @@ protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg $allParameters = $callbackAcceptor->getParameters(); $requiredParameters = array_filter( $allParameters, - function(\PHPStan\Reflection\ParameterReflection $parameter): bool { + static function (\PHPStan\Reflection\ParameterReflection $parameter): bool { return ! $parameter->isOptional(); } ); From 75b473e402381b1d7587d844acdd8c3eb21155a8 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Mon, 25 Jul 2022 20:50:17 +0100 Subject: [PATCH 39/55] This isn't used. --- src/HookCallbackRule.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index f57645c..592aa4c 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -33,9 +33,6 @@ class HookCallbackRule implements \PHPStan\Rules\Rule /** @var \PHPStan\Rules\RuleLevelHelper */ protected $ruleLevelHelper; - /** @var \PhpParser\Node\Expr\FuncCall */ - protected $currentNode; - /** @var \PHPStan\Analyser\Scope */ protected $currentScope; @@ -58,7 +55,6 @@ public function getNodeType(): string public function processNode(Node $node, Scope $scope): array { $name = $node->name; - $this->currentNode = $node; $this->currentScope = $scope; if (!($name instanceof \PhpParser\Node\Name)) { From 99ad16e8b5641aa823da9a79fe6e7d18714a6423 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 30 Jul 2022 15:54:24 +0100 Subject: [PATCH 40/55] More test cases. --- tests/data/hook-callback.php | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index ac02fba..a7bfab5 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -93,10 +93,13 @@ function no_return_value( $value ) {} add_filter(); // Invalid callback: -add_filter('post_class','i_do_not_exist'); +add_filter('post_class', 'i_do_not_exist'); + +// Invalid callback: +add_filter('post_class', __NAMESPACE__ . '\\i_do_not_exist'); // Unknown callback: -add_filter('post_class',$_GET['callback']); +add_filter('post_class', $_GET['callback']); // Invalid parameters: add_filter('post_class', function() { @@ -159,17 +162,35 @@ function no_return_value( $value ) {} return $classes; }); +// Action callback may exit, unfortunately +add_action('hello', function($result) { + if (! $result) { + exit('Goodbye'); + } +}); + // Various callback types add_filter('not_a_core_filter', '__return_false'); add_filter('not_a_core_filter', __NAMESPACE__ . '\\filter_callback'); add_filter('not_a_core_filter', new TestInvokable(), 10, 2); +add_filter('not_a_core_filter', [new TestClass, 'foo']); + +$test_class = new TestClass(); + +add_filter('not_a_core_filter', [$test_class, 'foo']); function filter_callback() { return 123; } class TestInvokable { - public function __invoke($one, $two) { + public function __invoke($one, $two) { + return 123; + } +} + +class TestClass { + public function foo() { return 123; - } + } } From b3a72c899e3a3af915b3a55a9953dbd03c89a1ea Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 30 Jul 2022 16:02:53 +0100 Subject: [PATCH 41/55] Tests for variadic callbacks. --- tests/HookCallbackRuleTest.php | 18 +++++++++++++++--- tests/data/hook-callback.php | 34 ++++++++++++++++++++++++++++++++-- 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index 58ec8f4..51200c3 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -82,15 +82,27 @@ public function testRule(): void ], [ 'Filter callback return statement is missing.', - 77, + 75, ], [ 'Action callback returns false but should not return anything.', - 80, + 78, ], [ 'Action callback returns 123 but should not return anything.', - 83, + 81, + ], + [ + 'Callback expects at least 1 parameter, $accepted_args is set to 0.', + 84, + ], + [ + 'Callback expects at least 1 parameter, $accepted_args is set to 0.', + 87, + ], + [ + 'Callback expects 0-3 parameters, $accepted_args is set to 4.', + 92, ], ] ); diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index a7bfab5..41ae7e3 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -71,8 +71,6 @@ } }); -function no_return_value( $value ) {} - // Filter callback return statement is missing. add_filter('not_a_core_filter', __NAMESPACE__ . '\\no_return_value'); @@ -82,6 +80,19 @@ function no_return_value( $value ) {} // Action callback returns 123 but should not return anything. add_action('hello', new TestInvokable(), 10, 2); +// Callback expects at least 1 parameter, $accepted_args is set to 0. +add_filter('not_a_core_filter', __NAMESPACE__ . '\\filter_variadic', 10, 0); + +// Callback expects at least 1 parameter, $accepted_args is set to 0. +add_filter('not_a_core_filter', function( $one, ...$two ) { + return 123; +}, 10, 0); + +// Callback expects 0-3 parameters, $accepted_args is set to 4. +add_filter('not_a_core_filter', function($one = null, $two = null, $three = null) { + return 123; +}, 10, 4); + /** * Incorrect usage that's handled by PHPStan: * @@ -145,6 +156,9 @@ function no_return_value( $value ) {} add_filter('not_a_core_filter', function($value = null) { return 123; }, 10, 1); +add_filter('not_a_core_filter', function($one = null, $two = null, $three = null) { + return 123; +}); // Action callbacks must return void add_action('hello', function() { @@ -179,10 +193,26 @@ function no_return_value( $value ) {} add_filter('not_a_core_filter', [$test_class, 'foo']); +// Variadic callbacks +add_filter('not_a_core_filter', __NAMESPACE__ . '\\filter_variadic'); +add_filter('not_a_core_filter', __NAMESPACE__ . '\\filter_variadic', 10, 1); +add_filter('not_a_core_filter', __NAMESPACE__ . '\\filter_variadic', 10, 2); +add_filter('not_a_core_filter', __NAMESPACE__ . '\\filter_variadic', 10, 999); + +/** + * Symbol definitions for use in these tests. + */ + +function no_return_value( $value ) {} + function filter_callback() { return 123; } +function filter_variadic( $one, ...$two ) { + return $one; +} + class TestInvokable { public function __invoke($one, $two) { return 123; From ab7c1eb18f2f6c61d7f324524098d8d2c3c5c389 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 30 Jul 2022 16:14:39 +0100 Subject: [PATCH 42/55] More specific handling for variadic callbacks. --- src/HookCallbackRule.php | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 592aa4c..558d70f 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -120,31 +120,37 @@ static function (\PHPStan\Reflection\ParameterReflection $parameter): bool { return ! $parameter->isOptional(); } ); - $expectedArgs = count($allParameters); - $expectedRequiredArgs = count($requiredParameters); + $maxArgs = count($allParameters); + $minArgs = count($requiredParameters); - if (($acceptedArgsParam >= $expectedRequiredArgs) && ($acceptedArgsParam <= $expectedArgs)) { + if (($acceptedArgsParam >= $minArgs) && ($acceptedArgsParam <= $maxArgs)) { return; } - if ($expectedArgs === 0 && $acceptedArgsParam === 1) { + if ($minArgs === 0 && $acceptedArgsParam === 1) { return; } - $expectedParametersMessage = $expectedArgs; + $expectedParametersMessage = $minArgs; + + if ($callbackAcceptor->isVariadic()) { + $message = ($minArgs === 1) + ? 'Callback expects at least %1$d parameter, $accepted_args is set to %2$d.' + : 'Callback expects at least %1$s parameters, $accepted_args is set to %2$d.'; + } else { + if ($maxArgs !== $minArgs) { + $expectedParametersMessage = sprintf( + '%1$d-%2$d', + $minArgs, + $maxArgs + ); + } - if ($expectedArgs !== $expectedRequiredArgs) { - $expectedParametersMessage = sprintf( - '%1$d-%2$d', - $expectedRequiredArgs, - $expectedArgs - ); + $message = ($expectedParametersMessage === 1) + ? 'Callback expects %1$d parameter, $accepted_args is set to %2$d.' + : 'Callback expects %1$s parameters, $accepted_args is set to %2$d.'; } - $message = ($expectedParametersMessage === 1) - ? 'Callback expects %1$d parameter, $accepted_args is set to %2$d.' - : 'Callback expects %1$s parameters, $accepted_args is set to %2$d.'; - throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException( sprintf( $message, From ee221053310f9736c69fb05eedf8a260e27919c2 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 30 Jul 2022 16:22:00 +0100 Subject: [PATCH 43/55] More tests. --- tests/data/hook-callback.php | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 41ae7e3..1fa62de 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -109,6 +109,12 @@ // Invalid callback: add_filter('post_class', __NAMESPACE__ . '\\i_do_not_exist'); +// Invalid callback: +add_filter('post_class', [new TestClass, 'i_do_not_exist']); + +// Invalid callback: +add_filter('post_class', new TestClass); + // Unknown callback: add_filter('post_class', $_GET['callback']); @@ -128,7 +134,7 @@ return 123; }, 10, 0); add_filter('not_a_core_filter', function() { - // We're allowing 0 parameters when `$accepted_args` is default value. + // We're allowing 0 parameters when `$accepted_args` is default value of 1. // This might change in the future to get more strict. return 123; }); @@ -166,6 +172,7 @@ }); add_action('hello', function() { }); +add_action('hello', __NAMESPACE__ . '\\no_return_value'); // Filter callback may exit, unfortunately add_filter('post_class', function(array $classes) { @@ -185,7 +192,7 @@ // Various callback types add_filter('not_a_core_filter', '__return_false'); -add_filter('not_a_core_filter', __NAMESPACE__ . '\\filter_callback'); +add_filter('not_a_core_filter', __NAMESPACE__ . '\\return_value'); add_filter('not_a_core_filter', new TestInvokable(), 10, 2); add_filter('not_a_core_filter', [new TestClass, 'foo']); @@ -205,7 +212,7 @@ function no_return_value( $value ) {} -function filter_callback() { +function return_value() { return 123; } From 72cfaf587510907f173306fd5b8b0c99ba4a5879 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 30 Jul 2022 16:23:27 +0100 Subject: [PATCH 44/55] The hooks names are not relevant to the tests. --- tests/data/hook-callback.php | 102 +++++++++++++++++------------------ 1 file changed, 51 insertions(+), 51 deletions(-) diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 1fa62de..0f07f2c 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -14,82 +14,82 @@ */ // Filter callback return statement is missing. -add_filter('not_a_core_filter', function() {}); +add_filter('filter', function() {}); // Filter callback return statement is missing. -add_filter('post_class', function() {}); +add_filter('filter', function() {}); // Filter callback return statement is missing. -add_filter('post_class', function(array $classes) {}); +add_filter('filter', function(array $classes) {}); // Callback expects 1 parameter, $accepted_args is set to 0. -add_filter('not_a_core_filter', function($value) { +add_filter('filter', function($value) { return 123; }, 10, 0); // Callback expects 1 parameter, $accepted_args is set to 2. -add_filter('not_a_core_filter', function($value) { +add_filter('filter', function($value) { return 123; }, 10, 2); // Callback expects 0-1 parameters, $accepted_args is set to 2. -add_filter('not_a_core_filter', function($value = null) { +add_filter('filter', function($value = null) { return 123; }, 10, 2); // Callback expects 2 parameters, $accepted_args is set to 1. -add_filter('not_a_core_filter', function($value1, $value2) { +add_filter('filter', function($value1, $value2) { return 123; }); // Callback expects 2-4 parameter, $accepted_args is set to 1. -add_filter('not_a_core_filter', function($value1, $value2, $value3 = null, $value4 = null) { +add_filter('filter', function($value1, $value2, $value3 = null, $value4 = null) { return 123; }); // Callback expects 2-3 parameter, $accepted_args is set to 4. -add_filter('not_a_core_filter', function($value1, $value2, $value3 = null) { +add_filter('filter', function($value1, $value2, $value3 = null) { return 123; }, 10, 4); // Filter callback return statement is missing. -add_filter('post_class', function($class) { +add_filter('filter', function($class) { if ($class) { return []; } }); // Action callback returns true but should not return anything. -add_action('hello', function() { +add_action('action', function() { return true; }); // Action callback returns true but should not return anything. -add_action('hello', function($value) { +add_action('action', function($value) { if ($value) { return true; } }); // Filter callback return statement is missing. -add_filter('not_a_core_filter', __NAMESPACE__ . '\\no_return_value'); +add_filter('filter', __NAMESPACE__ . '\\no_return_value'); // Action callback returns false but should not return anything. -add_action('hello', '__return_false'); +add_action('action', '__return_false'); // Action callback returns 123 but should not return anything. -add_action('hello', new TestInvokable(), 10, 2); +add_action('action', new TestInvokable(), 10, 2); // Callback expects at least 1 parameter, $accepted_args is set to 0. -add_filter('not_a_core_filter', __NAMESPACE__ . '\\filter_variadic', 10, 0); +add_filter('filter', __NAMESPACE__ . '\\filter_variadic', 10, 0); // Callback expects at least 1 parameter, $accepted_args is set to 0. -add_filter('not_a_core_filter', function( $one, ...$two ) { +add_filter('filter', function( $one, ...$two ) { return 123; }, 10, 0); // Callback expects 0-3 parameters, $accepted_args is set to 4. -add_filter('not_a_core_filter', function($one = null, $two = null, $three = null) { +add_filter('filter', function($one = null, $two = null, $three = null) { return 123; }, 10, 4); @@ -100,29 +100,29 @@ */ // Too few parameters: -add_filter('post_class'); +add_filter('filter'); add_filter(); // Invalid callback: -add_filter('post_class', 'i_do_not_exist'); +add_filter('filter', 'i_do_not_exist'); // Invalid callback: -add_filter('post_class', __NAMESPACE__ . '\\i_do_not_exist'); +add_filter('filter', __NAMESPACE__ . '\\i_do_not_exist'); // Invalid callback: -add_filter('post_class', [new TestClass, 'i_do_not_exist']); +add_filter('filter', [new TestClass, 'i_do_not_exist']); // Invalid callback: -add_filter('post_class', new TestClass); +add_filter('filter', new TestClass); // Unknown callback: -add_filter('post_class', $_GET['callback']); +add_filter('filter', $_GET['callback']); // Invalid parameters: -add_filter('post_class', function() { +add_filter('filter', function() { return 123; }, false); -add_filter('post_class', function() { +add_filter('filter', function() { return 123; }, 10, false); @@ -130,52 +130,52 @@ * Correct usage: */ -add_filter('not_a_core_filter', function() { +add_filter('filter', function() { return 123; }, 10, 0); -add_filter('not_a_core_filter', function() { +add_filter('filter', function() { // We're allowing 0 parameters when `$accepted_args` is default value of 1. // This might change in the future to get more strict. return 123; }); -add_filter('not_a_core_filter', function($value) { +add_filter('filter', function($value) { return 123; }); -add_filter('not_a_core_filter', function($value) { +add_filter('filter', function($value) { return 123; }, 10, 1); -add_filter('not_a_core_filter', function($value1, $value2) { +add_filter('filter', function($value1, $value2) { return 123; }, 10, 2); -add_filter('not_a_core_filter', function($value1, $value2, $value3 = null) { +add_filter('filter', function($value1, $value2, $value3 = null) { return 123; }, 10, 2); -add_filter('not_a_core_filter', function($value1, $value2, $value3 = null) { +add_filter('filter', function($value1, $value2, $value3 = null) { return 123; }, 10, 3); -add_filter('not_a_core_filter', function($value = null) { +add_filter('filter', function($value = null) { return 123; }); -add_filter('not_a_core_filter', function($value = null) { +add_filter('filter', function($value = null) { return 123; }, 10, 0); -add_filter('not_a_core_filter', function($value = null) { +add_filter('filter', function($value = null) { return 123; }, 10, 1); -add_filter('not_a_core_filter', function($one = null, $two = null, $three = null) { +add_filter('filter', function($one = null, $two = null, $three = null) { return 123; }); // Action callbacks must return void -add_action('hello', function() { +add_action('action', function() { return; }); -add_action('hello', function() { +add_action('action', function() { }); -add_action('hello', __NAMESPACE__ . '\\no_return_value'); +add_action('action', __NAMESPACE__ . '\\no_return_value'); // Filter callback may exit, unfortunately -add_filter('post_class', function(array $classes) { +add_filter('filter', function(array $classes) { if ($classes) { exit('Goodbye'); } @@ -184,27 +184,27 @@ }); // Action callback may exit, unfortunately -add_action('hello', function($result) { +add_action('action', function($result) { if (! $result) { exit('Goodbye'); } }); // Various callback types -add_filter('not_a_core_filter', '__return_false'); -add_filter('not_a_core_filter', __NAMESPACE__ . '\\return_value'); -add_filter('not_a_core_filter', new TestInvokable(), 10, 2); -add_filter('not_a_core_filter', [new TestClass, 'foo']); +add_filter('filter', '__return_false'); +add_filter('filter', __NAMESPACE__ . '\\return_value'); +add_filter('filter', new TestInvokable(), 10, 2); +add_filter('filter', [new TestClass, 'foo']); $test_class = new TestClass(); -add_filter('not_a_core_filter', [$test_class, 'foo']); +add_filter('filter', [$test_class, 'foo']); // Variadic callbacks -add_filter('not_a_core_filter', __NAMESPACE__ . '\\filter_variadic'); -add_filter('not_a_core_filter', __NAMESPACE__ . '\\filter_variadic', 10, 1); -add_filter('not_a_core_filter', __NAMESPACE__ . '\\filter_variadic', 10, 2); -add_filter('not_a_core_filter', __NAMESPACE__ . '\\filter_variadic', 10, 999); +add_filter('filter', __NAMESPACE__ . '\\filter_variadic'); +add_filter('filter', __NAMESPACE__ . '\\filter_variadic', 10, 1); +add_filter('filter', __NAMESPACE__ . '\\filter_variadic', 10, 2); +add_filter('filter', __NAMESPACE__ . '\\filter_variadic', 10, 999); /** * Symbol definitions for use in these tests. From 83516e95148755d802855e4b3bab1b0239f4cbb5 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sat, 30 Jul 2022 16:57:00 +0100 Subject: [PATCH 45/55] Remove an `else`. --- src/HookCallbackRule.php | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 558d70f..e83958d 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -133,22 +133,22 @@ static function (\PHPStan\Reflection\ParameterReflection $parameter): bool { $expectedParametersMessage = $minArgs; + if ($maxArgs !== $minArgs) { + $expectedParametersMessage = sprintf( + '%1$d-%2$d', + $minArgs, + $maxArgs + ); + } + + $message = ($expectedParametersMessage === 1) + ? 'Callback expects %1$d parameter, $accepted_args is set to %2$d.' + : 'Callback expects %1$s parameters, $accepted_args is set to %2$d.'; + if ($callbackAcceptor->isVariadic()) { $message = ($minArgs === 1) ? 'Callback expects at least %1$d parameter, $accepted_args is set to %2$d.' : 'Callback expects at least %1$s parameters, $accepted_args is set to %2$d.'; - } else { - if ($maxArgs !== $minArgs) { - $expectedParametersMessage = sprintf( - '%1$d-%2$d', - $minArgs, - $maxArgs - ); - } - - $message = ($expectedParametersMessage === 1) - ? 'Callback expects %1$d parameter, $accepted_args is set to %2$d.' - : 'Callback expects %1$s parameters, $accepted_args is set to %2$d.'; } throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException( From 4e16fd1283c8b108a67dba9f4dbae947dbcabb9d Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sun, 31 Jul 2022 01:42:36 +0100 Subject: [PATCH 46/55] I'm still not entirely sure why this works, but it does. Props @herndlm. --- composer.json | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/composer.json b/composer.json index bd2e0f3..20e18af 100644 --- a/composer.json +++ b/composer.json @@ -24,6 +24,11 @@ "phpunit/phpunit": "^8 || ^9", "szepeviktor/phpcs-psr-12-neutron-hybrid-ruleset": "^0.6" }, + "autoload-dev": { + "classmap": [ + "tests/" + ] + }, "autoload": { "psr-4": { "SzepeViktor\\PHPStan\\WordPress\\": "src/" From 448546fa352c708f214b4c7aafb918d2a45c965e Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sun, 31 Jul 2022 02:22:59 +0100 Subject: [PATCH 47/55] Another test for an action with a return value. --- tests/HookCallbackRuleTest.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index 51200c3..c08fdf1 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -104,6 +104,10 @@ public function testRule(): void 'Callback expects 0-3 parameters, $accepted_args is set to 4.', 92, ], + [ + 'Action callback returns 123 but should not return anything.', + 97, + ], ] ); } From b3d50cf81421dea3af8497bac03f3afc83bc83f4 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sun, 31 Jul 2022 02:23:17 +0100 Subject: [PATCH 48/55] More variadic handling. --- src/HookCallbackRule.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index e83958d..77b7e10 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -127,6 +127,10 @@ static function (\PHPStan\Reflection\ParameterReflection $parameter): bool { return; } + if (($acceptedArgsParam >= $minArgs) && $callbackAcceptor->isVariadic()) { + return; + } + if ($minArgs === 0 && $acceptedArgsParam === 1) { return; } From 463075ac11cbd7b6808fcc1d276e80ffd83a8a65 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Sun, 31 Jul 2022 11:45:17 +0100 Subject: [PATCH 49/55] Turns out PHPStan handles typed and untyped functions differently. --- tests/HookCallbackRuleTest.php | 4 +-- tests/data/hook-callback.php | 49 ++++++++++++++++++---------------- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index c08fdf1..15d8969 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -89,7 +89,7 @@ public function testRule(): void 78, ], [ - 'Action callback returns 123 but should not return anything.', + 'Action callback returns int but should not return anything.', 81, ], [ @@ -105,7 +105,7 @@ public function testRule(): void 92, ], [ - 'Action callback returns 123 but should not return anything.', + 'Action callback returns int but should not return anything.', 97, ], ] diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 0f07f2c..92e4181 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -72,16 +72,16 @@ }); // Filter callback return statement is missing. -add_filter('filter', __NAMESPACE__ . '\\no_return_value'); +add_filter('filter', __NAMESPACE__ . '\\no_return_value_typed'); // Action callback returns false but should not return anything. add_action('action', '__return_false'); -// Action callback returns 123 but should not return anything. -add_action('action', new TestInvokable(), 10, 2); +// Action callback returns int but should not return anything. +add_action('action', new TestInvokableTyped(), 10, 2); // Callback expects at least 1 parameter, $accepted_args is set to 0. -add_filter('filter', __NAMESPACE__ . '\\filter_variadic', 10, 0); +add_filter('filter', __NAMESPACE__ . '\\filter_variadic_typed', 10, 0); // Callback expects at least 1 parameter, $accepted_args is set to 0. add_filter('filter', function( $one, ...$two ) { @@ -93,6 +93,9 @@ return 123; }, 10, 4); +// Action callback returns int but should not return anything. +add_action('action', __NAMESPACE__ . '\\return_value_typed'); + /** * Incorrect usage that's handled by PHPStan: * @@ -110,10 +113,10 @@ add_filter('filter', __NAMESPACE__ . '\\i_do_not_exist'); // Invalid callback: -add_filter('filter', [new TestClass, 'i_do_not_exist']); +add_filter('filter', [new TestClassTyped, 'i_do_not_exist']); // Invalid callback: -add_filter('filter', new TestClass); +add_filter('filter', new TestClassTyped); // Unknown callback: add_filter('filter', $_GET['callback']); @@ -172,7 +175,7 @@ }); add_action('action', function() { }); -add_action('action', __NAMESPACE__ . '\\no_return_value'); +add_action('action', __NAMESPACE__ . '\\no_return_value_typed'); // Filter callback may exit, unfortunately add_filter('filter', function(array $classes) { @@ -192,42 +195,42 @@ // Various callback types add_filter('filter', '__return_false'); -add_filter('filter', __NAMESPACE__ . '\\return_value'); -add_filter('filter', new TestInvokable(), 10, 2); -add_filter('filter', [new TestClass, 'foo']); +add_filter('filter', __NAMESPACE__ . '\\return_value_typed'); +add_filter('filter', new TestInvokableTyped(), 10, 2); +add_filter('filter', [new TestClassTyped, 'foo']); -$test_class = new TestClass(); +$test_class = new TestClassTyped(); add_filter('filter', [$test_class, 'foo']); // Variadic callbacks -add_filter('filter', __NAMESPACE__ . '\\filter_variadic'); -add_filter('filter', __NAMESPACE__ . '\\filter_variadic', 10, 1); -add_filter('filter', __NAMESPACE__ . '\\filter_variadic', 10, 2); -add_filter('filter', __NAMESPACE__ . '\\filter_variadic', 10, 999); +add_filter('filter', __NAMESPACE__ . '\\filter_variadic_typed'); +add_filter('filter', __NAMESPACE__ . '\\filter_variadic_typed', 10, 1); +add_filter('filter', __NAMESPACE__ . '\\filter_variadic_typed', 10, 2); +add_filter('filter', __NAMESPACE__ . '\\filter_variadic_typed', 10, 999); /** * Symbol definitions for use in these tests. */ -function no_return_value( $value ) {} +function no_return_value_typed( $value ) : void {} -function return_value() { +function return_value_typed() : int { return 123; } -function filter_variadic( $one, ...$two ) { - return $one; +function filter_variadic_typed( $one, ...$two ) : int { + return 123; } -class TestInvokable { - public function __invoke($one, $two) { +class TestInvokableTyped { + public function __invoke($one, $two) : int { return 123; } } -class TestClass { - public function foo() { +class TestClassTyped { + public function foo() : int { return 123; } } From e6513f1f20bd35497aa4ad62325d628e669d6e42 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Mon, 1 Aug 2022 00:49:27 +0100 Subject: [PATCH 50/55] Partly broken callbacks will trigger an error in PHPStan so we don't need to handle them. --- tests/HookCallbackRuleTest.php | 22 +++++++++------------- tests/data/hook-callback.php | 14 +++++++------- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index 15d8969..35b98a5 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -68,45 +68,41 @@ public function testRule(): void 'Callback expects 2-3 parameters, $accepted_args is set to 4.', 51, ], - [ - 'Filter callback return statement is missing.', - 56, - ], [ 'Action callback returns true but should not return anything.', - 63, + 56, ], [ 'Action callback returns true but should not return anything.', - 68, + 61, ], [ 'Filter callback return statement is missing.', - 75, + 68, ], [ 'Action callback returns false but should not return anything.', - 78, + 71, ], [ 'Action callback returns int but should not return anything.', - 81, + 74, ], [ 'Callback expects at least 1 parameter, $accepted_args is set to 0.', - 84, + 77, ], [ 'Callback expects at least 1 parameter, $accepted_args is set to 0.', - 87, + 80, ], [ 'Callback expects 0-3 parameters, $accepted_args is set to 4.', - 92, + 85, ], [ 'Action callback returns int but should not return anything.', - 97, + 90, ], ] ); diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 92e4181..8a78794 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -52,13 +52,6 @@ return 123; }, 10, 4); -// Filter callback return statement is missing. -add_filter('filter', function($class) { - if ($class) { - return []; - } -}); - // Action callback returns true but should not return anything. add_action('action', function() { return true; @@ -129,6 +122,13 @@ return 123; }, 10, false); +// Filter callback return statement may be missing. +add_filter('filter', function($class) { + if ($class) { + return []; + } +}); + /** * Correct usage: */ From 08f34f9dd94b8e1463b392e39f2a1668069fbca0 Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Mon, 1 Aug 2022 00:56:49 +0100 Subject: [PATCH 51/55] Terminology. --- src/HookCallbackRule.php | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 77b7e10..a29739a 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -96,20 +96,20 @@ public function processNode(Node $node, Scope $scope): array return []; } - protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg $arg): void + protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg $acceptedArgsParam): void { - $acceptedArgsParam = 1; + $acceptedArgs = 1; - if (isset($arg)) { - $acceptedArgsParam = null; - $argumentType = $this->currentScope->getType($arg->value); + if (isset($acceptedArgsParam)) { + $acceptedArgs = null; + $argumentType = $this->currentScope->getType($acceptedArgsParam->value); if ($argumentType instanceof ConstantIntegerType) { - $acceptedArgsParam = $argumentType->getValue(); + $acceptedArgs = $argumentType->getValue(); } } - if ($acceptedArgsParam === null) { + if ($acceptedArgs === null) { return; } @@ -123,15 +123,15 @@ static function (\PHPStan\Reflection\ParameterReflection $parameter): bool { $maxArgs = count($allParameters); $minArgs = count($requiredParameters); - if (($acceptedArgsParam >= $minArgs) && ($acceptedArgsParam <= $maxArgs)) { + if (($acceptedArgs >= $minArgs) && ($acceptedArgs <= $maxArgs)) { return; } - if (($acceptedArgsParam >= $minArgs) && $callbackAcceptor->isVariadic()) { + if (($acceptedArgs >= $minArgs) && $callbackAcceptor->isVariadic()) { return; } - if ($minArgs === 0 && $acceptedArgsParam === 1) { + if ($minArgs === 0 && $acceptedArgs === 1) { return; } @@ -159,7 +159,7 @@ static function (\PHPStan\Reflection\ParameterReflection $parameter): bool { sprintf( $message, $expectedParametersMessage, - $acceptedArgsParam + $acceptedArgs ) ); } From b36e9d6fac1c79cdbd4690fe827a6416c85e19bc Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Mon, 1 Aug 2022 01:11:47 +0100 Subject: [PATCH 52/55] Refactoring. --- src/HookCallbackRule.php | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index a29739a..ff559f2 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -96,7 +96,7 @@ public function processNode(Node $node, Scope $scope): array return []; } - protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg $acceptedArgsParam): void + protected function getAcceptedArgs(?Arg $acceptedArgsParam): ?int { $acceptedArgs = 1; @@ -109,6 +109,13 @@ protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg } } + return $acceptedArgs; + } + + protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg $acceptedArgsParam): void + { + $acceptedArgs = $this->getAcceptedArgs($acceptedArgsParam); + if ($acceptedArgs === null) { return; } @@ -135,6 +142,18 @@ static function (\PHPStan\Reflection\ParameterReflection $parameter): bool { return; } + throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException( + self::buildParameterCountMessage( + $minArgs, + $maxArgs, + $acceptedArgs, + $callbackAcceptor, + ) + ); + } + + protected static function buildParameterCountMessage(int $minArgs, int $maxArgs, int $acceptedArgs, ParametersAcceptor $callbackAcceptor): string + { $expectedParametersMessage = $minArgs; if ($maxArgs !== $minArgs) { @@ -155,12 +174,10 @@ static function (\PHPStan\Reflection\ParameterReflection $parameter): bool { : 'Callback expects at least %1$s parameters, $accepted_args is set to %2$d.'; } - throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException( - sprintf( - $message, - $expectedParametersMessage, - $acceptedArgs - ) + return sprintf( + $message, + $expectedParametersMessage, + $acceptedArgs ); } From f47feef0a81509519bd4aba753dc7c6f652f5e0e Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Mon, 1 Aug 2022 01:16:28 +0100 Subject: [PATCH 53/55] PHP 7.2 compatibility. --- src/HookCallbackRule.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index ff559f2..5166d94 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -147,7 +147,7 @@ static function (\PHPStan\Reflection\ParameterReflection $parameter): bool { $minArgs, $maxArgs, $acceptedArgs, - $callbackAcceptor, + $callbackAcceptor ) ); } From 300f322896973dd17df354cc73370d510c29412d Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Tue, 16 Aug 2022 22:40:27 +0100 Subject: [PATCH 54/55] Reorder the processing so the return type is checked first. --- src/HookCallbackRule.php | 4 ++-- tests/HookCallbackRuleTest.php | 8 ++++++++ tests/data/hook-callback.php | 8 +++++++- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/HookCallbackRule.php b/src/HookCallbackRule.php index 5166d94..39bf49e 100644 --- a/src/HookCallbackRule.php +++ b/src/HookCallbackRule.php @@ -82,13 +82,13 @@ public function processNode(Node $node, Scope $scope): array $callbackAcceptor = ParametersAcceptorSelector::selectSingle($callbackType->getCallableParametersAcceptors($scope)); try { - $this->validateParamCount($callbackAcceptor, $args[3] ?? null); - if ($name->toString() === 'add_action') { $this->validateActionReturnType($callbackAcceptor); } else { $this->validateFilterReturnType($callbackAcceptor); } + + $this->validateParamCount($callbackAcceptor, $args[3] ?? null); } catch (\SzepeViktor\PHPStan\WordPress\HookCallbackException $e) { return [RuleErrorBuilder::message($e->getMessage())->build()]; } diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index 35b98a5..976d460 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -104,6 +104,14 @@ public function testRule(): void 'Action callback returns int but should not return anything.', 90, ], + [ + 'Callback expects 0 parameters, $accepted_args is set to 2.', + 93, + ], + [ + 'Action callback returns false but should not return anything.', + 96, + ], ] ); } diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index 8a78794..fec125a 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -89,6 +89,12 @@ // Action callback returns int but should not return anything. add_action('action', __NAMESPACE__ . '\\return_value_typed'); +// Callback expects 0 parameters, $accepted_args is set to 2. +add_filter('filter', '__return_false', 10, 2); + +// Action callback returns false but should not return anything (with incorrect number of accepted args). +add_action('action', '__return_false', 10, 2); + /** * Incorrect usage that's handled by PHPStan: * @@ -114,7 +120,7 @@ // Unknown callback: add_filter('filter', $_GET['callback']); -// Invalid parameters: +// Valid callback but with invalid parameters: add_filter('filter', function() { return 123; }, false); From fc456bb82edcb391bb2ee654454d21aac0c9c67e Mon Sep 17 00:00:00 2001 From: John Blackbourn Date: Tue, 16 Aug 2022 22:49:25 +0100 Subject: [PATCH 55/55] More tests. --- tests/HookCallbackRuleTest.php | 4 ++++ tests/data/hook-callback.php | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/tests/HookCallbackRuleTest.php b/tests/HookCallbackRuleTest.php index 976d460..cc161fe 100644 --- a/tests/HookCallbackRuleTest.php +++ b/tests/HookCallbackRuleTest.php @@ -112,6 +112,10 @@ public function testRule(): void 'Action callback returns false but should not return anything.', 96, ], + [ + 'Filter callback return statement is missing.', + 99, + ], ] ); } diff --git a/tests/data/hook-callback.php b/tests/data/hook-callback.php index fec125a..bf57d22 100644 --- a/tests/data/hook-callback.php +++ b/tests/data/hook-callback.php @@ -95,6 +95,9 @@ // Action callback returns false but should not return anything (with incorrect number of accepted args). add_action('action', '__return_false', 10, 2); +// Filter callback return statement is missing. +add_filter('filter', __NAMESPACE__ . '\\no_return_value_untyped'); + /** * Incorrect usage that's handled by PHPStan: * @@ -135,6 +138,14 @@ } }); +// Incorrect function return type: +add_filter('filter', function(array $classes): array { + return 123; +}); + +// Callback function with no declared return type. +add_action('action', __NAMESPACE__ . '\\return_value_untyped'); + /** * Correct usage: */ @@ -181,6 +192,8 @@ }); add_action('action', function() { }); +add_action('action', function(): void { +}); add_action('action', __NAMESPACE__ . '\\no_return_value_typed'); // Filter callback may exit, unfortunately @@ -225,6 +238,12 @@ function return_value_typed() : int { return 123; } +function no_return_value_untyped( $value ) {} + +function return_value_untyped() { + return 123; +} + function filter_variadic_typed( $one, ...$two ) : int { return 123; }