[doctrine] improve repository handler (#353)

psalm · Jul 3, 2024 · fb801a9 · fb801a9
1 parent 353c9b0
commit fb801a9
Show file tree

Hide file tree

Showing 6 changed files with 133 additions and 48 deletions.
diff --git a/README.md b/README.md
@@ -23,9 +23,10 @@ vendor/bin/psalm-plugin enable psalm/plugin-symfony
 ### Features
 
 - Detects the `ContainerInterface::get()` result type. Works better if you [configure](#configuration) a compiled container XML file.
+- Detects parameter return types from `ContainerInterface::getParameter()`.
 - Supports [Service Subscribers](https://github.com/psalm/psalm-plugin-symfony/issues/20). Works only if you [configure](#configuration) a compiled container XML file.
 - Detects return types from console arguments (`InputInterface::getArgument()`) and options (`InputInterface::getOption()`).
-Enforces to use "InputArgument" and "InputOption" constants as a best practise.
+Enforces to use "InputArgument" and "InputOption" constants as a best practice.
 - Detects Doctrine repository classes associated to entities when configured via annotations.
 - Fixes `PossiblyInvalidArgument` for `Symfony\Component\HttpFoundation\Request::getContent()`.
 The plugin determines the real return type by checking the given argument and marks it as either "string" or "resource".

diff --git a/src/Handler/DoctrineRepositoryHandler.php b/src/Handler/DoctrineRepositoryHandler.php
@@ -22,69 +22,73 @@ class DoctrineRepositoryHandler implements AfterMethodCallAnalysisInterface, Aft
 {
     public static function afterMethodCallAnalysis(AfterMethodCallAnalysisEvent $event): void
     {
-        $expr = $event->getExpr();
         $declaring_method_id = $event->getDeclaringMethodId();
-        $statements_source = $event->getStatementsSource();
+        if (!in_array($declaring_method_id, ['Doctrine\ORM\EntityManagerInterface::getrepository', 'Doctrine\Persistence\ObjectManager::getrepository'])) {
+            return;
+        }
 
-        if (in_array($declaring_method_id, ['Doctrine\ORM\EntityManagerInterface::getrepository', 'Doctrine\Persistence\ObjectManager::getrepository'])) {
-            if (!isset($expr->args[0]->value)) {
-                return;
-            }
+        $expr = $event->getExpr();
+        if (!isset($expr->args[0]->value)) {
+            return;
+        }
 
-            $entityName = $expr->args[0]->value;
+        $entityName = $expr->args[0]->value;
+        if (!$entityName instanceof Expr\ClassConstFetch) {
             if ($entityName instanceof String_) {
+                $statements_source = $event->getStatementsSource();
                 IssueBuffer::accepts(
                     new RepositoryStringShortcut(new CodeLocation($statements_source, $entityName)),
                     $statements_source->getSuppressedIssues()
                 );
-            } elseif ($entityName instanceof Expr\ClassConstFetch) {
-                /** @psalm-var class-string|null $className */
-                $className = $entityName->class->getAttribute('resolvedName');
+            }
 
-                if (null === $className) {
-                    return;
-                }
+            return;
+        }
 
-                try {
-                    $reflectionClass = new \ReflectionClass($className);
-
-                    if (\PHP_VERSION_ID >= 80000 && method_exists(\ReflectionClass::class, 'getAttributes')) {
-                        $entityAttributes = $reflectionClass->getAttributes(EntityAnnotation::class);
-
-                        foreach ($entityAttributes as $entityAttribute) {
-                            $arguments = $entityAttribute->getArguments();
-
-                            if (isset($arguments['repositoryClass']) && is_string($arguments['repositoryClass'])) {
-                                $event->setReturnTypeCandidate(new Union([new TNamedObject($arguments['repositoryClass'])]));
-                            }
-                        }
-                    }
-
-                    if (class_exists(AnnotationReader::class)) {
-                        $reader = new AnnotationReader();
-                        $entityAnnotation = $reader->getClassAnnotation(
-                            $reflectionClass,
-                            EntityAnnotation::class
-                        );
-
-                        if ($entityAnnotation instanceof EntityAnnotation && $entityAnnotation->repositoryClass) {
-                            $event->setReturnTypeCandidate(new Union([new TNamedObject($entityAnnotation->repositoryClass)]));
-                        }
-                    }
-                } catch (\ReflectionException $e) {
-                }
+        /** @psalm-var class-string|null $className */
+        $className = $entityName->class->getAttribute('resolvedName');
+        if (null === $className) {
+            return;
+        }
+
+        try {
+            $reflectionClass = new \ReflectionClass($className);
+        } catch (\ReflectionException) {
+            return;
+        }
+
+        $entityAttributes = $reflectionClass->getAttributes(EntityAnnotation::class);
+        foreach ($entityAttributes as $entityAttribute) {
+            $arguments = $entityAttribute->getArguments();
+
+            if (isset($arguments['repositoryClass']) && is_string($arguments['repositoryClass'])) {
+                $event->setReturnTypeCandidate(new Union([new TNamedObject($arguments['repositoryClass'])]));
+
+                return;
+            }
+        }
+
+        if (class_exists(AnnotationReader::class)) {
+            $reader = new AnnotationReader();
+            $entityAnnotation = $reader->getClassAnnotation(
+                $reflectionClass,
+                EntityAnnotation::class
+            );
+
+            if ($entityAnnotation instanceof EntityAnnotation && $entityAnnotation->repositoryClass) {
+                $event->setReturnTypeCandidate(new Union([new TNamedObject($entityAnnotation->repositoryClass)]));
             }
         }
     }
 
-    public static function afterClassLikeVisit(AfterClassLikeVisitEvent $event)
+    public static function afterClassLikeVisit(AfterClassLikeVisitEvent $event): void
     {
         $stmt = $event->getStmt();
         $statements_source = $event->getStatementsSource();
         $codebase = $event->getCodebase();
 
         $docblock = $stmt->getDocComment();
-        if ($docblock && false !== strpos((string) $docblock, 'repositoryClass')) {
+        if ($docblock && str_contains((string) $docblock, 'repositoryClass')) {
             try {
                 $parsedComment = DocComment::parsePreservingLength($docblock);
                 if (isset($parsedComment->tags['Entity'])) {
@@ -96,7 +100,7 @@ public static function afterClassLikeVisit(AfterClassLikeVisitEvent $event)
                     $codebase->queueClassLikeForScanning($repositoryClassName);
                     $file_storage->referenced_classlikes[strtolower($repositoryClassName)] = $repositoryClassName;
                 }
-            } catch (DocblockParseException $e) {
+            } catch (DocblockParseException) {
             }
         }
     }

diff --git a/src/Plugin.php b/src/Plugin.php
@@ -36,7 +36,6 @@ public function __invoke(RegistrationInterface $registration, ?\SimpleXMLElement
         require_once __DIR__.'/Handler/ConsoleHandler.php';
         require_once __DIR__.'/Handler/ContainerDependencyHandler.php';
         require_once __DIR__.'/Handler/RequiredSetterHandler.php';
-        require_once __DIR__.'/Handler/DoctrineQueryBuilderHandler.php';
         require_once __DIR__.'/Provider/FormGetErrorsReturnTypeProvider.php';
 
         $registration->registerHooksFromClass(HeaderBagHandler::class);
@@ -45,16 +44,18 @@ public function __invoke(RegistrationInterface $registration, ?\SimpleXMLElement
         $registration->registerHooksFromClass(RequiredSetterHandler::class);
 
         if (class_exists(\Doctrine\ORM\QueryBuilder::class)) {
+            require_once __DIR__.'/Handler/DoctrineQueryBuilderHandler.php';
             $registration->registerHooksFromClass(DoctrineQueryBuilderHandler::class);
+
+            require_once __DIR__.'/Handler/DoctrineRepositoryHandler.php';
+            $registration->registerHooksFromClass(DoctrineRepositoryHandler::class);
         }
 
         if (class_exists(AnnotationRegistry::class)) {
-            require_once __DIR__.'/Handler/DoctrineRepositoryHandler.php';
             if (method_exists(AnnotationRegistry::class, 'registerLoader')) {
                 /** @psalm-suppress DeprecatedMethod */
                 AnnotationRegistry::registerLoader('class_exists');
             }
-            $registration->registerHooksFromClass(DoctrineRepositoryHandler::class);
 
             require_once __DIR__.'/Handler/AnnotationHandler.php';
             $registration->registerHooksFromClass(AnnotationHandler::class);

diff --git a/tests/acceptance/acceptance/doctrine/RepositoryClassWithAttributes.feature b/tests/acceptance/acceptance/doctrine/RepositoryClassWithAttributes.feature
@@ -0,0 +1,53 @@
+@symfony-common
+Feature: RepositoryClass using attributes
+
+  Background:
+    Given I have issue handlers "UndefinedClass,UnusedVariable" suppressed
+    And I have Symfony plugin enabled
+    And I have the following code preamble
+      """
+      <?php
+      namespace RepositoryClass;
+
+      use Psalm\SymfonyPsalmPlugin\Tests\Fixture\Doctrine\EntityWithAttributes;
+      use Psalm\SymfonyPsalmPlugin\Tests\Fixture\Doctrine\EntityWithAttributesRepository;
+      use Doctrine\ORM\EntityManagerInterface;
+      """
+
+  Scenario: The plugin can find correct repository class from entity
+    Given I have the following code
+      """
+      class SomeService
+      {
+        public function __construct(EntityManagerInterface $entityManager)
+        {
+          /** @psalm-trace $repository */
+          $repository = $entityManager->getRepository(EntityWithAttributes::class);
+        }
+      }
+      """
+    When I run Psalm
+    Then I see these errors
+      | Type   | Message                                                                                      |
+      | Trace  | $repository: Psalm\SymfonyPsalmPlugin\Tests\Fixture\Doctrine\EntityWithAttributesRepository  |
+    And I see no other errors
+
+  Scenario: Passing variable class does not crash the plugin
+    Given I have the following code
+      """
+      class SomeService
+      {
+        public function __construct(EntityManagerInterface $entityManager)
+        {
+          $entity = 'Psalm\SymfonyPsalmPlugin\Tests\Fixture\Doctrine\EntityWithAttributes';
+          /** @psalm-trace $repository */
+          $repository = $entityManager->getRepository($entity::class);
+        }
+      }
+      """
+    When I run Psalm
+    Then I see these errors
+      | Type          | Message                                                                                                  |
+      | Trace         | $repository: Doctrine\ORM\EntityRepository<object>                                                       |
+      | MixedArgument | Argument 1 of Doctrine\ORM\EntityManagerInterface::getRepository cannot be mixed, expecting class-string |
+    And I see no other errors
diff --git a/tests/fixture/Doctrine/EntityWithAttributes.php b/tests/fixture/Doctrine/EntityWithAttributes.php
@@ -0,0 +1,12 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Psalm\SymfonyPsalmPlugin\Tests\Fixture\Doctrine;
+
+use Doctrine\ORM\Mapping\Entity;
+
+#[Entity(repositoryClass: EntityWithAttributesRepository::class)]
+class EntityWithAttributes
+{
+}
diff --git a/tests/fixture/Doctrine/EntityWithAttributesRepository.php b/tests/fixture/Doctrine/EntityWithAttributesRepository.php
@@ -0,0 +1,14 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Psalm\SymfonyPsalmPlugin\Tests\Fixture\Doctrine;
+
+use Doctrine\ORM\EntityRepository;
+
+/**
+ * @extends EntityRepository<EntityWithAttributes>
+ */
+class EntityWithAttributesRepository extends EntityRepository
+{
+}