Skip to content

Commit 62d9979

Browse files
mglamanmglaman
authored
Merge pull request #304 from mglaman/gh301
Fix #lazy_builder callback analysis and static::class concatenation warning
2 parents 8fcebb8 + cf936a7 commit 62d9979

File tree

4 files changed

+187
-64
lines changed

4 files changed

+187
-64
lines changed

‎src/Rules/Drupal/RenderCallbackRule.php

Lines changed: 122 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,16 @@
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;
1314
use PHPStan\Type\Constant\ConstantIntegerType;
1415
use PHPStan\Type\Constant\ConstantStringType;
16+
use PHPStan\Type\Generic\GenericClassStringType;
17+
use PHPStan\Type\IntersectionType;
1518
use PHPStan\Type\ObjectType;
19+
use PHPStan\Type\ThisType;
1620
use PHPStan\Type\Type;
1721
use PHPStan\Type\UnionType;
1822
use PHPStan\Type\VerbosityLevel;
@@ -42,94 +46,150 @@ public function processNode(Node $node, Scope $scope): array
4246
if (!$key instanceof Node\Scalar\String_) {
4347
return [];
4448
}
49+
50+
// @todo this should be 3 rules.
4551
// @see https://www.drupal.org/node/2966725
46-
$keysToCheck = ['#pre_render', '#post_render', '#lazy_builder', '#access_callback'];
52+
$keysToCheck = ['#pre_render', '#post_render', '#access_callback', '#lazy_builder'];
4753
$keySearch = array_search($key->value, $keysToCheck, true);
4854
if ($keySearch === false) {
4955
return [];
5056
}
5157
$keyChecked = $keysToCheck[$keySearch];
5258

5359
$value = $node->value;
54-
if (!$value instanceof Node\Expr\Array_) {
55-
return [
56-
RuleErrorBuilder::message(sprintf('The "%s" render array value expects an array of callbacks.', $keyChecked))
57-
->line($node->getLine())->build()
58-
];
59-
}
60-
if (count($value->items) === 0) {
61-
return [];
60+
61+
$errors = [];
62+
63+
// @todo Move into its own rule.
64+
if ($keyChecked === '#lazy_builder') {
65+
if (!$value instanceof Node\Expr\Array_) {
66+
return [
67+
RuleErrorBuilder::message(sprintf('The "%s" expects a callable array with arguments.', $keyChecked))
68+
->line($node->getLine())->build()
69+
];
70+
}
71+
if (count($value->items) === 0) {
72+
return [];
73+
}
74+
if ($value->items[0] === null) {
75+
return [];
76+
}
77+
// @todo take $value->items[1] and validate parameters against the callback.
78+
$errors[] = $this->doProcessNode($value->items[0]->value, $scope, $keyChecked, 0);
79+
} elseif ($keyChecked === '#access_callback') {
80+
// @todo move into its own rule.
81+
$errors[] = $this->doProcessNode($value, $scope, $keyChecked, 0);
82+
} else {
83+
// @todo keep here.
84+
if (!$value instanceof Node\Expr\Array_) {
85+
return [
86+
RuleErrorBuilder::message(sprintf('The "%s" render array value expects an array of callbacks.', $keyChecked))
87+
->line($node->getLine())->build()
88+
];
89+
}
90+
if (count($value->items) === 0) {
91+
return [];
92+
}
93+
foreach ($value->items as $pos => $item) {
94+
if (!$item instanceof Node\Expr\ArrayItem) {
95+
continue;
96+
}
97+
$errors[] = $this->doProcessNode($item->value, $scope, $keyChecked, $pos);
98+
}
6299
}
100+
return array_filter($errors);
101+
}
63102

