diff --git a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php index f5dcb7312e65e..1b2a35ce534a4 100644 --- a/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php +++ b/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php @@ -12,6 +12,7 @@ namespace Symfony\Bridge\Doctrine\Form\ChoiceList; use Doctrine\Common\Persistence\ObjectManager; +use Symfony\Component\Form\ChoiceList\ArrayChoiceList; use Symfony\Component\Form\ChoiceList\ChoiceListInterface; use Symfony\Component\Form\ChoiceList\Factory\ChoiceListFactoryInterface; use Symfony\Component\Form\ChoiceList\Loader\ChoiceLoaderInterface; @@ -60,20 +61,31 @@ class DoctrineChoiceLoader implements ChoiceLoaderInterface * 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 class name of the * loaded objects * @param IdReader $idReader The reader for the object * IDs. + * @param ChoiceListFactoryInterface $factory The factory for creating + * the loaded choice list * @param null|EntityLoaderInterface $objectLoader The objects loader */ - public function __construct(ChoiceListFactoryInterface $factory, ObjectManager $manager, $class, IdReader $idReader = null, EntityLoaderInterface $objectLoader = null) + public function __construct($manager, $class, $idReader = null, $objectLoader = null, $factory = null) { + // BC to be removed and replace with type hints in 4.0 + if ($manager instanceof ChoiceListFactoryInterface) { + @trigger_error(sprintf('Passing a ChoiceListFactoryInterface to %s is deprecated since version 3.1 and will no longer be supported in 4.0. You should either call "%s::loadChoiceList" or override it to return a ChoiceListInterface.', __CLASS__, __CLASS__)); + + // Provide a BC layer since $factory has changed + // form first to last argument as of 3.1 + $this->factory = $manager; + $manager = $class; + $class = $idReader; + $objectLoader = $factory; + } + $classMetadata = $manager->getClassMetadata($class); - $this->factory = $factory; $this->manager = $manager; $this->class = $classMetadata->getName(); $this->idReader = $idReader ?: new IdReader($manager, $classMetadata); @@ -93,9 +105,7 @@ public function loadChoiceList($value = null) ? $this->objectLoader->getEntities() : $this->manager->getRepository($this->class)->findAll(); - $this->choiceList = $this->factory->createListFromChoices($objects, $value); - - return $this->choiceList; + return $this->choiceList = new ArrayChoiceList($objects, $value); } /** @@ -146,7 +156,7 @@ public function loadChoicesForValues(array $values, $value = null) // Optimize performance in case we have an object loader and // a single-field identifier - $optimize = null === $value || is_array($value) && $value[0] === $this->idReader; + $optimize = null === $value || is_array($value) && $this->idReader === $value[0]; if ($optimize && !$this->choiceList && $this->objectLoader && $this->idReader->isSingleId()) { $unorderedObjects = $this->objectLoader->getEntitiesByIds($this->idReader->getIdField(), $values); diff --git a/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php b/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php index 9b3c50307ce61..244d5a42f5c0b 100644 --- a/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php +++ b/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php @@ -160,7 +160,6 @@ public function configureOptions(OptionsResolver $resolver) } $doctrineChoiceLoader = new DoctrineChoiceLoader( - $this->choiceListFactory, $options['em'], $options['class'], $options['id_reader'], diff --git a/src/Symfony/Component/Form/ChoiceList/LazyChoiceList.php b/src/Symfony/Component/Form/ChoiceList/LazyChoiceList.php index 27d8deb8825b4..c4de64ec0827c 100644 --- a/src/Symfony/Component/Form/ChoiceList/LazyChoiceList.php +++ b/src/Symfony/Component/Form/ChoiceList/LazyChoiceList.php @@ -45,9 +45,18 @@ class LazyChoiceList implements ChoiceListInterface /** * @var ChoiceListInterface|null + * + * @deprecated Since 3.1, to be remove in 4.0. Cache the choice list in the {@link ChoiceLoaderInterface} instead. */ private $loadedList; + /** + * @var bool + * + * @deprecated Flag used for BC layer since 3.1. To be removed in 4.0. + */ + private $loaded = false; + /** * Creates a lazily-loaded list using the given loader. * @@ -70,11 +79,23 @@ public function __construct(ChoiceLoaderInterface $loader, callable $value = nul */ public function getChoices() { - if (!$this->loadedList) { - $this->loadedList = $this->loader->loadChoiceList($this->value); + if ($this->loaded) { + // We can safely invoke the {@link ChoiceLoaderInterface} assuming it has the list + // in cache when the lazy list is already loaded + if ($this->loadedList !== $this->loader->loadChoiceList($this->value)) { + @trigger_error(sprintf('Caching the choice list in %s is deprecated since 3.1 and will not happen in 4.0. Cache the list in the %s instead.', __CLASS__, ChoiceLoaderInterface::class)); + } + + return $this->loadedList->getChoices(); } + // BC + $this->loadedList = $this->loader->loadChoiceList($this->value); + $this->loaded = true; + return $this->loadedList->getChoices(); + // In 4.0 keep the following line only: + // return $this->loader->loadChoiceList($this->value)->getChoices() } /** @@ -82,11 +103,22 @@ public function getChoices() */ public function getValues() { - if (!$this->loadedList) { - $this->loadedList = $this->loader->loadChoiceList($this->value); + if ($this->loaded) { + // Check whether the loader has the same cache + if ($this->loadedList !== $this->loader->loadChoiceList($this->value)) { + @trigger_error(sprintf('Caching the choice list in %s is deprecated since 3.1 and will not happen in 4.0. Cache the list in the %s instead.', __CLASS__, ChoiceLoaderInterface::class)); + } + + return $this->loadedList->getValues(); } + // BC + $this->loadedList = $this->loader->loadChoiceList($this->value); + $this->loaded = true; + return $this->loadedList->getValues(); + // In 4.0 keep the following line only: + // return $this->loader->loadChoiceList($this->value)->getValues() } /** @@ -94,11 +126,22 @@ public function getValues() */ public function getStructuredValues() { - if (!$this->loadedList) { - $this->loadedList = $this->loader->loadChoiceList($this->value); + if ($this->loaded) { + // Check whether the loader has the same cache + if ($this->loadedList !== $this->loader->loadChoiceList($this->value)) { + @trigger_error(sprintf('Caching the choice list in %s is deprecated since 3.1 and will not happen in 4.0. Cache the list in the %s instead.', __CLASS__, ChoiceLoaderInterface::class)); + } + + return $this->loadedList->getStructuredValues(); } + // BC + $this->loadedList = $this->loader->loadChoiceList($this->value); + $this->loaded = true; + return $this->loadedList->getStructuredValues(); + // In 4.0 keep the following line only: + // return $this->loader->loadChoiceList($this->value)->getStructuredValues(); } /** @@ -106,11 +149,22 @@ public function getStructuredValues() */ public function getOriginalKeys() { - if (!$this->loadedList) { - $this->loadedList = $this->loader->loadChoiceList($this->value); + if ($this->loaded) { + // Check whether the loader has the same cache + if ($this->loadedList !== $this->loader->loadChoiceList($this->value)) { + @trigger_error(sprintf('Caching the choice list in %s is deprecated since 3.1 and will not happen in 4.0. Cache the list in the %s instead.', __CLASS__, ChoiceLoaderInterface::class)); + } + + return $this->loadedList->getOriginalKeys(); } + // BC + $this->loadedList = $this->loader->loadChoiceList($this->value); + $this->loaded = true; + return $this->loadedList->getOriginalKeys(); + // In 4.0 keep the following line only: + // return $this->loader->loadChoiceList($this->value)->getOriginalKeys(); } /** @@ -118,11 +172,16 @@ public function getOriginalKeys() */ public function getChoicesForValues(array $values) { - if (!$this->loadedList) { - return $this->loader->loadChoicesForValues($values, $this->value); + if ($this->loaded) { + // Check whether the loader has the same cache + if ($this->loadedList !== $this->loader->loadChoiceList($this->value)) { + @trigger_error(sprintf('Caching the choice list in %s is deprecated since 3.1 and will not happen in 4.0. Cache the list in the %s instead.', __CLASS__, ChoiceLoaderInterface::class)); + } + + return $this->loadedList->getChoicesForValues($values); } - return $this->loadedList->getChoicesForValues($values); + return $this->loader->loadChoicesForValues($values, $this->value); } /** @@ -130,10 +189,15 @@ public function getChoicesForValues(array $values) */ public function getValuesForChoices(array $choices) { - if (!$this->loadedList) { - return $this->loader->loadValuesForChoices($choices, $this->value); + if ($this->loaded) { + // Check whether the loader has the same cache + if ($this->loadedList !== $this->loader->loadChoiceList($this->value)) { + @trigger_error(sprintf('Caching the choice list in %s is deprecated since 3.1 and will not happen in 4.0. Cache the list in the %s instead.', __CLASS__, ChoiceLoaderInterface::class)); + } + + return $this->loadedList->getValuesForChoices($choices); } - return $this->loadedList->getValuesForChoices($choices); + return $this->loader->loadValuesForChoices($choices, $this->value); } } diff --git a/src/Symfony/Component/Form/Tests/ChoiceList/LazyChoiceListTest.php b/src/Symfony/Component/Form/Tests/ChoiceList/LazyChoiceListTest.php index 5db96e6a7dee1..4102b7d3561eb 100644 --- a/src/Symfony/Component/Form/Tests/ChoiceList/LazyChoiceListTest.php +++ b/src/Symfony/Component/Form/Tests/ChoiceList/LazyChoiceListTest.php @@ -11,6 +11,7 @@ namespace Symfony\Component\Form\Tests\ChoiceList; +use Symfony\Component\Form\ChoiceList\ArrayChoiceList; use Symfony\Component\Form\ChoiceList\LazyChoiceList; /** @@ -26,7 +27,7 @@ class LazyChoiceListTest extends \PHPUnit_Framework_TestCase /** * @var \PHPUnit_Framework_MockObject_MockObject */ - private $innerList; + private $loadedList; /** * @var \PHPUnit_Framework_MockObject_MockObject @@ -37,20 +38,45 @@ class LazyChoiceListTest extends \PHPUnit_Framework_TestCase protected function setUp() { - $this->innerList = $this->getMock('Symfony\Component\Form\ChoiceList\ChoiceListInterface'); + $this->loadedList = $this->getMock('Symfony\Component\Form\ChoiceList\ChoiceListInterface'); $this->loader = $this->getMock('Symfony\Component\Form\ChoiceList\Loader\ChoiceLoaderInterface'); $this->value = function () {}; $this->list = new LazyChoiceList($this->loader, $this->value); } - public function testGetChoicesLoadsInnerListOnFirstCall() + public function testGetChoiceLoadersLoadsLoadedListOnFirstCall() { - $this->loader->expects($this->once()) + $this->loader->expects($this->exactly(2)) + ->method('loadChoiceList') + ->with($this->value) + ->will($this->returnValue($this->loadedList)); + + // The same list is returned by the loader + $this->loadedList->expects($this->exactly(2)) + ->method('getChoices') + ->will($this->returnValue('RESULT')); + + $this->assertSame('RESULT', $this->list->getChoices()); + $this->assertSame('RESULT', $this->list->getChoices()); + } + + /** + * @group legacy + */ + public function testGetChoicesUsesLoadedListWhenLoaderDoesNotCacheChoiceListOnFirstCall() + { + $this->loader->expects($this->at(0)) + ->method('loadChoiceList') + ->with($this->value) + ->willReturn($this->loadedList); + + $this->loader->expects($this->at(1)) ->method('loadChoiceList') ->with($this->value) - ->will($this->returnValue($this->innerList)); + ->willReturn(new ArrayChoiceList(array('a', 'b'))); - $this->innerList->expects($this->exactly(2)) + // The same list is returned by the lazy choice list + $this->loadedList->expects($this->exactly(2)) ->method('getChoices') ->will($this->returnValue('RESULT')); @@ -58,14 +84,15 @@ public function testGetChoicesLoadsInnerListOnFirstCall() $this->assertSame('RESULT', $this->list->getChoices()); } - public function testGetValuesLoadsInnerListOnFirstCall() + public function testGetValuesLoadsLoadedListOnFirstCall() { - $this->loader->expects($this->once()) + $this->loader->expects($this->exactly(2)) ->method('loadChoiceList') ->with($this->value) - ->will($this->returnValue($this->innerList)); + ->will($this->returnValue($this->loadedList)); - $this->innerList->expects($this->exactly(2)) + // The same list is returned by the loader + $this->loadedList->expects($this->exactly(2)) ->method('getValues') ->will($this->returnValue('RESULT')); @@ -73,14 +100,15 @@ public function testGetValuesLoadsInnerListOnFirstCall() $this->assertSame('RESULT', $this->list->getValues()); } - public function testGetStructuredValuesLoadsInnerListOnFirstCall() + public function testGetStructuredValuesLoadsLoadedListOnFirstCall() { - $this->loader->expects($this->once()) + $this->loader->expects($this->exactly(2)) ->method('loadChoiceList') ->with($this->value) - ->will($this->returnValue($this->innerList)); + ->will($this->returnValue($this->loadedList)); - $this->innerList->expects($this->exactly(2)) + // The same list is returned by the loader + $this->loadedList->expects($this->exactly(2)) ->method('getStructuredValues') ->will($this->returnValue('RESULT')); @@ -88,14 +116,15 @@ public function testGetStructuredValuesLoadsInnerListOnFirstCall() $this->assertSame('RESULT', $this->list->getStructuredValues()); } - public function testGetOriginalKeysLoadsInnerListOnFirstCall() + public function testGetOriginalKeysLoadsLoadedListOnFirstCall() { - $this->loader->expects($this->once()) + $this->loader->expects($this->exactly(2)) ->method('loadChoiceList') ->with($this->value) - ->will($this->returnValue($this->innerList)); + ->will($this->returnValue($this->loadedList)); - $this->innerList->expects($this->exactly(2)) + // The same list is returned by the loader + $this->loadedList->expects($this->exactly(2)) ->method('getOriginalKeys') ->will($this->returnValue('RESULT')); @@ -116,15 +145,17 @@ public function testGetChoicesForValuesForwardsCallIfListNotLoaded() public function testGetChoicesForValuesUsesLoadedList() { - $this->loader->expects($this->once()) + $this->loader->expects($this->exactly(3)) ->method('loadChoiceList') ->with($this->value) - ->will($this->returnValue($this->innerList)); + // For BC, the same choice loaded list is returned 3 times + // It should only twice in 4.0 + ->will($this->returnValue($this->loadedList)); $this->loader->expects($this->never()) ->method('loadChoicesForValues'); - $this->innerList->expects($this->exactly(2)) + $this->loadedList->expects($this->exactly(2)) ->method('getChoicesForValues') ->with(array('a', 'b')) ->will($this->returnValue('RESULT')); @@ -136,6 +167,7 @@ public function testGetChoicesForValuesUsesLoadedList() $this->assertSame('RESULT', $this->list->getChoicesForValues(array('a', 'b'))); } + // To be remove in 4.0 public function testGetValuesForChoicesForwardsCallIfListNotLoaded() { $this->loader->expects($this->exactly(2)) @@ -149,15 +181,17 @@ public function testGetValuesForChoicesForwardsCallIfListNotLoaded() public function testGetValuesForChoicesUsesLoadedList() { - $this->loader->expects($this->once()) + $this->loader->expects($this->exactly(3)) ->method('loadChoiceList') ->with($this->value) - ->will($this->returnValue($this->innerList)); + // For BC, the same choice loaded list is returned 3 times + // It should only twice in 4.0 + ->will($this->returnValue($this->loadedList)); $this->loader->expects($this->never()) ->method('loadValuesForChoices'); - $this->innerList->expects($this->exactly(2)) + $this->loadedList->expects($this->exactly(2)) ->method('getValuesForChoices') ->with(array('a', 'b')) ->will($this->returnValue('RESULT'));