Add opt-in rule requiring ConstraintValidator::validate() to narrow the constraint type#977
Add opt-in rule requiring ConstraintValidator::validate() to narrow the constraint type#977
Conversation
…he 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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds an opt-in PHPStan rule to enforce Drupal’s convention that ConstraintValidator::validate() narrows the concrete constraint type (via assert($constraint instanceof …)), helping PHPStan infer constraint-specific properties and avoid “undefined property” reports.
Changes:
- Introduces
ConstraintValidatorValidateNarrowsConstraintTypeRuleto detect missing type-narrowing asserts based on theFooValidator→Foonaming convention. - Adds a PHPUnit rule test and a fixture file demonstrating expected behaviors.
- Registers the rule as an opt-in conditional rule via
extension.neon+rules.neon.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/Rules/Drupal/ConstraintValidatorValidateNarrowsConstraintTypeRule.php |
New rule implementation that derives the expected constraint class and checks for a narrowing assert() in validate(). |
tests/src/Rules/ConstraintValidatorValidateNarrowsConstraintTypeRuleTest.php |
New test case wiring the rule into the existing DrupalRuleTestCase harness. |
tests/src/Rules/data/constraint-validator.php |
New fixture file containing example constraints/validators for the rule to analyze. |
rules.neon |
Registers the rule as a conditional (opt-in) PHPStan rule. |
extension.neon |
Adds the opt-in parameter and schema entry for enabling the rule. |
- 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 <noreply@anthropic.com>
| // Check if the method body asserts the concrete constraint type. | ||
| if ($this->hasConstraintAssertion($node->stmts ?? [], $constraintParamName, $constraintClass, $scope)) { |
There was a problem hiding this comment.
processNode() will report an error for abstract validate() methods (or interface methods) because $node->stmts is null and you pass an empty array into hasConstraintAssertion(). In those cases the suggested fix (adding an assert in the method body) is impossible, so this should be skipped (e.g., return [] when $node->stmts === null or when the method is abstract).
| // Check if the method body asserts the concrete constraint type. | |
| if ($this->hasConstraintAssertion($node->stmts ?? [], $constraintParamName, $constraintClass, $scope)) { | |
| if ($node->stmts === null) { | |
| return []; | |
| } | |
| // Check if the method body asserts the concrete constraint type. | |
| if ($this->hasConstraintAssertion($node->stmts, $constraintParamName, $constraintClass, $scope)) { |
| 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) { | ||
| continue; | ||
| } | ||
| if (!$assertArg->class instanceof Node\Name) { | ||
| continue; | ||
| } | ||
| $assertedClass = $scope->resolveName($assertArg->class); | ||
| if ($assertedClass === $expectedConstraintFqcn) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
hasConstraintAssertion() only inspects top-level Expression statements, so it will miss valid narrowing asserts that appear inside control-flow blocks (e.g., inside an if/try) or other nested statements. This can lead to false positives even though the method body does contain the required assert; consider using a NodeFinder (or recursive traversal) to search the whole statement subtree for an assert($param instanceof Expected) call.
|
|
||
| } |
There was a problem hiding this comment.
The tests currently cover only the simplest assert-at-top-level case. Since the rule is sensitive to AST shape (e.g., assert nested in an if block, or a non-standard $constraint parameter name), add fixtures/tests for those scenarios to prevent regressions and to validate the intended detection behavior.
| } | |
| public function testRuleAllowsNestedAssert(): void | |
| { | |
| $fixture = $this->createFixture(<<<'PHP' | |
| <?php | |
| declare(strict_types=1); | |
| use Symfony\Component\Validator\Constraint; | |
| use Symfony\Component\Validator\ConstraintValidator; | |
| final class UniqueItem extends Constraint | |
| { | |
| public string $message = 'Invalid'; | |
| } | |
| final class NestedAssertValidator extends ConstraintValidator | |
| { | |
| public function validate(mixed $value, Constraint $constraint): void | |
| { | |
| if ($value !== null) { | |
| assert($constraint instanceof UniqueItem); | |
| $constraint->message; | |
| } | |
| } | |
| } | |
| PHP); | |
| try { | |
| $this->analyse([$fixture], []); | |
| } finally { | |
| @unlink($fixture); | |
| } | |
| } | |
| public function testRuleAllowsNonStandardConstraintParameterName(): void | |
| { | |
| $fixture = $this->createFixture(<<<'PHP' | |
| <?php | |
| declare(strict_types=1); | |
| use Symfony\Component\Validator\Constraint; | |
| use Symfony\Component\Validator\ConstraintValidator; | |
| final class UniqueItem extends Constraint | |
| { | |
| public string $message = 'Invalid'; | |
| } | |
| final class AlternateNameValidator extends ConstraintValidator | |
| { | |
| public function validate(mixed $value, Constraint $customConstraint): void | |
| { | |
| assert($customConstraint instanceof UniqueItem); | |
| $customConstraint->message; | |
| } | |
| } | |
| PHP); | |
| try { | |
| $this->analyse([$fixture], []); | |
| } finally { | |
| @unlink($fixture); | |
| } | |
| } | |
| private function createFixture(string $contents): string | |
| { | |
| $fixture = tempnam(sys_get_temp_dir(), 'constraint-validator-'); | |
| if ($fixture === false) { | |
| self::fail('Failed to create temporary fixture file.'); | |
| } | |
| $path = $fixture . '.php'; | |
| if (!rename($fixture, $path)) { | |
| @unlink($fixture); | |
| self::fail('Failed to prepare temporary fixture file.'); | |
| } | |
| if (file_put_contents($path, $contents) === false) { | |
| @unlink($path); | |
| self::fail('Failed to write temporary fixture file.'); | |
| } | |
| return $path; | |
| } | |
| } |
Summary
Closes #466
PHPStan reports "Access to an undefined property" when a
ConstraintValidatorsubclass accesses constraint-specific properties, because$constraintis typed as the baseSymfony\Component\Validator\Constraintclass in the method signature.phpstan-symfonydoes not handle this use case (phpstan/phpstan-symfony#16).Drupal core's own convention — visible in
ConfigExistsConstraintValidator— is to addassert($constraint instanceof SpecificConstraint)at the top ofvalidate(), which PHPStan narrows natively.What changed
ConstraintValidatorValidateNarrowsConstraintTypeRulethat fires when a class following theFooValidator/Foonaming convention does not narrow$constraintin itsvalidate()methodassert()call to addfalse), enabled with:How it works
validate()methods in classes extendingConstraintValidatorValidatorsuffix from the FQCN to derive the expected constraint class (e.g.UniqueItemValidator→UniqueItem)Constraintassert($constraint instanceof MatchingClass)— if not, reports an errorTest plan
php vendor/bin/phpunit --filter=ConstraintValidatorValidateNarrowsConstraintTypeRuleTestpassesphp vendor/bin/phpstan analyzepasses on the new rule filephp vendor/bin/phpcspasses on the new rule file🤖 Generated with Claude Code