From 1581e5b2e8404446c008648698c3503e38546cf7 Mon Sep 17 00:00:00 2001 From: Reio Remma Date: Mon, 22 Sep 2025 10:55:13 +0300 Subject: [PATCH 1/2] Check valid field names are used in orderBy parameter of findBy and findOneBy repository methods. --- .../Doctrine/ORM/RepositoryMethodCallRule.php | 31 ++++++++++++++++++ .../ORM/RepositoryMethodCallRuleTest.php | 32 +++++++++++++++++++ ...CallRuleWithoutObjectManagerLoaderTest.php | 32 +++++++++++++++++++ .../ORM/data/repository-findBy-etc.php | 21 ++++++++++++ 4 files changed, 116 insertions(+) diff --git a/src/Rules/Doctrine/ORM/RepositoryMethodCallRule.php b/src/Rules/Doctrine/ORM/RepositoryMethodCallRule.php index 10b79867..c2b8f115 100644 --- a/src/Rules/Doctrine/ORM/RepositoryMethodCallRule.php +++ b/src/Rules/Doctrine/ORM/RepositoryMethodCallRule.php @@ -87,6 +87,37 @@ public function processNode(Node $node, Scope $scope): array } } + $orderByType = count($node->getArgs()) > 1 ? $scope->getType($node->getArgs()[1]->value) : null; + + if ( + in_array($methodName, [ + 'findBy', + 'findOneBy', + ], true) + && $orderByType !== null + ) { + foreach ($orderByType->getConstantArrays() as $constantArray) { + foreach ($constantArray->getKeyTypes() as $keyType) { + foreach ($keyType->getConstantStrings() as $fieldName) { + if ( + $classMetadata->hasField($fieldName->getValue()) + || $classMetadata->hasAssociation($fieldName->getValue()) + ) { + continue; + } + + $messages[] = RuleErrorBuilder::message(sprintf( + 'Call to method %s::%s() - entity %s does not have a field named $%s.', + $calledOnType->describe(VerbosityLevel::typeOnly()), + $methodName, + $entityClassNames[0], + $fieldName->getValue(), + ))->identifier(sprintf('doctrine.%sArgument', $methodName))->build(); + } + } + } + } + return $messages; } diff --git a/tests/Rules/Doctrine/ORM/RepositoryMethodCallRuleTest.php b/tests/Rules/Doctrine/ORM/RepositoryMethodCallRuleTest.php index da10cbf3..4204e35f 100644 --- a/tests/Rules/Doctrine/ORM/RepositoryMethodCallRuleTest.php +++ b/tests/Rules/Doctrine/ORM/RepositoryMethodCallRuleTest.php @@ -76,6 +76,38 @@ public function testRule(): void 'Call to method Doctrine\ORM\EntityRepository::count() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $transient.', 45, ], + [ + 'Call to method Doctrine\ORM\EntityRepository::findBy() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $transient.', + 54, + ], + [ + 'Call to method Doctrine\ORM\EntityRepository::findBy() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $nonexistent.', + 55, + ], + [ + 'Call to method Doctrine\ORM\EntityRepository::findBy() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $nonexistent.', + 56, + ], + [ + 'Call to method Doctrine\ORM\EntityRepository::findBy() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $transient.', + 56, + ], + [ + 'Call to method Doctrine\ORM\EntityRepository::findOneBy() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $transient.', + 64, + ], + [ + 'Call to method Doctrine\ORM\EntityRepository::findOneBy() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $nonexistent.', + 65, + ], + [ + 'Call to method Doctrine\ORM\EntityRepository::findOneBy() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $nonexistent.', + 66, + ], + [ + 'Call to method Doctrine\ORM\EntityRepository::findOneBy() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $transient.', + 66, + ], ]); } diff --git a/tests/Rules/Doctrine/ORM/RepositoryMethodCallRuleWithoutObjectManagerLoaderTest.php b/tests/Rules/Doctrine/ORM/RepositoryMethodCallRuleWithoutObjectManagerLoaderTest.php index 849d3675..0cc4f38b 100644 --- a/tests/Rules/Doctrine/ORM/RepositoryMethodCallRuleWithoutObjectManagerLoaderTest.php +++ b/tests/Rules/Doctrine/ORM/RepositoryMethodCallRuleWithoutObjectManagerLoaderTest.php @@ -76,6 +76,38 @@ public function testRule(): void 'Call to method Doctrine\ORM\EntityRepository::count() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $transient.', 45, ], + [ + 'Call to method Doctrine\ORM\EntityRepository::findBy() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $transient.', + 54, + ], + [ + 'Call to method Doctrine\ORM\EntityRepository::findBy() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $nonexistent.', + 55, + ], + [ + 'Call to method Doctrine\ORM\EntityRepository::findBy() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $nonexistent.', + 56, + ], + [ + 'Call to method Doctrine\ORM\EntityRepository::findBy() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $transient.', + 56, + ], + [ + 'Call to method Doctrine\ORM\EntityRepository::findOneBy() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $transient.', + 64, + ], + [ + 'Call to method Doctrine\ORM\EntityRepository::findOneBy() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $nonexistent.', + 65, + ], + [ + 'Call to method Doctrine\ORM\EntityRepository::findOneBy() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $nonexistent.', + 66, + ], + [ + 'Call to method Doctrine\ORM\EntityRepository::findOneBy() - entity PHPStan\Rules\Doctrine\ORM\MyEntity does not have a field named $transient.', + 66, + ], ]); } diff --git a/tests/Rules/Doctrine/ORM/data/repository-findBy-etc.php b/tests/Rules/Doctrine/ORM/data/repository-findBy-etc.php index ee5fa6a7..c4271b5c 100644 --- a/tests/Rules/Doctrine/ORM/data/repository-findBy-etc.php +++ b/tests/Rules/Doctrine/ORM/data/repository-findBy-etc.php @@ -45,4 +45,25 @@ public function doCountBy(): void $entityRepository->count(['nonexistent' => 'test', 'transient' => 'test']); } + + public function doFindByOrder(): void + { + $entityRepository = $this->entityManager->getRepository(MyEntity::class); + $entityRepository->findBy([], ['id' => 'ASC']); + $entityRepository->findBy([], ['title' => 'DESC']); + $entityRepository->findBy([], ['transient' => 'ASC']); + $entityRepository->findBy([], ['nonexistent' => 'DESC']); + $entityRepository->findBy([], ['nonexistent' => 'ASC', 'transient' => 'DESC']); + } + + public function doFindOneByOrder(): void + { + $entityRepository = $this->entityManager->getRepository(MyEntity::class); + $entityRepository->findOneBy([], ['id' => 'ASC']); + $entityRepository->findOneBy([], ['title' => 'DESC']); + $entityRepository->findOneBy([], ['transient' => 'ASC']); + $entityRepository->findOneBy([], ['nonexistent' => 'DESC']); + $entityRepository->findOneBy([], ['nonexistent' => 'ASC', 'transient' => 'DESC']); + } + } From 7c56b87c541a9e7a64c60861d0d5c5abbd865061 Mon Sep 17 00:00:00 2001 From: Reio Remma Date: Sat, 30 May 2026 14:51:46 +0300 Subject: [PATCH 2/2] Only check orderBy fields when bleedingEdge is enabled and test with checkOrderByFields enabled. --- rules.neon | 6 ++ .../Doctrine/ORM/RepositoryMethodCallRule.php | 63 +++++++++++-------- .../data/entityRepositoryDynamicReturn-0.json | 10 +++ .../ORM/RepositoryMethodCallRuleTest.php | 2 +- ...CallRuleWithoutObjectManagerLoaderTest.php | 2 +- 5 files changed, 56 insertions(+), 27 deletions(-) diff --git a/rules.neon b/rules.neon index 0853184d..1a864a70 100644 --- a/rules.neon +++ b/rules.neon @@ -46,6 +46,12 @@ services: allowNullablePropertyForRequiredField: %doctrine.allowNullablePropertyForRequiredField% tags: - phpstan.rules.rule + - + class: PHPStan\Rules\Doctrine\ORM\RepositoryMethodCallRule + arguments: + checkOrderByFields: %featureToggles.bleedingEdge% + tags: + - phpstan.rules.rule - class: PHPStan\Classes\DoctrineProxyForbiddenClassNamesExtension tags: diff --git a/src/Rules/Doctrine/ORM/RepositoryMethodCallRule.php b/src/Rules/Doctrine/ORM/RepositoryMethodCallRule.php index c2b8f115..38ca3e9b 100644 --- a/src/Rules/Doctrine/ORM/RepositoryMethodCallRule.php +++ b/src/Rules/Doctrine/ORM/RepositoryMethodCallRule.php @@ -21,9 +21,15 @@ class RepositoryMethodCallRule implements Rule private ObjectMetadataResolver $objectMetadataResolver; - public function __construct(ObjectMetadataResolver $objectMetadataResolver) + private bool $checkOrderByFields; + + public function __construct( + ObjectMetadataResolver $objectMetadataResolver, + bool $checkOrderByFields = false + ) { $this->objectMetadataResolver = $objectMetadataResolver; + $this->checkOrderByFields = $checkOrderByFields; } public function getNodeType(): string @@ -87,33 +93,40 @@ public function processNode(Node $node, Scope $scope): array } } + if (!$this->checkOrderByFields) { + return $messages; + } + + if (!in_array($methodName, [ + 'findBy', + 'findOneBy', + ], true)) { + return $messages; + } + $orderByType = count($node->getArgs()) > 1 ? $scope->getType($node->getArgs()[1]->value) : null; - if ( - in_array($methodName, [ - 'findBy', - 'findOneBy', - ], true) - && $orderByType !== null - ) { - foreach ($orderByType->getConstantArrays() as $constantArray) { - foreach ($constantArray->getKeyTypes() as $keyType) { - foreach ($keyType->getConstantStrings() as $fieldName) { - if ( - $classMetadata->hasField($fieldName->getValue()) - || $classMetadata->hasAssociation($fieldName->getValue()) - ) { - continue; - } - - $messages[] = RuleErrorBuilder::message(sprintf( - 'Call to method %s::%s() - entity %s does not have a field named $%s.', - $calledOnType->describe(VerbosityLevel::typeOnly()), - $methodName, - $entityClassNames[0], - $fieldName->getValue(), - ))->identifier(sprintf('doctrine.%sArgument', $methodName))->build(); + if ($orderByType === null) { + return $messages; + } + + foreach ($orderByType->getConstantArrays() as $constantArray) { + foreach ($constantArray->getKeyTypes() as $keyType) { + foreach ($keyType->getConstantStrings() as $fieldName) { + if ( + $classMetadata->hasField($fieldName->getValue()) + || $classMetadata->hasAssociation($fieldName->getValue()) + ) { + continue; } + + $messages[] = RuleErrorBuilder::message(sprintf( + 'Call to method %s::%s() - entity %s does not have a field named $%s.', + $calledOnType->describe(VerbosityLevel::typeOnly()), + $methodName, + $entityClassNames[0], + $fieldName->getValue(), + ))->identifier(sprintf('doctrine.%sArgument', $methodName))->build(); } } } diff --git a/tests/DoctrineIntegration/ORM/data/entityRepositoryDynamicReturn-0.json b/tests/DoctrineIntegration/ORM/data/entityRepositoryDynamicReturn-0.json index 36e04f9a..501f5273 100644 --- a/tests/DoctrineIntegration/ORM/data/entityRepositoryDynamicReturn-0.json +++ b/tests/DoctrineIntegration/ORM/data/entityRepositoryDynamicReturn-0.json @@ -4,6 +4,16 @@ "line": 94, "ignorable": true }, + { + "message": "Call to method Doctrine\\ORM\\EntityRepository::findOneBy() - entity PHPStan\\DoctrineIntegration\\ORM\\EntityRepositoryDynamicReturn\\MyEntity does not have a field named $blah.", + "line": 94, + "ignorable": true + }, + { + "message": "Call to method Doctrine\\ORM\\EntityRepository::findBy() - entity PHPStan\\DoctrineIntegration\\ORM\\EntityRepositoryDynamicReturn\\MyEntity does not have a field named $blah.", + "line": 116, + "ignorable": true + }, { "message": "Call to method Doctrine\\ORM\\EntityRepository::findBy() - entity PHPStan\\DoctrineIntegration\\ORM\\EntityRepositoryDynamicReturn\\MyEntity does not have a field named $blah.", "line": 116, diff --git a/tests/Rules/Doctrine/ORM/RepositoryMethodCallRuleTest.php b/tests/Rules/Doctrine/ORM/RepositoryMethodCallRuleTest.php index 4204e35f..cf60ea16 100644 --- a/tests/Rules/Doctrine/ORM/RepositoryMethodCallRuleTest.php +++ b/tests/Rules/Doctrine/ORM/RepositoryMethodCallRuleTest.php @@ -14,7 +14,7 @@ class RepositoryMethodCallRuleTest extends RuleTestCase protected function getRule(): Rule { - return new RepositoryMethodCallRule(new ObjectMetadataResolver(__DIR__ . '/entity-manager.php', __DIR__ . '/../../../../tmp')); + return new RepositoryMethodCallRule(new ObjectMetadataResolver(__DIR__ . '/entity-manager.php', __DIR__ . '/../../../../tmp'), true); } /** diff --git a/tests/Rules/Doctrine/ORM/RepositoryMethodCallRuleWithoutObjectManagerLoaderTest.php b/tests/Rules/Doctrine/ORM/RepositoryMethodCallRuleWithoutObjectManagerLoaderTest.php index 0cc4f38b..717188a8 100644 --- a/tests/Rules/Doctrine/ORM/RepositoryMethodCallRuleWithoutObjectManagerLoaderTest.php +++ b/tests/Rules/Doctrine/ORM/RepositoryMethodCallRuleWithoutObjectManagerLoaderTest.php @@ -14,7 +14,7 @@ class RepositoryMethodCallRuleWithoutObjectManagerLoaderTest extends RuleTestCas protected function getRule(): Rule { - return new RepositoryMethodCallRule(new ObjectMetadataResolver(null, __DIR__ . '/../../../../tmp')); + return new RepositoryMethodCallRule(new ObjectMetadataResolver(null, __DIR__ . '/../../../../tmp'), true); } /**