-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
#2648 fix last insert id consistency #2765
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also deprecate the SQLSrv\LastInsertId
in favor of the new one?
return $this->value; | ||
} | ||
|
||
public function set(string $value) : void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for renaming this to register()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me. Do we have anything producing empty strings though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} 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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to also check the error code? Otherwise, the DBAL may be suppressing irrelevant exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* 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>. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The license header is no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll run the entire thing through diffcs anyway
return parent::query($args[0], $args[1]); | ||
$stmt = parent::query($args[0], $args[1]); | ||
|
||
$this->trackLastInsertId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think our CS doesn't even allow that anymore, but I'll check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still doesn't seem a valid reason to duplicate code this way. Variadic arguments may work here too,
ab81cf0
to
b7e559d
Compare
…hardcoded value
A pull request from the |
b7e559d
to
bd9bdd1
Compare
The PDO failures and deadlocks are fixed. However, the newly introduced test is now failing in some configurations (Postges, MariaDB) with:
Looks like a connection resource leak in the test. |
Closing in favor of #3074. Travis stopped building this PR as PR and only builds it as a branch. |
…rever holes have been forgotten
…rever holes have been forgotten
…rever holes have been forgotten
…rever holes have been forgotten
…rever holes have been forgotten
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?