Skip to content

Commit c33bef6

Browse files
mglamanmglaman
authored
Improve error message for alterInfo in PluginManagerInspectionRule (#799)
* Improve error message for alterInfo in PluginManagerInspectionRule * Cleanup PluginManagerInspectionRule statement logic * optimize imports * phpstan * ignore test and default plugin manager * make plugin manager inspection opt-in * add service * schema
1 parent 2bc25a5 commit c33bef6

File tree

5 files changed

+157
-68
lines changed

5 files changed

+157
-68
lines changed

‎extension.neon

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ parameters:
2626
dependencySerializationTraitPropertyRule: false
2727
accessResultConditionRule: false
2828
classExtendsInternalClassRule: true
29+
pluginManagerInspectionRule: false
2930
entityMapping:
3031
aggregator_feed:
3132
class: Drupal\aggregator\Entity\Feed
@@ -252,6 +253,7 @@ parametersSchema:
252253
dependencySerializationTraitPropertyRule: boolean()
253254
accessResultConditionRule: boolean()
254255
classExtendsInternalClassRule: boolean()
256+
pluginManagerInspectionRule: boolean()
255257
])
256258
entityMapping: arrayOf(anyOf(
257259
structure([

‎rules.neon

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ rules:
33
- mglaman\PHPStanDrupal\Rules\Drupal\GlobalDrupalDependencyInjectionRule
44
- mglaman\PHPStanDrupal\Rules\Drupal\PluginManager\PluginManagerSetsCacheBackendRule
55
- mglaman\PHPStanDrupal\Rules\Deprecations\AccessDeprecatedConstant
6-
- mglaman\PHPStanDrupal\Rules\Classes\PluginManagerInspectionRule
76
- mglaman\PHPStanDrupal\Rules\Deprecations\ConditionManagerCreateInstanceContextConfigurationRule
87
- mglaman\PHPStanDrupal\Rules\Drupal\RenderCallbackRule
98
- mglaman\PHPStanDrupal\Rules\Deprecations\StaticServiceDeprecatedServiceRule
@@ -28,6 +27,8 @@ conditionalTags:
2827
phpstan.rules.rule: %drupal.rules.accessResultConditionRule%
2928
mglaman\PHPStanDrupal\Rules\Classes\ClassExtendsInternalClassRule:
3029
phpstan.rules.rule: %drupal.rules.classExtendsInternalClassRule%
30+
mglaman\PHPStanDrupal\Rules\Classes\PluginManagerInspectionRule:
31+
phpstan.rules.rule: %drupal.rules.pluginManagerInspectionRule%
3132

3233
services:
3334
-
@@ -40,3 +41,5 @@ services:
4041
treatPhpDocTypesAsCertain: %treatPhpDocTypesAsCertain%
4142
-
4243
class: mglaman\PHPStanDrupal\Rules\Classes\ClassExtendsInternalClassRule
44+
-
45+
class: mglaman\PHPStanDrupal\Rules\Classes\PluginManagerInspectionRule

‎src/Rules/Classes/PluginManagerInspectionRule.php

Lines changed: 63 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,14 @@
22

33
namespace mglaman\PHPStanDrupal\Rules\Classes;
44

5+
use Drupal\Component\Plugin\PluginManagerInterface;
6+
use Drupal\Core\Plugin\DefaultPluginManager;
57
use PhpParser\Node;
8+
use PhpParser\NodeFinder;
69
use PHPStan\Analyser\Scope;
710
use PHPStan\Reflection\ReflectionProvider;
811
use PHPStan\Rules\Rule;
12+
use PHPStan\Rules\RuleErrorBuilder;
913
use PHPStan\Type\ObjectType;
1014
use function sprintf;
1115

@@ -35,80 +39,78 @@ public function processNode(Node $node, Scope $scope): array
3539
if ($node->extends === null) {
3640
return [];
3741
}
38-
$className = (string) $node->namespacedName;
39-
$pluginManagerType = new ObjectType($className);
40-
$pluginManagerInterfaceType = new ObjectType('\Drupal\Component\Plugin\PluginManagerInterface');
42+
if (str_contains($node->namespacedName->toLowerString(), 'test')) {
43+
return [];
44+
}
45+
46+
$pluginManagerType = $scope->resolveTypeByName($node->namespacedName);
47+
$pluginManagerInterfaceType = new ObjectType(PluginManagerInterface::class);
4148
if (!$pluginManagerInterfaceType->isSuperTypeOf($pluginManagerType)->yes()) {
4249
return [];
4350
}
51+
$defaultPluginManager = new ObjectType(DefaultPluginManager::class);
52+
if ($defaultPluginManager->equals($pluginManagerType)) {
53+
return [];
54+
}
55+
56+
$constructorMethodNode = (new NodeFinder())->findFirst($node->stmts, static function (Node $node) {
57+
return $node instanceof Node\Stmt\ClassMethod && $node->name->toString() === '__construct';
58+
});
59+
if (!$constructorMethodNode instanceof Node\Stmt\ClassMethod) {
60+
return [];
61+
}
4462

4563
$errors = [];
4664
if ($this->isYamlDiscovery($node)) {
47-
$errors = $this->inspectYamlPluginManager($node);
65+
$errors = $this->inspectYamlPluginManager($node, $constructorMethodNode);
4866
} else {
4967
// @todo inspect annotated plugin managers.
5068
}
5169

52-
$hasAlterInfoSet = false;
53-
54-
foreach ($node->stmts as $stmt) {
55-
if ($stmt instanceof Node\Stmt\ClassMethod && $stmt->name->toString() === '__construct') {
56-
foreach ($stmt->stmts ?? [] as $statement) {
57-
if ($statement instanceof Node\Stmt\Expression) {
58-
$statement = $statement->expr;
59-
}
60-
if ($statement instanceof Node\Expr\MethodCall
61-
&& $statement->name instanceof Node\Identifier
62-
&& $statement->name->name === 'alterInfo') {
63-
$hasAlterInfoSet = true;
64-
}
65-
}
66-
}
67-
}
70+
$alterInfoMethodNode = (new NodeFinder())->findFirst($constructorMethodNode->stmts ?? [], static function (Node $node) {
71+
return $node instanceof Node\Stmt\Expression
72+
&& $node->expr instanceof Node\Expr\MethodCall
73+
&& $node->expr->name instanceof Node\Identifier
74+
&& $node->expr->name->toString() === 'alterInfo';
75+
});
6876

69-
if (!$hasAlterInfoSet) {
70-
$errors[] = 'Plugin definitions cannot be altered.';
77+
if ($alterInfoMethodNode === null) {
78+
$errors[] = RuleErrorBuilder::message(
79+
'Plugin managers should call alterInfo to allow plugin definitions to be altered.'
80+
)
81+
->identifier('plugin.manager.alterInfoMissing')
82+
->tip('For example, to invoke hook_mymodule_data_alter() call alterInfo with "mymodule_data".')
83+
->line($node->getStartLine())
84+
->build();
7185
}
7286

7387
return $errors;
7488
}
7589

7690
private function isYamlDiscovery(Node\Stmt\Class_ $class): bool
7791
{
78-
foreach ($class->stmts as $stmt) {
79-
// YAML discovery plugin managers must override getDiscovery.
80-
if ($stmt instanceof Node\Stmt\ClassMethod && $stmt->name->toString() === 'getDiscovery') {
81-
foreach ($stmt->stmts ?? [] as $methodStmt) {
82-
if ($methodStmt instanceof Node\Stmt\If_) {
83-
foreach ($methodStmt->stmts as $ifStmt) {
84-
if ($ifStmt instanceof Node\Stmt\Expression) {
85-
$ifStmtExpr = $ifStmt->expr;
86-
if ($ifStmtExpr instanceof Node\Expr\Assign) {
87-
$ifStmtExprVar = $ifStmtExpr->var;
88-
if ($ifStmtExprVar instanceof Node\Expr\PropertyFetch
89-
&& $ifStmtExprVar->var instanceof Node\Expr\Variable
90-
&& $ifStmtExprVar->name instanceof Node\Identifier
91-
&& $ifStmtExprVar->name->name === 'discovery'
92-
) {
93-
$ifStmtExprExpr = $ifStmtExpr->expr;
94-
if ($ifStmtExprExpr instanceof Node\Expr\New_
95-
&& ($ifStmtExprExpr->class instanceof Node\Name)
96-
&& $ifStmtExprExpr->class->toString() === 'Drupal\Core\Plugin\Discovery\YamlDiscovery') {
97-
return true;
98-
}
99-
}
100-
}
101-
}
102-
}
103-
}
104-
}
105-
}
92+
$nodeFinder = new NodeFinder();
93+
$getDiscoveryMethodNode = $nodeFinder->findFirst($class->stmts, static function (Node $node) {
94+
return $node instanceof Node\Stmt\ClassMethod && $node->name->toString() === 'getDiscovery';
95+
});
96+
if (!$getDiscoveryMethodNode instanceof Node\Stmt\ClassMethod) {
97+
return false;
98+
}
99+
100+
$assignDiscovery = $nodeFinder->findFirstInstanceOf($getDiscoveryMethodNode->stmts ?? [], Node\Expr\Assign::class);
101+
if ($assignDiscovery === null) {
102+
return false;
103+
}
104+
if ($assignDiscovery->expr instanceof Node\Expr\New_
105+
&& $assignDiscovery->expr->class instanceof Node\Name
106+
&& $assignDiscovery->expr->class->toString() === 'Drupal\Core\Plugin\Discovery\YamlDiscovery') {
107+
return true;
106108
}
107109

108110
return false;
109111
}
110112

111-
private function inspectYamlPluginManager(Node\Stmt\Class_ $class): array
113+
private function inspectYamlPluginManager(Node\Stmt\Class_ $class, Node\Stmt\ClassMethod $constructorMethodNode): array
112114
{
113115
$errors = [];
114116

@@ -119,20 +121,16 @@ private function inspectYamlPluginManager(Node\Stmt\Class_ $class): array
119121
if ($constructor->getDeclaringClass()->getName() !== $fqn) {
120122
$errors[] = sprintf('%s must override __construct if using YAML plugins.', $fqn);
121123
} else {
122-
foreach ($class->stmts as $stmt) {
123-
if ($stmt instanceof Node\Stmt\ClassMethod && $stmt->name->toString() === '__construct') {
124-
foreach ($stmt->stmts ?? [] as $constructorStmt) {
125-
if ($constructorStmt instanceof Node\Stmt\Expression) {
126-
$constructorStmt = $constructorStmt->expr;
127-
}
128-
if ($constructorStmt instanceof Node\Expr\StaticCall
129-
&& $constructorStmt->class instanceof Node\Name
130-
&& ((string)$constructorStmt->class === 'parent')
131-
&& $constructorStmt->name instanceof Node\Identifier
132-
&& $constructorStmt->name->name === '__construct') {
133-
$errors[] = sprintf('YAML plugin managers should not invoke its parent constructor.');
134-
}
135-
}
124+
foreach ($constructorMethodNode->stmts ?? [] as $constructorStmt) {
125+
if ($constructorStmt instanceof Node\Stmt\Expression) {
126+
$constructorStmt = $constructorStmt->expr;
127+
}
128+
if ($constructorStmt instanceof Node\Expr\StaticCall
129+
&& $constructorStmt->class instanceof Node\Name
130+
&& ((string)$constructorStmt->class === 'parent')
131+
&& $constructorStmt->name instanceof Node\Identifier
132+
&& $constructorStmt->name->name === '__construct') {
133+
$errors[] = 'YAML plugin managers should not invoke its parent constructor.';
136134
}
137135
}
138136
}

‎tests/src/Rules/PluginManagerInspectionRuleTest.php

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,15 @@
44

55
use mglaman\PHPStanDrupal\Rules\Classes\PluginManagerInspectionRule;
66
use mglaman\PHPStanDrupal\Tests\DrupalRuleTestCase;
7+
use PHPStan\Rules\Rule;
78

89
final class PluginManagerInspectionRuleTest extends DrupalRuleTestCase
910
{
1011

11-
protected function getRule(): \PHPStan\Rules\Rule
12+
protected function getRule(): Rule
1213
{
1314
return new PluginManagerInspectionRule(
14-
$this->createReflectionProvider()
15+
self::createReflectionProvider()
1516
);
1617
}
1718

@@ -33,6 +34,21 @@ public static function pluginManagerData(): \Generator
3334
__DIR__ . '/../../fixtures/drupal/modules/phpstan_fixtures/src/ExamplePluginManager.php',
3435
[]
3536
];
37+
yield [
38+
__DIR__ . '/data/plugin-manager-alter-info.php',
39+
[
40+
[
41+
'Plugin managers should call alterInfo to allow plugin definitions to be altered.',
42+
9,
43+
'For example, to invoke hook_mymodule_data_alter() call alterInfo with "mymodule_data".'
44+
],
45+
[
46+
'Plugin managers should call alterInfo to allow plugin definitions to be altered.',
47+
41,
48+
'For example, to invoke hook_mymodule_data_alter() call alterInfo with "mymodule_data".'
49+
],
50+
]
51+
];
3652
}
3753

3854
}

‎tests/src/Rules/data/plugin-manager-alter-info.php

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
<?php
2+
3+
namespace PluginManagerAlterInfo;
4+
5+
use Drupal\Core\Plugin\Discovery\YamlDiscovery;
6+
use Drupal\Core\Extension\ModuleHandlerInterface;
7+
use Drupal\Core\Plugin\DefaultPluginManager;
8+
9+
class AnnotationsMissingAlterInfo extends DefaultPluginManager {
10+
11+
public function __construct(
12+
\Traversable $namespaces,
13+
ModuleHandlerInterface $module_handler,
14+
) {
15+
parent::__construct(
16+
'Plugin/Bar',
17+
$namespaces,
18+
$module_handler,
19+
'BarInterface',
20+
'BarAnnotation',
21+
);
22+
}
23+
}
24+
class AnnotationsWithAlterInfo extends DefaultPluginManager {
25+
26+
public function __construct(
27+
\Traversable $namespaces,
28+
ModuleHandlerInterface $module_handler,
29+
) {
30+
parent::__construct(
31+
'Plugin/Bar',
32+
$namespaces,
33+
$module_handler,
34+
'BarInterface',
35+
'BarAnnotation',
36+
);
37+
$this->alterInfo('bar');
38+
}
39+
}
40+
41+
class YamlWithoutAlterInfo extends DefaultPluginManager {
42+
43+
public function __construct(
44+
) {
45+
}
46+
47+
protected function getDiscovery()
48+
{
49+
if (!$this->discovery) {
50+
$this->discovery = new YamlDiscovery('foo', []);
51+
}
52+
}
53+
54+
}
55+
56+
class YamlWithAlterInfo extends DefaultPluginManager {
57+
58+
public function __construct(
59+
) {
60+
$this->alterInfo('baz');
61+
}
62+
63+
protected function getDiscovery()
64+
{
65+
if (!$this->discovery) {
66+
$this->discovery = new YamlDiscovery('foo', []);
67+
}
68+
}
69+
70+
}

0 commit comments

Comments
 (0)