Deprecate passing a StatementInterface object to Connection::query

#2345451: Introduce a Connection::prepareStatement method allowing to pass options to the Statement, deprecate Connection::prepareQuery() and Connection::prepare() has landed.

1. Why do we add the trigger_error() to Drupal\Core\Database\Driver\mysql\Connection::query() and Drupal\Core\Database\Driver\pgsql\Connection::query()? Is the one in Drupal\Core\Database\Connection::query() not enough?
2. AFAIK we only have one simple test for getting the last inserted ID. We are doing some strange stuff with the last inserted ID in Drupal\Core\Database\Driver\pgsql\Insert::execute(). Can we add some testing for this?

3. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
   @@ -101,15 +103,30 @@ public function execute() {
   +        $exception = new IntegrityConstraintViolationException($message, $e->getCode(), $e);
   ...
   +        $exception = new DatabaseExceptionWrapper($message, 0, $e);
   
   +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Update.php
   @@ -75,9 +75,10 @@ public function execute() {
   +      $stmt->allowRowCount = TRUE;
   

   Can we add some testing for the exceptions being thrown here.

4. We also need deprecation message testing.

Updated the IS and created an CR.

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -745,6 +745,7 @@ public function query($query, array $args = [], $options = []) {
   +        @trigger_error('Passing a StatementInterface object as a $query argument to ' . __METHOD__ . ' is deprecated in drupal:9.TODO.0 and is removed in drupal:10.0.0. Call the execute method from the StatementInterface object directly instead. See https://www.drupal.org/node/TODO', E_USER_DEPRECATED);
   

   Fix the TODO parts.

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1015,6 +1016,32 @@ public function insert($table, array $options = []) {
   +   * If the PDO driver does not support this capability, \PDO::lastInsertId()
   +   * triggers an IM001 SQLSTATE.
   ...
   +  public function lastInsertId(?string $name = NULL): string {
   

   Should we not add a @throws annotation to the docblock?

3. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
   @@ -101,15 +103,30 @@ public function execute() {
   +    catch (\PDOException $e) {
   +      $this->connection->rollbackSavepoint();
   +      $message = $e->getMessage() . ": " . $stmt->getQueryString();
   +      // Match all SQLSTATE 23xxx errors.
   +      if (substr($e->getCode(), -6, -3) == '23') {
   +        $exception = new IntegrityConstraintViolationException($message, $e->getCode(), $e);
   +      }
   +      else {
   +        $exception = new DatabaseExceptionWrapper($message, 0, $e);
   +      }
   +      throw $exception;
   +    }
   

   Is this part PostgreSQL specific or does this belong in the method Connection::lastInsertId()? Just asking.

@mondrake: Thank you for your explanation. If you could do #19.1, then it is RTBC for me.

All looks good to me.
All my remarks are addressed.
For me it is RTBC.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
@@ -101,15 +103,30 @@ public function execute() {
       $this->connection->releaseSavepoint();
     }
+    catch (\PDOException $e) {
+      $this->connection->rollbackSavepoint();
+      $message = $e->getMessage() . ": " . $stmt->getQueryString();
+      // Match all SQLSTATE 23xxx errors.
+      if (substr($e->getCode(), -6, -3) == '23') {
+        $exception = new IntegrityConstraintViolationException($message, $e->getCode(), $e);
+      }
+      else {
+        $exception = new DatabaseExceptionWrapper($message, 0, $e);
+      }
+      throw $exception;
+    }

Is this actually necessary in order to deprecate passing a statement, or is it a bug found while working on the patch?

Is this change actually worth it? In PHP 8.0 we'll get the ability to do string|StatementInterface as a typehint.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1015,6 +1016,32 @@ public function insert($table, array $options = []) {
+  public function lastInsertId(?string $name = NULL): string {

Adding this has it's own risks and adds new API in order to deprecate other API - which seems odd.

+++ b/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php
@@ -134,6 +134,18 @@ public function testTransactionsOptionDeprecation() {
+   * @expectedDeprecation Passing a StatementInterface object as a $query argument to Drupal\Core\Database\Connection::query is deprecated in drupal:9.1.0 and is removed in drupal:10.0.0. Call the execute method from the StatementInterface object directly instead. See https://www.drupal.org/node/3154439

needs re-roll for https://www.drupal.org/node/3176667

NW for #30

New API is a way to get rid of getClientConnection() (btw why not getDriverConnection)

Sequences API mostly unused by both core and contrib so there's at least #2665216: Deprecate Drupal\Core\Database\Connection::nextId() and the {sequences} table and schema

And even https://www.php.net/manual/ru/sqlite3.lastinsertrowid.php non-PDO drivers implement it

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1015,6 +1016,32 @@ public function insert($table, array $options = []) {
+   * @return string
+   *   The value returned by the wrapped PDO connection.
...
+   * @see \PDO::lastInsertId
...
+  public function lastInsertId(?string $name = NULL): string {
+    return $this->connection->lastInsertId($name);

Better to mark it @internal as return value may flux depending on driver

PHP 8 defines it as @return string|false
https://github.com/php/php-src/blob/php-8.0.0RC2/ext/pdo/pdo_dbh.stub.ph...

Also looking at implementation it may throw https://github.com/php/php-src/blob/php-8.0.0RC2/ext/pdo/pdo_dbh.c#L955-... but returns FALSE

Fix for #30 and re-roll

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1046,6 +1047,34 @@ public function insert($table, array $options = []) {
   +   * If the PDO driver does not support this capability, \PDO::lastInsertId()
   +   * triggers an IM001 SQLSTATE.
   

   Should we not move this text to a @throws in the docblock of this method?

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
   @@ -103,15 +105,30 @@ public function execute() {
   +        $exception = new DatabaseExceptionWrapper($message, 0, $e);
   

   Why do we not add the $e->getCode() DatabaseExceptionWrapper call?

3. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
   @@ -103,15 +105,30 @@ public function execute() {
   +      if (substr($e->getCode(), -6, -3) == '23') {
   +        $exception = new IntegrityConstraintViolationException($message, $e->getCode(), $e);
   +      }
   +      else {
   +        $exception = new DatabaseExceptionWrapper($message, 0, $e);
   +      }
   +      throw $exception;
   

   Can we change the construction to (for better readability):

        if (substr($e->getCode(), -6, -3) == '23') {
          throw new IntegrityConstraintViolationException($message, $e->getCode(), $e);
        }
        else {
          throw new DatabaseExceptionWrapper($message, 0, $e);
        }
   

4. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -776,6 +776,7 @@ public function query($query, array $args = [], $options = []) {
   +        @trigger_error('Passing a StatementInterface object as a $query argument to ' . __METHOD__ . ' is deprecated in drupal:9.1.0 and is removed in drupal:10.0.0. Call the execute method from the StatementInterface object directly instead. See https://www.drupal.org/node/3154439', E_USER_DEPRECATED);
   
   +++ b/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php
   @@ -134,6 +134,18 @@ public function testTransactionsOptionDeprecation() {
   +    $this->expectDeprecation('Passing a StatementInterface object as a $query argument to Drupal\Core\Database\Connection::query is deprecated in drupal:9.1.0 and is removed in drupal:10.0.0. Call the execute method from the StatementInterface object directly instead. See https://www.drupal.org/node/3154439');
   

   We are now working on 9.2.0 and not with 9.1.0

For #29: If we want to use the construction parameter type hint with string|StatementInterface we shall have to wait for A Drupal version that only supports PHP 8.0 or higher. Personally I do not want to wait for that. Just my opinion. :)

Hi,

Creating a patch as per the #38 comment suggestion.

Please review the patch.

Thanks.

@Pooja Ganjage can you please provide interdiff as well.

Attached interdiff for #39 patch.

@Pooja: All your changes look good. Only point #38.3 is still open. After that is done, will I give it a RTBC.

Hi,

@daffie, I have not understand the point of #38.3, what need to change?

Please give me suggestion for this. So, I can create a patch again.

Thanks.

Fixing code standard.
Point #38.3 is still open.

Hi,

Updated patch with mentioned changes into the #38.3 point.

Please review the patch.

Thanks.

Attached interdiff for #47 patch.

I reviewed the patch from Pooja Ganjage. Just one point to fix:

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1078,6 +1079,36 @@ public function insert($table, array $options = []) {
+   * @throws IM001 SQLSTATE
+   *    If the PDO driver does not support this capability, \PDO::lastInsertId()
+   *    triggers an IM001 SQLSTATE.
+   *
+   * @return string|false
+   *   The value returned by the wrapped PDO connection.

According to the Drupal coding standards the tag @returns comes before @throws. See: https://www.drupal.org/docs/develop/standards/api-documentation-and-comm.... Almost RTBC.

@paulocs: I did not review your patch, because the other patch has comment #38.3 fixed.

I agree with @mondrake. Lets remove the @throws from the docblock.

Removed the @throws from the docblock.

The patch does what is in the IS.
There is deprecation testing.
The CR has the deprecation and the API addition in it.
All changes in the patch look good to me.
For me it is RTBC.

Updated patch as per the #55 comment suggestion.

Updated patch.

Attached interdiff for #58 patch.

The patch does not what @mondrake has asked for in comment #57. Therefor back to needs work.

The docblock for lastInsertId looks good to me.
Back to RTBC.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
@@ -103,15 +105,29 @@ public function execute() {
     try {
+      $stmt->execute(NULL, $options);
       // Only use the returned last_insert_id if it is not already set.
-      if (!empty($last_insert_id)) {
-        $this->connection->query($stmt, [], $options);
+      // PostgreSQL requires the table name to be specified explicitly when
+      // requesting the last insert ID.
+      if ($options['return'] === Database::RETURN_INSERT_ID && isset($options['sequence_name'])) {
+        $last_insert_id = $this->connection->lastInsertId($options['sequence_name']);
       }
       else {
-        $last_insert_id = $this->connection->query($stmt, [], $options);
+        $last_insert_id = NULL;
       }
       $this->connection->releaseSavepoint();
     }

I think this needs to match the previous logic. And not overwrite the $last_insert_id when it is already set.

Like the comment is // Only use the returned last_insert_id if it is not already set. is still there but with the current logic it is not working.

Something like

    try {
      $stmt->execute(NULL, $options);
      // Only use the returned last_insert_id if it is not already set.
      // PostgreSQL requires the table name to be specified explicitly when
      // requesting the last insert ID.
      if (empty($last_insert_id) && ['return'] === Database::RETURN_INSERT_ID && isset($options['sequence_name'])) {
        $last_insert_id = $this->connection->lastInsertId($options['sequence_name']);
      }
      $this->connection->releaseSavepoint();
    }

I'm not sure about adding API to remove API. But also I'm wondering if it is really needed?

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -836,7 +837,7 @@ public function query($query, array $args = [], $options = []) {
-          return $this->connection->lastInsertId($sequence_name);
+          return $this->lastInsertId($sequence_name);

This is the only place where the new method is used and I'm not sure it is needed. Is it really necessary to add Connection::lastInsertId() in this issue?

What we can do in postgres is do a query :) that's what return $this->connection->lastInsertId($sequence_name); does anyway - see https://github.com/php/php-src/blob/master/ext/pdo_pgsql/pgsql_driver.c#...

Looking at https://stackoverflow.com/questions/2944297/postgresql-function-for-last... - there's we can do something like SELECT currval('sequence_name');

Or maybe even better we can use the RETURNING id thing because we're constructing the query here to. And there we can avoid the extra query completely :)

@mondrake I think we should decouple the new API from deprecating.

The simplest thing to do here seems to be to do the SELECT currval('sequence_name'); in the postgres insert

We can then open a followup about deprecating Database::RETURN_INSERT_ID and replacing that with a new method on the connection object.

We can also open a follow-up to explore the possible performance improvements of using RETURNING id

@mondrake: Maybe the problem is that there are to many quotes in the following line:

$last_insert_id = $this->query("SELECT currval('{$options['sequence_name']}')")->fetchField();

I have updated the CR.

Could we change the following code:

$last_insert_id = $this->connection->query("SELECT currval('{$options['sequence_name']}')")->fetchField();

to:

$sequence_name = $options['sequence_name'];
$last_insert_id = $this->connection->query("SELECT currval('$sequence_name')")->fetchField();

For better readability.
The rest of the patch is for me RTBC.

Updated the IS.

The patch looks good now.
For me it is RTBC.

@mondrake: Thank you.

I tried leaving a code review on the merge request. Looking at the comment above we need to integrate this a bit better. Here's the a link to the comment - https://git.drupalcode.org/project/drupal/-/merge_requests/74#note_5067 and also this sets this back to needs work.

A further tidy up can be done.

Patch looks good. Just 2 minor remarks.

1.    * @param string|\Drupal\Core\Database\StatementInterface|\PDOStatement $query
      *   The query to execute. This is a string containing an SQL query
      *   with placeholders.
      *   (deprecated) An already-prepared instance of StatementInterface may
      *   also be passed in order to allow calling code to manually bind
      *   variables to a query. If a StatementInterface is passed, the $args
      *   array will be ignored.
   

   We are not only deprecating StatementInterface input, but also PDOStatement input. Can we mentioning that in the docblock.

2.       // We allow either a pre-bound statement object (deprecated) or a literal string.
   

   Nitpick: This line goes over the 80 character limit.

The patch now looks great.
For me it is RTBC.

I agree this patch looks good now - waiting for the test runs before committing. Noticed a couple of extra things that we need to file issues for:

• \Drupal\Core\Database\Driver\pgsql\Upsert::execute() should return the last insert ID but atm it returns TRUE!
• The return value docs for \Drupal\Core\Database\Query\Query::execute() leave quite a bit to be desired. What is return depends upon the query type.

I couldn't find an issue to the upsert one and I didn't look for the other one.

I assigned issue credit for patch review based on issue comments.

Committed 6aab223 and pushed to 9.2.x. Thanks!

diff --git a/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php b/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
index 60cb01f247..c7b67f9fa0 100644
--- a/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
@@ -103,6 +103,7 @@ public function execute() {
       // requesting the last insert ID.
       if (!isset($last_insert_id)) {
         if ($this->queryOptions['return'] === Database::RETURN_INSERT_ID && $sequence_name !== NULL) {
+          // cspell:ignore currval
           $last_insert_id = $this->connection->query("SELECT currval('$sequence_name')")->fetchField();
         }
         else {

I added an ignore for currval for cspell as this failed the check.

Filed a follow-up issue #3210071: DatabaseExceptionWrapper has incorrect arguments in Insert as the arguments passed in this line, throw new DatabaseExceptionWrapper($message, 0, $e->getCode());, aren't correct.