Skip to content
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

fix primary key type after save #133

Merged
merged 1 commit into from
Jan 12, 2017

Conversation

nipaTCM
Copy link
Collaborator

@nipaTCM nipaTCM commented Jan 12, 2017

No description provided.

@coveralls
Copy link

coveralls commented Jan 12, 2017

Coverage Status

Coverage increased (+0.005%) to 95.409% when pulling 08867d4 on nipaTCM:fixPrimaryKeyType into 1584c51 on thecodingmachine:4.2.

@moufmouf moufmouf merged commit 4d50560 into thecodingmachine:4.2 Jan 12, 2017
@@ -765,7 +765,10 @@ public function save(AbstractTDBMObject $object)

if (!$isPkSet && count($primaryKeyColumns) == 1) {
$id = $this->connection->lastInsertId();
$primaryKeys[$primaryKeyColumns[0]] = $id;
$pkColumn = $primaryKeyColumns[0];
// lastInsertId returns a string but the column type is usually a int. Let's convert it back to the correct type.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ne serait-il pas intéressant de faire une PR sur DBAL pour que lastInserId() retourne l'id dans le bon type?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

En fait, je crois qu'ils ne le font pas pour une raison de perf (de la même manière, ils ne convertissent pas automatiquement les résultats fetchés de la base de donnée et c'est à l'utilisateur de le faire à la main si il le souhaite).
Je crois que la question leur a déjà été posée et qu'ils ont dit qu'ils ne le feraient pas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants