Skip to content

Commit

Permalink
bug #17990 [DoctrineBridge][Form] Fix performance regression in Entit…
Browse files Browse the repository at this point in the history
…yType (kimlai)

This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes #17990).

Discussion
----------

[DoctrineBridge][Form] Fix performance regression in EntityType

| Q             | A
| ------------- | ---
| Branch        | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

A performance regression was introduced in 2336d5c

Before, the default behaviour of the `DoctrineLoader` was to only fetch the entities selected in the submitted form.

After, the optimization was only performed when the `choice_value` option was set to `null`.
However, the `DoctrineType` sets a non-null default value to `choice_value`, which means that the default behaviour was not using the optimization anymore.

This commit restores the default behaviour (while keeping the previous commit intent).

References:
- https://github.com/symfony/symfony/blob/v2.7.10/src/Symfony/Bridge/Doctrine/Form/ChoiceList/DoctrineChoiceLoader.php#L149
- https://github.com/symfony/symfony/blob/v2.7.10/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php#L216

Commits
-------

64c80a6 [DoctrineBridge][Form] Fix performance regression in EntityType
  • Loading branch information
fabpot committed Mar 3, 2016
2 parents 5b79649 + 64c80a6 commit 46d1d24
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 1 deletion.
Expand Up @@ -146,7 +146,9 @@ public function loadChoicesForValues(array $values, $value = null)

// Optimize performance in case we have an object loader and
// a single-field identifier
if (null === $value && !$this->choiceList && $this->objectLoader && $this->idReader->isSingleId()) {
$optimize = null === $value || is_array($value) && $value[0] === $this->idReader;

if ($optimize && !$this->choiceList && $this->objectLoader && $this->idReader->isSingleId()) {
$unorderedObjects = $this->objectLoader->getEntitiesByIds($this->idReader->getIdField(), $values);
$objectsById = array();
$objects = array();
Expand Down
24 changes: 24 additions & 0 deletions src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php
Expand Up @@ -789,6 +789,30 @@ public function testOverrideChoicesValuesWithCallable()
$this->assertSame('BooGroup/Bar', $field->getViewData());
}

public function testChoicesForValuesOptimization()
{
$entity1 = new SingleIntIdEntity(1, 'Foo');
$entity2 = new SingleIntIdEntity(2, 'Bar');

$this->persist(array($entity1, $entity2));

$field = $this->factory->createNamed('name', 'Symfony\Bridge\Doctrine\Form\Type\EntityType', null, array(
'em' => 'default',
'class' => self::SINGLE_IDENT_CLASS,
'choice_label' => 'name',
));

$this->em->clear();

$field->submit(1);

$unitOfWorkIdentityMap = $this->em->getUnitOfWork()->getIdentityMap();
$managedEntitiesNames = array_map('strval', $unitOfWorkIdentityMap['Symfony\Bridge\Doctrine\Tests\Fixtures\SingleIntIdEntity']);

$this->assertContains((string) $entity1, $managedEntitiesNames);
$this->assertNotContains((string) $entity2, $managedEntitiesNames);
}

public function testGroupByChoices()
{
$item1 = new GroupableEntity(1, 'Foo', 'Group1');
Expand Down

0 comments on commit 46d1d24

Please sign in to comment.