103+
private function doProcessNode(Node\Expr $node, Scope $scope, string $keyChecked, int $pos): ?RuleError
104+
{
64105
$trustedCallbackType = new UnionType([
65106
new ObjectType('Drupal\Core\Security\TrustedCallbackInterface'),
66107
new ObjectType('Drupal\Core\Render\Element\RenderCallbackInterface'),
67108
]);
68-
$errors = [];
69-
foreach ($value->items as $pos => $item) {
70-
if (!$item instanceof Node\Expr\ArrayItem) {
71-
continue;
109+
110+
$errorLine = $node->getLine();
111+
$type = $this->getType($node, $scope);
112+
113+
if ($type instanceof ConstantStringType) {
114+
if (!$type->isCallable()->yes()) {
115+
return RuleErrorBuilder::message(
116+
sprintf("%s callback %s at key '%s' is not callable.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
117+
)->line($errorLine)->build();
118+
}
119+
// We can determine if the callback is callable through the type system. However, we cannot determine
120+
// if it is just a function or a static class call (MyClass::staticFunc).
121+
if ($this->reflectionProvider->hasFunction(new Node\Name($type->getValue()), null)) {
122+
return RuleErrorBuilder::message(
123+
sprintf("%s callback %s at key '%s' is not trusted.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
124+
)->line($errorLine)
125+
->tip('Change record: https://www.drupal.org/node/2966725.')
126+
->build();
127+
}
128+
} elseif ($type instanceof ConstantArrayType) {
129+
if (!$type->isCallable()->yes()) {
130+
return RuleErrorBuilder::message(
131+
sprintf("%s callback %s at key '%s' is not callable.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
132+
)->line($errorLine)->build();
133+
}
134+
$typeAndMethodName = $type->findTypeAndMethodName();
135+
if ($typeAndMethodName === null) {
136+
throw new \PHPStan\ShouldNotHappenException();
72137
}
73-
$errorLine = $item->value->getLine();
74-
$type = $this->getType($item->value, $scope);
75138

76-
if ($type instanceof ConstantStringType) {
77-
if (!$type->isCallable()->yes()) {
78-
$errors[] = RuleErrorBuilder::message(
79-
sprintf("%s callback %s at key '%s' is not callable.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
80-
)->line($errorLine)->build();
81-
continue;
82-
}
83-
// We can determine if the callback is callable through the type system. However, we cannot determine
84-
// if it is just a function or a static class call (MyClass::staticFunc).
85-
if ($this->reflectionProvider->hasFunction(new \PhpParser\Node\Name($type->getValue()), null)) {
86-
$errors[] = RuleErrorBuilder::message(
87-
sprintf("%s callback %s at key '%s' is not trusted.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
88-
)->line($errorLine)
89-
->tip('Change record: https://www.drupal.org/node/2966725.')
90-
->build();
139+
if (!$trustedCallbackType->isSuperTypeOf($typeAndMethodName->getType())->yes()) {
140+
return RuleErrorBuilder::message(
141+
sprintf("%s callback class '%s' at key '%s' does not implement Drupal\Core\Security\TrustedCallbackInterface.", $keyChecked, $typeAndMethodName->getType()->describe(VerbosityLevel::value()), $pos)
142+
)->line($errorLine)->tip('Change record: https://www.drupal.org/node/2966725.')->build();
143+
}
144+
} elseif ($type instanceof ClosureType) {
145+
if ($scope->isInClass()) {
146+
$classReflection = $scope->getClassReflection();
147+
if ($classReflection === null) {
148+
throw new \PHPStan\ShouldNotHappenException();
91149
}
92-
} elseif ($type instanceof ConstantArrayType) {
93-
if (!$type->isCallable()->yes()) {
94-
$errors[] = RuleErrorBuilder::message(
95-
sprintf("%s callback %s at key '%s' is not callable.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
150+
$classType = new ObjectType($classReflection->getName());
151+
$formType = new ObjectType('\Drupal\Core\Form\FormInterface');
152+
if ($formType->isSuperTypeOf($classType)->yes()) {
153+
return RuleErrorBuilder::message(
154+
sprintf("%s may not contain a closure at key '%s' as forms may be serialized and serialization of closures is not allowed.", $keyChecked, $pos)
96155
)->line($errorLine)->build();
97-
continue;
98156
}
99-
$typeAndMethodName = $type->findTypeAndMethodName();
100-
if ($typeAndMethodName === null) {
101-
throw new \PHPStan\ShouldNotHappenException();
157+
}
158+
} elseif ($type instanceof ThisType) {
159+
if (!$type->isCallable()->yes()) {
160+
return RuleErrorBuilder::message(
161+
sprintf("%s callback %s at key '%s' is not callable.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
162+
)->line($errorLine)->build();
163+
}
164+
} elseif ($type instanceof IntersectionType) {
165+
// Try to provide a tip for this weird occurrence.
166+
$tip = '';
167+
if ($node instanceof Node\Expr\BinaryOp\Concat) {
168+
$leftStringType = $scope->getType($node->left)->toString();
169+
$rightStringType = $scope->getType($node->right)->toString();
170+
if ($leftStringType instanceof GenericClassStringType && $rightStringType instanceof ConstantStringType) {
171+
$methodName = str_replace(':', '', $rightStringType->getValue());
172+
$tip = "Refactor concatenation of `static::class` with method name to an array callback: [static::class, '$methodName']";
102173
}
174+
}
103175

104-
if (!$trustedCallbackType->isSuperTypeOf($typeAndMethodName->getType())->yes()) {
105-
$errors[] = RuleErrorBuilder::message(
106-
sprintf("%s callback class '%s' at key '%s' does not implement Drupal\Core\Security\TrustedCallbackInterface.", $keyChecked, $typeAndMethodName->getType()->describe(VerbosityLevel::value()), $pos)
107-
)->line($errorLine)->tip('Change record: https://www.drupal.org/node/2966725.')->build();
108-
}
109-
} elseif ($type instanceof ClosureType) {
110-
if ($scope->isInClass()) {
111-
$classReflection = $scope->getClassReflection();
112-
if ($classReflection === null) {
113-
throw new \PHPStan\ShouldNotHappenException();
114-
}
115-
$classType = new ObjectType($classReflection->getName());
116-
$formType = new ObjectType('\Drupal\Core\Form\FormInterface');
117-
if ($formType->isSuperTypeOf($classType)->yes()) {
118-
$errors[] = RuleErrorBuilder::message(
119-
sprintf("%s may not contain a closure at key '%s' as forms may be serialized and serialization of closures is not allowed.", $keyChecked, $pos)
120-
)->line($errorLine)->build();
121-
}
122-
}
123-
} else {
124-
$errors[] = RuleErrorBuilder::message(
125-
sprintf("%s value '%s' at key '%s' is invalid.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
126-
)->line($errorLine)->build();
176+
if ($tip === '') {
177+
$tip = 'If this error is unexpected, open an issue with the error and sample code https://github.com/mglaman/phpstan-drupal/issues/new';
127178
}
179+
180+
return RuleErrorBuilder::message(
181+
sprintf("%s value '%s' at key '%s' is invalid.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
182+
)->line($errorLine)->tip($tip)->build();
183+
} else {
184+
return RuleErrorBuilder::message(
185+
sprintf("%s value '%s' at key '%s' is invalid.", $keyChecked, $type->describe(VerbosityLevel::value()), $pos)
186+
)->line($errorLine)->build();
128187
}
129188

130-
return $errors;
189+
return null;
131190
}
132191

192+
// @todo move to a helper, as Drupal uses `service:method` references a lot.
133193
private function getType(Node\Expr $node, Scope $scope): Type
134194
{
135195
$type = $scope->getType($node);

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

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
<?php declare(strict_types=1);
2+
3+
namespace Drupal\pre_render_callback_rule;
4+
5+
use Drupal\Core\Security\TrustedCallbackInterface;
6+
use Drupal\Core\Url;
7+
8+
class LazyBuilderWithConstant implements TrustedCallbackInterface {
9+
10+
public function staticCallback(): array {
11+
return [
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+
],
27+
];
28+
}
29+
30+
public static function lazyBuilder(string $foo, bool $bar) {
31+
return [
32+
'#markup' => "$foo $bar",
33+
];
34+
}
35+
36+
public static function trustedCallbacks()
37+
{
38+
return ['lazyBuilder'];
39+
}
40+
}

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
use Drupal\Core\Security\TrustedCallbackInterface;
66
use Drupal\Core\Url;
77

8-
final class RenderArrayWithPreRenderCallback implements TrustedCallbackInterface {
8+
class RenderArrayWithPreRenderCallback implements TrustedCallbackInterface {
99

1010
public function staticCallback(): array {
1111
return [
@@ -14,6 +14,9 @@ public function staticCallback(): array {
1414
'#title' => 'FooBar',
1515
'#pre_render' => [
1616
[self::class, 'preRenderCallback'],
17+
[static::class, 'preRenderCallback'],
18+
self::class . '::preRenderCallback',
19+
static::class . '::preRenderCallback',
1720
[$this, 'preRenderCallback'],
1821
static function(array $element): array {
1922
return $element;

‎tests/src/Rules/PreRenderCallbackRuleTest.php

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,12 +65,28 @@ public function fileData(): \Generator
6565
];
6666
yield [
6767
__DIR__ . '/../../fixtures/drupal/modules/pre_render_callback_rule/src/RenderArrayWithPreRenderCallback.php',
68-
[]
68+
[
69+
[
70+
"#pre_render value 'non-empty-string' at key '3' is invalid.",
71+
19,
72+
"Refactor concatenation of `static::class` with method name to an array callback: [static::class, 'preRenderCallback']"
73+
]
74+
]
6975
];
7076
yield [
7177
__DIR__ . '/../../fixtures/drupal/modules/pre_render_callback_rule/src/RenderCallbackInterfaceObject.php',
7278
[]
7379
];
80+
yield [
81+
__DIR__ . '/../../fixtures/drupal/modules/pre_render_callback_rule/src/LazyBuilderWithConstant.php',
82+
[
83+
[
84+
"#lazy_builder value 'non-empty-string' at key '0' is invalid.",
85+
25,
86+
"Refactor concatenation of `static::class` with method name to an array callback: [static::class, 'lazyBuilder']"
87+
]
88+
]
89+
];
7490
yield [
7591
__DIR__ . '/../../fixtures/drupal/modules/pre_render_callback_rule/src/FormWithClosure.php',
7692
[
@@ -80,6 +96,10 @@ public function fileData(): \Generator
8096
]
8197
]
8298
];
99+
yield [
100+
__DIR__ . '/../../fixtures/drupal/core/lib/Drupal/Core/Access/RouteProcessorCsrf.php',
101+
[]
102+
];
83103
}
84104

85105

0 commit comments

Comments
 (0)