diff --git a/README.md b/README.md index 737b35f5f..3ea70e14e 100644 --- a/README.md +++ b/README.md @@ -1583,13 +1583,47 @@ final class Product {} ### RequireQueryBuilderOnRepositoryRule -Prevents using `$entityManager->createQueryBuilder('...')`, use `$repository->createQueryBuilder()` as safer. +Prevents using `$entityManager->createQueryBuilder()` inside a repository class, use `$repository->createQueryBuilder()` as safer. + +Builders that the repository shortcut cannot express are skipped: `update()`/`delete()` builders and `from()` on another entity than the repository's own one (e.g. cross-entity subqueries). ```yaml rules: - Symplify\PHPStanRules\Rules\Doctrine\RequireQueryBuilderOnRepositoryRule ``` +```php +final class SomeRepository extends EntityRepository +{ + public function getSome(): array + { + return $this->getEntityManager()->createQueryBuilder() + ->select('s') + ->from(SomeEntity::class, 's') + ->getQuery() + ->getResult(); + } +} +``` + +:x: + +
+ +```php +final class SomeRepository extends EntityRepository +{ + public function getSome(): array + { + return $this->createQueryBuilder('s') + ->getQuery() + ->getResult(); + } +} +``` + +:+1: +
### NoGetRepositoryOutsideServiceRule diff --git a/src/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule.php b/src/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule.php index 6cd44c45d..fb19e3db8 100644 --- a/src/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule.php +++ b/src/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule.php @@ -5,8 +5,16 @@ namespace Symplify\PHPStanRules\Rules\Doctrine; use PhpParser\Node; +use PhpParser\Node\Expr\Assign; +use PhpParser\Node\Expr\ClassConstFetch; use PhpParser\Node\Expr\MethodCall; +use PhpParser\Node\Expr\Variable; +use PhpParser\Node\Identifier; +use PhpParser\Node\Name; +use PhpParser\Node\Stmt\ClassMethod; +use PhpParser\NodeFinder; use PHPStan\Analyser\Scope; +use PHPStan\Node\InClassMethodNode; use PHPStan\Reflection\ClassReflection; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; @@ -18,24 +26,38 @@ use Symplify\PHPStanRules\Helper\NamingHelper; /** - * @implements Rule + * @implements Rule * @see \Symplify\PHPStanRules\Tests\Rules\Doctrine\RequireQueryBuilderOnRepositoryRule\RequireQueryBuilderOnRepositoryRuleTest */ -final class RequireQueryBuilderOnRepositoryRule implements Rule +final readonly class RequireQueryBuilderOnRepositoryRule implements Rule { - public const string ERROR_MESSAGE = 'Avoid calling ->createQueryBuilder() directly on EntityManager as it requires select() + from() calls with specific values. Use $repository->createQueryBuilder() to be safe instead'; + public const string ERROR_MESSAGE = 'Avoid calling ->createQueryBuilder() directly on EntityManager inside a repository class, as it requires select() + from() calls with specific values. Use $repository->createQueryBuilder() to be safe instead'; + + /** + * UPDATE/DELETE builders are not plain SELECTs, the repository shortcut cannot express them + * @var string[] + */ + private const array NON_SELECT_BUILDER_METHODS = ['update', 'delete']; + + private NodeFinder $nodeFinder; + + public function __construct() + { + $this->nodeFinder = new NodeFinder(); + } public function getNodeType(): string { - return MethodCall::class; + return InClassMethodNode::class; } /** - * @param MethodCall $node + * @param InClassMethodNode $node */ public function processNode(Node $node, Scope $scope): array { - if (! NamingHelper::isName($node->name, 'createQueryBuilder')) { + $classMethod = $node->getOriginalNode(); + if ($classMethod->stmts === null) { return []; } @@ -44,16 +66,34 @@ public function processNode(Node $node, Scope $scope): array return []; } - $callerType = $scope->getType($node->var); - if ($this->isValidRepositoryObjectType($callerType)) { - return []; - } + /** @var MethodCall[] $methodCalls */ + $methodCalls = $this->nodeFinder->findInstanceOf($classMethod->stmts, MethodCall::class); + + $repositoryEntityClassNames = $this->resolveRepositoryEntityClassNames($scope); - $identifierRuleError = RuleErrorBuilder::message(self::ERROR_MESSAGE) - ->identifier(DoctrineRuleIdentifier::REQUIRE_QUERY_BUILDER_ON_REPOSITORY) - ->build(); + $ruleErrors = []; + foreach ($methodCalls as $methodCall) { + if (! NamingHelper::isName($methodCall->name, 'createQueryBuilder')) { + continue; + } + + $callerType = $scope->getType($methodCall->var); + if ($this->isValidRepositoryObjectType($callerType)) { + continue; + } - return [$identifierRuleError]; + // the query builder cannot be swapped for $this->createQueryBuilder() + if ($this->isUnconvertibleBuilder($methodCall, $methodCalls, $classMethod, $repositoryEntityClassNames, $scope)) { + continue; + } + + $ruleErrors[] = RuleErrorBuilder::message(self::ERROR_MESSAGE) + ->identifier(DoctrineRuleIdentifier::REQUIRE_QUERY_BUILDER_ON_REPOSITORY) + ->line($methodCall->getStartLine()) + ->build(); + } + + return $ruleErrors; } private function isInsideRepositoryClass(Scope $scope): bool @@ -95,4 +135,177 @@ private function isValidRepositoryObjectType(Type $type): bool return $type->isInstanceOf(DoctrineClass::CONNECTION)->yes(); } + + /** + * @param MethodCall[] $allMethodCalls + * @param string[] $repositoryEntityClassNames + */ + private function isUnconvertibleBuilder( + MethodCall $methodCall, + array $allMethodCalls, + ClassMethod $classMethod, + array $repositoryEntityClassNames, + Scope $scope + ): bool { + $builderMethodCalls = $this->collectBuilderMethodCalls($methodCall, $allMethodCalls, $classMethod); + + foreach ($builderMethodCalls as $builderMethodCall) { + if (! $builderMethodCall->name instanceof Identifier) { + continue; + } + + $calledMethodName = $builderMethodCall->name->toString(); + + if (in_array($calledMethodName, self::NON_SELECT_BUILDER_METHODS, true)) { + return true; + } + + // from() on another entity than the repository's own one cannot use the repository shortcut + if ($calledMethodName === 'from' && ! $this->isFromOnRepositoryEntity( + $builderMethodCall, + $repositoryEntityClassNames, + $scope + )) { + return true; + } + } + + return false; + } + + /** + * Collects every method call made on the query builder that this createQueryBuilder() call produces, + * both as a fluent chain and via an intermediate variable. + * + * @param MethodCall[] $allMethodCalls + * @return MethodCall[] + */ + private function collectBuilderMethodCalls( + MethodCall $createQueryBuilderCall, + array $allMethodCalls, + ClassMethod $classMethod + ): array { + $builderMethodCalls = []; + + foreach ($allMethodCalls as $methodCall) { + if ($methodCall !== $createQueryBuilderCall && $this->chainPassesThrough($methodCall, $createQueryBuilderCall)) { + $builderMethodCalls[] = $methodCall; + } + } + + $assignedVariableName = $this->resolveAssignedVariableName($createQueryBuilderCall, $classMethod); + if ($assignedVariableName !== null) { + foreach ($allMethodCalls as $allMethodCall) { + if ($this->resolveRootVariableName($allMethodCall) === $assignedVariableName) { + $builderMethodCalls[] = $allMethodCall; + } + } + } + + return $builderMethodCalls; + } + + private function chainPassesThrough(MethodCall $methodCall, MethodCall $targetMethodCall): bool + { + $current = $methodCall->var; + while ($current instanceof MethodCall) { + if ($current === $targetMethodCall) { + return true; + } + + $current = $current->var; + } + + return false; + } + + private function resolveRootVariableName(MethodCall $methodCall): ?string + { + $current = $methodCall->var; + while ($current instanceof MethodCall) { + $current = $current->var; + } + + if ($current instanceof Variable && is_string($current->name)) { + return $current->name; + } + + return null; + } + + private function resolveAssignedVariableName(MethodCall $methodCall, ClassMethod $classMethod): ?string + { + /** @var Assign[] $assigns */ + $assigns = $this->nodeFinder->findInstanceOf((array) $classMethod->stmts, Assign::class); + + foreach ($assigns as $assign) { + $isBuilderAssign = $assign->expr === $methodCall + || ($assign->expr instanceof MethodCall && $this->chainPassesThrough($assign->expr, $methodCall)); + + if (! $isBuilderAssign) { + continue; + } + + if ($assign->var instanceof Variable && is_string($assign->var->name)) { + return $assign->var->name; + } + } + + return null; + } + + /** + * @param string[] $repositoryEntityClassNames + */ + private function isFromOnRepositoryEntity(MethodCall $fromMethodCall, array $repositoryEntityClassNames, Scope $scope): bool + { + $args = $fromMethodCall->getArgs(); + if (! isset($args[0])) { + return false; + } + + $firstArg = $args[0]->value; + if (! $firstArg instanceof ClassConstFetch) { + return false; + } + + if (! $firstArg->class instanceof Name) { + return false; + } + + if (! $firstArg->name instanceof Identifier || $firstArg->name->toString() !== 'class') { + return false; + } + + $fromClassName = $scope->resolveName($firstArg->class); + + return in_array($fromClassName, $repositoryEntityClassNames, true); + } + + /** + * @return string[] + */ + private function resolveRepositoryEntityClassNames(Scope $scope): array + { + $classReflection = $scope->getClassReflection(); + if (! $classReflection instanceof ClassReflection) { + return []; + } + + $entityClassNames = []; + foreach ([DoctrineClass::ENTITY_REPOSITORY, DoctrineClass::DOCUMENT_REPOSITORY] as $repositoryBaseClass) { + $ancestorClassReflection = $classReflection->getAncestorWithClassName($repositoryBaseClass); + if (! $ancestorClassReflection instanceof ClassReflection) { + continue; + } + + foreach ($ancestorClassReflection->getActiveTemplateTypeMap()->getTypes() as $templateType) { + foreach ($templateType->getObjectClassNames() as $objectClassName) { + $entityClassNames[] = $objectClassName; + } + } + } + + return array_unique($entityClassNames); + } } diff --git a/stubs/Doctrine/ORM/EntityManagerInterface.php b/stubs/Doctrine/ORM/EntityManagerInterface.php index c38ecc95a..830ecc5e6 100644 --- a/stubs/Doctrine/ORM/EntityManagerInterface.php +++ b/stubs/Doctrine/ORM/EntityManagerInterface.php @@ -14,5 +14,5 @@ interface EntityManagerInterface */ public function getRepository(string $class): object; - public function createQueryBuilder(); + public function createQueryBuilder(): QueryBuilder; } diff --git a/stubs/Doctrine/ORM/EntityRepository.php b/stubs/Doctrine/ORM/EntityRepository.php index 8e117539a..188678948 100644 --- a/stubs/Doctrine/ORM/EntityRepository.php +++ b/stubs/Doctrine/ORM/EntityRepository.php @@ -6,9 +6,12 @@ return; } +/** + * @template T of object + */ class EntityRepository { - public function createQueryBuilder() + public function createQueryBuilder(string $alias, ?string $indexBy = null): QueryBuilder { } } diff --git a/stubs/Doctrine/ORM/QueryBuilder.php b/stubs/Doctrine/ORM/QueryBuilder.php new file mode 100644 index 000000000..8ca83e948 --- /dev/null +++ b/stubs/Doctrine/ORM/QueryBuilder.php @@ -0,0 +1,38 @@ + + */ +final class MixedBuildersInOneMethod extends EntityRepository +{ + public function run(EntityManagerInterface $entityManager): void + { + $mainQueryBuilder = $entityManager->createQueryBuilder(); + $mainQueryBuilder->select('r') + ->from(RandomEntity::class, 'r'); + + $subQueryBuilder = $entityManager->createQueryBuilder(); + $subQueryBuilder->select('count(a.id)') + ->from(AnotherEntity::class, 'a'); + } +} diff --git a/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/Fixture/ReportOnSelfEntityFrom.php b/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/Fixture/ReportOnSelfEntityFrom.php new file mode 100644 index 000000000..cd65955be --- /dev/null +++ b/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/Fixture/ReportOnSelfEntityFrom.php @@ -0,0 +1,29 @@ + + */ +final class ReportOnSelfEntityFrom extends EntityRepository +{ + public function fluent(EntityManagerInterface $entityManager): void + { + $entityManager->createQueryBuilder() + ->select('r') + ->from(RandomEntity::class, 'r'); + } + + public function assigned(EntityManagerInterface $entityManager): void + { + $queryBuilder = $entityManager->createQueryBuilder(); + $queryBuilder->select('r') + ->from(RandomEntity::class, 'r'); + } +} diff --git a/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/Fixture/SkipCrossEntityFrom.php b/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/Fixture/SkipCrossEntityFrom.php new file mode 100644 index 000000000..520cde801 --- /dev/null +++ b/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/Fixture/SkipCrossEntityFrom.php @@ -0,0 +1,30 @@ + + */ +final class SkipCrossEntityFrom extends EntityRepository +{ + public function fluent(EntityManagerInterface $entityManager): void + { + $entityManager->createQueryBuilder() + ->select('a') + ->from(AnotherEntity::class, 'a'); + } + + public function assigned(EntityManagerInterface $entityManager): void + { + $subQueryBuilder = $entityManager->createQueryBuilder(); + $subQueryBuilder->select('a') + ->from(AnotherEntity::class, 'a'); + } +} diff --git a/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/Fixture/SkipDeleteBuilder.php b/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/Fixture/SkipDeleteBuilder.php new file mode 100644 index 000000000..6ffffc58f --- /dev/null +++ b/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/Fixture/SkipDeleteBuilder.php @@ -0,0 +1,22 @@ + + */ +final class SkipDeleteBuilder extends EntityRepository +{ + public function assigned(EntityManagerInterface $entityManager): void + { + $queryBuilder = $entityManager->createQueryBuilder(); + $queryBuilder->delete(RandomEntity::class, 'r') + ->where('r.id = :id'); + } +} diff --git a/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/Fixture/SkipUpdateBuilder.php b/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/Fixture/SkipUpdateBuilder.php new file mode 100644 index 000000000..5d61825db --- /dev/null +++ b/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/Fixture/SkipUpdateBuilder.php @@ -0,0 +1,28 @@ + + */ +final class SkipUpdateBuilder extends EntityRepository +{ + public function fluent(EntityManagerInterface $entityManager): void + { + $entityManager->createQueryBuilder() + ->update(RandomEntity::class, 'r') + ->where('r.active = :active'); + } + + public function assigned(EntityManagerInterface $entityManager): void + { + $queryBuilder = $entityManager->createQueryBuilder(); + $queryBuilder->update(RandomEntity::class, 'r'); + } +} diff --git a/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/RequireQueryBuilderOnRepositoryRuleTest.php b/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/RequireQueryBuilderOnRepositoryRuleTest.php index ae9c38a79..d412d6ca9 100644 --- a/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/RequireQueryBuilderOnRepositoryRuleTest.php +++ b/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/RequireQueryBuilderOnRepositoryRuleTest.php @@ -31,6 +31,11 @@ public static function provideData(): Iterator yield [__DIR__ . '/Fixture/SkipConnection.php', []]; yield [__DIR__ . '/Fixture/SkipNonRepositoryClass.php', []]; + // builders that cannot be swapped for $this->createQueryBuilder() + yield [__DIR__ . '/Fixture/SkipUpdateBuilder.php', []]; + yield [__DIR__ . '/Fixture/SkipDeleteBuilder.php', []]; + yield [__DIR__ . '/Fixture/SkipCrossEntityFrom.php', []]; + yield [__DIR__ . '/Fixture/ReportOnEntityManager.php', [ [RequireQueryBuilderOnRepositoryRule::ERROR_MESSAGE, 14], ]]; @@ -38,6 +43,15 @@ public static function provideData(): Iterator yield [__DIR__ . '/Fixture/ReportOnDocumentManager.php', [ [RequireQueryBuilderOnRepositoryRule::ERROR_MESSAGE, 14], ]]; + + yield [__DIR__ . '/Fixture/ReportOnSelfEntityFrom.php', [ + [RequireQueryBuilderOnRepositoryRule::ERROR_MESSAGE, 18], + [RequireQueryBuilderOnRepositoryRule::ERROR_MESSAGE, 25], + ]]; + + yield [__DIR__ . '/Fixture/MixedBuildersInOneMethod.php', [ + [RequireQueryBuilderOnRepositoryRule::ERROR_MESSAGE, 19], + ]]; } protected function getRule(): Rule diff --git a/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/Source/AnotherEntity.php b/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/Source/AnotherEntity.php new file mode 100644 index 000000000..1d8aa7735 --- /dev/null +++ b/tests/Rules/Doctrine/RequireQueryBuilderOnRepositoryRule/Source/AnotherEntity.php @@ -0,0 +1,7 @@ +