#2648 fix last insert id consistency by Ocramius · Pull Request #2765 · doctrine/dbal

Ocramius · 2017-07-02T11:40:25Z

Replaces #2648

This patch is rebased, includes PHP 7.1 hints, and shows the failures that we already saw in #2648.

@deeky666 are those failures expected?

morozov

Should we also deprecate the SQLSrv\LastInsertId in favor of the new one?

morozov · 2017-07-04T18:19:28Z

lib/Doctrine/DBAL/Driver/LastInsertId.php

+        return $this->value;
+    }
+
+    public function set(string $value) : void


Based on the logic inside, can we name it register() instead? The set() name may be misleading in the way that setting a '0' value doesn't actually set it and doesn't produce any error message.

I see the current contract in 2.x is Connection::lastInsertId() : string, but besides BC, is there a reason the ID is a string? Does the DBAL support any sequences which produce non-numeric values? If not, then it would make sense to me to store the value as int and return a string from the function call. Otherwise, most adapters have to cast their values to (string).

I don't remember the specific details here, but we settled for a string identifier because auto-generation may really produce anything. I don't think we built a test case for it though.

👍 for renaming this to register()

I believe I inspected all supported platforms one by one and didn't find anything else than integers in their API. I believe we should try switching back to integers and proving that they work across platforms. A hypothetically existing platform which is not compliant with this API is not really an excuse. Is it?

Fine by me. Do we have anything producing empty strings though?

If we expect integers in the domain but some drivers return them as strings (which is a regular thing), we'll have to cast them to an (int), so an empty string will be cast to a 0 which we can handle in a special way. So there's no concern in this regard.

morozov · 2017-07-04T18:29:23Z

lib/Doctrine/DBAL/Driver/OCI8/OCI8Connection.php

+        } catch (OCI8Exception $exception) {
+            // In case no sequence value is available yet, we get the error:
+            // ORA-08002: sequence $name.CURRVAL is not yet defined in this session
+            return '0';


Would it make sense to also check the error code? Otherwise, the DBAL may be suppressing irrelevant exceptions.

Not sure if we can do that in a reliable way without starting to hit false-positives. The API is a bit too squishy to rely on precise codes.

morozov · 2018-03-29T03:42:04Z

@Ocramius @deeky666 Let me know if you need help rebasing this. Looks like the rebase will be less painful if squashed first. Please also see my questions above.

morozov · 2018-03-29T03:45:11Z

lib/Doctrine/DBAL/Driver/LastInsertId.php

+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the MIT license. For more information, see
+ * <http://www.doctrine-project.org>.
+ */


The license header is no longer needed.

I'll run the entire thing through diffcs anyway

morozov · 2018-03-29T03:49:24Z

lib/Doctrine/DBAL/Driver/PDOConnection.php

-                return parent::query($args[0], $args[1]);
+                $stmt = parent::query($args[0], $args[1]);
+
+                $this->trackLastInsertId();


Do these lines need to be duplicated? If the case with $args[0] is wrapped in the else block, you can use a single return statement and call to $this->trackLastInsertId(); prior to that.

I think our CS doesn't even allow that anymore, but I'll check

Still doesn't seem a valid reason to duplicate code this way. Variadic arguments may work here too,

…cting

…hardcoded value

morozov · 2018-03-30T05:50:37Z

A pull request from the doctrine repository triggers two builds per build service: one for the pull request and the other for the branch itself. @Ocramius can you disable building arbitrary branches on Travis and Appveyor?

morozov · 2018-03-31T18:03:19Z

The PDO failures and deadlocks are fixed. However, the newly introduced test is now failing in some configurations (Postges, MariaDB) with:

1) Doctrine\Tests\DBAL\Functional\LastInsertIdTest::testLastInsertIdAfterSelectQuery
Exception: [Doctrine\DBAL\Exception\ConnectionException] An exception occurred in driver: SQLSTATE[08006] [7] FATAL:  sorry, too many clients already
FATAL:  sorry, too many clients already

Looks like a connection resource leak in the test.

morozov · 2018-03-31T18:35:48Z

Closing in favor of #3074. Travis stopped building this PR as PR and only builds it as a branch.

…rever holes have been forgotten

Ocramius added Bug Drivers Improvement labels Jul 2, 2017

Ocramius added this to the 2.7 milestone Jul 2, 2017

