-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use the correct types for generated ids #6152
Use the correct types for generated ids #6152
Conversation
According to the conversion rules of a specific DBAL mapping type.
@@ -890,7 +887,14 @@ private function persistNew($class, $entity) | |||
$idValue = $idGen->generate($this->em, $entity); | |||
|
|||
if ( ! $idGen instanceof \Doctrine\ORM\Id\AssignedGenerator) { | |||
$idValue = array($class->identifier[0] => $idValue); | |||
$idField = $class->identifier[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just digged into this conversion in depth, and it seems wrong. The AssignedGenerator
already uses a PHP value internally, and never produces an SQL value. This code can therefore be dropped, and test code needs to be re-validated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nvm, applies to non-assigned generators -.- Misread the !
in front of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then it's a bit confusing... should this conversion be generators' responsibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be, but can't refactor it now, since generators have very little access to external scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it 😉
@lcobucci merged and closed all related issues, thanks a lot! Note: I added a few additional commits to just extract the common code. |
This merges #5935 and #6020 in order to solve #5684.