Skip to content

Commit d6d93dd

Browse files
fabpotfabpot
committed
bug symfony#14372 [DoctrineBridge][Form] fix EntityChoiceList when indexing 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
2 parents 0ea11e4 + fe4246a commit d6d93dd

7 files changed

+310
-15
lines changed

‎src/Symfony/Bridge/Doctrine/Form/ChoiceList/EntityChoiceList.php

Lines changed: 83 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,13 @@ class EntityChoiceList extends ObjectChoiceList
4040
*/
4141
private $classMetadata;
4242

43+
/**
44+
* Metadata for target class of primary key association.
45+
*
46+
* @var ClassMetadata
47+
*/
48+
private $idClassMetadata;
49+
4350
/**
4451
* Contains the query builder that builds the query for fetching the
4552
* entities.
@@ -107,16 +114,21 @@ public function __construct(ObjectManager $manager, $class, $labelPath = null, E
107114
$this->class = $this->classMetadata->getName();
108115
$this->loaded = is_array($entities) || $entities instanceof \Traversable;
109116
$this->preferredEntities = $preferredEntities;
117+
list(
118+
$this->idAsIndex,
119+
$this->idAsValue,
120+
$this->idField
121+
) = $this->getIdentifierInfoForClass($this->classMetadata);
122+
123+
if (null !== $this->idField && $this->classMetadata->hasAssociation($this->idField)) {
124+
$this->idClassMetadata = $this->em->getClassMetadata(
125+
$this->classMetadata->getAssociationTargetClass($this->idField)
126+
);
110127

111-
$identifier = $this->classMetadata->getIdentifierFieldNames();
112-
113-
if (1 === count($identifier)) {
114-
$this->idField = $identifier[0];
115-
$this->idAsValue = true;
116-
117-
if (in_array($this->classMetadata->getTypeOfField($this->idField), array('integer', 'smallint', 'bigint'))) {
118-
$this->idAsIndex = true;
119-
}
128+
list(
129+
$this->idAsIndex,
130+
$this->idAsValue
131+
) = $this->getIdentifierInfoForClass($this->idClassMetadata);
120132
}
121133

122134
if (!$this->loaded) {
@@ -228,7 +240,7 @@ public function getChoicesForValues(array $values)
228240
// "INDEX BY" clause to the Doctrine query in the loader,
229241
// but I'm not sure whether that's doable in a generic fashion.
230242
foreach ($unorderedEntities as $entity) {
231-
$value = $this->fixValue(current($this->getIdentifierValues($entity)));
243+
$value = $this->fixValue($this->getSingleIdentifierValue($entity));
232244
$entitiesByValue[$value] = $entity;
233245
}
234246

@@ -274,7 +286,7 @@ public function getValuesForChoices(array $entities)
274286
foreach ($entities as $i => $entity) {
275287
if ($entity instanceof $this->class) {
276288
// Make sure to convert to the right format
277-
$values[$i] = $this->fixValue(current($this->getIdentifierValues($entity)));
289+
$values[$i] = $this->fixValue($this->getSingleIdentifierValue($entity));
278290
}
279291
}
280292

@@ -314,7 +326,7 @@ public function getIndicesForChoices(array $entities)
314326
foreach ($entities as $i => $entity) {
315327
if ($entity instanceof $this->class) {
316328
// Make sure to convert to the right format
317-
$indices[$i] = $this->fixIndex(current($this->getIdentifierValues($entity)));
329+
$indices[$i] = $this->fixIndex($this->getSingleIdentifierValue($entity));
318330
}
319331
}
320332

@@ -372,7 +384,7 @@ public function getIndicesForValues(array $values)
372384
protected function createIndex($entity)
373385
{
374386
if ($this->idAsIndex) {
375-
return $this->fixIndex(current($this->getIdentifierValues($entity)));
387+
return $this->fixIndex($this->getSingleIdentifierValue($entity));
376388
}
377389

378390
return parent::createIndex($entity);
@@ -392,7 +404,7 @@ protected function createIndex($entity)
392404
protected function createValue($entity)
393405
{
394406
if ($this->idAsValue) {
395-
return (string) current($this->getIdentifierValues($entity));
407+
return (string) $this->getSingleIdentifierValue($entity);
396408
}
397409

398410
return parent::createValue($entity);
@@ -415,6 +427,36 @@ protected function fixIndex($index)
415427
return $index;
416428
}
417429

430+
/**
431+
* Get identifier information for a class.
432+
*
433+
* @param ClassMetadata $classMetadata The entity metadata
434+
*
435+
* @return array Return an array with idAsIndex, idAsValue and identifier
436+
*/
437+
private function getIdentifierInfoForClass(ClassMetadata $classMetadata)
438+
{
439+
$identifier = null;
440+
$idAsIndex = false;
441+
$idAsValue = false;
442+
443+
$identifiers = $classMetadata->getIdentifierFieldNames();
444+
445+
if (1 === count($identifiers)) {
446+
$identifier = $identifiers[0];
447+
448+
if (!$classMetadata->hasAssociation($identifier)) {
449+
$idAsValue = true;
450+
451+
if (in_array($classMetadata->getTypeOfField($identifier), array('integer', 'smallint', 'bigint'))) {
452+
$idAsIndex = true;
453+
}
454+
}
455+
}
456+
457+
return array($idAsIndex, $idAsValue, $identifier);
458+
}
459+
418460
/**
419461
* Loads the list with entities.
420462
*
@@ -438,6 +480,33 @@ private function load()
438480
$this->loaded = true;
439481
}
440482

483+
/**
484+
* Returns the first (and only) value of the identifier fields of an entity.
485+
*
486+
* Doctrine must know about this entity, that is, the entity must already
487+
* be persisted or added to the identity map before. Otherwise an
488+
* exception is thrown.
489+
*
490+
* @param object $entity The entity for which to get the identifier
491+
*
492+
* @return array The identifier values
493+
*
494+
* @throws RuntimeException If the entity does not exist in Doctrine's identity map
495+
*/
496+
private function getSingleIdentifierValue($entity)
497+
{
498+
$value = current($this->getIdentifierValues($entity));
499+
500+
if ($this->idClassMetadata) {
501+
$class = $this->idClassMetadata->getName();
502+
if ($value instanceof $class) {
503+
$value = current($this->idClassMetadata->getIdentifierValues($value));
504+
}
505+
}
506+
507+
return $value;
508+
}
509+
441510
/**
442511
* Returns the values of the identifier fields of an entity.
443512
*

‎src/Symfony/Bridge/Doctrine/Tests/Fixtures/SingleAssociationToIntIdEntity.php

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <[email protected]>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Bridge\Doctrine\Tests\Fixtures;
13+
14+
use Doctrine\ORM\Mapping\Id;
15+
use Doctrine\ORM\Mapping\Column;
16+
use Doctrine\ORM\Mapping\Entity;
17+
use Doctrine\ORM\Mapping\OneToOne;
18+
19+
/** @Entity */
20+
class SingleAssociationToIntIdEntity
21+
{
22+
/** @Id @OneToOne(targetEntity="SingleIntIdNoToStringEntity", cascade={"ALL"}) */
23+
protected $entity;
24+
25+
/** @Column(type="string", nullable=true) */
26+
public $name;
27+
28+
public function __construct(SingleIntIdNoToStringEntity $entity, $name)
29+
{
30+
$this->entity = $entity;
31+
$this->name = $name;
32+
}
33+
34+
public function __toString()
35+
{
36+
return (string) $this->name;
37+
}
38+
}

