Skip to content

Commit ea78bdc

Browse files
mglamanmglaman
committed
Fix inspection for #access_callback and ThisType
1 parent 6d3cd40 commit ea78bdc

File tree

3 files changed

+134
-108
lines changed

3 files changed

+134
-108
lines changed

‎src/Rules/Drupal/RenderCallbackRule.php

Lines changed: 113 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use PHPStan\Analyser\Scope;
88
use PHPStan\Reflection\ReflectionProvider;
99
use PHPStan\Rules\Rule;
10+
use PHPStan\Rules\RuleError;
1011
use PHPStan\Rules\RuleErrorBuilder;
1112
use PHPStan\Type\ClosureType;
1213
use PHPStan\Type\Constant\ConstantArrayType;
@@ -15,6 +16,7 @@
1516
use PHPStan\Type\Generic\GenericClassStringType;
1617
use PHPStan\Type\IntersectionType;
1718
use PHPStan\Type\ObjectType;
19+
use PHPStan\Type\ThisType;
1820
use PHPStan\Type\Type;
1921
use PHPStan\Type\UnionType;
2022
use PHPStan\Type\VerbosityLevel;
@@ -44,8 +46,9 @@ public function processNode(Node $node, Scope $scope): array
4446
if (!$key instanceof Node\Scalar\String_) {
4547
return [];
4648
}
47-
// @see https://www.drupal.org/node/2966725
4849

