From 6aab22375b182b5dcfc3218ba29f27ecc6c78bf9 Mon Sep 17 00:00:00 2001
From: Alex Pott <alex.a.pott@googlemail.com>
Date: Sun, 29 Nov 2020 00:17:47 +0000
Subject: [PATCH] Issue #3137883 by mondrake, Pooja Ganjage, andypost, paulocs,
anmolgoyal74, daffie, alexpott, catch: Deprecate passing a StatementInterface
object to Connection::query
---
core/lib/Drupal/Core/Database/Connection.php | 21 +++++----
.../Core/Database/Driver/pgsql/Insert.php | 43 ++++++++++++-------
.../Core/Database/Driver/pgsql/Update.php | 5 ++-
.../Core/Database/Driver/pgsql/Upsert.php | 2 +-
.../Core/Database/ConnectionTest.php | 31 +++++++++++++
5 files changed, 75 insertions(+), 27 deletions(-)
diff --git a/core/lib/Drupal/Core/Database/Connection.php b/core/lib/Drupal/Core/Database/Connection.php
index 8d86059aeacc..ff172b5a6c44 100644
--- a/core/lib/Drupal/Core/Database/Connection.php
+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -749,11 +749,12 @@ protected function filterComment($comment = '') {
* statements.
*
* @param string|\Drupal\Core\Database\StatementInterface|\PDOStatement $query
- * The query to execute. In most cases this will be a string containing
- * an SQL query with placeholders. 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.
+ * The query to execute. This is a string containing an SQL query with
+ * placeholders.
+ * (deprecated) An already-prepared instance of StatementInterface or of
+ * \PDOStatement may also be passed in order to allow calling code to
+ * manually bind variables to a query. In such cases, the content of the
+ * $args array will be ignored.
* It is extremely rare that module code will need to pass a statement
* object to this method. It is used primarily for database drivers for
* databases that require special LOB field handling.
@@ -792,14 +793,16 @@ public function query($query, array $args = [], $options = []) {
assert(!isset($options['target']), 'Passing "target" option to query() has no effect. See https://www.drupal.org/node/2993033');
try {
- // We allow either a pre-bound statement object or a literal string.
- // In either case, we want to end up with an executed statement object,
- // which we pass to PDOStatement::execute.
+ // We allow either a pre-bound statement object (deprecated) or a literal
+ // string. In either case, we want to end up with an executed statement
+ // object, which we pass to PDOStatement::execute.
if ($query instanceof StatementInterface) {
+ @trigger_error('Passing a StatementInterface object as a $query argument to ' . __METHOD__ . ' is deprecated in drupal:9.2.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);
$stmt = $query;
$stmt->execute(NULL, $options);
}
elseif ($query instanceof \PDOStatement) {
+ @trigger_error('Passing a \\PDOStatement object as a $query argument to ' . __METHOD__ . ' is deprecated in drupal:9.2.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);
$stmt = $query;
$stmt->execute();
}
@@ -878,6 +881,8 @@ protected function handleQueryException(\PDOException $e, $query, array $args =
// Wrap the exception in another exception, because PHP does not allow
// overriding Exception::getMessage(). Its message is the extra database
// debug information.
+ // @todo in Drupal 10, remove checking if $query is a statement object.
+ // @see https://www.drupal.org/node/3154439
if ($query instanceof StatementInterface) {
$query_string = $query->getQueryString();
}
diff --git a/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php b/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
index bbab92babfac..c7b67f9fa00f 100644
--- a/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Insert.php
@@ -3,6 +3,8 @@
namespace Drupal\Core\Database\Driver\pgsql;
use Drupal\Core\Database\Database;
+use Drupal\Core\Database\DatabaseExceptionWrapper;
+use Drupal\Core\Database\IntegrityConstraintViolationException;
use Drupal\Core\Database\Query\Insert as QueryInsert;
// cSpell:ignore nextval setval
@@ -84,18 +86,10 @@ public function execute() {
}
}
- // PostgreSQL requires the table name to be specified explicitly
- // when requesting the last insert ID, so we pass that in via
- // the options array.
- $options = $this->queryOptions;
-
- if (!empty($table_information->sequences)) {
- $options['sequence_name'] = $table_information->sequences[0];
- }
- // If there are no sequences then we can't get a last insert id.
- elseif ($options['return'] == Database::RETURN_INSERT_ID) {
- $options['return'] = Database::RETURN_NULL;
- }
+ // PostgreSQL requires the table name to be specified explicitly when
+ // requesting the last insert ID. If there are no sequences, then we can't
+ // get a last insert id.
+ $sequence_name = $table_information->sequences[0] ?? NULL;
// Create a savepoint so we can rollback a failed query. This is so we can
// mimic MySQL and SQLite transactions which don't fail if a single query
@@ -103,14 +97,31 @@ public function execute() {
// example, \Drupal\Core\Cache\DatabaseBackend.
$this->connection->addSavepoint();
try {
+ $stmt->execute(NULL, $this->queryOptions);
// 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 (!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 {
+ $last_insert_id = NULL;
+ }
+ }
+ $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') {
+ throw new IntegrityConstraintViolationException($message, $e->getCode(), $e);
}
else {
- $last_insert_id = $this->connection->query($stmt, [], $options);
+ throw new DatabaseExceptionWrapper($message, 0, $e->getCode());
}
- $this->connection->releaseSavepoint();
}
catch (\Exception $e) {
$this->connection->rollbackSavepoint();
diff --git a/core/lib/Drupal/Core/Database/Driver/pgsql/Update.php b/core/lib/Drupal/Core/Database/Driver/pgsql/Update.php
index 7e49310175a3..8a3d918c9096 100644
--- a/core/lib/Drupal/Core/Database/Driver/pgsql/Update.php
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Update.php
@@ -75,9 +75,10 @@ public function execute() {
$this->connection->addSavepoint();
try {
- $result = $this->connection->query($stmt, [], $options);
+ $stmt->execute(NULL, $options);
$this->connection->releaseSavepoint();
- return $result;
+ $stmt->allowRowCount = TRUE;
+ return $stmt->rowCount();
}
catch (\Exception $e) {
$this->connection->rollbackSavepoint();
diff --git a/core/lib/Drupal/Core/Database/Driver/pgsql/Upsert.php b/core/lib/Drupal/Core/Database/Driver/pgsql/Upsert.php
index 4aa51e588589..c5ffacdddd78 100644
--- a/core/lib/Drupal/Core/Database/Driver/pgsql/Upsert.php
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Upsert.php
@@ -80,7 +80,7 @@ public function execute() {
// example, \Drupal\Core\Cache\DatabaseBackend.
$this->connection->addSavepoint();
try {
- $this->connection->query($stmt, [], $options);
+ $stmt->execute(NULL, $options);
$this->connection->releaseSavepoint();
}
catch (\Exception $e) {
diff --git a/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php b/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php
index c92ed677642d..ff840e19b47d 100644
--- a/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php
@@ -6,6 +6,7 @@
use Drupal\Core\Database\Database;
use Drupal\Core\Database\DatabaseExceptionWrapper;
use Drupal\Core\Database\Query\Condition;
+use Drupal\Core\Database\StatementWrapper;
/**
* Tests of the core database system.
@@ -134,6 +135,36 @@ public function testTransactionsOptionDeprecation() {
$this->assertTrue($foo_connection->supportsTransactions());
}
+ /**
+ * Tests the deprecation of passing a statement object to ::query.
+ *
+ * @group legacy
+ */
+ public function testStatementQueryDeprecation(): void {
+ $this->expectDeprecation('Passing a StatementInterface object as a $query argument to Drupal\Core\Database\Connection::query is deprecated in drupal:9.2.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');
+ $db = Database::getConnection();
+ $stmt = $db->prepareStatement('SELECT * FROM {test}', []);
+ $this->assertNotNull($db->query($stmt));
+ }
+
+ /**
+ * Tests the deprecation of passing a PDOStatement object to ::query.
+ *
+ * @group legacy
+ */
+ public function testPDOStatementQueryDeprecation(): void {
+ $db = Database::getConnection();
+ $stmt = $db->prepareStatement('SELECT * FROM {test}', []);
+ if (!$stmt instanceof StatementWrapper) {
+ $this->markTestSkipped("This test only runs for db drivers using StatementWrapper.");
+ }
+ if (!$stmt->getClientStatement() instanceof \PDOStatement) {
+ $this->markTestSkipped("This test only runs for PDO-based db drivers.");
+ }
+ $this->expectDeprecation('Passing a \\PDOStatement object as a $query argument to Drupal\Core\Database\Connection::query is deprecated in drupal:9.2.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');
+ $this->assertNotNull($db->query($stmt->getClientStatement()));
+ }
+
/**
* Ensure that you cannot execute multiple statements on MySQL.
*/
--
GitLab