From 763169267ea617b36a10d5df0a3d3939d1632341 Mon Sep 17 00:00:00 2001 From: Matt Glaman Date: Wed, 15 Apr 2026 09:33:48 -0500 Subject: [PATCH 1/2] Add opt-in rule requiring ConstraintValidator::validate() to narrow the constraint type PHPStan reports "Access to an undefined property" when a ConstraintValidator subclass accesses constraint-specific properties because $constraint is typed as the base Symfony\Component\Validator\Constraint class. phpstan-symfony does not handle this (phpstan/phpstan-symfony#16). The new ConstraintValidatorValidateNarrowsConstraintTypeRule detects validators following the FooValidator/Foo naming convention and requires an `assert($constraint instanceof Foo)` assertion in validate(), matching Drupal core's own convention. The rule is opt-in via `parameters: drupal: rules: constraintValidatorMustNarrowConstraintType: true`. Closes #466 Co-Authored-By: Claude Sonnet 4.6 --- extension.neon | 2 + rules.neon | 4 + ...datorValidateNarrowsConstraintTypeRule.php | 148 ++++++++++++++++++ ...rValidateNarrowsConstraintTypeRuleTest.php | 33 ++++ tests/src/Rules/data/constraint-validator.php | 63 ++++++++ 5 files changed, 250 insertions(+) create mode 100644 src/Rules/Drupal/ConstraintValidatorValidateNarrowsConstraintTypeRule.php create mode 100644 tests/src/Rules/ConstraintValidatorValidateNarrowsConstraintTypeRuleTest.php create mode 100644 tests/src/Rules/data/constraint-validator.php diff --git a/extension.neon b/extension.neon index 6d50e3c5..fe294170 100644 --- a/extension.neon +++ b/extension.neon @@ -37,6 +37,7 @@ parameters: hookRules: false loggerFromFactoryPropertyAssignmentRule: false entityStorageDirectInjectionRule: false + constraintValidatorMustNarrowConstraintType: false entityMapping: aggregator_feed: class: Drupal\aggregator\Entity\Feed @@ -275,6 +276,7 @@ parametersSchema: hookRules: boolean() loggerFromFactoryPropertyAssignmentRule: boolean() entityStorageDirectInjectionRule: boolean() + constraintValidatorMustNarrowConstraintType: boolean() ]) entityMapping: arrayOf(anyOf( structure([ diff --git a/rules.neon b/rules.neon index b66d6497..0c927286 100644 --- a/rules.neon +++ b/rules.neon @@ -34,6 +34,8 @@ conditionalTags: phpstan.rules.rule: %drupal.rules.entityStorageDirectInjectionRule% mglaman\PHPStanDrupal\Rules\Drupal\EntityStoragePropertyAssignmentRule: phpstan.rules.rule: %drupal.rules.entityStorageDirectInjectionRule% + mglaman\PHPStanDrupal\Rules\Drupal\ConstraintValidatorValidateNarrowsConstraintTypeRule: + phpstan.rules.rule: %drupal.rules.constraintValidatorMustNarrowConstraintType% services: - @@ -58,3 +60,5 @@ services: class: mglaman\PHPStanDrupal\Rules\Drupal\EntityStorageDirectInjectionRule - class: mglaman\PHPStanDrupal\Rules\Drupal\EntityStoragePropertyAssignmentRule + - + class: mglaman\PHPStanDrupal\Rules\Drupal\ConstraintValidatorValidateNarrowsConstraintTypeRule diff --git a/src/Rules/Drupal/ConstraintValidatorValidateNarrowsConstraintTypeRule.php b/src/Rules/Drupal/ConstraintValidatorValidateNarrowsConstraintTypeRule.php new file mode 100644 index 00000000..00369647 --- /dev/null +++ b/src/Rules/Drupal/ConstraintValidatorValidateNarrowsConstraintTypeRule.php @@ -0,0 +1,148 @@ + + */ +final class ConstraintValidatorValidateNarrowsConstraintTypeRule implements Rule +{ + + private ReflectionProvider $reflectionProvider; + + public function __construct(ReflectionProvider $reflectionProvider) + { + $this->reflectionProvider = $reflectionProvider; + } + + public function getNodeType(): string + { + return Node\Stmt\ClassMethod::class; + } + + public function processNode(Node $node, Scope $scope): array + { + if ($node->name->toString() !== 'validate') { + return []; + } + if (!$scope->isInClass()) { + return []; + } + + $classReflection = $scope->getClassReflection(); + if (!$classReflection->isSubclassOfClass($this->reflectionProvider->getClass(ConstraintValidator::class))) { + return []; + } + + // Derive the matching constraint class name by stripping 'Validator' suffix. + $fqcn = $classReflection->getName(); + if (!str_ends_with($fqcn, 'Validator')) { + return []; + } + $constraintClass = substr($fqcn, 0, -9); + + if (!$this->reflectionProvider->hasClass($constraintClass)) { + return []; + } + $constraintReflection = $this->reflectionProvider->getClass($constraintClass); + if (!$constraintReflection->isSubclassOfClass($this->reflectionProvider->getClass(Constraint::class))) { + return []; + } + + // Find the $constraint parameter name (second param by Symfony convention). + $constraintParamName = 'constraint'; + if (array_key_exists(1, $node->params)) { + $param = $node->params[1]; + if ($param->var instanceof Node\Expr\Variable && is_string($param->var->name)) { + $constraintParamName = $param->var->name; + } + } + + // Check if the method body asserts the concrete constraint type. + if ($this->hasConstraintAssertion($node->stmts ?? [], $constraintParamName)) { + return []; + } + + $validatorFqcn = $classReflection->getName(); + $validatorPos = strrpos($validatorFqcn, '\\'); + $validatorShortName = $validatorPos !== false ? substr($validatorFqcn, $validatorPos + 1) : $validatorFqcn; + $constraintFqcn = $constraintReflection->getName(); + $constraintPos = strrpos($constraintFqcn, '\\'); + $constraintShortName = $constraintPos !== false ? substr($constraintFqcn, $constraintPos + 1) : $constraintFqcn; + + return [ + RuleErrorBuilder::message(sprintf( + 'Method %s::validate() does not narrow the $%s parameter type. Add assert($%s instanceof %s) to allow PHPStan to infer constraint-specific properties.', + $validatorShortName, + $constraintParamName, + $constraintParamName, + $constraintShortName + )) + ->identifier('drupal.constraintValidator.missingNarrow') + ->tip('See https://www.drupal.org/project/drupal/issues/3246287') + ->build(), + ]; + } + + /** + * Returns true if any statement in the list is assert($var instanceof SomeClass). + * + * @param Node\Stmt[] $stmts + */ + private function hasConstraintAssertion(array $stmts, string $paramName): bool + { + foreach ($stmts as $stmt) { + if (!$stmt instanceof Node\Stmt\Expression) { + continue; + } + $expr = $stmt->expr; + if (!$expr instanceof Node\Expr\FuncCall) { + continue; + } + if (!$expr->name instanceof Node\Name || $expr->name->toString() !== 'assert') { + continue; + } + $args = $expr->getArgs(); + if (count($args) === 0) { + continue; + } + $assertArg = $args[0]->value; + if (!$assertArg instanceof Node\Expr\Instanceof_) { + continue; + } + if (!$assertArg->expr instanceof Node\Expr\Variable) { + continue; + } + if ($assertArg->expr->name === $paramName) { + return true; + } + } + + return false; + } +} diff --git a/tests/src/Rules/ConstraintValidatorValidateNarrowsConstraintTypeRuleTest.php b/tests/src/Rules/ConstraintValidatorValidateNarrowsConstraintTypeRuleTest.php new file mode 100644 index 00000000..f07a5d0c --- /dev/null +++ b/tests/src/Rules/ConstraintValidatorValidateNarrowsConstraintTypeRuleTest.php @@ -0,0 +1,33 @@ +getByType(ConstraintValidatorValidateNarrowsConstraintTypeRule::class); + } + + public function testRule(): void + { + $this->analyse( + [__DIR__ . '/data/constraint-validator.php'], + [ + [ + 'Method UniqueItemValidator::validate() does not narrow the $constraint parameter type. Add assert($constraint instanceof UniqueItem) to allow PHPStan to infer constraint-specific properties.', + 23, + 'See https://www.drupal.org/project/drupal/issues/3246287', + ], + ] + ); + } + +} diff --git a/tests/src/Rules/data/constraint-validator.php b/tests/src/Rules/data/constraint-validator.php new file mode 100644 index 00000000..f8d34509 --- /dev/null +++ b/tests/src/Rules/data/constraint-validator.php @@ -0,0 +1,63 @@ +context->addViolation($constraint->alreadyExists); + } + +} + +// No error: validate() uses assert() to narrow the constraint type. +class UniqueItemValidatorWithAssert extends ConstraintValidator +{ + + public function validate(mixed $value, Constraint $constraint): void + { + assert($constraint instanceof UniqueItem); + $this->context->addViolation($constraint->alreadyExists); + } + +} + +// No error: validator class name does not follow the FooValidator/Foo convention, +// so no matching constraint class can be derived. +class StandaloneValidator extends ConstraintValidator +{ + + public function validate(mixed $value, Constraint $constraint): void + { + $this->context->addViolation('some message'); + } + +} + +// No error: 'OrphanedValidator' strips to 'Orphaned', which is not a known constraint. +class OrphanedValidator extends ConstraintValidator +{ + + public function validate(mixed $value, Constraint $constraint): void + { + $this->context->addViolation('some message'); + } + +} From 5379f01d2c634a86eb96a5b4c745a4f2ad957ada Mon Sep 17 00:00:00 2001 From: Matt Glaman Date: Wed, 15 Apr 2026 10:39:43 -0500 Subject: [PATCH 2/2] Address Copilot review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add hasClass() guards for ConstraintValidator/Constraint before calling getClass(), so the rule exits cleanly when symfony/validator is absent - Verify the asserted class in assert() matches the expected constraint FQCN, preventing false negatives from assert($constraint instanceof WrongClass) - Rename UniqueItemValidatorWithAssert → UniqueItemWithAssertValidator so the assert-detection path is actually exercised (naming convention was wrong) - Fix misleading comment on StandaloneValidator (it does match the suffix convention; no error because no matching constraint class exists) - Remove unused ExecutionContextInterface import from fixture Co-Authored-By: Claude Sonnet 4.6 --- ...datorValidateNarrowsConstraintTypeRule.php | 21 +++++++++++++++---- ...rValidateNarrowsConstraintTypeRuleTest.php | 2 +- tests/src/Rules/data/constraint-validator.php | 21 ++++++++++++------- 3 files changed, 32 insertions(+), 12 deletions(-) diff --git a/src/Rules/Drupal/ConstraintValidatorValidateNarrowsConstraintTypeRule.php b/src/Rules/Drupal/ConstraintValidatorValidateNarrowsConstraintTypeRule.php index 00369647..e7c54ae5 100644 --- a/src/Rules/Drupal/ConstraintValidatorValidateNarrowsConstraintTypeRule.php +++ b/src/Rules/Drupal/ConstraintValidatorValidateNarrowsConstraintTypeRule.php @@ -54,6 +54,12 @@ public function processNode(Node $node, Scope $scope): array return []; } + // Guard: symfony/validator may not be installed in every project. + if (!$this->reflectionProvider->hasClass(ConstraintValidator::class) + || !$this->reflectionProvider->hasClass(Constraint::class)) { + return []; + } + $classReflection = $scope->getClassReflection(); if (!$classReflection->isSubclassOfClass($this->reflectionProvider->getClass(ConstraintValidator::class))) { return []; @@ -84,7 +90,7 @@ public function processNode(Node $node, Scope $scope): array } // Check if the method body asserts the concrete constraint type. - if ($this->hasConstraintAssertion($node->stmts ?? [], $constraintParamName)) { + if ($this->hasConstraintAssertion($node->stmts ?? [], $constraintParamName, $constraintClass, $scope)) { return []; } @@ -110,11 +116,11 @@ public function processNode(Node $node, Scope $scope): array } /** - * Returns true if any statement in the list is assert($var instanceof SomeClass). + * Returns true if the statement list contains assert($var instanceof ExpectedConstraintClass). * * @param Node\Stmt[] $stmts */ - private function hasConstraintAssertion(array $stmts, string $paramName): bool + private function hasConstraintAssertion(array $stmts, string $paramName, string $expectedConstraintFqcn, Scope $scope): bool { foreach ($stmts as $stmt) { if (!$stmt instanceof Node\Stmt\Expression) { @@ -138,7 +144,14 @@ private function hasConstraintAssertion(array $stmts, string $paramName): bool if (!$assertArg->expr instanceof Node\Expr\Variable) { continue; } - if ($assertArg->expr->name === $paramName) { + if ($assertArg->expr->name !== $paramName) { + continue; + } + if (!$assertArg->class instanceof Node\Name) { + continue; + } + $assertedClass = $scope->resolveName($assertArg->class); + if ($assertedClass === $expectedConstraintFqcn) { return true; } } diff --git a/tests/src/Rules/ConstraintValidatorValidateNarrowsConstraintTypeRuleTest.php b/tests/src/Rules/ConstraintValidatorValidateNarrowsConstraintTypeRuleTest.php index f07a5d0c..dd6a4457 100644 --- a/tests/src/Rules/ConstraintValidatorValidateNarrowsConstraintTypeRuleTest.php +++ b/tests/src/Rules/ConstraintValidatorValidateNarrowsConstraintTypeRuleTest.php @@ -23,7 +23,7 @@ public function testRule(): void [ [ 'Method UniqueItemValidator::validate() does not narrow the $constraint parameter type. Add assert($constraint instanceof UniqueItem) to allow PHPStan to infer constraint-specific properties.', - 23, + 22, 'See https://www.drupal.org/project/drupal/issues/3246287', ], ] diff --git a/tests/src/Rules/data/constraint-validator.php b/tests/src/Rules/data/constraint-validator.php index f8d34509..9381b845 100644 --- a/tests/src/Rules/data/constraint-validator.php +++ b/tests/src/Rules/data/constraint-validator.php @@ -6,7 +6,6 @@ use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\ConstraintValidator; -use Symfony\Component\Validator\Context\ExecutionContextInterface; // Constraint class following the naming convention: UniqueItem / UniqueItemValidator. class UniqueItem extends Constraint @@ -16,7 +15,7 @@ class UniqueItem extends Constraint } -// Error: validate() accesses constraint properties but never asserts the type. +// Error: validate() does not assert the concrete constraint type. class UniqueItemValidator extends ConstraintValidator { @@ -27,20 +26,28 @@ public function validate(mixed $value, Constraint $constraint): void } -// No error: validate() uses assert() to narrow the constraint type. -class UniqueItemValidatorWithAssert extends ConstraintValidator +// Constraint class for the assert-detection test case. +class UniqueItemWithAssert extends Constraint +{ + + public string $alreadyExists = 'The item %value already exists.'; + +} + +// No error: validate() uses assert() to narrow to the expected constraint type. +class UniqueItemWithAssertValidator extends ConstraintValidator { public function validate(mixed $value, Constraint $constraint): void { - assert($constraint instanceof UniqueItem); + assert($constraint instanceof UniqueItemWithAssert); $this->context->addViolation($constraint->alreadyExists); } } -// No error: validator class name does not follow the FooValidator/Foo convention, -// so no matching constraint class can be derived. +// No error: 'StandaloneValidator' strips to 'Standalone', but there is no +// corresponding constraint class with that name. class StandaloneValidator extends ConstraintValidator {