‎src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/AbstractEntityChoiceListSingleAssociationToIntIdTest.php

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <[email protected]>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Bridge\Doctrine\Tests\Form\ChoiceList;
13+
14+
use Symfony\Bridge\Doctrine\Tests\Fixtures\SingleAssociationToIntIdEntity;
15+
use Symfony\Bridge\Doctrine\Tests\Fixtures\SingleIntIdNoToStringEntity;
16+
use Symfony\Bridge\Doctrine\Form\ChoiceList\EntityChoiceList;
17+
18+
/**
19+
* Test choices generated from an entity with a primary foreign key.
20+
*
21+
* @author Premi Giorgio <[email protected]>
22+
* @author Bernhard Schussek <[email protected]>
23+
*/
24+
abstract class AbstractEntityChoiceListSingleAssociationToIntIdTest extends AbstractEntityChoiceListTest
25+
{
26+
protected function getEntityClass()
27+
{
28+
return 'Symfony\Bridge\Doctrine\Tests\Fixtures\SingleAssociationToIntIdEntity';
29+
}
30+
31+
protected function getClassesMetadata()
32+
{
33+
return array(
34+
$this->em->getClassMetadata($this->getEntityClass()),
35+
$this->em->getClassMetadata('Symfony\Bridge\Doctrine\Tests\Fixtures\SingleIntIdNoToStringEntity'),
36+
);
37+
}
38+
39+
protected function createChoiceList()
40+
{
41+
return new EntityChoiceList($this->em, $this->getEntityClass(), 'name');
42+
}
43+
44+
/**
45+
* @return \Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceListInterface
46+
*/
47+
protected function createObjects()
48+
{
49+
$innerA = new SingleIntIdNoToStringEntity(-10, 'inner_A');
50+
$innerB = new SingleIntIdNoToStringEntity(10, 'inner_B');
51+
$innerC = new SingleIntIdNoToStringEntity(20, 'inner_C');
52+
$innerD = new SingleIntIdNoToStringEntity(30, 'inner_D');
53+
54+
$this->em->persist($innerA);
55+
$this->em->persist($innerB);
56+
$this->em->persist($innerC);
57+
$this->em->persist($innerD);
58+
59+
return array(
60+
new SingleAssociationToIntIdEntity($innerA, 'A'),
61+
new SingleAssociationToIntIdEntity($innerB, 'B'),
62+
new SingleAssociationToIntIdEntity($innerC, 'C'),
63+
new SingleAssociationToIntIdEntity($innerD, 'D'),
64+
);
65+
}
66+
67+
protected function getChoices()
68+
{
69+
return array('_10' => $this->obj1, 10 => $this->obj2, 20 => $this->obj3, 30 => $this->obj4);
70+
}
71+
72+
protected function getLabels()
73+
{
74+
return array('_10' => 'A', 10 => 'B', 20 => 'C', 30 => 'D');
75+
}
76+
77+
protected function getValues()
78+
{
79+
return array('_10' => '-10', 10 => '10', 20 => '20', 30 => '30');
80+
}
81+
82+
protected function getIndices()
83+
{
84+
return array('_10', 10, 20, 30);
85+
}
86+
}

