Skip to content

Fixed pgsql lastInserteId#145

Closed
lulco wants to merge 4 commits into
nette:masterfrom
lulco:pgsql_last_insert_id
Closed

Fixed pgsql lastInserteId#145
lulco wants to merge 4 commits into
nette:masterfrom
lulco:pgsql_last_insert_id

Conversation

@lulco

@lulco lulco commented Oct 15, 2016

Copy link
Copy Markdown
Contributor

No description provided.

@lulco

lulco commented Oct 15, 2016

Copy link
Copy Markdown
Contributor Author

In PHP 7.0.11 Postgres PDO lastInsertId() was changed: http://php.net/ChangeLog-7.php#7.0.11
It returns "0" instead of false

#hacktoberfest

Comment thread src/Database/Table/Selection.php Outdated
: NULL
);
if ($primaryKey === FALSE) {
if ($primaryKey === FALSE || $primaryKey === "0") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the string should be in single quotes, i.e. $primaryKey === '0'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sure, fixed

@dg

dg commented Oct 17, 2016

Copy link
Copy Markdown
Member

What about simple if (!$primaryKey)?

@JanTvrdik

JanTvrdik commented Oct 17, 2016

Copy link
Copy Markdown
Contributor

@dg That's less obvious. I think the current solution is better.


Even better (again for clarity) might be to add comment or split the condition based on PHP_VERSION_ID or both, e.g.

if ($primaryKey === '0' && (PHP_VERSION_ID < 70011 && $primaryKey === FALSE)) { // php issue #72633

@lulco

lulco commented Oct 17, 2016

Copy link
Copy Markdown
Contributor Author

So? Which alternative will be accepted and merged? I am OK with all of them, just need this fix.

@dg

dg commented Oct 17, 2016

Copy link
Copy Markdown
Member

I dont like fix for specific DB in common class.

@lulco

lulco commented Oct 18, 2016

Copy link
Copy Markdown
Contributor Author

Well, I just realize that it is not just pgsql problem. $this->context->getInsertId() returns '0' also with MySql DB if you have non-autoincrement primary key. And in MySql DB it is happening also in previous PHP versions.

This part of code is executed:

$row = $this->createSelectionInstance()
            ->select('*')
            ->wherePrimary($primaryKey)
            ->fetch();

And returns FALSE, because the executed query is: ... WHERE primary_key = '0' ...

In this query is the only driver-specific problem with pgsql uuid type, because it throws exception:
Nette\Database\DriverException: SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for uuid: "0". The exception in query that shouldn't be executed at all. Definitelly not with $primaryKey = '0'.

@dg dg force-pushed the master branch 12 times, most recently from 06e5f54 to 44a230d Compare October 18, 2016 12:57
@dg

dg commented Oct 18, 2016

Copy link
Copy Markdown
Member

It seems that it is '' in mssql

@hrach

hrach commented Oct 18, 2016

Copy link
Copy Markdown
Contributor

This seems to be a wrong fix; personally, I have a db with table with primay value 0 (yes, big bad design, but that's the current state) . The getLastIndsertedId() should be fixed in the proper layer (Driver/Connection), shouldn't be?

@dg

dg commented Oct 18, 2016

Copy link
Copy Markdown
Member

So '0' can be missing primary and correct primary? I don't know how to differentiate between them. The only solution I can see is to not support primary value '0'.

@hrach

hrach commented Oct 18, 2016

Copy link
Copy Markdown
Contributor

You can definitely insert '0' into a PK column in MySQL. I'm ok with not-supporting the PK '0', but the unification may be done in PgSQLDriver.

@dg

dg commented Oct 18, 2016

Copy link
Copy Markdown
Member

MySQL returns '0' when primary key is missing. It seems that condition in Selection was wrong.

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.

4 participants