From e6739bf05e07ebd5d0ba22f76bb7247af69d62de Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Wed, 25 Mar 2015 14:31:10 +0100 Subject: [PATCH] [DoctrineBridge] DoctrineType now respects the "query_builder" option when caching the choice loader --- .../Form/ChoiceList/ORMQueryBuilderLoader.php | 2 + .../Doctrine/Form/Type/DoctrineType.php | 56 ++++++++++++------- .../Bridge/Doctrine/Form/Type/EntityType.php | 6 +- .../Tests/Form/Type/EntityTypeTest.php | 31 +++++----- 4 files changed, 57 insertions(+), 38 deletions(-) diff --git a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/ORMQueryBuilderLoader.php b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/ORMQueryBuilderLoader.php index 9cfdd1fe48..9d34601c9f 100644 --- a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/ORMQueryBuilderLoader.php +++ b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/ORMQueryBuilderLoader.php @@ -51,6 +51,8 @@ class ORMQueryBuilderLoader implements EntityLoaderInterface 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 index 76e78cb1f0..ed8ded5bad 100644 --- a/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php +++ b/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php @@ -13,6 +13,7 @@ namespace Symfony\Bridge\Doctrine\Form\Type; 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\ChoiceListFactoryInterface; 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 @@ abstract class DoctrineType extends AbstractType $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'], - )); + // 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; - if (isset($choiceLoaders[$hash])) { - return $choiceLoaders[$hash]; - } + $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 @@ abstract class DoctrineType extends AbstractType $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 @@ abstract class DoctrineType extends AbstractType 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 @@ abstract class DoctrineType extends AbstractType $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 index 675c289c76..236b9290c7 100644 --- 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 index 9f1591f308..7d80819f6b 100644 --- a/src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php +++ b/src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php @@ -14,6 +14,7 @@ namespace Symfony\Bridge\Doctrine\Tests\Form\Type; 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\ChoiceGroupView; 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 @@ class EntityTypeTest extends TypeTestCase } /** - * @expectedException \Symfony\Component\Form\Exception\UnexpectedTypeException + * @expectedException \Symfony\Component\OptionsResolver\Exception\InvalidOptionsException */ public function testConfigureQueryBuilderWithNonQueryBuilderAndNonClosure() { @@ -786,8 +788,7 @@ class EntityTypeTest extends TypeTestCase $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 @@ class EntityTypeTest extends TypeTestCase $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 @@ class EntityTypeTest extends TypeTestCase 'property3' => 2, )); - $reflectionClass = new \ReflectionObject($entityType); - $reflectionProperty = $reflectionClass->getProperty('loaderCache'); - $reflectionProperty->setAccessible(true); + $choiceList1 = $form->get('property1')->getConfig()->getOption('choice_list'); + $choiceList2 = $form->get('property2')->getConfig()->getOption('choice_list'); + $choiceList3 = $form->get('property3')->getConfig()->getOption('choice_list'); - $loaders = $reflectionProperty->getValue($entityType); - - $reflectionProperty->setAccessible(false); - - $this->assertCount(1, $loaders); + $this->assertInstanceOf('Symfony\Component\Form\ChoiceList\ChoiceListInterface', $choiceList1); + $this->assertSame($choiceList1, $choiceList2); + $this->assertSame($choiceList1, $choiceList3); } public function testCacheChoiceLists()