‎src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/AbstractEntityChoiceListTest.php

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ protected function setUp()
3939
$this->em = DoctrineTestHelper::createTestEntityManager();
4040

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

4444
try {
4545
$schemaTool->dropSchema($classes);
@@ -73,6 +73,11 @@ abstract protected function getEntityClass();
7373

7474
abstract protected function createObjects();
7575

76+
protected function getClassesMetadata()
77+
{
78+
return array($this->em->getClassMetadata($this->getEntityClass()));
79+
}
80+
7681
/**
7782
* @return \Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceListInterface
7883
*/

‎src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/LoadedEntityChoiceListSingleAssociationToIntIdTest.php

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <[email protected]>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Bridge\Doctrine\Tests\Form\ChoiceList;
13+
14+
/**
15+
* @author Premi Giorgio <[email protected]>
16+
* @author Bernhard Schussek <[email protected]>
17+
*/
18+
class LoadedEntityChoiceListSingleAssociationToIntIdTest extends AbstractEntityChoiceListSingleAssociationToIntIdTest
19+
{
20+
/**
21+
* @return \Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceListInterface
22+
*/
23+
protected function createChoiceList()
24+
{
25+
$list = parent::createChoiceList();
26+
27+
// load list
28+
$list->getChoices();
29+
30+
return $list;
31+
}
32+
}

‎src/Symfony/Bridge/Doctrine/Tests/Form/ChoiceList/UnloadedEntityChoiceListSingleAssociationToIntIdTest.php

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <[email protected]>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Bridge\Doctrine\Tests\Form\ChoiceList;
13+
14+
/**
15+
* @author Premi Giorgio <[email protected]>
16+
* @author Bernhard Schussek <[email protected]>
17+
*/
18+
class UnloadedEntityChoiceListSingleAssociationToIntIdTest extends AbstractEntityChoiceListSingleAssociationToIntIdTest
19+
{
20+
public function testGetIndicesForValuesIgnoresNonExistingValues()
21+
{
22+
$this->markTestSkipped('Non-existing values are not detected for unloaded choice lists.');
23+
}
24+
25+
/**
26+
* @group legacy
27+
*/
28+
public function testLegacyGetIndicesForValuesIgnoresNonExistingValues()
29+
{
30+
$this->markTestSkipped('Non-existing values are not detected for unloaded choice lists.');
31+
}
32+
}

0 commit comments

Comments
 (0)