Skip to content

Commit 92d4494

Browse files
herndlmherndlm
committed
Enforce safe constructor overrides with @phpstan-consistent-constructor 
1 parent 0b925a9 commit 92d4494

File tree

9 files changed

+121
-29
lines changed

9 files changed

+121
-29
lines changed

‎conf/config.neon‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1077,6 +1077,11 @@ services:
10771077
arguments:
10781078
genericPrototypeMessage: %featureToggles.genericPrototypeMessage%
10791079

1080+
-
1081+
class: PHPStan\Rules\Methods\MethodVisibilityComparisonHelper
1082+
arguments:
1083+
genericPrototypeMessage: %featureToggles.genericPrototypeMessage%
1084+
10801085
-
10811086
class: PHPStan\Rules\MissingTypehintCheck
10821087
arguments:

‎src/PhpDoc/StubValidator.php‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@
6565
use PHPStan\Rules\Methods\ExistingClassesInTypehintsRule;
6666
use PHPStan\Rules\Methods\MethodParameterComparisonHelper;
6767
use PHPStan\Rules\Methods\MethodSignatureRule;
68+
use PHPStan\Rules\Methods\MethodVisibilityComparisonHelper;
6869
use PHPStan\Rules\Methods\MissingMethodParameterTypehintRule;
6970
use PHPStan\Rules\Methods\MissingMethodReturnTypehintRule;
7071
use PHPStan\Rules\Methods\MissingMethodSelfOutTypeRule;
@@ -196,7 +197,17 @@ private function getRuleRegistry(Container $container): RuleRegistry
196197
new ExistingClassesInTypehintsRule($functionDefinitionCheck),
197198
new \PHPStan\Rules\Functions\ExistingClassesInTypehintsRule($functionDefinitionCheck),
198199
new ExistingClassesInPropertiesRule($reflectionProvider, $classNameCheck, $unresolvableTypeHelper, $phpVersion, true, false),
199-
new OverridingMethodRule($phpVersion, new MethodSignatureRule($phpClassReflectionExtension, true, true, $container->getParameter('featureToggles')['abstractTraitMethod']), true, new MethodParameterComparisonHelper($phpVersion, $container->getParameter('featureToggles')['genericPrototypeMessage']), $phpClassReflectionExtension, $container->getParameter('featureToggles')['genericPrototypeMessage'], $container->getParameter('featureToggles')['finalByPhpDoc'], $container->getParameter('checkMissingOverrideMethodAttribute')),
200+
new OverridingMethodRule(
201+
$phpVersion,
202+
new MethodSignatureRule($phpClassReflectionExtension, true, true, $container->getParameter('featureToggles')['abstractTraitMethod']),
203+
true,
204+
new MethodParameterComparisonHelper($phpVersion, $container->getParameter('featureToggles')['genericPrototypeMessage']),
205+
new MethodVisibilityComparisonHelper($container->getParameter('featureToggles')['genericPrototypeMessage']),
206+
$phpClassReflectionExtension,
207+
$container->getParameter('featureToggles')['genericPrototypeMessage'],
208+
$container->getParameter('featureToggles')['finalByPhpDoc'],
209+
$container->getParameter('checkMissingOverrideMethodAttribute'),
210+
),
200211
new DuplicateDeclarationRule(),
201212
new LocalTypeAliasesRule($localTypeAliasesCheck),
202213
new LocalTypeTraitAliasesRule($localTypeAliasesCheck, $reflectionProvider),

‎src/Rules/Methods/ConsistentConstructorRule.php‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use PHPStan\Node\InClassMethodNode;
88
use PHPStan\Reflection\Dummy\DummyConstructorReflection;
99
use PHPStan\Rules\Rule;
10+
use function array_merge;
1011
use function strtolower;
1112

1213
/** @implements Rule<InClassMethodNode> */
@@ -15,6 +16,7 @@ final class ConsistentConstructorRule implements Rule
1516

1617
public function __construct(
1718
private MethodParameterComparisonHelper $methodParameterComparisonHelper,
19+
private MethodVisibilityComparisonHelper $methodVisibilityComparisonHelper,
1820
)
1921
{
2022
}
@@ -47,7 +49,10 @@ public function processNode(Node $node, Scope $scope): array
4749
return [];
4850
}
4951

50-
return $this->methodParameterComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method, true);
52+
return array_merge(
53+
$this->methodParameterComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method, true),
54+
$this->methodVisibilityComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method),
55+
);
5156
}
5257

5358
}

