Skip to content

Commit

Permalink
bug #14372 [DoctrineBridge][Form] fix EntityChoiceList when indexing …
Browse files Browse the repository at this point in the history
…by primary foreign key (giosh94mhz)

This PR was merged into the 2.3 branch.

Discussion
----------

[DoctrineBridge][Form] fix EntityChoiceList when indexing by primary foreign key

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

I've found a bug while using the 'entity' FormType.

Doctrine allow the definition of primary keys which are foreign key of other entities. In this scenario, the `EntityChoiceList` instance check if:
  * the entity has a id composed by a single column and
  * eventually, the column is an integer

When this happens, it use the primary key as "choices indices", but since is an entity it fails in many places, where it expects integer.

The easy solution is to check whether the single-column id is not an association. Anyway, I've fixed it the RightWay™ :), and now it resolve the entity reference to the actual column type, and restart the logic. Code speaks better then words.

Commits
-------

fe4246a [DoctrineBridge][Form] Fix EntityChoiceList when indexing by primary foreign key
  • Loading branch information
fabpot committed Aug 12, 2015
2 parents 0ea11e4 + fe4246a commit d6d93dd
Show file tree
Hide file tree
Showing 7 changed files with 310 additions and 15 deletions.
97 changes: 83 additions & 14 deletions src/Symfony/Bridge/Doctrine/Form/ChoiceList/EntityChoiceList.php
Expand Up @@ -40,6 +40,13 @@ class EntityChoiceList extends ObjectChoiceList
*/
private $classMetadata;

/**
* Metadata for target class of primary key association.
*
* @var ClassMetadata
*/
private $idClassMetadata;

/**
* Contains the query builder that builds the query for fetching the
* entities.
Expand Down Expand Up @@ -107,16 +114,21 @@ public function __construct(ObjectManager $manager, $class, $labelPath = null, E
$this->class = $this->classMetadata->getName();
$this->loaded = is_array($entities) || $entities instanceof \Traversable;
$this->preferredEntities = $preferredEntities;
list(
$this->idAsIndex,
$this->idAsValue,
$this->idField
) = $this->getIdentifierInfoForClass($this->classMetadata);

if (null !== $this->idField && $this->classMetadata->hasAssociation($this->idField)) {
$this->idClassMetadata = $this->em->getClassMetadata(
$this->classMetadata->getAssociationTargetClass($this->idField)
);

$identifier = $this->classMetadata->getIdentifierFieldNames();

if (1 === count($identifier)) {
$this->idField = $identifier[0];
$this->idAsValue = true;

if (in_array($this->classMetadata->getTypeOfField($this->idField), array('integer', 'smallint', 'bigint'))) {
$this->idAsIndex = true;
}
list(
$this->idAsIndex,
$this->idAsValue
) = $this->getIdentifierInfoForClass($this->idClassMetadata);
}

if (!$this->loaded) {
Expand Down Expand Up @@ -228,7 +240,7 @@ public function getChoicesForValues(array $values)
// "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) {
$value = $this->fixValue(current($this->getIdentifierValues($entity)));
$value = $this->fixValue($this->getSingleIdentifierValue($entity));
$entitiesByValue[$value] = $entity;
}

Expand Down Expand Up @@ -274,7 +286,7 @@ public function getValuesForChoices(array $entities)
foreach ($entities as $i => $entity) {
if ($entity instanceof $this->class) {
// Make sure to convert to the right format
$values[$i] = $this->fixValue(current($this->getIdentifierValues($entity)));
$values[$i] = $this->fixValue($this->getSingleIdentifierValue($entity));
}
}

Expand Down Expand Up @@ -314,7 +326,7 @@ public function getIndicesForChoices(array $entities)
foreach ($entities as $i => $entity) {
if ($entity instanceof $this->class) {
// Make sure to convert to the right format
$indices[$i] = $this->fixIndex(current($this->getIdentifierValues($entity)));
$indices[$i] = $this->fixIndex($this->getSingleIdentifierValue($entity));
}
}

Expand Down Expand Up @@ -372,7 +384,7 @@ public function getIndicesForValues(array $values)
protected function createIndex($entity)
{
if ($this->idAsIndex) {
return $this->fixIndex(current($this->getIdentifierValues($entity)));
return $this->fixIndex($this->getSingleIdentifierValue($entity));
}

return parent::createIndex($entity);
Expand All @@ -392,7 +404,7 @@ protected function createIndex($entity)
protected function createValue($entity)
{
if ($this->idAsValue) {
return (string) current($this->getIdentifierValues($entity));
return (string) $this->getSingleIdentifierValue($entity);
}

return parent::createValue($entity);
Expand All @@ -415,6 +427,36 @@ protected function fixIndex($index)
return $index;
}

/**
* Get identifier information for a class.
*
* @param ClassMetadata $classMetadata The entity metadata
*
* @return array Return an array with idAsIndex, idAsValue and identifier
*/
private function getIdentifierInfoForClass(ClassMetadata $classMetadata)
{
$identifier = null;
$idAsIndex = false;
$idAsValue = false;

$identifiers = $classMetadata->getIdentifierFieldNames();

if (1 === count($identifiers)) {
$identifier = $identifiers[0];

if (!$classMetadata->hasAssociation($identifier)) {
$idAsValue = true;

if (in_array($classMetadata->getTypeOfField($identifier), array('integer', 'smallint', 'bigint'))) {
$idAsIndex = true;
}
}
}

return array($idAsIndex, $idAsValue, $identifier);
}

