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

[giosh94mhz]

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.

[webmozart]

You can shorten this to

@var ClassMetadata

as the class is already imported in the namespace.

[webmozart]

Thank you for the great PR! This looks good.

[giosh94mhz]

Thank you for your feedback! I've fixed everything you've have spotted.

I agree with you that mixing variable initialization and return value was not clean, but I fixed the initIdentifierAs in another way. I've renamed it to getIdentifierInfoForClass, and simply return an array of [idField, idAsValue, idAsIndex]; I think it's clearer this way what the method does (and can be reused, if ever), and the list operator make it clear when and how the state is updated.

[phansys]

Missing docblock for function param.

[giosh94mhz]

Thanks @phansys , I'll fix it.

[giosh94mhz]

Ok, I've applied all your suggestions. In addition there is a small comment to the new unit test class (if it's ok to have it, and is clear).

[webmozart]

I think you can paste the same code here as above instead of calling array_slice(), no? The ID field should remain the same.

For brevity, I still prefer a initIdentifierInfoForClass() method.

[giosh94mhz]

The array_slice call is required (or a similar approach obviously :), since when called the second time the returned identifier is the one from the associated entity. The field name may be different in the two entities, so it is important that only the type information are carried on, without the foreign name.

I can refactor the method as initIdentifierInfoForClass, but still I have to do something like:

$bakIdField = $this->idField;
$this->initIdentifierInfoForClass($this->idClassMetadata); 
$this->idField = $bakIdField;

or like:

$this->initIdentifierInfoForClass($this->idClassMetadata, false); 
// ...
private function getIdentifierInfoForClass(ClassMetadata $classMetadata, $initIdField)
// ...

Which one is clearer? I vote for the second one...

[webmozart]

I see. What about moving the $idField to the last position in the array then? Then you don't need array_slice().

[webmozart]

Thanks! As you may have seen, the EntityChoiceList was replaced by a DoctrineChoiceLoader in Symfony 2.7. The appropriate class to fix it there is probably IdReader. Could you open a second PR for the 2.7 branch that fixes the same issue for the new implementation?

[giosh94mhz]

I still have to dig into 2.7 sources, but I could give it try (since I have some spare time in the following days).

Anyway, should I fix this commit or throw it away since it will never be merged?

[webmozart]

@giosh94mhz No, this will be merged too, but into the 2.3 branch. The other one goes into the 2.7 branch so that we don't have a regression with the new functionality.

Check the IdReader class, it should be pretty familiar to you.

[giosh94mhz]

@webmozart That's good news! So I'll fix the 2.3 ASAP. For the 2.7 branch I'll check it in the next days (IdReader is familiar indeed).

[giosh94mhz]

Ok, I've swapped the returned array and removed the array_slice call. No notice or other is triggered so it may be good as is.

This was the quickest solution, but I'm open to rename it as initIdentifierInfoForClass if you think is still foggy.

[webmozart]

No, it's good now. Once the 2.7 PR is ready, I'll merge both in one go. Thanks! :)

[cursedcoder]

👍 @giosh94mhz I have the same issue using your bundle :)

[giosh94mhz]

@cursedcoder Thank you for the feedback. First, make sure you checked out the branch entity_choice_list_with_primary_key_association (not master), then check if your code have a chain of foreign key defined in Doctrine. If this is the case, you may remove the chained entities (e.g. A[id], B[a_id==A.id], C[b_id==A.id==B.id]) with direct association (e.g. A[id], B[a_id==A.id], C[a_id==A.id]).
Otherwise, can you add a failing test?

[cursedcoder]

@giosh94mhz I'm not sure I understand you.

Just saying when using geonames bundle country entity I found the same bug.

[giosh94mhz]

@cursedcoder aaah sorry. I though you found a case in which this fix didn't work by using my branch. GeonamesBundle didn't cross my mind (and definitely that bundle deserves some more love from me :)

[fabpot]

Thank you @giosh94mhz.