diff --git a/conf/config.neon b/conf/config.neon index c3c2c7bded..b9c52c9a57 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -986,6 +986,9 @@ services: - class: PHPStan\Rules\Methods\MethodParameterComparisonHelper + - + class: PHPStan\Rules\Methods\MethodVisibilityComparisonHelper + - class: PHPStan\Rules\MissingTypehintCheck arguments: diff --git a/src/PhpDoc/StubValidator.php b/src/PhpDoc/StubValidator.php index 33fde1e46e..0374aa268f 100644 --- a/src/PhpDoc/StubValidator.php +++ b/src/PhpDoc/StubValidator.php @@ -68,6 +68,7 @@ use PHPStan\Rules\Methods\ExistingClassesInTypehintsRule; use PHPStan\Rules\Methods\MethodParameterComparisonHelper; use PHPStan\Rules\Methods\MethodSignatureRule; +use PHPStan\Rules\Methods\MethodVisibilityComparisonHelper; use PHPStan\Rules\Methods\MissingMethodParameterTypehintRule; use PHPStan\Rules\Methods\MissingMethodReturnTypehintRule; use PHPStan\Rules\Methods\MissingMethodSelfOutTypeRule; @@ -209,7 +210,15 @@ private function getRuleRegistry(Container $container): RuleRegistry new ExistingClassesInTypehintsRule($functionDefinitionCheck), new \PHPStan\Rules\Functions\ExistingClassesInTypehintsRule($functionDefinitionCheck), new ExistingClassesInPropertiesRule($reflectionProvider, $classNameCheck, $unresolvableTypeHelper, $phpVersion, true, false), - new OverridingMethodRule($phpVersion, new MethodSignatureRule($phpClassReflectionExtension, true, true), true, new MethodParameterComparisonHelper($phpVersion), $phpClassReflectionExtension, $container->getParameter('checkMissingOverrideMethodAttribute')), + new OverridingMethodRule( + $phpVersion, + new MethodSignatureRule($phpClassReflectionExtension, true, true), + true, + new MethodParameterComparisonHelper($phpVersion), + new MethodVisibilityComparisonHelper(), + $phpClassReflectionExtension, + $container->getParameter('checkMissingOverrideMethodAttribute'), + ), new DuplicateDeclarationRule(), new LocalTypeAliasesRule($localTypeAliasesCheck), new LocalTypeTraitAliasesRule($localTypeAliasesCheck, $reflectionProvider), diff --git a/src/Rules/Methods/ConsistentConstructorRule.php b/src/Rules/Methods/ConsistentConstructorRule.php index ab8553b5cc..16226a1074 100644 --- a/src/Rules/Methods/ConsistentConstructorRule.php +++ b/src/Rules/Methods/ConsistentConstructorRule.php @@ -7,6 +7,7 @@ use PHPStan\Node\InClassMethodNode; use PHPStan\Reflection\Dummy\DummyConstructorReflection; use PHPStan\Rules\Rule; +use function array_merge; use function strtolower; /** @implements Rule */ @@ -15,6 +16,7 @@ final class ConsistentConstructorRule implements Rule public function __construct( private MethodParameterComparisonHelper $methodParameterComparisonHelper, + private MethodVisibilityComparisonHelper $methodVisibilityComparisonHelper, ) { } @@ -47,7 +49,10 @@ public function processNode(Node $node, Scope $scope): array return []; } - return $this->methodParameterComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method, true); + return array_merge( + $this->methodParameterComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method, true), + $this->methodVisibilityComparisonHelper->compare($parentConstructor, $parentConstructor->getDeclaringClass(), $method), + ); } } diff --git a/src/Rules/Methods/MethodVisibilityComparisonHelper.php b/src/Rules/Methods/MethodVisibilityComparisonHelper.php new file mode 100755 index 0000000000..4807453f2a --- /dev/null +++ b/src/Rules/Methods/MethodVisibilityComparisonHelper.php @@ -0,0 +1,51 @@ + */ + public function compare(ExtendedMethodReflection $prototype, ClassReflection $prototypeDeclaringClass, PhpMethodFromParserNodeReflection $method): array + { + /** @var list $messages */ + $messages = []; + + if ($prototype->isPublic()) { + if (!$method->isPublic()) { + $messages[] = RuleErrorBuilder::message(sprintf( + '%s method %s::%s() overriding public method %s::%s() should also be public.', + $method->isPrivate() ? 'Private' : 'Protected', + $method->getDeclaringClass()->getDisplayName(), + $method->getName(), + $prototypeDeclaringClass->getDisplayName(true), + $prototype->getName(), + )) + ->nonIgnorable() + ->identifier('method.visibility') + ->build(); + } + } elseif ($method->isPrivate()) { + $messages[] = RuleErrorBuilder::message(sprintf( + 'Private method %s::%s() overriding protected method %s::%s() should be protected or public.', + $method->getDeclaringClass()->getDisplayName(), + $method->getName(), + $prototypeDeclaringClass->getDisplayName(true), + $prototype->getName(), + )) + ->nonIgnorable() + ->identifier('method.visibility') + ->build(); + } + + return $messages; + } + +} diff --git a/src/Rules/Methods/OverridingMethodRule.php b/src/Rules/Methods/OverridingMethodRule.php index 38c588f9db..81a9b1b0e1 100644 --- a/src/Rules/Methods/OverridingMethodRule.php +++ b/src/Rules/Methods/OverridingMethodRule.php @@ -35,6 +35,7 @@ public function __construct( private MethodSignatureRule $methodSignatureRule, private bool $checkPhpDocMethodSignatures, private MethodParameterComparisonHelper $methodParameterComparisonHelper, + private MethodVisibilityComparisonHelper $methodVisibilityComparisonHelper, private PhpClassReflectionExtension $phpClassReflectionExtension, private bool $checkMissingOverrideMethodAttribute, ) @@ -165,32 +166,7 @@ public function processNode(Node $node, Scope $scope): array } if ($checkVisibility) { - if ($prototype->isPublic()) { - if (!$method->isPublic()) { - $messages[] = RuleErrorBuilder::message(sprintf( - '%s method %s::%s() overriding public method %s::%s() should also be public.', - $method->isPrivate() ? 'Private' : 'Protected', - $method->getDeclaringClass()->getDisplayName(), - $method->getName(), - $prototypeDeclaringClass->getDisplayName(true), - $prototype->getName(), - )) - ->nonIgnorable() - ->identifier('method.visibility') - ->build(); - } - } elseif ($method->isPrivate()) { - $messages[] = RuleErrorBuilder::message(sprintf( - 'Private method %s::%s() overriding protected method %s::%s() should be protected or public.', - $method->getDeclaringClass()->getDisplayName(), - $method->getName(), - $prototypeDeclaringClass->getDisplayName(true), - $prototype->getName(), - )) - ->nonIgnorable() - ->identifier('method.visibility') - ->build(); - } + $messages = array_merge($messages, $this->methodVisibilityComparisonHelper->compare($prototype, $prototypeDeclaringClass, $method)); } $prototypeVariants = $prototype->getVariants(); diff --git a/tests/PHPStan/Rules/Methods/ConsistentConstructorRuleTest.php b/tests/PHPStan/Rules/Methods/ConsistentConstructorRuleTest.php index 28b0091af9..adcb69cdbe 100644 --- a/tests/PHPStan/Rules/Methods/ConsistentConstructorRuleTest.php +++ b/tests/PHPStan/Rules/Methods/ConsistentConstructorRuleTest.php @@ -12,7 +12,10 @@ class ConsistentConstructorRuleTest extends RuleTestCase protected function getRule(): Rule { - return new ConsistentConstructorRule(self::getContainer()->getByType(MethodParameterComparisonHelper::class)); + return new ConsistentConstructorRule( + self::getContainer()->getByType(MethodParameterComparisonHelper::class), + self::getContainer()->getByType(MethodVisibilityComparisonHelper::class), + ); } public function testRule(): void @@ -42,4 +45,14 @@ public function testRuleNoErrors(): void $this->analyse([__DIR__ . '/data/consistent-constructor-no-errors.php'], []); } + public function testBug12137(): void + { + $this->analyse([__DIR__ . '/data/bug-12137.php'], [ + [ + 'Private method Bug12137\ChildClass::__construct() overriding protected method Bug12137\ParentClass::__construct() should be protected or public.', + 20, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php b/tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php index 5f75cd0554..580d21787c 100644 --- a/tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php +++ b/tests/PHPStan/Rules/Methods/MethodSignatureRuleTest.php @@ -29,6 +29,7 @@ protected function getRule(): Rule new MethodSignatureRule($phpClassReflectionExtension, $this->reportMaybes, $this->reportStatic), true, new MethodParameterComparisonHelper($phpVersion), + new MethodVisibilityComparisonHelper(), $phpClassReflectionExtension, false, ); diff --git a/tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php b/tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php index abbe2927ab..9c65a99d35 100644 --- a/tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php +++ b/tests/PHPStan/Rules/Methods/OverridingMethodRuleTest.php @@ -31,6 +31,7 @@ protected function getRule(): Rule new MethodSignatureRule($phpClassReflectionExtension, true, true), false, new MethodParameterComparisonHelper($phpVersion), + new MethodVisibilityComparisonHelper(), $phpClassReflectionExtension, $this->checkMissingOverrideMethodAttribute, ); diff --git a/tests/PHPStan/Rules/Methods/data/bug-12137.php b/tests/PHPStan/Rules/Methods/data/bug-12137.php new file mode 100755 index 0000000000..eacb78bbf3 --- /dev/null +++ b/tests/PHPStan/Rules/Methods/data/bug-12137.php @@ -0,0 +1,23 @@ +