/**
* Loads the list with entities.
*
Expand All @@ -438,6 +480,33 @@ private function load()
$this->loaded = true;
}

/**
* Returns the first (and only) value of the identifier fields of an entity.
*
* Doctrine must know about this entity, that is, the entity must already
* be persisted or added to the identity map before. Otherwise an
* exception is thrown.
*
* @param object $entity The entity for which to get the identifier
*
* @return array The identifier values
*
* @throws RuntimeException If the entity does not exist in Doctrine's identity map
*/
private function getSingleIdentifierValue($entity)
{
$value = current($this->getIdentifierValues($entity));

if ($this->idClassMetadata) {
$class = $this->idClassMetadata->getName();
if ($value instanceof $class) {
$value = current($this->idClassMetadata->getIdentifierValues($value));
}
}

return $value;
}

/**
* Returns the values of the identifier fields of an entity.
*
Expand Down
@@ -0,0 +1,38 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bridge\Doctrine\Tests\Fixtures;

use Doctrine\ORM\Mapping\Id;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\OneToOne;

/** @Entity */
class SingleAssociationToIntIdEntity
{
/** @Id @OneToOne(targetEntity="SingleIntIdNoToStringEntity", cascade={"ALL"}) */
protected $entity;

/** @Column(type="string", nullable=true) */
public $name;

public function __construct(SingleIntIdNoToStringEntity $entity, $name)
{
$this->entity = $entity;
$this->name = $name;
}

public function __toString()
{
return (string) $this->name;
}
}
@@ -0,0 +1,86 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bridge\Doctrine\Tests\Form\ChoiceList;

use Symfony\Bridge\Doctrine\Tests\Fixtures\SingleAssociationToIntIdEntity;
use Symfony\Bridge\Doctrine\Tests\Fixtures\SingleIntIdNoToStringEntity;
use Symfony\Bridge\Doctrine\Form\ChoiceList\EntityChoiceList;