50+
// @todo this should be 3 rules.
51+
// @see https://www.drupal.org/node/2966725
4952
$keysToCheck = ['#pre_render', '#post_render', '#access_callback', '#lazy_builder'];
5053
$keySearch = array_search($key->value, $keysToCheck, true);
5154
if ($keySearch === false) {
@@ -54,121 +57,132 @@ public function processNode(Node $node, Scope $scope): array
5457
$keyChecked = $keysToCheck[$keySearch];
5558

5659
$value = $node->value;
57-
if (!$value instanceof Node\Expr\Array_) {
58-
return [
59-
RuleErrorBuilder::message(sprintf('The "%s" render array value expects an array of callbacks.', $keyChecked))
60-
->line($node->getLine())->build()
61-
];
62-
}
63-
if (count($value->items) === 0) {
64-
return [];
60+
61+
$errors = [];
62+
63+
if ($keyChecked === '#lazy_builder') {
64+
if (!$value instanceof Node\Expr\Array_) {
65+
return [
66+
RuleErrorBuilder::message(sprintf('The "%s" expects a callable array with arguments.', $keyChecked))
67+
->line($node->getLine())->build()
68+
];
69+
}
70+
if (count($value->items) === 0) {
71+
return [];
72+
}
73+
if ($value->items[0] === null) {
74+
return [];
75+
}
76+
$errors[] = $this->doProcessNode($value->items[0]->value, $scope, $keyChecked, 0);
77+
} elseif ($keyChecked === '#access_callback') {
78+
$errors[] = $this->doProcessNode($value, $scope, $keyChecked, 0);
79+
} else {
80+
if (!$value instanceof Node\Expr\Array_) {
81+
return [
82+
RuleErrorBuilder::message(sprintf('The "%s" render array value expects an array of callbacks.', $keyChecked))
83+
->line($node->getLine())->build()
84+
];
85+
}
86+
if (count($value->items) === 0) {
87+
return [];
88+
}
89+
foreach ($value->items as $pos => $item) {
90+
if (!$item instanceof Node\Expr\ArrayItem) {
91+
continue;
92+
}
93+
$errors[] = $this->doProcessNode($item->value, $scope, $keyChecked, $pos);
94+
}
6595
}
96+
return array_filter($errors);
97+
}
6698

99+
private function doProcessNode(Node\Expr $node, Scope $scope, string $keyChecked, int $pos): ?RuleError
100+
{
67101
$trustedCallbackType = new UnionType([
68102
new ObjectType('Drupal\Core\Security\TrustedCallbackInterface'),
69103
new ObjectType('Drupal\Core\Render\Element\RenderCallbackInterface'),
70104
]);
71-
$errors = [];
72-
foreach ($value->items as $pos => $item) {
73-
if (!$item instanceof Node\Expr\ArrayItem) {
74-
continue;
75-
}
76-
// '#lazy_builder' has two items, callback and args. Others are direct callbacks.
77-
// Lazy builder in Renderer: $elements['#lazy_builder'][0], $elements['#lazy_builder'][1]
78-
if ($keyChecked === '#lazy_builder') {
79-
if (!$item->value instanceof Node\Expr\Array_) {
80-
$errors[] = RuleErrorBuilder::message(
81-
sprintf("%s callback %s at key '%s' is not callable.", $keyChecked, $scope->getType($item->value)->describe(VerbosityLevel::value()), $pos)
82-
)->line($item->value->getLine())->build();
83-
continue;
84-
}
85105

86-
if (count($item->value->items) !== 2 || $item->value->items[0] === null) {
87-
$errors[] = RuleErrorBuilder::message(
88-
sprintf("%s callback %s at key '%s' is not valid. First value must be a callback and second value its arguments.", $keyChecked, $scope->getType($item->value)->describe(VerbosityLevel::value()), $pos)
89-
)->line($item->value->getLine())->build();
90-
continue;
91-
}
92-
// Replace $item with our nested callback.
93-
$item = $item->value->items[0];
106+
$errorLine = $node->getLine();
107+
$type = $this->getType($node, $scope);
108+
109+
if ($type instanceof ConstantStringType) {
110+
if (!$type->isCallable()->yes()) {
111+
return RuleErrorBuilder::message(
112+
sprintf("%s callback %s at key '%s' is not callable.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
113+
)->line($errorLine)->build();
114+
}
115+
// We can determine if the callback is callable through the type system. However, we cannot determine
116+
// if it is just a function or a static class call (MyClass::staticFunc).
117+
if ($this->reflectionProvider->hasFunction(new \PhpParser\Node\Name($type->getValue()), null)) {
118+
return RuleErrorBuilder::message(
119+
sprintf("%s callback %s at key '%s' is not trusted.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
120+
)->line($errorLine)
121+
->tip('Change record: https://www.drupal.org/node/2966725.')
122+
->build();
123+
}
124+
} elseif ($type instanceof ConstantArrayType) {
125+
if (!$type->isCallable()->yes()) {
126+
return RuleErrorBuilder::message(
127+
sprintf("%s callback %s at key '%s' is not callable.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
128+
)->line($errorLine)->build();
129+
}
130+
$typeAndMethodName = $type->findTypeAndMethodName();
131+
if ($typeAndMethodName === null) {
132+
throw new \PHPStan\ShouldNotHappenException();
94133
}
95-
$errorLine = $item->value->getLine();
96-
$type = $this->getType($item->value, $scope);
97134

98-
if ($type instanceof ConstantStringType) {
99-
if (!$type->isCallable()->yes()) {
100-
$errors[] = RuleErrorBuilder::message(
101-
sprintf("%s callback %s at key '%s' is not callable.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
102-
)->line($errorLine)->build();
103-
continue;
104-
}
105-
// We can determine if the callback is callable through the type system. However, we cannot determine
106-
// if it is just a function or a static class call (MyClass::staticFunc).
107-
if ($this->reflectionProvider->hasFunction(new \PhpParser\Node\Name($type->getValue()), null)) {
108-
$errors[] = RuleErrorBuilder::message(
109-
sprintf("%s callback %s at key '%s' is not trusted.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
110-
)->line($errorLine)
111-
->tip('Change record: https://www.drupal.org/node/2966725.')
112-
->build();
113-
}
114-
} elseif ($type instanceof ConstantArrayType) {
115-
if (!$type->isCallable()->yes()) {
116-
$errors[] = RuleErrorBuilder::message(
117-
sprintf("%s callback %s at key '%s' is not callable.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
118-
)->line($errorLine)->build();
119-
continue;
120-
}
121-
$typeAndMethodName = $type->findTypeAndMethodName();
122-
if ($typeAndMethodName === null) {
135+
if (!$trustedCallbackType->isSuperTypeOf($typeAndMethodName->getType())->yes()) {
136+
return RuleErrorBuilder::message(
137+
sprintf("%s callback class '%s' at key '%s' does not implement Drupal\Core\Security\TrustedCallbackInterface.", $keyChecked, $typeAndMethodName->getType()->describe(VerbosityLevel::value()), $pos)
138+
)->line($errorLine)->tip('Change record: https://www.drupal.org/node/2966725.')->build();
139+
}
140+
} elseif ($type instanceof ClosureType) {
141+
if ($scope->isInClass()) {
142+
$classReflection = $scope->getClassReflection();
143+
if ($classReflection === null) {
123144
throw new \PHPStan\ShouldNotHappenException();
124145
}
125-
126-
if (!$trustedCallbackType->isSuperTypeOf($typeAndMethodName->getType())->yes()) {
127-
$errors[] = RuleErrorBuilder::message(
128-
sprintf("%s callback class '%s' at key '%s' does not implement Drupal\Core\Security\TrustedCallbackInterface.", $keyChecked, $typeAndMethodName->getType()->describe(VerbosityLevel::value()), $pos)
129-
)->line($errorLine)->tip('Change record: https://www.drupal.org/node/2966725.')->build();
130-
}
131-
} elseif ($type instanceof ClosureType) {
132-
if ($scope->isInClass()) {
133-
$classReflection = $scope->getClassReflection();
134-
if ($classReflection === null) {
135-
throw new \PHPStan\ShouldNotHappenException();
136-
}
137-
$classType = new ObjectType($classReflection->getName());
138-
$formType = new ObjectType('\Drupal\Core\Form\FormInterface');
139-
if ($formType->isSuperTypeOf($classType)->yes()) {
140-
$errors[] = RuleErrorBuilder::message(
141-
sprintf("%s may not contain a closure at key '%s' as forms may be serialized and serialization of closures is not allowed.", $keyChecked, $pos)
142-
)->line($errorLine)->build();
143-
}
144-
}
145-
} elseif ($type instanceof IntersectionType) {
146-
// Try to provide a tip for this weird occurrence.
147-
$tip = '';
148-
if ($item->value instanceof Node\Expr\BinaryOp\Concat) {
149-
$leftStringType = $scope->getType($item->value->left)->toString();
150-
$rightStringType = $scope->getType($item->value->right)->toString();
151-
if ($leftStringType instanceof GenericClassStringType && $rightStringType instanceof ConstantStringType) {
152-
$methodName = str_replace(':', '', $rightStringType->getValue());
153-
$tip = "Refactor concatenation of `static::class` with method name to an array callback: [static::class, '$methodName']";
154-
}
146+
$classType = new ObjectType($classReflection->getName());
147+
$formType = new ObjectType('\Drupal\Core\Form\FormInterface');
148+
if ($formType->isSuperTypeOf($classType)->yes()) {
149+
return RuleErrorBuilder::message(
150+
sprintf("%s may not contain a closure at key '%s' as forms may be serialized and serialization of closures is not allowed.", $keyChecked, $pos)
151+
)->line($errorLine)->build();
155152
}
156-
157-
if ($tip === '') {
158-
$tip = 'If this error is unexpected, open an issue with the error and sample code https://github.com/mglaman/phpstan-drupal/issues/new';
153+
}
154+
} elseif ($type instanceof ThisType) {
155+
if (!$type->isCallable()->yes()) {
156+
return RuleErrorBuilder::message(
157+
sprintf("%s callback %s at key '%s' is not callable.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
158+
)->line($errorLine)->build();
159+
}
160+
} elseif ($type instanceof IntersectionType) {
161+
// Try to provide a tip for this weird occurrence.
162+
$tip = '';
163+
if ($node instanceof Node\Expr\BinaryOp\Concat) {
164+
$leftStringType = $scope->getType($node->left)->toString();
165+
$rightStringType = $scope->getType($node->right)->toString();
166+
if ($leftStringType instanceof GenericClassStringType && $rightStringType instanceof ConstantStringType) {
167+
$methodName = str_replace(':', '', $rightStringType->getValue());
168+
$tip = "Refactor concatenation of `static::class` with method name to an array callback: [static::class, '$methodName']";
159169
}
170+
}
160171

161-
$errors[] = RuleErrorBuilder::message(
162-
sprintf("%s value '%s' at key '%s' is invalid.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
163-
)->line($errorLine)->tip($tip)->build();
164-
} else {
165-
$errors[] = RuleErrorBuilder::message(
166-
sprintf("%s value '%s' at key '%s' is invalid.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
167-
)->line($errorLine)->build();
172+
if ($tip === '') {
173+
$tip = 'If this error is unexpected, open an issue with the error and sample code https://github.com/mglaman/phpstan-drupal/issues/new';
168174
}
175+
176+
return RuleErrorBuilder::message(
177+
sprintf("%s value '%s' at key '%s' is invalid.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
178+
)->line($errorLine)->tip($tip)->build();
179+
} else {
180+
return RuleErrorBuilder::message(
181+
sprintf("%s value '%s' at key '%s' is invalid.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
182+
)->line($errorLine)->build();
169183
}
170184

171-
return $errors;
185+
return null;
172186
}
173187

174188
private function getType(Node\Expr $node, Scope $scope): Type

‎tests/fixtures/drupal/modules/pre_render_callback_rule/src/LazyBuilderWithConstant.php

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,21 @@ class LazyBuilderWithConstant implements TrustedCallbackInterface {
99

1010
public function staticCallback(): array {
1111
return [
12-
'#lazy_builder' => [
13-
[[self::class, 'lazyBuilder'], ['baz', true]],
14-
[[static::class, 'lazyBuilder'], ['mars', false]],
15-
[[$this, 'lazyBuilder'], ['mars', false]],
16-
[self::class . '::lazyBuilder', ['baz', true]],
17-
[static::class . '::lazyBuilder', ['mars', false]],
18-
]
12+
'foo' => [
13+
'#lazy_builder' => [[self::class, 'lazyBuilder'], ['baz', true]],
14+
],
15+
'bar' => [
16+
'#lazy_builder' => [[static::class, 'lazyBuilder'], ['mars', false]],
17+
],
18+
'baz' => [
19+
'#lazy_builder' => [[$this, 'lazyBuilder'], ['mars', false]],
20+
],
21+
'abc' => [
22+
'#lazy_builder' => [self::class . '::lazyBuilder', ['baz', true]],
23+
],
24+
'def' => [
25+
'#lazy_builder' => [static::class . '::lazyBuilder', ['mars', false]],
26+
],
1927
];
2028
}
2129

‎tests/src/Rules/PreRenderCallbackRuleTest.php

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,8 @@ public function fileData(): \Generator
8181
__DIR__ . '/../../fixtures/drupal/modules/pre_render_callback_rule/src/LazyBuilderWithConstant.php',
8282
[
8383
[
84-
"#lazy_builder value 'non-empty-string' at key '4' is invalid.",
85-
17,
84+
"#lazy_builder value 'non-empty-string' at key '0' is invalid.",
85+
25,
8686
"Refactor concatenation of `static::class` with method name to an array callback: [static::class, 'lazyBuilder']"
8787
]
8888
]
@@ -96,6 +96,10 @@ public function fileData(): \Generator
9696
]
9797
]
9898
];
99+
yield [
100+
__DIR__ . '/../../fixtures/drupal/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php',
101+
[]
102+
];
99103
}
100104

101105

0 commit comments

Comments
 (0)