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

[WIP] [3.0] Extract getSequenceNumber() from lastInsertId() #3368

Closed
wants to merge 106 commits into from

Conversation

BenMorel
Copy link
Contributor

Work In Progress: this requires #3335 and #3367 to be merged first. Their commits are included here; this PR actually starts at commit 69b6f3d.

I'm opening this PR early to be able to run CI tests on all platforms, and will rebase it later.

Q A
Type improvement
BC Break yes
Fixed issues N/A

Summary

While talking about Driver\Connection::lastInsertId(), @morozov suggested the idea that:

(...) the calls with and without the $name are targeted at the same method but have totally different semantics (named sequences and identity columns). This confusing API was originally borrowed from PDO but we don't have to follow it strictly.

Proposed changes

This PR splits the existing lastInsertId(?string $name = null) : string method into 2 methods:

  • lastInsertId() : string
  • getSequenceNumber(string $name) : string

Naming is a first draft, suggestions welcome (getLastInsertId() for consistency?)

Tests have been extended to cover the following scenarios:

  • lastInsertId() on supported platforms
  • lastInsertId() on unsupported platforms (exception)
  • lastInsertId() on all platforms, on a fresh connection, i.e. with no ID available (exception)
  • getSequenceNumber() with a known sequence name on supported platforms
  • getSequenceNumber() with an unknown sequence name on all platforms (exception)

guilhermeblanco and others added 30 commits November 24, 2018 18:04
…having to replicate the \PDOStatement interface in ResultStatement
@BenMorel
Copy link
Contributor Author

BenMorel commented Nov 3, 2019

@morozov Same here, has this been implemented elsewhere?

@morozov
Copy link
Member

morozov commented Nov 3, 2019

No. GitHub closed this automatically.

@morozov morozov reopened this Nov 4, 2019
@morozov morozov changed the base branch from develop to master November 4, 2019 14:35
@morozov
Copy link
Member

morozov commented Nov 4, 2019

@BenMorel are you planning to work on this? This is definitely what I'd like to see in 3.0.

@BenMorel
Copy link
Contributor Author

BenMorel commented Dec 1, 2019

@morozov I'm trying to rebase the relevant commits onto master. There's quite some work do to, but I should get there. We'll see how it goes.

@BenMorel
Copy link
Contributor Author

BenMorel commented Dec 1, 2019

@morozov Please see this new PR: #3759.
There are a few tests failing with some of the drivers, I'll try to fix them tomorrow.

@BenMorel BenMorel closed this Dec 1, 2019
@morozov morozov removed this from the 4.0.0 milestone Mar 19, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants