Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Connection::getLastGeneratedValue should throw exception when unable to get an Id #343

Open
2 tasks done
rjd22 opened this issue Nov 14, 2018 · 2 comments
Open
2 tasks done

Comments

@rjd22
Copy link

rjd22 commented Nov 14, 2018

When trying to get and Id for a record by using the Connection::getLastGeneratedValue method you would expect that an exception is thrown when trying to do something that is not possible or when the database fails to return an ID because this is an exceptional situation.

But at this moment the method returns null when you use it incorrectly or false when getting the ID from the database fails combined with PHP type system this can cause new errors that are hard to trace.

Code to reproduce the issue

Example combined with PostgreSQL database.

$connection = Zend\Db\Adapter\Driver\Pdo\Connection;

$connection->getLastGeneratedValue(); // will always return null

$connection->getLastGeneratedValue('incorrect_seq_name'); // will always return false

Expected results

At this moment the method is not defensive enough and it is easy to make mistakes with it. I would expect it to throw an exception that will explain what is going wrong. It's possible that an migration goes wrong and a sequence is not created causing this code to always return false what can be a valid value.

@0x3175633435
Copy link

0x3175633435 commented Dec 11, 2018

Since primary keys are not allowed to be null I really believe that null as the last generated value is not something acceptable, at least in MySQL SELECT LAST_INSERT_ID(); would return zero if no data was ever inserted, so I believe it's not returning null or false in MySQL scenarios.

In the Postgres case, it's very annoying for someone used to implement zend-db with MySQL to get this behavior for the first time, I myself have spent some hours trying to understand what was going on at the first time this happened to me.

This null or false behavior should really not exists.

@weierophinney
Copy link
Member

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#25.

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

No branches or pull requests

3 participants