Skip to content

Commit 1572c47

Browse files
mariano-dagostinomglaman
mariano-dagostino
and
mglaman
authored
Detect calling addCacheableDependency on non-CacheableDependencyInterface values (#913)
* New rule to detect incorrect use of addCacheableDependency * Fix coding standards * Fix coding standards * Use nodeFinder * Fixup CacheableDependencyRule * Use type check and add test cases * handle different interfaces with addCacheableDependency * make sure the rule is behind a feature flag * fix a bug on renderer check * remove old file --------- Co-authored-by: Matt Glaman <[email protected]>
1 parent df786b7 commit 1572c47

File tree

6 files changed

+196
-0
lines changed

6 files changed

+196
-0
lines changed

‎bleedingEdge.neon‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,3 +8,4 @@ parameters:
88
testClassSuffixNameRule: true
99
dependencySerializationTraitPropertyRule: true
1010
accessResultConditionRule: true
11+
cacheableDependencyRule: true

‎extension.neon‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ parameters:
3333
accessResultConditionRule: false
3434
classExtendsInternalClassRule: true
3535
pluginManagerInspectionRule: false
36+
cacheableDependencyRule: false
3637
entityMapping:
3738
aggregator_feed:
3839
class: Drupal\aggregator\Entity\Feed
@@ -267,6 +268,7 @@ parametersSchema:
267268
accessResultConditionRule: boolean()
268269
classExtendsInternalClassRule: boolean()
269270
pluginManagerInspectionRule: boolean()
271+
cacheableDependencyRule: boolean()
270272
])
271273
entityMapping: arrayOf(anyOf(
272274
structure([

‎rules.neon‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ conditionalTags:
2424
phpstan.rules.rule: %drupal.rules.classExtendsInternalClassRule%
2525
mglaman\PHPStanDrupal\Rules\Classes\PluginManagerInspectionRule:
2626
phpstan.rules.rule: %drupal.rules.pluginManagerInspectionRule%
27+
mglaman\PHPStanDrupal\Rules\Drupal\CacheableDependencyRule:
28+
phpstan.rules.rule: %drupal.rules.cacheableDependencyRule%
2729

2830
services:
2931
-
@@ -38,3 +40,5 @@ services:
3840
class: mglaman\PHPStanDrupal\Rules\Classes\ClassExtendsInternalClassRule
3941
-
4042
class: mglaman\PHPStanDrupal\Rules\Classes\PluginManagerInspectionRule
43+
-
44+
class: mglaman\PHPStanDrupal\Rules\Drupal\CacheableDependencyRule

‎src/Rules/Drupal/CacheableDependencyRule.php‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace mglaman\PHPStanDrupal\Rules\Drupal;
6+
7+
use Drupal\Core\Cache\CacheableDependencyInterface;
8+
use Drupal\Core\Cache\CacheableResponseInterface;
9+
use Drupal\Core\Cache\RefinableCacheableDependencyInterface;
10+
use Drupal\Core\Plugin\Context\ContextInterface;
11+
use Drupal\Core\Render\RendererInterface;
12+
use PhpParser\Node;
13+
use PhpParser\Node\Identifier;
14+
use PHPStan\Analyser\Scope;
15+
use PHPStan\Rules\Rule;
16+
use PHPStan\Rules\RuleErrorBuilder;
17+
use PHPStan\Type\ObjectType;
18+
19+
/**
20+
* @implements Rule<Node\Expr\MethodCall>
21+
*/
22+
class CacheableDependencyRule implements Rule
23+
{
24+
25+
public function getNodeType(): string
26+
{
27+
return Node\Expr\MethodCall::class;
28+
}
29+
30+
public function processNode(Node $node, Scope $scope): array
31+
{
32+
if (!$node->name instanceof Identifier || $node->name->toString() !== 'addCacheableDependency') {
33+
return [];
34+
}
35+
36+
$receiverType = $scope->getType($node->var);
37+
38+
$allowedInterfaces = [
39+
RefinableCacheableDependencyInterface::class => 0,
40+
CacheableResponseInterface::class => 0,
41+
ContextInterface::class => 0,
42+
RendererInterface::class => 1,
43+
];
44+
45+
$argumentIndex = null;
46+
foreach ($allowedInterfaces as $interfaceName => $argPosition) {
47+
$interfaceType = new ObjectType($interfaceName);
48+
if ($interfaceType->isSuperTypeOf($receiverType)->yes()) {
49+
$argumentIndex = $argPosition;
50+
break;
51+
}
52+
}
53+
54+
if ($argumentIndex === null) {
55+
return [];
56+
}
57+
58+
$args = $node->getArgs();
59+
if (count($args) <= $argumentIndex) {
60+
return [];
61+
}
62+
63+
$dependencyArg = $args[$argumentIndex]->value;
64+
$object = $scope->getType($dependencyArg);
65+
66+
$interfaceType = new ObjectType(CacheableDependencyInterface::class);
67+
$implementsInterface = $interfaceType->isSuperTypeOf($object);
68+
69+
if (!$implementsInterface->no()) {
70+
return [];
71+
}
72+
73+
return [
74+
RuleErrorBuilder::message('Calling addCacheableDependency($object) when $object does not implement CacheableDependencyInterface effectively disables caching and should be avoided.')
75+
->identifier('cacheable.dependency')
76+
->build(),
77+
];
78+
}
79+
}

‎tests/src/Rules/CachableDependencyRuleTest.php‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
<?php declare(strict_types=1);
2+
3+
namespace mglaman\PHPStanDrupal\Tests\Rules;
4+
5+
use mglaman\PHPStanDrupal\Rules\Drupal\CacheableDependencyRule;
6+
use mglaman\PHPStanDrupal\Tests\DrupalRuleTestCase;
7+
8+
final class CachableDependencyRuleTest extends DrupalRuleTestCase {
9+
10+
protected function getRule(): \PHPStan\Rules\Rule
11+
{
12+
return new CacheableDependencyRule();
13+
}
14+
15+
/**
16+
* @dataProvider resultData
17+
*
18+
* @param list<array{0: string, 1: int, 2?: string|null}> $errorMessages
19+
*/
20+
public function testRule(string $path, array $errorMessages): void
21+
{
22+
$this->analyse([$path], $errorMessages);
23+
}
24+
25+
public static function resultData(): \Generator
26+
{
27+
yield 'all test cases' => [
28+
__DIR__ . '/data/cacheable-dependency.php',
29+
[
30+
[
31+
'Calling addCacheableDependency($object) when $object does not implement CacheableDependencyInterface effectively disables caching and should be avoided.',
32+
13
33+
],
34+
[
35+
'Calling addCacheableDependency($object) when $object does not implement CacheableDependencyInterface effectively disables caching and should be avoided.',
36+
36
37+
],
38+
[
39+
'Calling addCacheableDependency($object) when $object does not implement CacheableDependencyInterface effectively disables caching and should be avoided.',
40+
39
41+
],
42+
[
43+
'Calling addCacheableDependency($object) when $object does not implement CacheableDependencyInterface effectively disables caching and should be avoided.',
44+
55
45+
],
46+
]
47+
];
48+
}
49+
50+
51+
}

‎tests/src/Rules/data/cacheable-dependency.php‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
<?php
2+
3+
namespace Drupal\phpstan_fixtures;
4+
5+
use Drupal\Core\Cache\CacheableMetadata;
6+
7+
class UsesIncorrectCacheableDependency {
8+
public function test() {
9+
$element = [];
10+
$cacheable_metadata = CacheableMetadata::createFromRenderArray($element);
11+
12+
$object = new \Stdclass;
13+
$cacheable_metadata->addCacheableDependency($object);
14+
}
15+
}
16+
17+
class UsesCorrectCacheableDependency {
18+
public function test() {
19+
$element = [];
20+
$cacheable_metadata = CacheableMetadata::createFromRenderArray($element);
21+
22+
$correct_dependency = new CacheableMetadata();
23+
$cacheable_metadata->addCacheableDependency($correct_dependency);
24+
}
25+
}
26+
27+
class MultipleCacheableDependencyCalls {
28+
public function test() {
29+
$element = [];
30+
$cacheable_metadata = CacheableMetadata::createFromRenderArray($element);
31+
32+
$correct_dependency = new CacheableMetadata();
33+
$cacheable_metadata->addCacheableDependency($correct_dependency);
34+
35+
$object = new \StdClass;
36+
$cacheable_metadata->addCacheableDependency($object);
37+
38+
$another_object = new \DateTime();
39+
$cacheable_metadata->addCacheableDependency($another_object);
40+
}
41+
}
42+
43+
class RendererInterfaceTestCase {
44+
public function testCorrectUsage(\Drupal\Core\Render\RendererInterface $renderer) {
45+
$elements = [];
46+
47+
$correct_dependency = new CacheableMetadata();
48+
$renderer->addCacheableDependency($elements, $correct_dependency);
49+
}
50+
51+
public function testIncorrectUsage(\Drupal\Core\Render\RendererInterface $renderer) {
52+
$elements = [];
53+
54+
$object = new \StdClass;
55+
$renderer->addCacheableDependency($elements, $object);
56+
}
57+
}
58+
59+

0 commit comments

Comments
 (0)