From 3846b3750ad60e02e55f4dc7b91d06366a1695f8 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Wed, 25 Mar 2015 11:41:36 +0100 Subject: [PATCH] [DoctrineBridge] Fixed: don't cache choice lists if query builders are constructed dynamically --- ...iceLoader.php => DoctrineChoiceLoader.php} | 101 +++++++++--------- .../Form/ChoiceList/EntityChoiceList.php | 2 +- .../Doctrine/Form/Type/DoctrineType.php | 63 ++++++----- .../Bridge/Doctrine/Form/Type/EntityType.php | 65 ++--------- 4 files changed, 97 insertions(+), 134 deletions(-) rename src/Symfony/Bridge/Doctrine/Form/ChoiceList/{EntityChoiceLoader.php => DoctrineChoiceLoader.php} (71%) diff --git a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/EntityChoiceLoader.php b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php similarity index 71% rename from src/Symfony/Bridge/Doctrine/Form/ChoiceList/EntityChoiceLoader.php rename to src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php index d1f7971e63..c00c258ca5 100644 --- a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/EntityChoiceLoader.php +++ b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php @@ -23,7 +23,7 @@ use Symfony\Component\Form\Exception\RuntimeException; * * @author Bernhard Schussek */ -class EntityChoiceLoader implements ChoiceLoaderInterface +class DoctrineChoiceLoader implements ChoiceLoaderInterface { /** * @var ChoiceListFactoryInterface @@ -48,7 +48,7 @@ class EntityChoiceLoader implements ChoiceLoaderInterface /** * @var null|EntityLoaderInterface */ - private $entityLoader; + private $objectLoader; /** * The identifier field, unless the identifier is composite @@ -70,20 +70,20 @@ class EntityChoiceLoader implements ChoiceLoaderInterface private $choiceList; /** - * Returns the value of the identifier field of an entity. + * Returns the value of the identifier field of an object. * - * Doctrine must know about this entity, that is, the entity must already + * Doctrine must know about this object, that is, the object must already * be persisted or added to the identity map before. Otherwise an * exception is thrown. * - * This method assumes that the entity has a single-column identifier and + * This method assumes that the object has a single-column identifier and * will return a single value instead of an array. * - * @param object $object The entity for which to get the identifier + * @param object $object The object for which to get the identifier * * @return int|string The identifier value * - * @throws RuntimeException If the entity does not exist in Doctrine's identity map + * @throws RuntimeException If the object does not exist in Doctrine's identity map * * @internal Should not be accessed by user-land code. This method is public * only to be usable as callback. @@ -106,22 +106,23 @@ class EntityChoiceLoader implements ChoiceLoaderInterface * Creates a new choice loader. * * Optionally, an implementation of {@link EntityLoaderInterface} can be - * passed which optimizes the entity loading for one of the Doctrine + * passed which optimizes the object loading for one of the Doctrine * mapper implementations. * * @param ChoiceListFactoryInterface $factory The factory for creating * the loaded choice list * @param ObjectManager $manager The object manager - * @param string $class The entity class name - * @param null|EntityLoaderInterface $entityLoader The entity loader + * @param string $class The class name of the + * loaded objects + * @param null|EntityLoaderInterface $objectLoader The objects loader */ - public function __construct(ChoiceListFactoryInterface $factory, ObjectManager $manager, $class, EntityLoaderInterface $entityLoader = null) + public function __construct(ChoiceListFactoryInterface $factory, ObjectManager $manager, $class, EntityLoaderInterface $objectLoader = null) { $this->factory = $factory; $this->manager = $manager; $this->classMetadata = $manager->getClassMetadata($class); $this->class = $this->classMetadata->getName(); - $this->entityLoader = $entityLoader; + $this->objectLoader = $objectLoader; $identifier = $this->classMetadata->getIdentifierFieldNames(); @@ -140,51 +141,51 @@ class EntityChoiceLoader implements ChoiceLoaderInterface return $this->choiceList; } - $entities = $this->entityLoader - ? $this->entityLoader->getEntities() + $objects = $this->objectLoader + ? $this->objectLoader->getEntities() : $this->manager->getRepository($this->class)->findAll(); // If the class has a multi-column identifier, we cannot index the - // entities by their IDs + // objects by their IDs if ($this->compositeId) { - $this->choiceList = $this->factory->createListFromChoices($entities, $value); + $this->choiceList = $this->factory->createListFromChoices($objects, $value); return $this->choiceList; } - // Index the entities by ID - $entitiesById = array(); + // Index the objects by ID + $objectsById = array(); - foreach ($entities as $entity) { - $id = self::getIdValue($this->manager, $this->classMetadata, $entity); - $entitiesById[$id] = $entity; + foreach ($objects as $object) { + $id = self::getIdValue($this->manager, $this->classMetadata, $object); + $objectsById[$id] = $object; } - $this->choiceList = $this->factory->createListFromChoices($entitiesById, $value); + $this->choiceList = $this->factory->createListFromChoices($objectsById, $value); return $this->choiceList; } /** - * Loads the values corresponding to the given entities. + * Loads the values corresponding to the given objects. * * The values are returned with the same keys and in the same order as the - * corresponding entities in the given array. + * corresponding objects in the given array. * * Optionally, a callable can be passed for generating the choice values. - * The callable receives the entity as first and the array key as the second + * The callable receives the object as first and the array key as the second * argument. * - * @param array $entities An array of entities. Non-existing entities - * in this array are ignored + * @param array $objects An array of objects. Non-existing objects in + * this array are ignored * @param null|callable $value The callable generating the choice values * * @return string[] An array of choice values */ - public function loadValuesForChoices(array $entities, $value = null) + public function loadValuesForChoices(array $objects, $value = null) { // Performance optimization - if (empty($entities)) { + if (empty($objects)) { return array(); } @@ -195,41 +196,41 @@ class EntityChoiceLoader implements ChoiceLoaderInterface if (!$this->choiceList && !$this->compositeId) { $values = array(); - // Maintain order and indices of the given entities - foreach ($entities as $i => $entity) { - if ($entity instanceof $this->class) { + // Maintain order and indices of the given objects + foreach ($objects as $i => $object) { + if ($object instanceof $this->class) { // Make sure to convert to the right format - $values[$i] = (string) self::getIdValue($this->manager, $this->classMetadata, $entity); + $values[$i] = (string) self::getIdValue($this->manager, $this->classMetadata, $object); } } return $values; } - return $this->loadChoiceList($value)->getValuesForChoices($entities); + return $this->loadChoiceList($value)->getValuesForChoices($objects); } /** - * Loads the entities corresponding to the given values. + * Loads the objects corresponding to the given values. * - * The entities are returned with the same keys and in the same order as the + * The objects are returned with the same keys and in the same order as the * corresponding values in the given array. * * Optionally, a callable can be passed for generating the choice values. - * The callable receives the entity as first and the array key as the second + * The callable receives the object as first and the array key as the second * argument. * * @param string[] $values An array of choice values. Non-existing * values in this array are ignored * @param null|callable $value The callable generating the choice values * - * @return array An array of entities + * @return array An array of objects */ public function loadChoicesForValues(array $values, $value = null) { // Performance optimization // Also prevents the generation of "WHERE id IN ()" queries through the - // entity loader. At least with MySQL and on the development machine + // object loader. At least with MySQL and on the development machine // this was tested on, no exception was thrown for such invalid // statements, consequently no test fails when this code is removed. // https://github.com/symfony/symfony/pull/8981#issuecomment-24230557 @@ -237,29 +238,29 @@ class EntityChoiceLoader implements ChoiceLoaderInterface return array(); } - // Optimize performance in case we have an entity loader and + // Optimize performance in case we have an object loader and // a single-field identifier - if (!$this->choiceList && !$this->compositeId && $this->entityLoader) { - $unorderedEntities = $this->entityLoader->getEntitiesByIds($this->idField, $values); - $entitiesById = array(); - $entities = array(); + if (!$this->choiceList && !$this->compositeId && $this->objectLoader) { + $unorderedObjects = $this->objectLoader->getEntitiesByIds($this->idField, $values); + $objectsById = array(); + $objects = array(); // Maintain order and indices from the given $values // An alternative approach to the following loop is to add the // "INDEX BY" clause to the Doctrine query in the loader, // but I'm not sure whether that's doable in a generic fashion. - foreach ($unorderedEntities as $entity) { - $id = self::getIdValue($this->manager, $this->classMetadata, $entity); - $entitiesById[$id] = $entity; + foreach ($unorderedObjects as $object) { + $id = self::getIdValue($this->manager, $this->classMetadata, $object); + $objectsById[$id] = $object; } foreach ($values as $i => $id) { - if (isset($entitiesById[$id])) { - $entities[$i] = $entitiesById[$id]; + if (isset($objectsById[$id])) { + $objects[$i] = $objectsById[$id]; } } - return $entities; + return $objects; } return $this->loadChoiceList($value)->getChoicesForValues($values); diff --git a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/EntityChoiceList.php b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/EntityChoiceList.php index 3566a33d7d..f3d4ff48f6 100644 --- a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/EntityChoiceList.php +++ b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/EntityChoiceList.php @@ -24,7 +24,7 @@ use Symfony\Component\PropertyAccess\PropertyAccessorInterface; * @author Bernhard Schussek * * @deprecated Deprecated since Symfony 2.7, to be removed in Symfony 3.0. - * Use {@link EntityChoiceLoader} instead. + * Use {@link DoctrineChoiceLoader} instead. */ class EntityChoiceList extends ObjectChoiceList { diff --git a/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php b/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php index 6c90a2eeb0..76e78cb1f0 100644 --- a/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php +++ b/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php @@ -13,7 +13,7 @@ namespace Symfony\Bridge\Doctrine\Form\Type; use Doctrine\Common\Persistence\ManagerRegistry; use Doctrine\Common\Persistence\ObjectManager; -use Symfony\Bridge\Doctrine\Form\ChoiceList\EntityChoiceLoader; +use Symfony\Bridge\Doctrine\Form\ChoiceList\DoctrineChoiceLoader; use Symfony\Bridge\Doctrine\Form\ChoiceList\EntityLoaderInterface; use Symfony\Bridge\Doctrine\Form\DataTransformer\CollectionToArrayTransformer; use Symfony\Bridge\Doctrine\Form\EventListener\MergeDoctrineCollectionListener; @@ -41,7 +41,7 @@ abstract class DoctrineType extends AbstractType private $choiceListFactory; /** - * @var EntityChoiceLoader[] + * @var DoctrineChoiceLoader[] */ private $choiceLoaders = array(); @@ -71,32 +71,43 @@ 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']) { - $hash = CachingFactoryDecorator::generateHash(array( - $options['em'], - $options['class'], - $options['query_builder'], - $options['loader'], - )); - - if (!isset($choiceLoaders[$hash])) { - if ($options['loader']) { - $loader = $options['loader']; - } elseif (null !== $options['query_builder']) { - $loader = $type->getLoader($options['em'], $options['query_builder'], $options['class']); - } else { - $queryBuilder = $options['em']->getRepository($options['class'])->createQueryBuilder('e'); - $loader = $type->getLoader($options['em'], $queryBuilder, $options['class']); - } - - $choiceLoaders[$hash] = new EntityChoiceLoader( - $choiceListFactory, + // 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'], - $loader - ); + $options['query_builder'], + $options['loader'], + )); + + if (isset($choiceLoaders[$hash])) { + return $choiceLoaders[$hash]; + } } - return $choiceLoaders[$hash]; + if ($options['loader']) { + $entityLoader = $options['loader']; + } elseif (null !== $options['query_builder']) { + $entityLoader = $type->getLoader($options['em'], $options['query_builder'], $options['class']); + } else { + $queryBuilder = $options['em']->getRepository($options['class'])->createQueryBuilder('e'); + $entityLoader = $type->getLoader($options['em'], $queryBuilder, $options['class']); + } + + $choiceLoader = new DoctrineChoiceLoader( + $choiceListFactory, + $options['em'], + $options['class'], + $entityLoader + ); + + if (null !== $hash) { + $choiceLoaders[$hash] = $choiceLoader; + } + + return $choiceLoader; } }; @@ -131,7 +142,7 @@ abstract class DoctrineType extends AbstractType }; // The choices are always indexed by ID (see "choices" normalizer - // and EntityChoiceLoader), unless the ID is composite. Then they + // and DoctrineChoiceLoader), unless the ID is composite. Then they // are indexed by an incrementing integer. // Use the ID/incrementing integer as choice value. $choiceValue = function ($entity, $key) { @@ -181,7 +192,7 @@ abstract class DoctrineType extends AbstractType $entitiesById = array(); foreach ($entities as $entity) { - $id = EntityChoiceLoader::getIdValue($om, $classMetadata, $entity); + $id = DoctrineChoiceLoader::getIdValue($om, $classMetadata, $entity); $entitiesById[$id] = $entity; } diff --git a/src/Symfony/Bridge/Doctrine/Form/Type/EntityType.php b/src/Symfony/Bridge/Doctrine/Form/Type/EntityType.php index e9b89302db..675c289c76 100644 --- a/src/Symfony/Bridge/Doctrine/Form/Type/EntityType.php +++ b/src/Symfony/Bridge/Doctrine/Form/Type/EntityType.php @@ -17,71 +17,22 @@ use Symfony\Bridge\Doctrine\Form\ChoiceList\ORMQueryBuilderLoader; class EntityType extends DoctrineType { - /** - * @var ORMQueryBuilderLoader[] - */ - private $loaderCache = array(); - /** * Return the default loader object. * - * @param ObjectManager $manager - * @param mixed $queryBuilder - * @param string $class + * @param ObjectManager $manager + * @param QueryBuilder|\Closure $queryBuilder + * @param string $class * * @return ORMQueryBuilderLoader */ public function getLoader(ObjectManager $manager, $queryBuilder, $class) { - if (!$queryBuilder instanceof QueryBuilder) { - return new ORMQueryBuilderLoader( - $queryBuilder, - $manager, - $class - ); - } - - $queryBuilderHash = $this->getQueryBuilderHash($queryBuilder); - $loaderHash = $this->getLoaderHash($manager, $queryBuilderHash, $class); - - if (!isset($this->loaderCache[$loaderHash])) { - $this->loaderCache[$loaderHash] = new ORMQueryBuilderLoader( - $queryBuilder, - $manager, - $class - ); - } - - return $this->loaderCache[$loaderHash]; - } - - /** - * @param QueryBuilder $queryBuilder - * - * @return string - */ - private function getQueryBuilderHash(QueryBuilder $queryBuilder) - { - return hash('sha256', json_encode(array( - 'sql' => $queryBuilder->getQuery()->getSQL(), - 'parameters' => $queryBuilder->getParameters(), - ))); - } - - /** - * @param ObjectManager $manager - * @param string $queryBuilderHash - * @param string $class - * - * @return string - */ - private function getLoaderHash(ObjectManager $manager, $queryBuilderHash, $class) - { - return hash('sha256', json_encode(array( - 'manager' => spl_object_hash($manager), - 'queryBuilder' => $queryBuilderHash, - 'class' => $class, - ))); + return new ORMQueryBuilderLoader( + $queryBuilder, + $manager, + $class + ); } public function getName()