[GH-8043] Make IdentityFunction handle primary keys when they are part of an association #8288
Conversation
|
Any news on this? |
| if (isset($targetEntity->fieldMappings[$this->fieldMapping])) { | ||
| $columnName = $targetEntity->fieldMappings[$this->fieldMapping]['columnName']; | ||
| } elseif (isset($targetEntity->associationMappings[$this->fieldMapping])) { | ||
| $columnName = reset($targetEntity->associationMappings[$this->fieldMapping]['joinColumns'])['name']; |
There was a problem hiding this comment.
I think reset() is here because we don't support IDENTITY with composite keys?
There was a problem hiding this comment.
In that case could we throw an exception if there are more than one key?
There was a problem hiding this comment.
Maybe, not sure why it wasn't done like that in the first place. Right now, it will only return the value of the first part of the composite key, right?
There was a problem hiding this comment.
Hm from https://www.doctrine-project.org/projects/doctrine-orm/en/2.7/reference/dql-doctrine-query-language.html#:~:text=the%20IDENTITY()%20DQL I read
the IDENTITY() DQL function also works for composite primary keys
So there must be another reason 🤔
|
The bottom of https://www.doctrine-project.org/projects/doctrine-orm/en/2.7/reference/dql-doctrine-query-language.html#dql-select-examples shows some addition for every version change. This can be extended for this PR. |
|
Hi everyone I’m back 👋 I changed the target branch to |
|
@SenseException I feel like my PR is a bugfix so I’m not sure if I should add an example in the documentation? |
Bugfix or not, I think an example always worth :) |
|
The documentation already has an example for
Should I add |
|
An addition that IdentityFunction can handle primary keys now would be sufficient. |
|
@SenseException this PR allows
Now that it really works what could be to add? |
MatTheCat
changed the title
|
@greg0ire I added a test and rebased |
Fix #8043
When primary keys are part of an association they won’t be found in
fieldMappingsbut inassociationMappingsso I refactoredIdentityFunctionto handle this case.I’m not completely sure about the logic (especially the
resetwhich I copied from line 61) but it worked for every case I thought of.