Skip to content

Commit e150358

Browse files
johnbillionszepeviktor
johnbillion
and
szepeviktor
authored
Add type checking for hook callbacks in add_action and add_filter (#107)
* Setup the hook callback rule and test. * Add the wp-hooks library. * Start adding tests. * Preparing more tests. * If we don't have enough arguments, bail out and let PHPStan handle the error. * If the callback is not valid, bail out and let PHPStan handle the error: * Simplify this. * More valid test cases. * Update some test line numbers. * First pass at detecting mismatching accepted and expected argument counts. * Remove some accidental duplicates. * Fix some logic. * Let's split this up before it gets out of hand. * Reduce this down. * More tidying up. * We're going to be needing this in multiple methods soon. * Prep for callback return type checking, not yet functional. * Split action return type assertion into its own method. * Bring this message more inline with those used by PHPStan. * Add detection for filter callbacks with no return value. * Don't need this just yet. * Use early exit to reduce code nesting. * Remove unused imports. * Yoda comparisons are disallowed. * Coding standards. * Remove unused code. * Use early return to reduce code nesting. * Remove more unused code. * Renaming. * Use early return to reduce code nesting. * Tidying up. * Correct logic introduced during refactoring. * Exclude "maybe" callables. * More handling for parameter counts. * More handling for optional parameters in hook callbacks. * Make the tests more manageable. * Tests for more callback types. * Tidying up. * This isn't used. * More test cases. * Tests for variadic callbacks. * More specific handling for variadic callbacks. * More tests. * The hooks names are not relevant to the tests. * Remove an `else`. * I'm still not entirely sure why this works, but it does. Props @herndlm. * Another test for an action with a return value. * More variadic handling. * Turns out PHPStan handles typed and untyped functions differently. * Partly broken callbacks will trigger an error in PHPStan so we don't need to handle them. * Terminology. * Refactoring. * PHP 7.2 compatibility. * Reorder the processing so the return type is checked first. * More tests. Co-authored-by: Viktor Szépe <[email protected]>
1 parent e644df7 commit e150358

File tree

6 files changed

+626
-0
lines changed

6 files changed

+626
-0
lines changed

‎composer.json

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,11 @@
2424
"phpunit/phpunit": "^8 || ^9",
2525
"szepeviktor/phpcs-psr-12-neutron-hybrid-ruleset": "^0.6"
2626
},
27+
"autoload-dev": {
28+
"classmap": [
29+
"tests/"
30+
]
31+
},
2732
"autoload": {
2833
"psr-4": {
2934
"SzepeViktor\\PHPStan\\WordPress\\": "src/"

‎extension.neon

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ services:
9292
tags:
9393
- phpstan.parser.richParserNodeVisitor
9494
rules:
95+
- SzepeViktor\PHPStan\WordPress\HookCallbackRule
9596
- SzepeViktor\PHPStan\WordPress\HookDocsRule
9697
- SzepeViktor\PHPStan\WordPress\IsWpErrorRule
9798
parameters:

‎src/HookCallbackException.php

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
<?php
2+
3+
/**
4+
* Exception thrown when validating a hook callback.
5+
*/
6+
7+
declare(strict_types=1);
8+
9+
namespace SzepeViktor\PHPStan\WordPress;
10+
11+
// phpcs:ignore SlevomatCodingStandard.Classes.SuperfluousExceptionNaming.SuperfluousSuffix
12+
class HookCallbackException extends \DomainException
13+
{
14+
}

‎src/HookCallbackRule.php

Lines changed: 218 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,218 @@
1+
<?php
2+
3+
/**
4+
* Custom rule to validate the callback function for WordPress core actions and filters.
5+
*/
6+
7+
declare(strict_types=1);
8+
9+
namespace SzepeViktor\PHPStan\WordPress;
10+
11+
use PhpParser\Node;
12+
use PhpParser\Node\Arg;
13+
use PhpParser\Node\Expr\FuncCall;
14+
use PHPStan\Analyser\Scope;
15+
use PHPStan\Reflection\ParametersAcceptor;
16+
use PHPStan\Reflection\ParametersAcceptorSelector;
17+
use PHPStan\Rules\RuleErrorBuilder;
18+
use PHPStan\Rules\RuleLevelHelper;
19+
use PHPStan\Type\Constant\ConstantIntegerType;
20+
use PHPStan\Type\VerbosityLevel;
21+
use PHPStan\Type\VoidType;
22+
23+
/**
24+
* @implements \PHPStan\Rules\Rule<\PhpParser\Node\Expr\FuncCall>
25+
*/
26+
class HookCallbackRule implements \PHPStan\Rules\Rule
27+
{
28+
private const SUPPORTED_FUNCTIONS = [
29+
'add_filter',
30+
'add_action',
31+
];
32+
33+
/** @var \PHPStan\Rules\RuleLevelHelper */
34+
protected $ruleLevelHelper;
35+
36+
/** @var \PHPStan\Analyser\Scope */
37+
protected $currentScope;
38+
39+
public function __construct(
40+
RuleLevelHelper $ruleLevelHelper
41+
) {
42+
$this->ruleLevelHelper = $ruleLevelHelper;
43+
}
44+
45+
public function getNodeType(): string
46+
{
47+
return FuncCall::class;
48+
}
49+
50+
/**
51+
* @param \PhpParser\Node\Expr\FuncCall $node
52+
* @param \PHPStan\Analyser\Scope $scope
53+
* @return array<int, \PHPStan\Rules\RuleError>
54+
*/
55+
public function processNode(Node $node, Scope $scope): array
56+
{
57+
$name = $node->name;
58+
$this->currentScope = $scope;
59+
60+
if (!($name instanceof \PhpParser\Node\Name)) {
61+
return [];
62+
}
63+
64+
if (!in_array($name->toString(), self::SUPPORTED_FUNCTIONS, true)) {
65+
return [];
66+
}
67+
68+
$args = $node->getArgs();
69+
70+
// If we don't have enough arguments, let PHPStan handle the error:
71+
if (count($args) < 2) {
72+
return [];
73+
}
74+
75+
$callbackType = $scope->getType($args[1]->value);
76+
77+
// If the callback is not valid, let PHPStan handle the error:
78+
if (! $callbackType->isCallable()->yes()) {
79+
return [];
80+
}
81+
82+
$callbackAcceptor = ParametersAcceptorSelector::selectSingle($callbackType->getCallableParametersAcceptors($scope));
83+
84+
try {
85+
if ($name->toString() === 'add_action') {
86+
$this->validateActionReturnType($callbackAcceptor);
87+
} else {
88+
$this->validateFilterReturnType($callbackAcceptor);
89+
}
90+
91+
$this->validateParamCount($callbackAcceptor, $args[3] ?? null);
92+
} catch (\SzepeViktor\PHPStan\WordPress\HookCallbackException $e) {
93+
return [RuleErrorBuilder::message($e->getMessage())->build()];
94+
}
95+
96+
return [];
97+
}
98+
99+
protected function getAcceptedArgs(?Arg $acceptedArgsParam): ?int
100+
{
101+
$acceptedArgs = 1;
102+
103+
if (isset($acceptedArgsParam)) {
104+
$acceptedArgs = null;
105+
$argumentType = $this->currentScope->getType($acceptedArgsParam->value);
106+
107+
if ($argumentType instanceof ConstantIntegerType) {
108+
$acceptedArgs = $argumentType->getValue();
109+
}
110+
}
111+
112+
return $acceptedArgs;
113+
}
114+
115+
protected function validateParamCount(ParametersAcceptor $callbackAcceptor, ?Arg $acceptedArgsParam): void
116+
{
117+
$acceptedArgs = $this->getAcceptedArgs($acceptedArgsParam);
118+
119+
if ($acceptedArgs === null) {
120+
return;
121+
}
122+
123+
$allParameters = $callbackAcceptor->getParameters();
124+
$requiredParameters = array_filter(
125+
$allParameters,
126+
static function (\PHPStan\Reflection\ParameterReflection $parameter): bool {
127+
return ! $parameter->isOptional();
128+
}
129+
);
130+
$maxArgs = count($allParameters);
131+
$minArgs = count($requiredParameters);
132+
133+
if (($acceptedArgs >= $minArgs) && ($acceptedArgs <= $maxArgs)) {
134+
return;
135+
}
136+
137+
if (($acceptedArgs >= $minArgs) && $callbackAcceptor->isVariadic()) {
138+
return;
139+
}
140+
141+
if ($minArgs === 0 && $acceptedArgs === 1) {
142+
return;
143+
}
144+
145+
throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException(
146+
self::buildParameterCountMessage(
147+
$minArgs,
148+
$maxArgs,
149+
$acceptedArgs,
150+
$callbackAcceptor
151+
)
152+
);
153+
}
154+
155+
protected static function buildParameterCountMessage(int $minArgs, int $maxArgs, int $acceptedArgs, ParametersAcceptor $callbackAcceptor): string
156+
{
157+
$expectedParametersMessage = $minArgs;
158+
159+
if ($maxArgs !== $minArgs) {
160+
$expectedParametersMessage = sprintf(
161+
'%1$d-%2$d',
162+
$minArgs,
163+
$maxArgs
164+
);
165+
}
166+
167+
$message = ($expectedParametersMessage === 1)
168+
? 'Callback expects %1$d parameter, $accepted_args is set to %2$d.'
169+
: 'Callback expects %1$s parameters, $accepted_args is set to %2$d.';
170+
171+
if ($callbackAcceptor->isVariadic()) {
172+
$message = ($minArgs === 1)
173+
? 'Callback expects at least %1$d parameter, $accepted_args is set to %2$d.'
174+
: 'Callback expects at least %1$s parameters, $accepted_args is set to %2$d.';
175+
}
176+
177+
return sprintf(
178+
$message,
179+
$expectedParametersMessage,
180+
$acceptedArgs
181+
);
182+
}
183+
184+
protected function validateActionReturnType(ParametersAcceptor $callbackAcceptor): void
185+
{
186+
$acceptedType = $callbackAcceptor->getReturnType();
187+
$accepted = $this->ruleLevelHelper->accepts(
188+
new VoidType(),
189+
$acceptedType,
190+
true
191+
);
192+
193+
if ($accepted) {
194+
return;
195+
}
196+
197+
throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException(
198+
sprintf(
199+
'Action callback returns %s but should not return anything.',
200+
$acceptedType->describe(VerbosityLevel::getRecommendedLevelByType($acceptedType))
201+
)
202+
);
203+
}
204+
205+
protected function validateFilterReturnType(ParametersAcceptor $callbackAcceptor): void
206+
{
207+
$returnType = $callbackAcceptor->getReturnType();
208+
$isVoidSuperType = $returnType->isSuperTypeOf(new VoidType());
209+
210+
if (! $isVoidSuperType->yes()) {
211+
return;
212+
}
213+
214+
throw new \SzepeViktor\PHPStan\WordPress\HookCallbackException(
215+
'Filter callback return statement is missing.'
216+
);
217+
}
218+
}

‎tests/HookCallbackRuleTest.php

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace SzepeViktor\PHPStan\WordPress\Tests;
6+
7+
use PHPStan\Rules\RuleLevelHelper;
8+
use SzepeViktor\PHPStan\WordPress\HookCallbackRule;
9+
10+
/**
11+
* @extends \PHPStan\Testing\RuleTestCase<\SzepeViktor\PHPStan\WordPress\HookCallbackRule>
12+
*/
13+
class HookCallbackRuleTest extends \PHPStan\Testing\RuleTestCase
14+
{
15+
protected function getRule(): \PHPStan\Rules\Rule
16+
{
17+
/** @var \PHPStan\Rules\RuleLevelHelper */
18+
$ruleLevelHelper = self::getContainer()->getByType(RuleLevelHelper::class);
19+
20+
// getRule() method needs to return an instance of the tested rule
21+
return new HookCallbackRule($ruleLevelHelper);
22+
}
23+
24+
// phpcs:ignore NeutronStandard.Functions.LongFunction.LongFunction
25+
public function testRule(): void
26+
{
27+
// first argument: path to the example file that contains some errors that should be reported by HookCallbackRule
28+
// second argument: an array of expected errors,
29+
// each error consists of the asserted error message, and the asserted error file line
30+
$this->analyse(
31+
[
32+
__DIR__ . '/data/hook-callback.php',
33+
],
34+
[
35+
[
36+
'Filter callback return statement is missing.',
37+
17,
38+
],
39+
[
40+
'Filter callback return statement is missing.',
41+
20,
42+
],
43+
[
44+
'Filter callback return statement is missing.',
45+
23,
46+
],
47+
[
48+
'Callback expects 1 parameter, $accepted_args is set to 0.',
49+
26,
50+
],
51+
[
52+
'Callback expects 1 parameter, $accepted_args is set to 2.',
53+
31,
54+
],
55+
[
56+
'Callback expects 0-1 parameters, $accepted_args is set to 2.',
57+
36,
58+
],
59+
[
60+
'Callback expects 2 parameters, $accepted_args is set to 1.',
61+
41,
62+
],
63+
[
64+
'Callback expects 2-4 parameters, $accepted_args is set to 1.',
65+
46,
66+
],
67+
[
68+
'Callback expects 2-3 parameters, $accepted_args is set to 4.',
69+
51,
70+
],
71+
[
72+
'Action callback returns true but should not return anything.',
73+
56,
74+
],
75+
[
76+
'Action callback returns true but should not return anything.',
77+
61,
78+
],
79+
[
80+
'Filter callback return statement is missing.',
81+
68,
82+
],
83+
[
84+
'Action callback returns false but should not return anything.',
85+
71,
86+
],
87+
[
88+
'Action callback returns int but should not return anything.',
89+
74,
90+
],
91+
[
92+
'Callback expects at least 1 parameter, $accepted_args is set to 0.',
93+
77,
94+
],
95+
[
96+
'Callback expects at least 1 parameter, $accepted_args is set to 0.',
97+
80,
98+
],
99+
[
100+
'Callback expects 0-3 parameters, $accepted_args is set to 4.',
101+
85,
102+
],
103+
[
104+
'Action callback returns int but should not return anything.',
105+
90,
106+
],
107+
[
108+
'Callback expects 0 parameters, $accepted_args is set to 2.',
109+
93,
110+
],
111+
[
112+
'Action callback returns false but should not return anything.',
113+
96,
114+
],
115+
[
116+
'Filter callback return statement is missing.',
117+
99,
118+
],
119+
]
120+
);
121+
}
122+
123+
public static function getAdditionalConfigFiles(): array
124+
{
125+
return [dirname(__DIR__) . '/vendor/szepeviktor/phpstan-wordpress/extension.neon'];
126+
}
127+
}

0 commit comments

Comments
 (0)