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

Fixed pgsql lastInserteId #145

Closed
wants to merge 4 commits into from
Closed

Fixed pgsql lastInserteId #145

wants to merge 4 commits into from

Conversation

@lulco
Copy link
Contributor

lulco commented Oct 15, 2016

No description provided.

@lulco

This comment has been minimized.

Copy link
Contributor Author

lulco commented Oct 15, 2016

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

@@ -821,7 +821,7 @@ public function insert($data)
? implode('.', array_map([$this->context->getConnection()->getSupplementalDriver(), 'delimite'], explode('.', $tmp)))
: NULL
);
if ($primaryKey === FALSE) {
if ($primaryKey === FALSE || $primaryKey === "0") {

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Oct 15, 2016

Contributor

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

This comment has been minimized.

Copy link
@lulco

lulco Oct 16, 2016

Author Contributor

sure, fixed

@dg

This comment has been minimized.

Copy link
Member

dg commented Oct 17, 2016

What about simple if (!$primaryKey)?

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Oct 17, 2016

@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

This comment has been minimized.

Copy link
Contributor Author

lulco commented Oct 17, 2016

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

@dg

This comment has been minimized.

Copy link
Member

dg commented Oct 17, 2016

I dont like fix for specific DB in common class.

@lulco

This comment has been minimized.

Copy link
Contributor Author

lulco commented Oct 18, 2016

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 nette:master branch 12 times, most recently from 06e5f54 to 44a230d Oct 18, 2016
@dg

This comment has been minimized.

Copy link
Member

dg commented Oct 18, 2016

It seems that it is '' in mssql

@dg dg force-pushed the nette:master branch from 44a230d to 829b936 Oct 18, 2016
@dg dg closed this in 829b936 Oct 18, 2016
dg added a commit that referenced this pull request Oct 18, 2016
@hrach

This comment has been minimized.

Copy link
Contributor

hrach commented Oct 18, 2016

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

This comment has been minimized.

Copy link
Member

dg commented Oct 18, 2016

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

This comment has been minimized.

Copy link
Contributor

hrach commented Oct 18, 2016

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

This comment has been minimized.

Copy link
Member

dg commented Oct 18, 2016

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
Projects
None yet
4 participants
You can’t perform that action at this time.