‎src/Rules/Methods/MethodVisibilityComparisonHelper.php‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace PHPStan\Rules\Methods;
4+
5+
use PHPStan\Reflection\ClassReflection;
6+
use PHPStan\Reflection\ExtendedMethodReflection;
7+
use PHPStan\Reflection\Php\PhpMethodFromParserNodeReflection;
8+
use PHPStan\Rules\IdentifierRuleError;
9+
use PHPStan\Rules\RuleErrorBuilder;
10+
use function sprintf;
11+
12+
final class MethodVisibilityComparisonHelper
13+
{
14+
15+
public function __construct(private bool $genericPrototypeMessage)
16+
{
17+
}
18+
19+
/**
20+
* @return list<IdentifierRuleError>
21+
*/
22+
public function compare(ExtendedMethodReflection $prototype, ClassReflection $prototypeDeclaringClass, PhpMethodFromParserNodeReflection $method): array
23+
{
24+
/** @var list<IdentifierRuleError> $messages */
25+
$messages = [];
26+
27+
if ($prototype->isPublic()) {
28+
if (!$method->isPublic()) {
29+
$messages[] = RuleErrorBuilder::message(sprintf(
30+
'%s method %s::%s() overriding public method %s::%s() should also be public.',
31+
$method->isPrivate() ? 'Private' : 'Protected',
32+
$method->getDeclaringClass()->getDisplayName(),
33+
$method->getName(),
34+
$prototypeDeclaringClass->getDisplayName($this->genericPrototypeMessage),
35+
$prototype->getName(),
36+
))
37+
->nonIgnorable()
38+
->identifier('method.visibility')
39+
->build();
40+
}
41+
} elseif ($method->isPrivate()) {
42+
$messages[] = RuleErrorBuilder::message(sprintf(
43+
'Private method %s::%s() overriding protected method %s::%s() should be protected or public.',
44+
$method->getDeclaringClass()->getDisplayName(),
45+
$method->getName(),
46+
$prototypeDeclaringClass->getDisplayName($this->genericPrototypeMessage),
47+
$prototype->getName(),
48+
))
49+
->nonIgnorable()
50+
->identifier('method.visibility')
51+
->build();
52+
}
53+
54+
return $messages;
55+
}
56+
57+
}

‎src/Rules/Methods/OverridingMethodRule.php‎

Lines changed: 2 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ public function __construct(
3636
private MethodSignatureRule $methodSignatureRule,
3737
private bool $checkPhpDocMethodSignatures,
3838
private MethodParameterComparisonHelper $methodParameterComparisonHelper,
39+
private MethodVisibilityComparisonHelper $methodVisibilityComparisonHelper,
3940
private PhpClassReflectionExtension $phpClassReflectionExtension,
4041
private bool $genericPrototypeMessage,
4142
private bool $finalByPhpDoc,
@@ -168,32 +169,7 @@ public function processNode(Node $node, Scope $scope): array
168169
}
169170

170171
if ($checkVisibility) {
171-
if ($prototype->isPublic()) {
172-
if (!$method->isPublic()) {
173-
$messages[] = RuleErrorBuilder::message(sprintf(
174-
'%s method %s::%s() overriding public method %s::%s() should also be public.',
175-
$method->isPrivate() ? 'Private' : 'Protected',
176-
$method->getDeclaringClass()->getDisplayName(),
177-
$method->getName(),
178-
$prototypeDeclaringClass->getDisplayName($this->genericPrototypeMessage),
179-
$prototype->getName(),
180-
))
181-
->nonIgnorable()
182-
->identifier('method.visibility')
183-
->build();
184-
}
185-
} elseif ($method->isPrivate()) {
186-
$messages[] = RuleErrorBuilder::message(sprintf(
187-
'Private method %s::%s() overriding protected method %s::%s() should be protected or public.',
188-
$method->getDeclaringClass()->getDisplayName(),
189-
$method->getName(),
190-
$prototypeDeclaringClass->getDisplayName($this->genericPrototypeMessage),
191-
$prototype->getName(),
192-
))
193-
->nonIgnorable()
194-
->identifier('method.visibility')
195-
->build();
196-
}
172+
$messages = array_merge($messages, $this->methodVisibilityComparisonHelper->compare($prototype, $prototypeDeclaringClass, $method));
197173
}
198174

199175
$prototypeVariants = $prototype->getVariants();

‎tests/PHPStan/Rules/Methods/ConsistentConstructorRuleTest.php‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,10 @@ class ConsistentConstructorRuleTest extends RuleTestCase
1313

1414
protected function getRule(): Rule
1515
{
16-
return new ConsistentConstructorRule(self::getContainer()->getByType(MethodParameterComparisonHelper::class));
16+
return new ConsistentConstructorRule(
17+
self::getContainer()->getByType(MethodParameterComparisonHelper::class),
18+
self::getContainer()->getByType(MethodVisibilityComparisonHelper::class),
19+
);
1720
}
1821

1922
public function testRule(): void
@@ -46,4 +49,14 @@ public function testRuleNoErrors(): void
4649
);
4750
}
4851

52+
public function testBug12137(): void
53+
{
54+
$this->analyse([__DIR__ . '/data/bug-12137.php'], [
55+
[
56+
'Private method Bug12137\ChildClass::__construct() overriding protected method Bug12137\ParentClass::__construct() should be protected or public.',
57+
20,
58+
],
59+
]);
60+
}
61+
4962
}

‎tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ protected function getRule(): Rule
2929
new MethodSignatureRule($phpClassReflectionExtension, $this->reportMaybes, $this->reportStatic, true),
3030
true,
3131
new MethodParameterComparisonHelper($phpVersion, true),
32+
new MethodVisibilityComparisonHelper(true),
3233
$phpClassReflectionExtension,
3334
true,
3435
true,

‎tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ protected function getRule(): Rule
3131
new MethodSignatureRule($phpClassReflectionExtension, true, true, true),
3232
false,
3333
new MethodParameterComparisonHelper($phpVersion, true),
34+
new MethodVisibilityComparisonHelper(true),
3435
$phpClassReflectionExtension,
3536
true,
3637
true,

‎tests/PHPStan/Rules/Methods/data/bug-12137.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug12137;
4+
5+
/** @phpstan-consistent-constructor */
6+
abstract class ParentClass
7+
{
8+
protected function __construct()
9+
{
10+
}
11+
12+
public static function create(): static
13+
{
14+
return new static();
15+
}
16+
}
17+
18+
class ChildClass extends ParentClass
19+
{
20+
private function __construct()
21+
{
22+
}
23+
}

0 commit comments

Comments
 (0)