Decouple Connection from the wrapped PDO connection to allow alternative clients

Looks great, just one question about ability to override lastInsertId() from driver side

For the unresolved thread on the MR.

\PDO::FETCH_* maybe just replace'em with some drupalism? curious how dbal/sqlserv doing as they "not bound" to PDO

doctrine/dbal removed the use of those constants in public facing APIs, and pushed it to internals of their PDO drivers implementations.

Doing this in a followup is to me a very good idea.

The MR is for me RTBC.

All code changes look good to me.
I have updated the IS.
I have added a CR.
For me it is RTBC.

This looks very good to me, a lot less changes than I was expecting.

Tagging for framework manager review because it'd be worth a second set of committer eyes (or at least, if another committer doesn't get to it, to remind me to review it again properly before commit).

I had to change the Connection::open() method to abstract and fix StubConnection, because the method was missing a return statement which violates PHPStan-0.

This is to me a bit of a stupid reason to change the method to an abstract one, but fine. The 2 contrib database drivers at the moment are overriding the method, so the change is not a problem to me.

I found one nitpick, but for the rest it is RTBC for me.

@mondrake: You are right.

Back to RTBC.

This looks like a really nice clean up. I read all the comments and code changes and left a comment on the one that I think can improved.

Committed 617d96e and pushed to 10.0.x. Thanks!

I think we should backport this change to 9.5.x because there are no real API changes here and having 9.5.x and 10.0.x aligned is helpful.

The reroll for D9.5 looks good to me.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -967,7 +969,7 @@ public function query($query, array $args = [], $options = []) {
         case Database::RETURN_INSERT_ID:
           $sequence_name = $options['sequence_name'] ?? NULL;
-          return $this->connection->lastInsertId($sequence_name);
+          return $this->lastInsertId($sequence_name);

@@ -1264,10 +1266,6 @@ public function insert($table, array $options = []) {
-   *
-   * @see \PDO::lastInsertId
-   *
-   * @internal
    */
   public function lastInsertId(?string $name = NULL): string {

TBH I think it was fine for this to remain @internal. Nothing from outside a DB driver module should ever call this. And I think DB driver modules are fine to call through to their expected client. I agree that the Connection class should call its own method though.

Going to proceed here with the commit but I think it would be fine to open a follow-up to add @internal back and also to other places in the current DB API that we don't think should be called by anything external to the DB API or a driver module.

Committed 9093210 and pushed to 9.5.x. Thanks!

I'm not going to publish the CR until we've had a discussion about #34

Looks to me like the backport broke the HEAD of 9.5.x-dev for any PHP < 8.1.
(https://www.drupal.org/node/3060/qa)

Example: https://www.drupal.org/pift-ci-job/2388374

Testing Drupal\KernelTests\Core\Database\DatabaseExceptionWrapperTest
...E...                                                             7 / 7 (100%)

Time: 00:06.400, Memory: 4.00 MB

There was 1 error:

1) Drupal\KernelTests\Core\Database\DatabaseExceptionWrapperTest::testPrepareStatementFailOnExecution
ReflectionException: Cannot access non-public member Drupal\mysql\Driver\Database\mysql\Connection::$connection

/var/www/html/core/tests/Drupal/KernelTests/Core/Database/DatabaseExceptionWrapperTest.php:75
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

I've reverted this on 9.5.x so we can fix this here.

Testbot is not happy.

Failure on PHP 7.3 on ComposerIntegrationTest is being fixed here: #3282050: Failing HEAD on PHP 7.3 on ComposerIntegrationTest::testComposerLockHash other 2 seem like the "usual random" JS test failures.

Ther testbot failures are not related to this patch.

Committed 0c5c369 and pushed to 9.5.x. Thanks!

Updated the CR to show that this was introduced in 9.5 and published the CR.