[DoctrineBridge] DoctrineType now respects the "query_builder" option…

… when caching the choice loader
symfony · webmozart · Apr 1, 2015 · Sep 26, 2014 · Mar 25, 2015 · Mar 25, 2015
commit e6739bf05e07ebd5d0ba22f76bb7247af69d62de
diff --git a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/ORMQueryBuilderLoader.php b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/ORMQueryBuilderLoader.php
@@ -51,6 +51,8 @@ public function __construct($queryBuilder, $manager = null, $class = null)
             throw new UnexpectedTypeException($queryBuilder, 'Doctrine\ORM\QueryBuilder or \Closure');
         }
 
+        // This block is not executed anymore since Symfony 2.7. The query
+        // builder closure is already invoked in DoctrineType
         if ($queryBuilder instanceof \Closure) {
             if (!$manager instanceof EntityManager) {
                 throw new UnexpectedTypeException($manager, 'Doctrine\ORM\EntityManager');

diff --git a/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php b/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php
@@ -13,6 +13,7 @@
 
 use Doctrine\Common\Persistence\ManagerRegistry;
 use Doctrine\Common\Persistence\ObjectManager;
+use Doctrine\ORM\QueryBuilder;
 use Symfony\Bridge\Doctrine\Form\ChoiceList\DoctrineChoiceLoader;
 use Symfony\Bridge\Doctrine\Form\ChoiceList\EntityLoaderInterface;
 use Symfony\Bridge\Doctrine\Form\DataTransformer\CollectionToArrayTransformer;
@@ -23,6 +24,7 @@
 use Symfony\Component\Form\ChoiceList\Factory\DefaultChoiceListFactory;
 use Symfony\Component\Form\ChoiceList\Factory\PropertyAccessDecorator;
 use Symfony\Component\Form\Exception\RuntimeException;
+use Symfony\Component\Form\Exception\UnexpectedTypeException;
 use Symfony\Component\Form\FormBuilderInterface;
 use Symfony\Component\OptionsResolver\Options;
 use Symfony\Component\OptionsResolver\OptionsResolver;
@@ -71,20 +73,24 @@ public function configureOptions(OptionsResolver $resolver)
         $choiceLoader = function (Options $options) use ($choiceListFactory, &$choiceLoaders, $type) {
             // Unless the choices are given explicitly, load them on demand
             if (null === $options['choices']) {
-                // Don't cache if the query builder is constructed dynamically
-                if ($options['query_builder'] instanceof \Closure) {
-                    $hash = null;
-                } else {
-                    $hash = CachingFactoryDecorator::generateHash(array(
-                        $options['em'],
-                        $options['class'],
-                        $options['query_builder'],
-                        $options['loader'],
-                    ));
-
-                    if (isset($choiceLoaders[$hash])) {
-                        return $choiceLoaders[$hash];
-                    }
+                // We consider two query builders with an equal SQL string and
+                // equal parameters to be equal
+                $qbParts = $options['query_builder']
+                    ? array(
+                        $options['query_builder']->getQuery()->getSQL(),
+                        $options['query_builder']->getParameters()->toArray(),
+                    )
+                    : null;
+
+                $hash = CachingFactoryDecorator::generateHash(array(
+                    $options['em'],
+                    $options['class'],
+                    $qbParts,
+                    $options['loader'],
+                ));
+
+                if (isset($choiceLoaders[$hash])) {
+                    return $choiceLoaders[$hash];
                 }
 
                 if ($options['loader']) {
@@ -96,18 +102,14 @@ public function configureOptions(OptionsResolver $resolver)
                     $entityLoader = $type->getLoader($options['em'], $queryBuilder, $options['class']);
                 }
 
-               $choiceLoader = new DoctrineChoiceLoader(
+                $choiceLoaders[$hash] = new DoctrineChoiceLoader(
                     $choiceListFactory,
                     $options['em'],
                     $options['class'],
                     $entityLoader
                 );
 
-                if (null !== $hash) {
-                    $choiceLoaders[$hash] = $choiceLoader;
-                }
-
-                return $choiceLoader;
+                return $choiceLoaders[$hash];
             }
         };
 
@@ -199,6 +201,20 @@ public function configureOptions(OptionsResolver $resolver)
             return $entitiesById;
         };
 
+        // Invoke the query builder closure so that we can cache choice lists
+        // for equal query builders
+        $queryBuilderNormalizer = function (Options $options, $queryBuilder) {
+            if (is_callable($queryBuilder)) {
+                $queryBuilder = call_user_func($queryBuilder, $options['em']->getRepository($options['class']));
+
+                if (!$queryBuilder instanceof QueryBuilder) {
+                    throw new UnexpectedTypeException($queryBuilder, 'Doctrine\ORM\QueryBuilder');
+                }
+            }
+
+            return $queryBuilder;
+        };
+
         $resolver->setDefaults(array(
             'em' => null,
             'property' => null, // deprecated, use "choice_label"
@@ -216,9 +232,11 @@ public function configureOptions(OptionsResolver $resolver)
 
         $resolver->setNormalizer('em', $emNormalizer);
         $resolver->setNormalizer('choices', $choicesNormalizer);
+        $resolver->setNormalizer('query_builder', $queryBuilderNormalizer);
 
         $resolver->setAllowedTypes('em', array('null', 'string', 'Doctrine\Common\Persistence\ObjectManager'));
         $resolver->setAllowedTypes('loader', array('null', 'Symfony\Bridge\Doctrine\Form\ChoiceList\EntityLoaderInterface'));
+        $resolver->setAllowedTypes('query_builder', array('null', 'callable', 'Doctrine\ORM\QueryBuilder'));
     }
 
     /**

diff --git a/src/Symfony/Bridge/Doctrine/Form/Type/EntityType.php b/src/Symfony/Bridge/Doctrine/Form/Type/EntityType.php
@@ -28,11 +28,7 @@ class EntityType extends DoctrineType
      */
     public function getLoader(ObjectManager $manager, $queryBuilder, $class)
     {
-        return new ORMQueryBuilderLoader(
-            $queryBuilder,
-            $manager,
-            $class
-        );
+        return new ORMQueryBuilderLoader($queryBuilder, $manager, $class);
     }
 
     public function getName()

diff --git a/src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php b/src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php
@@ -14,6 +14,7 @@
 use Doctrine\Common\Collections\ArrayCollection;
 use Doctrine\Common\Persistence\ManagerRegistry;
 use Doctrine\ORM\EntityManager;
+use Doctrine\ORM\EntityRepository;
 use Doctrine\ORM\Tools\SchemaTool;
 use Symfony\Bridge\Doctrine\Form\DoctrineOrmExtension;
 use Symfony\Bridge\Doctrine\Form\DoctrineOrmTypeGuesser;
@@ -28,6 +29,7 @@
 use Symfony\Component\Form\ChoiceList\View\ChoiceView;
 use Symfony\Component\Form\Forms;
 use Symfony\Component\Form\Test\TypeTestCase;
+use Symfony\Component\OptionsResolver\Options;
 use Symfony\Component\PropertyAccess\PropertyAccess;
 
 class EntityTypeTest extends TypeTestCase
@@ -172,7 +174,7 @@ public function testSetDataToUninitializedEntityWithNonRequiredQueryBuilder()
     }
 
     /**
-     * @expectedException \Symfony\Component\Form\Exception\UnexpectedTypeException
+     * @expectedException \Symfony\Component\OptionsResolver\Exception\InvalidOptionsException
      */
     public function testConfigureQueryBuilderWithNonQueryBuilderAndNonClosure()
     {
@@ -786,8 +788,7 @@ public function testLoaderCaching()
 
         $this->persist(array($entity1, $entity2, $entity3));
 
-        $repository = $this->em->getRepository(self::SINGLE_IDENT_CLASS);
-        $qb = $repository->createQueryBuilder('e')->where('e.id IN (1, 2)');
+        $repo = $this->em->getRepository(self::SINGLE_IDENT_CLASS);
 
         $entityType = new EntityType(
             $this->emRegistry,
@@ -806,19 +807,23 @@ public function testLoaderCaching()
         $formBuilder->add('property1', 'entity', array(
             'em' => 'default',
             'class' => self::SINGLE_IDENT_CLASS,
-            'query_builder' => $qb,
+            'query_builder' => $repo->createQueryBuilder('e')->where('e.id IN (1, 2)'),
         ));
 
         $formBuilder->add('property2', 'entity', array(
             'em' => 'default',
             'class' => self::SINGLE_IDENT_CLASS,
-            'query_builder' => $qb,
+            'query_builder' => function (EntityRepository $repo) {
+                return $repo->createQueryBuilder('e')->where('e.id IN (1, 2)');
+            },
         ));
 
         $formBuilder->add('property3', 'entity', array(
             'em' => 'default',
             'class' => self::SINGLE_IDENT_CLASS,
-            'query_builder' => $qb,
+            'query_builder' => function (EntityRepository $repo) {
+                return $repo->createQueryBuilder('e')->where('e.id IN (1, 2)');
+            },
         ));
 
         $form = $formBuilder->getForm();
@@ -829,15 +834,13 @@ public function testLoaderCaching()
             'property3' => 2,
         ));
 
-        $reflectionClass = new \ReflectionObject($entityType);
-        $reflectionProperty = $reflectionClass->getProperty('loaderCache');
-        $reflectionProperty->setAccessible(true);
-
-        $loaders = $reflectionProperty->getValue($entityType);
-
-        $reflectionProperty->setAccessible(false);
+        $choiceList1 = $form->get('property1')->getConfig()->getOption('choice_list');
+        $choiceList2 = $form->get('property2')->getConfig()->getOption('choice_list');
+        $choiceList3 = $form->get('property3')->getConfig()->getOption('choice_list');
 
-        $this->assertCount(1, $loaders);
+        $this->assertInstanceOf('Symfony\Component\Form\ChoiceList\ChoiceListInterface', $choiceList1);
+        $this->assertSame($choiceList1, $choiceList2);
+        $this->assertSame($choiceList1, $choiceList3);
     }
 
     public function testCacheChoiceLists()