Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[Form] [DoctrineBridge] optimized LazyChoiceList and DoctrineChoiceLoader #18359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 28, 2016
Merged

[Form] [DoctrineBridge] optimized LazyChoiceList and DoctrineChoiceLoader #18359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
86b2ff1
[DoctrineBridge] optimized DoctrineChoiceLoader
HeahDude Mar 29, 2016
98621f4
[Form] optimized LazyChoiceList
HeahDude Apr 2, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions 26 src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a BC break. We cannot reorder the arguments of the constructor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I just realized what you are doing below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I'm just feel bad about removing the type hints but @iltar and @stof were suggesting that change for good reasons I guess.

{
// 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);
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is broken, as it means that $value cannot be a property path anymore (as you don't handle the conversion to a closure anymore)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, this also prevents re-using existing choice lists in the caching factory

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the ChoiceListFactory create first a LazyChoiceList instance (the first and mandatory call) all the decorators get pass the $value so they can resolve it.

Once this step done the LazyChoiceList will be waken up to use its loader and resolved value all together.

The thing is actually instead of return an ArrayChoiceList instance with the loaded choices, the DoctrineChoiceLoader (which is the only ChoiceLoaderInterface implemented in core) get passed the hole factory with all the decorators (and might add some in the future as it is one a the few BC ways to add feature for now).

I think this final call should be delegated, that remains coherent with the interfaces and has a performance impact.

Anyway it does not break the decorators' resolution of the $value callback as it is stored in the LazyChoiceList.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test with the property path added in #17694 is not broken (ref https://github.com/symfony/symfony/pull/17694/files#diff-641188f07a887e51cad48e9d72d69d07R754)

}

/**
Expand Down Expand Up @@ -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);
Expand Down
1 change: 0 additions & 1 deletion 1 src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,6 @@ public function configureOptions(OptionsResolver $resolver)
}

$doctrineChoiceLoader = new DoctrineChoiceLoader(
$this->choiceListFactory,
$options['em'],
$options['class'],
$options['id_reader'],
Expand Down
92 changes: 78 additions & 14 deletions 92 src/Symfony/Component/Form/ChoiceList/LazyChoiceList.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -70,70 +79,125 @@ 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()
}

/**
* {@inheritdoc}
*/
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()
}

/**
* {@inheritdoc}
*/
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();
}

/**
* {@inheritdoc}
*/
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();
}

/**
* {@inheritdoc}
*/
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);
}

/**
* {@inheritdoc}
*/
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);
}
}
Loading
Morty Proxy This is a proxified and sanitized view of the page, visit original site.