Throw exception from lastInsertId() if there is no identity value

[BenMorel]

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

Summary

Supersedes #3759 and #3368.

Proposed changes

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

• getLastInsertId() : int|string
• getSequenceNumber(string $name): int|string

Tests have been extended to cover the following scenarios:

• getLastInsertId() on supported platforms
• getLastInsertId() on supported platforms, on a fresh connection, i.e. with no ID available (NoIdentityValue exception)
• getLastInsertId() on unsupported platforms (IdentityColumnsNotSupported exception)
• getSequenceNumber() with a known sequence name on supported platforms
• getSequenceNumber() with an unknown sequence name on supported platforms (SequenceDoesNotExist exception)
• getSequenceNumber() on unsupported platforms (SequencesNotSupported exception)

[BenMorel]

@morozov Ready for discussion!

[BenMorel]

@morozov I'd like to come back to this. Before diving again in this PR, should I retarget 3.0.x instead of master?

[morozov]

I'd like to come back to this.

That would be awesome!

Before diving again in this PR, should I retarget 3.0.x instead of master?

3.0.0 is already released.

[BenMorel]

3.0.0 is already released.

Oh wow, I didn't expect it anytime soon. I had quite a few things I would have liked to propose for 3.0, but looks like they'll have to wait for 4.0 now. Any rough ETA on this one? I mean, can we expect it in a reasonable timeframe like a year from now, or will we have to wait for multiple years like Doctrine 2 to 3?

[BenMorel]

@morozov Ready for review again. The main points that you may want to have a look at are:

• PDO driver-specific checks in Driver\PDO\Connection::getLastInsertId() and getSequenceNumber(), rather than creating a decorator for each PDO driver just to throw an exception
• PGSQL-specific exception checks that we cannot move to the ExceptionConverter without modifying its signature somehow to accept a context (the same error code can mean 2 different things depending on the operation that was being performed when it was thrown); I think that adding these 2 checks right in the Driver\PDO\Connection class is a pragmatic way to avoid adding unnecessary complexity to the ExceptionConverter
• Check for underlying driver exception in tests, as you don't want to introduce wrapper-level exceptions for identity / sequence errors, but do want to introduce driver-level exceptions to be used in tests
• settle on whether an exception should be thrown when no identity value / sequence number is available (Throw exception from lastInsertId() if there is no identity value #4324 (comment))
• and finally naming: I suggest using getLastInsertId() instead of lastInsertId(), which is already reflected in the code

[morozov]

@BenMorel thanks for the update. I've been short on time for OSS lately and I cannot promise I'll look at it immediately but I'll do as soon as I can.

[morozov]

@BenMorel, just FYI, I squashed and rebased your PR to accommodate the changes from #4691. This way, the scope of this PR boils down to changing the return type of lastInsertId() (we could probably leave the method unchanged) and improving error handling.

I have a couple more ideas on improving the code, will document later.

[morozov]

@BenMorel I did some more cleanup, and with the reduced scope, I believe it's good to go. The part about exceptions could still use some improvement but it's orthogonal to this patch, but it's orthogonal to this patch and should be taken care of by #3166.

[morozov]

Thanks for the patch!