/**
* Test choices generated from an entity with a primary foreign key.
*
* @author Premi Giorgio <giosh94mhz@gmail.com>
* @author Bernhard Schussek <bschussek@gmail.com>
*/
abstract class AbstractEntityChoiceListSingleAssociationToIntIdTest extends AbstractEntityChoiceListTest
{
protected function getEntityClass()
{
return 'Symfony\Bridge\Doctrine\Tests\Fixtures\SingleAssociationToIntIdEntity';
}

protected function getClassesMetadata()
{
return array(
$this->em->getClassMetadata($this->getEntityClass()),
$this->em->getClassMetadata('Symfony\Bridge\Doctrine\Tests\Fixtures\SingleIntIdNoToStringEntity'),
);
}

protected function createChoiceList()
{
return new EntityChoiceList($this->em, $this->getEntityClass(), 'name');
}

/**
* @return \Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceListInterface
*/
protected function createObjects()
{
$innerA = new SingleIntIdNoToStringEntity(-10, 'inner_A');
$innerB = new SingleIntIdNoToStringEntity(10, 'inner_B');
$innerC = new SingleIntIdNoToStringEntity(20, 'inner_C');
$innerD = new SingleIntIdNoToStringEntity(30, 'inner_D');

$this->em->persist($innerA);
$this->em->persist($innerB);
$this->em->persist($innerC);
$this->em->persist($innerD);

return array(
new SingleAssociationToIntIdEntity($innerA, 'A'),
new SingleAssociationToIntIdEntity($innerB, 'B'),
new SingleAssociationToIntIdEntity($innerC, 'C'),
new SingleAssociationToIntIdEntity($innerD, 'D'),
);
}

protected function getChoices()
{
return array('_10' => $this->obj1, 10 => $this->obj2, 20 => $this->obj3, 30 => $this->obj4);
}

protected function getLabels()
{
return array('_10' => 'A', 10 => 'B', 20 => 'C', 30 => 'D');
}

protected function getValues()
{
return array('_10' => '-10', 10 => '10', 20 => '20', 30 => '30');
}

protected function getIndices()
{
return array('_10', 10, 20, 30);
}
}
Expand Up @@ -39,7 +39,7 @@ protected function setUp()
$this->em = DoctrineTestHelper::createTestEntityManager();

$schemaTool = new SchemaTool($this->em);
$classes = array($this->em->getClassMetadata($this->getEntityClass()));
$classes = $this->getClassesMetadata();

try {
$schemaTool->dropSchema($classes);
Expand Down Expand Up @@ -73,6 +73,11 @@ abstract protected function getEntityClass();

abstract protected function createObjects();

protected function getClassesMetadata()
{
return array($this->em->getClassMetadata($this->getEntityClass()));
}

/**
* @return \Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceListInterface
*/
Expand Down
@@ -0,0 +1,32 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bridge\Doctrine\Tests\Form\ChoiceList;

/**
* @author Premi Giorgio <giosh94mhz@gmail.com>
* @author Bernhard Schussek <bschussek@gmail.com>
*/
class LoadedEntityChoiceListSingleAssociationToIntIdTest extends AbstractEntityChoiceListSingleAssociationToIntIdTest
{
/**
* @return \Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceListInterface
*/
protected function createChoiceList()
{
$list = parent::createChoiceList();

// load list
$list->getChoices();

return $list;
}
}
@@ -0,0 +1,32 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bridge\Doctrine\Tests\Form\ChoiceList;

/**
* @author Premi Giorgio <giosh94mhz@gmail.com>
* @author Bernhard Schussek <bschussek@gmail.com>
*/
class UnloadedEntityChoiceListSingleAssociationToIntIdTest extends AbstractEntityChoiceListSingleAssociationToIntIdTest
{
public function testGetIndicesForValuesIgnoresNonExistingValues()
{
$this->markTestSkipped('Non-existing values are not detected for unloaded choice lists.');
}

/**
* @group legacy
*/
public function testLegacyGetIndicesForValuesIgnoresNonExistingValues()
{
$this->markTestSkipped('Non-existing values are not detected for unloaded choice lists.');
}
}

0 comments on commit d6d93dd

Please sign in to comment.