Encapsulate \PDOStatement instead of extending from it

Interesting idea, overall it's nice to be less bound to PDO and mimic less its interfaces

In a long run - PDO is not thread safe so other implementations (native, swoole) drivers could be better choice

Quick review

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -247,6 +247,16 @@ public function __construct(\PDO $connection, array $connection_options) {
   +   * @return mixed
   +   *   The client-level database connection, for example \PDO.
   +   */
   +  public function getClientConnection() {
   +    return $this->connection;
   

   I bet it always object|null

2. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -69,6 +70,68 @@ class Connection extends DatabaseConnection {
   +    if (isset($connection_options['transactions'])) {
   +      @trigger_error('Passing a \'transactions\' connection option to Drupal\Core\Database\Connection::__construct is deprecated in drupal:9.1.0 and is removed in drupal:10.0.0. All database drivers must support transactions. See https://www.drupal.org/node/2278745', E_USER_DEPRECATED);
   ...
   +  public function prepare($statement, array $driver_options = []) {
   +    @trigger_error('Connection::prepare() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Database drivers should instantiate \PDOStatement objects by calling \PDO::prepare in their Collection::prepareStatement method instead. \PDO::prepare should not be called outside of driver code. See https://www.drupal.org/node/3137786', E_USER_DEPRECATED);
   +    throw new DatabaseExceptionWrapper('Connection::prepare() is deprecated');
   
   +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -63,10 +63,35 @@ class Connection extends DatabaseConnection {
   +    if (isset($connection_options['transactions'])) {
   +      @trigger_error('Passing a \'transactions\' connection option to Drupal\Core\Database\Connection::__construct is deprecated in drupal:9.1.0 and is removed in drupal:10.0.0. All database drivers must support transactions. See https://www.drupal.org/node/2278745', E_USER_DEPRECATED);
   +      unset($connection_options['transactions']);
   
   @@ -77,6 +102,22 @@ public function __construct(\PDO $connection, array $connection_options) {
   +  public function prepare($statement, array $driver_options = []) {
   +    @trigger_error('Connection::prepare() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Database drivers should instantiate \PDOStatement objects by calling \PDO::prepare in their Collection::prepareStatement method instead. \PDO::prepare should not be called outside of driver code. See https://www.drupal.org/node/3137786', E_USER_DEPRECATED);
   +    throw new DatabaseExceptionWrapper('Connection::prepare() is deprecated');
   
   +++ b/core/lib/Drupal/Core/Database/Statement.php
   @@ -2,6 +2,8 @@
   +@trigger_error(__CLASS__ . '() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Database drivers should use or extend StatementWrapper instead, and encapsulate client-level statement objects. See https://www.drupal.org/node/TODO', E_USER_DEPRECATED);
   
   @@ -12,6 +14,12 @@
   + * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Database
   + *   drivers should use or extend StatementWrapper instead, and encapsulate
   ...
   + * @see https://www.drupal.org/node/TODO
   

   both needs deprecation tests

3. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -69,6 +70,68 @@ class Connection extends DatabaseConnection {
   +   * Destructs a MySql Connection object.
   +   */
   +  public function __destruct() {
   +    if ($this->needsCleanup) {
   +      $this->nextIdDelete();
   +    }
   +    $this->schema = NULL;
   +    $this->connection = NULL;
   
   @@ -218,16 +281,6 @@ public function serialize() {
   -   * {@inheritdoc}
   -   */
   -  public function __destruct() {
   -    if ($this->needsCleanup) {
   -      $this->nextIdDelete();
   -    }
   -    parent::__destruct();
   

   Why it's not calling parent destructor?
   Also it could use to change existing method to minify diff

4. +++ b/core/lib/Drupal/Core/Database/StatementWrapper.php
   @@ -0,0 +1,274 @@
   +   * Reference to the database connection object for this statement.
   ...
   +   * @var \Drupal\Core\Database\Connection
   ...
   +  public $dbh;
   ...
   +   * Is rowCount() execution allowed.
   ...
   +   * @var bool
   ...
   +  public $allowRowCount = FALSE;
   

   Why this public?

5. +++ b/core/lib/Drupal/Core/Database/StatementWrapper.php
   @@ -0,0 +1,274 @@
   +
   +  public function __construct(Connection $connection, string $query, array $options) {
   

   needs docs

Also for TODO link

1. @mondrake thanks for working on this. It looks very promising to me

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1743,7 +1755,10 @@ public function commit() {
   -    return $this->connection->prepare($statement, $driver_options);
   +    if (!is_a($this->statementClass, StatementWrapper::class, TRUE)) {
   +      return $this->connection->prepare($statement, $driver_options);
   +    }
   +    throw new DatabaseExceptionWrapper('Connection::prepare() is deprecated.');
   

   This looks a bit awkward. If we're using the StatementWrapper class shouldn't the deprecated path but to return $statement?

3. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -47,6 +47,11 @@ class Connection extends DatabaseConnection {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected $statementClass = StatementWrapper::class;
   

   I don't think we should use the statementClass system for this. I think we should add a new protected property called $statementWrapper instead.

1. +++ b/core/lib/Drupal/Core/Database/StatementWrapper.php
   @@ -0,0 +1,367 @@
   +  /**
   +   * Returns the client-level database statement object.
   +   *
   +   * @return object
   +   *   The client-level database statement, for example \PDOStatement.
   +   */
   +  public function getClientStatement() {
   

   Do you think we should document that this should only be called from the database driver?

2. +++ b/core/lib/Drupal/Core/Database/StatementWrapper.php
   @@ -0,0 +1,367 @@
   +  public function bindColumn($column, &$param, int $type = 0, int $maxlen = 0, $driverdata = NULL): bool {
   

   How come this is here? As far as I can see there are no calls to it (i.e. 0 test coverage) - is this here for completeness?

RE #29.3 I think the wrapper thing should be separate from $statementClass... ie. looking at the patch:

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -535,7 +545,9 @@ public function prepareStatement(string $query, array $options): StatementInterf
-    return $this->connection->prepare($query, $options['pdo'] ?? []);
+    return is_a($this->statementClass, StatementWrapper::class, TRUE) ?
+    new $this->statementClass($this, $query, $options['pdo'] ?? []) :
+      $this->connection->prepare($query, $options['pdo'] ?? []);

Should look something like:

return $this->statementWrapperClass ?
  new $this->statementWrapperClass($this, $query, $options['pdo'] ?? []) :
  $this->connection->prepare($query, $options['pdo'] ?? []);

This would allow $statementClass to continue working as it does today and separate it from this new statement wrapper logic. And we'd use a new protected $statementWrapperClass instead.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1743,7 +1755,10 @@ public function commit() {
     @trigger_error('Connection::prepare() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Database drivers should instantiate \PDOStatement objects by calling \PDO::prepare in their Collection::prepareStatement method instead. \PDO::prepare should not be called outside of driver code. See https://www.drupal.org/node/3137786', E_USER_DEPRECATED);
-    return $this->connection->prepare($statement, $driver_options);
+    if (!is_a($this->statementClass, StatementWrapper::class, TRUE)) {
+      return $this->connection->prepare($statement, $driver_options);
+    }
+    throw new DatabaseExceptionWrapper('Connection::prepare() is deprecated.');

I guess by #29.2 what I'm trying to ask is "Is throwing an exception the correct behaviour here?"

Re #32/#30.2 - I think testing using mocks of this isn't really enough. I think we need to test calling the methods for real because that's the risky thing.

++ to #47 PDO has extensive testing in PHP itself, probably it needs follow-up to review current test suite

This looks very encouraging, still need to do a thorough review but +1 to the general approach.

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -247,6 +262,18 @@ public function __construct(\PDO $connection, array $connection_options) {
   +  /**
   +   * Returns the client-level database connection object.
   +   *
   +   * This method should normally be used only within database driver code.
   +   *
   +   * @return object
   +   *   The client-level database connection, for example \PDO.
   +   */
   +  public function getClientConnection() {
   +    return $this->connection;
   +  }
   
   +++ b/core/lib/Drupal/Core/Database/StatementWrapper.php
   @@ -0,0 +1,369 @@
   +  /**
   +   * Constructs a StatementWrapper object.
   +   *
   +   * @param \Drupal\Core\Database\Connection $connection
   +   *   Database connection object.
   +   * @param string $query
   +   *   The SQL query string.
   +   * @param array $options
   +   *   Array of query options.
   +   */
   +  public function __construct(Connection $connection, string $query, array $options) {
   +    $this->dbh = $connection;
   +    $this->clientStatement = $connection->getClientConnection()->prepare($query, $options);
   +    $this->setFetchMode(\PDO::FETCH_OBJ);
   +  }
   

   I think that we should inject the statement into the wrapper instead of the client connection. Or maybe inject the client connection if you want. But it'd be great to not expose a public API for getting the client connection.

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -38,6 +39,16 @@ class Connection extends DatabaseConnection {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected $statementClass = NULL;
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected $statementWrapperClass = StatementWrapper::class;
   

   Ah I see now why the patch originally chose to use $statementClass

   I wonder if I've over complicated things. Is there a use-case for both a wrapper and a statement class.

   Not sure. Hmmm.

@mondrake re #50.1 because we've not added that so far so it is a unnecessary API widening at this point. We can add it if someone has a case for it - but I'd rather we keep the new API to a minimum here.

+++ b/core/lib/Drupal/Core/Database/StatementWrapper.php
@@ -32,15 +34,17 @@ class StatementWrapper implements \IteratorAggregate, StatementInterface {
+  public function __construct(Connection $connection, $client_connection, string $query, array $options) {
     $this->dbh = $connection;
-    $this->clientStatement = $connection->getClientConnection()->prepare($query, $options);
+    $this->clientStatement = $client_connection->prepare($query, $options);
     $this->setFetchMode(\PDO::FETCH_OBJ);
   }

I get the feeling this is tightly coupled to PDO-ness. See the use of PDO constants. If your DB uses something other than a PDOStatement then I think you'd need a different wrapper. For that reason I'm unsure of why we'd not inject the \PDOStatement here and not the client connection.

#56 & #57 is solid reasoning. And I agree the ideally the abstract implementation of Connection would be PDO agnostic - that said it really is not right now - no harm in not make it worse though.

I tested this patch manually on Drupal 9.2-dev (as of today) on PHP 8. I tested with installing Umami and then randomly clicking through the site and in admin panel. No errors reported anywhere. Some screenshots in a Twitter thread at https://twitter.com/hussainweb/status/1317449063533600770 and the test code is at https://github.com/hussainweb/drupal-php8-composer2.

Drafted CR https://www.drupal.org/node/3177488 and follow-up for TODO #3177490: Remove \Drupal\Core\Database\Connection::$statementClass

Re-roll and fix for CR and TODO links

There are some things that need to be fixed

Checking changed files...
CSPELL: checking all files
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/StatementWrapper.php:294:18 - Unknown word (maxlen)
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/StatementWrapper.php:296:20 - Unknown word (driverdata)
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/StatementWrapper.php:308:68 - Unknown word (maxlen)
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/StatementWrapper.php:308:81 - Unknown word (driverdata)
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/StatementWrapper.php:318:76 - Unknown word (maxlen)
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/StatementWrapper.php:321:76 - Unknown word (maxlen)
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/StatementWrapper.php:321:85 - Unknown word (driverdata)
/Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Database/StatementWrapper.php:336:32 - Unknown word (INOUT)
CSpell: Files checked: 12, Issues found: 8 in 1 files

FILE: ...rupal8alt.dev/core/lib/Drupal/Core/Database/StatementWrapper.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
----------------------------------------------------------------------
  54 | ERROR | [x] The text '@deprecated in drupal:9.1.0 and is
     |       |     removed in drupal:10.0.0.' does not match the
     |       |     standard format: @deprecated in
     |       |     %deprecation-version% and is removed from
     |       |     %removal-version%. %extra-info%.
  68 | ERROR | [x] The text '@deprecated in drupal:9.1.0 and is
     |       |     removed in drupal:10.0.0.' does not match the
     |       |     standard format: @deprecated in
     |       |     %deprecation-version% and is removed from
     |       |     %removal-version%. %extra-info%.
 302 | ERROR | [ ] The text '@deprecated StatementWrapper::bindColumn
     |       |     should not be called in drupal:9.1.0 and will
     |       |     error in drupal:10.0.0. Access the client-level
     |       |     statement object via ::getClientStatement().' does
     |       |     not match the standard format: @deprecated in
     |       |     %deprecation-version% and is removed from
     |       |     %removal-version%. %extra-info%.
 349 | ERROR | [ ] The text '@deprecated StatementWrapper::bindParam
     |       |     should not be called in drupal:9.1.0 and will
     |       |     error in drupal:10.0.0. Access the client-level
     |       |     statement object via ::getClientStatement().' does
     |       |     not match the standard format: @deprecated in
     |       |     %deprecation-version% and is removed from
     |       |     %removal-version%. %extra-info%.
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 1.88 secs; Memory: 10MB

Fixing the above issues and changing deprecated text to read that this will be deprecated in Drupal 9.2.0 instead of 9.1.0.

I missed the errors from cspell earlier, but how should we go about fixing it? These parameter names match the names in PHP and I think it is okay to just add the underscore to make it recognisable. OTOH, I can't think of what to replace `INOUT` with.

For the cspell stuff we can add

// cSpell:ignore maxlen driverdata INOUT

to the file. With PHP 8.0 parameter names are now part of the API!!!

Remaining 2 cs errors looks related to #3137883: Deprecate passing a StatementInterface object to Connection::query

+++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
@@ -604,6 +605,18 @@ public function testNamespaceDefault() {
+  /**
+   * Tests deprecation of the Statement class.
+   *
+   * @group legacy
+   */
+  public function testStatementDeprecation() {
+    $this->expectDeprecation('\Drupal\Core\Database\Statement is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Database drivers should use or extend StatementWrapper instead, and encapsulate client-level statement objects. See https://www.drupal.org/node/3177488');
+    $mock_statement = $this->getMockBuilder(Statement::class)
+      ->disableOriginalConstructor()
+      ->getMock();
+  }

This will fail in PHP 8 - because the class is not compatible with it at all. So we should add

    if (PHP_VERSION_ID >= 80000) {
      $this->markTestSkipped('Drupal\Core\Database\Statement is incompatible with PHP 8.0.');
    }

Addressed #71

With a link to remove

Can someone address #69

Fix for #69 and CS

I wonder what we should be doing with the other things that implement\Drupal\Core\Database\StatementInterface - i.e. \Drupal\Core\Database\StatementPrefetch, \Drupal\Core\Database\Driver\sqlite\Statement and \Drupal\Core\Database\StatementEmpty.

This looks really good, resolves a longstanding complaint with the database layer which has finally bit us, but turned out to be a scratch rather than fatal.

One nit:

+++ b/core/lib/Drupal/Core/Database/StatementWrapper.php
@@ -0,0 +1,377 @@
+
+  /**
+   * The Drupal database connection object.
+   *
+   * @var \Drupal\Core\Database\Connection
+   */
+  public $dbh;
+

Nit: could we use a more self-descriptive property name? This is only used once in the class to get the logger. Could just be $connection?

@mondrake hopefully that's the way it works out. I agree that in D10 we should have a good look at the StatementInterface and decouple it a bit from \PDOStatement.

Updating issue summary and change record.

Didn't mean to change the status (yet)

Fair enough in #79, let's open a follow-up.

Discussed with @catch - we want to land this in 9.1.0 so changing deprecation notices - which weren't actually consistent with their @trigger_error's anyway.

Removing incorrect hunks added in #75

I also fixed a bug in \Drupal\Core\Database\StatementWrapper::fetch() not passing the correct args. Obvs we don't actually use this code in core.

Fixing StubConnection to not use the deprecated \Drupal\Core\Database\Connection::$statementClass and also properly deprecating that property and testing said deprecation.

I think this is now rtbc.

• We have passing test on all db drivers (the mariadb fail is a random JS fail)
• We have passing tests against PHP 8.0
• We have a change record and release note

I think I'm eligible to rtbc as 99% of the work here is @mondrake's and my two recent patches are tidy up as I result of code review.

This looks ready to go to me, and good to get this in prior to the 8.1.x alpha in case something unexpected comes up - most of the remaining PHP8 compatibility changes after this one are comparatively much smaller.

Committed f67095e and pushed to 9.1.x. Thanks!