Ocramius assigned deeky666 Jul 2, 2017

Ocramius requested review from deeky666, lcobucci and morozov July 2, 2017 11:40

Ocramius mentioned this pull request Jul 2, 2017

Fix last insert ID consistency accross drivers #2648

Closed

morozov reviewed Jul 4, 2017

View reviewed changes

Ocramius mentioned this pull request Jul 7, 2017

No exception thrown while insert if the sequence is not found using postgresql doctrine/orm#6327

Open

morozov reviewed Mar 29, 2018

View reviewed changes

deeky666 added 3 commits March 29, 2018 22:18

make last insert id behaviour consistent accross drivers

ed9e7b3

fix tearing down functional tests that have been skipped before conne…

527ebf0

…cting

fix last insert ID tests

006dbe0

morozov force-pushed the fix/#2648-fix-last-insert-id-consistency branch from ab81cf0 to b7e559d Compare March 30, 2018 05:29

deeky666 and others added 12 commits March 29, 2018 22:37

new approach (WIP)

4060f69

let's see what's left to do

4893893

update PDOStatement todo

9ad6db9

fix oci8 connection test

e543754

make LastInsertId container class final

b12c9ae

simplify oci8 last insert retrieval

d169866

use LastInsertId container object in PDO connection

bafcca3

refactor last insert ID extension points in PDO connection

56add9a

fix code style

cdad005

#2648 adding return hints and parameter hints to newly introduced API

ef19f5b

#2648 adding type hints to the LastInsertId class

414f84e

#2648 deprecated LAST_INSERT_ID_SQL class constant

806dc46

Ocramius and others added 5 commits March 29, 2018 22:37

#2648 removed nullability of the lastInsertId private property

45ea224

#2648 wrap ERRMODE_EXCEPTION restoring in a finally block

5af7279

#2648 restoring original error mode instead of overwriting it with a …

079d037

…hardcoded value

#2648 Renamed set() to register()

f98a9f3

#2648 Replaced duplicated code with variadic argument

bd9bdd1

morozov force-pushed the fix/#2648-fix-last-insert-id-consistency branch from b7e559d to bd9bdd1 Compare March 30, 2018 05:54

morozov added 2 commits March 29, 2018 23:18

#2648 Temporarily disabled the test causing a dead lock on PDO drivers

7dbdceb

#2648 Fixed failures on PDO drivers

71b1239

morozov mentioned this pull request Mar 31, 2018

Fix last insert ID consistency across drivers #3074

Closed

5 tasks

morozov closed this Mar 31, 2018

morozov deleted the fix/#2648-fix-last-insert-id-consistency branch March 31, 2018 18:36

morozov removed this from the 2.7.0 milestone Mar 31, 2018

Ocramius added a commit to Ocramius/dbal that referenced this pull request Apr 2, 2018

doctrine#3074 doctrine#2765 Applying CS and adding lacking types whe…

e9daa7b

…rever holes have been forgotten

Ocramius mentioned this pull request Apr 2, 2018

#3074 #2765 Applying CS and adding lacking types wherever holes have been forgotten morozov/dbal#1

Merged

Ocramius added a commit to Ocramius/dbal that referenced this pull request Apr 3, 2018

doctrine#3074 doctrine#2765 Applying CS and adding lacking types whe…

b05053c

…rever holes have been forgotten

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 3, 2018

doctrine#3074 doctrine#2765 Applying CS and adding lacking types whe…

eb85408

…rever holes have been forgotten

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 17, 2018

doctrine#3074 doctrine#2765 Applying CS and adding lacking types whe…

f8e1b97

…rever holes have been forgotten

morozov pushed a commit to morozov/dbal that referenced this pull request Apr 17, 2018

doctrine#3074 doctrine#2765 Applying CS and adding lacking types whe…

46371ea

…rever holes have been forgotten

github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2022

Uh oh!

Conversation

Ocramius commented Jul 2, 2017

Uh oh!

morozov left a comment

Choose a reason for hiding this comment

Uh oh!

morozov Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov commented Mar 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov commented Mar 30, 2018

Uh oh!

morozov commented Mar 31, 2018

Uh oh!

morozov commented Mar 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

morozov Jul 4, 2017 •

edited

Loading

morozov Jul 4, 2017 •

edited

Loading

morozov Mar 29, 2018 •

edited

Loading

morozov Mar 29, 2018 •

edited

Loading