From 6950cac3d4f73cd73d34473d846bc79dca0378c7 Mon Sep 17 00:00:00 2001
From: catch <catch@35733.no-reply.drupal.org>
Date: Fri, 15 Sep 2023 20:09:04 +0100
Subject: [PATCH] Issue #3384960 by mondrake, daffie, longwave: Strengthen
TransactionManager
---
core/lib/Drupal/Core/Database/Connection.php | 2 +-
core/lib/Drupal/Core/Database/Transaction.php | 17 ++-
.../Core/Database/Transaction/StackItem.php | 26 ++++
.../Transaction/TransactionManagerBase.php | 129 ++++++++++++------
.../TransactionManagerInterface.php | 18 ++-
core/misc/cspell/dictionary.txt | 1 +
.../DriverSpecificTransactionTestBase.php | 24 ++++
7 files changed, 170 insertions(+), 47 deletions(-)
create mode 100644 core/lib/Drupal/Core/Database/Transaction/StackItem.php
diff --git a/core/lib/Drupal/Core/Database/Connection.php b/core/lib/Drupal/Core/Database/Connection.php
index 67dc4a1a1a4b..97b528c25bed 100644
--- a/core/lib/Drupal/Core/Database/Connection.php
+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1480,7 +1480,7 @@ public function startTransaction($name = '') {
public function rollBack($savepoint_name = 'drupal_transaction') {
@trigger_error(__METHOD__ . '() is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Do not rollback the connection, roll back the Transaction objects instead. See https://www.drupal.org/node/3381002', E_USER_DEPRECATED);
if ($this->transactionManager()) {
- $this->transactionManager()->rollback($savepoint_name);
+ $this->transactionManager()->rollback($savepoint_name, 'bc-force-rollback');
return;
}
if (!$this->inTransaction()) {
diff --git a/core/lib/Drupal/Core/Database/Transaction.php b/core/lib/Drupal/Core/Database/Transaction.php
index 47069baafbaa..062dd177d09e 100644
--- a/core/lib/Drupal/Core/Database/Transaction.php
+++ b/core/lib/Drupal/Core/Database/Transaction.php
@@ -34,6 +34,11 @@ class Transaction {
* A boolean value to indicate whether this transaction has been rolled back.
*
* @var bool
+ *
+ * @deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. There is
+ * no replacement.
+ *
+ * @see https://www.drupal.org/node/3381002
*/
protected $rolledBack = FALSE;
@@ -47,7 +52,11 @@ class Transaction {
*/
protected $name;
- public function __construct(Connection $connection, $name = NULL) {
+ public function __construct(
+ Connection $connection,
+ $name = NULL,
+ protected readonly string $id = '',
+ ) {
if ($connection->transactionManager()) {
$this->connection = $connection;
$this->name = $name;
@@ -76,11 +85,12 @@ public function __construct(Connection $connection, $name = NULL) {
public function __destruct() {
if ($this->connection->transactionManager()) {
- $this->connection->transactionManager()->unpile($this->name);
+ $this->connection->transactionManager()->unpile($this->name, $this->id);
return;
}
// Start of BC layer.
// If we rolled back then the transaction would have already been popped.
+ // @phpstan-ignore-next-line
if (!$this->rolledBack) {
// @phpstan-ignore-next-line
$this->connection->popTransaction($this->name);
@@ -107,10 +117,11 @@ public function name() {
*/
public function rollBack() {
if ($this->connection->transactionManager()) {
- $this->connection->transactionManager()->rollback($this->name);
+ $this->connection->transactionManager()->rollback($this->name, $this->id);
return;
}
// Start of BC layer.
+ // @phpstan-ignore-next-line
$this->rolledBack = TRUE;
// @phpstan-ignore-next-line
$this->connection->rollBack($this->name);
diff --git a/core/lib/Drupal/Core/Database/Transaction/StackItem.php b/core/lib/Drupal/Core/Database/Transaction/StackItem.php
new file mode 100644
index 000000000000..30b9126e7ca1
--- /dev/null
+++ b/core/lib/Drupal/Core/Database/Transaction/StackItem.php
@@ -0,0 +1,26 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Core\Database\Transaction;
+
+/**
+ * A value object for items on the transaction stack.
+ */
+final class StackItem {
+
+ /**
+ * Constructor.
+ *
+ * @param string $name
+ * The name of the transaction.
+ * @param StackItemType $type
+ * The stack item type.
+ */
+ public function __construct(
+ public readonly string $name,
+ public readonly StackItemType $type,
+ ) {
+ }
+
+}
diff --git a/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php b/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php
index 112077b90eeb..9c50d552f34f 100644
--- a/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php
+++ b/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php
@@ -27,21 +27,31 @@ abstract class TransactionManagerBase implements TransactionManagerInterface {
/**
* The stack of Drupal transactions currently active.
*
- * This is not a real LIFO (Last In, First Out) stack, where we would only
- * remove the layers according to the order they were introduced. For commits
- * the layer order is enforced, while for rollbacks the API allows to
- * rollback to savepoints before the last one.
+ * This property is keeping track of the Transaction objects started and
+ * ended as a LIFO (Last In, First Out) stack.
*
- * @var array<string,StackItemType>
- */
- private array $stack = [];
-
- /**
- * A list of Drupal transactions rolled back but not yet unpiled.
+ * The database API allows to begin transactions, add an arbitrary number of
+ * additional savepoints, and release any savepoint in the sequence. When
+ * this happens, the database will implicitly release all the savepoints
+ * created after the one released. Given Drupal implementation of the
+ * Transaction objects, we cannot force descoping of the corresponding
+ * Transaction savepoint objects from the manager, because they live in the
+ * scope of the calling code. This stack ensures that when an outlived
+ * Transaction object gets out of scope, it will not try to release on the
+ * database a savepoint that no longer exists.
+ *
+ * Differently, rollbacks are strictly being checked for LIFO order: if a
+ * rollback is requested against a savepoint that is not the last created,
+ * the manager will throw a TransactionOutOfOrderException.
+ *
+ * The array key is the transaction's unique id, its value a StackItem.
*
- * @var array<string,true>
+ * @var array<string,StackItem>
+ *
+ * @todo in https://www.drupal.org/project/drupal/issues/3384995, complete
+ * the LIFO logic extending it to the root transaction too.
*/
- private array $rollbacks = [];
+ private array $stack = [];
/**
* A list of post-transaction callbacks.
@@ -91,7 +101,7 @@ public function stackDepth(): int {
* $stack property.
*
* phpcs:ignore Drupal.Commenting.FunctionComment.InvalidReturn
- * @return array<string,StackItemType>
+ * @return array<string,StackItem>
* The elements of the transaction stack.
*/
protected function stack(): array {
@@ -114,13 +124,13 @@ protected function resetStack(): void {
* Drivers should not override this method unless they also override the
* $stack property.
*
- * @param string $name
- * The name of the transaction.
- * @param \Drupal\Core\Database\Transaction\StackItemType $type
- * The stack item type.
+ * @param string $id
+ * The id of the transaction.
+ * @param \Drupal\Core\Database\Transaction\StackItem $item
+ * The stack item.
*/
- protected function addStackItem(string $name, StackItemType $type): void {
- $this->stack[$name] = $type;
+ protected function addStackItem(string $id, StackItem $item): void {
+ $this->stack[$id] = $item;
}
/**
@@ -129,11 +139,11 @@ protected function addStackItem(string $name, StackItemType $type): void {
* Drivers should not override this method unless they also override the
* $stack property.
*
- * @param string $name
- * The name of the transaction.
+ * @param string $id
+ * The id of the transaction.
*/
- protected function removeStackItem(string $name): void {
- unset($this->stack[$name]);
+ protected function removeStackItem(string $id): void {
+ unset($this->stack[$id]);
}
/**
@@ -176,23 +186,30 @@ public function push(string $name = ''): Transaction {
$type = StackItemType::Savepoint;
}
- // Push the transaction on the stack, increasing its depth.
- $this->addStackItem($name, $type);
+ // Define an unique id for the transaction.
+ $id = uniqid();
+
+ // Add an item on the stack, increasing its depth.
+ $this->addStackItem($id, new StackItem($name, $type));
- return new Transaction($this->connection, $name);
+ // Actually return a new Transaction object.
+ return new Transaction($this->connection, $name, $id);
}
/**
* {@inheritdoc}
*/
- public function unpile(string $name): void {
- // If an already rolled back Drupal transaction, do nothing on the client
- // connection, just cleanup the list of transactions rolled back.
- if (isset($this->rollbacks[$name])) {
- unset($this->rollbacks[$name]);
+ public function unpile(string $name, string $id): void {
+ // If the $id does not correspond to the one in the stack for that $name,
+ // we are facing an orphaned Transaction object (for example in case of a
+ // DDL statement breaking an active transaction), so there is nothing more
+ // to do.
+ if (!isset($this->stack()[$id]) || $this->stack()[$id]->name !== $name) {
return;
}
+ // If unpiling a savepoint, but that does not exist on the stack, the stack
+ // got corrupted.
if ($name !== 'drupal_transaction' && !$this->has($name)) {
throw new TransactionOutOfOrderException();
}
@@ -201,14 +218,20 @@ public function unpile(string $name): void {
// is not a root one.
if (
$this->has($name)
- && $this->stack()[$name] === StackItemType::Savepoint
+ && $this->stack()[$id]->type === StackItemType::Savepoint
&& $this->getConnectionTransactionState() === ClientConnectionTransactionState::Active
) {
+ // If we are not releasing the last savepoint but an earlier one, all
+ // subsequent savepoints will have been released as well. The stack must
+ // be diminished accordingly.
+ while (($i = array_key_last($this->stack())) != $id) {
+ $this->removeStackItem((string) $i);
+ }
$this->releaseClientSavepoint($name);
}
// Remove the transaction from the stack.
- $this->removeStackItem($name);
+ $this->removeStackItem($id);
// If this was the last Drupal transaction open, we can commit the client
// transaction.
@@ -223,24 +246,47 @@ public function unpile(string $name): void {
/**
* {@inheritdoc}
*/
- public function rollback(string $name): void {
+ public function rollback(string $name, string $id): void {
+ // @todo remove in drupal:11.0.0.
+ // Start of BC layer.
+ if ($id === 'bc-force-rollback') {
+ foreach ($this->stack() as $stackId => $item) {
+ if ($item->name === $name) {
+ $id = $stackId;
+ break;
+ }
+ }
+ if ($id === 'bc-force-rollback') {
+ throw new TransactionOutOfOrderException();
+ }
+ }
+ // End of BC layer.
+
+ // If the $id does not correspond to the one in the stack for that $name,
+ // we are facing an orphaned Transaction object (for example in case of a
+ // DDL statement breaking an active transaction), so there is nothing more
+ // to do.
+ if (!isset($this->stack()[$id]) || $this->stack()[$id]->name !== $name) {
+ return;
+ }
+
if (!$this->inTransaction()) {
throw new TransactionNoActiveException();
}
// Do the client-level processing.
- match ($this->stack()[$name]) {
+ match ($this->stack()[$id]->type) {
StackItemType::Root => $this->processRootRollback(),
StackItemType::Savepoint => $this->rollbackClientSavepoint($name),
};
// Rolled back item should match the last one in stack.
- if ($name !== array_key_last($this->stack())) {
+ if ($id != array_key_last($this->stack())) {
throw new TransactionOutOfOrderException();
}
- $this->rollbacks[$name] = TRUE;
- $this->removeStackItem($name);
+ // Remove the transaction from the stack.
+ $this->removeStackItem($id);
// If this was the last Drupal transaction open, we can commit the client
// transaction.
@@ -263,7 +309,12 @@ public function addPostTransactionCallback(callable $callback): void {
* {@inheritdoc}
*/
public function has(string $name): bool {
- return isset($this->stack()[$name]);
+ foreach ($this->stack() as $item) {
+ if ($item->name === $name) {
+ return TRUE;
+ }
+ }
+ return FALSE;
}
/**
diff --git a/core/lib/Drupal/Core/Database/Transaction/TransactionManagerInterface.php b/core/lib/Drupal/Core/Database/Transaction/TransactionManagerInterface.php
index 86d5de4ac5ad..1bd0cd3e5fde 100644
--- a/core/lib/Drupal/Core/Database/Transaction/TransactionManagerInterface.php
+++ b/core/lib/Drupal/Core/Database/Transaction/TransactionManagerInterface.php
@@ -36,6 +36,8 @@ public function has(string $name): bool;
* This begins a client connection transaction if there is not one active,
* or adds a savepoint to the active one.
*
+ * This method should only be called internally by a database driver.
+ *
* @param string $name
* (optional) The name of the savepoint.
*
@@ -54,15 +56,19 @@ public function push(string $name = ''): Transaction;
* This method should only be called by a Transaction object going out of
* scope.
*
+ * This method should only be called internally by a database driver.
+ *
* @param string $name
- * (optional) The name of the savepoint.
+ * The name of the transaction.
+ * @param string $id
+ * The id of the transaction.
*
* @throws \Drupal\Core\Database\TransactionOutOfOrderException
* If a Drupal Transaction with the specified name does not exist.
* @throws \Drupal\Core\Database\TransactionCommitFailedException
* If the commit of the root transaction failed.
*/
- public function unpile(string $name): void;
+ public function unpile(string $name, string $id): void;
/**
* Rolls back a Drupal transaction.
@@ -72,8 +78,12 @@ public function unpile(string $name): void;
* to rolling back the client connection (or to committing it in the edge
* case when the root was unpiled earlier).
*
+ * This method should only be called internally by a database driver.
+ *
* @param string $name
- * (optional) The name of the savepoint.
+ * The name of the transaction.
+ * @param string $id
+ * The id of the transaction.
*
* @throws \Drupal\Core\Database\TransactionNoActiveException
* If there is no active client connection.
@@ -83,7 +93,7 @@ public function unpile(string $name): void;
* @throws \Drupal\Core\Database\TransactionCommitFailedException
* If the commit of the root transaction failed.
*/
- public function rollback(string $name): void;
+ public function rollback(string $name, string $id): void;
/**
* Adds a root transaction end callback.
diff --git a/core/misc/cspell/dictionary.txt b/core/misc/cspell/dictionary.txt
index 0d890fb02bed..5e3d3a80ccc7 100644
--- a/core/misc/cspell/dictionary.txt
+++ b/core/misc/cspell/dictionary.txt
@@ -306,6 +306,7 @@ desaturate
desaturated
desaturates
desaturating
+descoping
descripcion
destructable
deutsch
diff --git a/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificTransactionTestBase.php b/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificTransactionTestBase.php
index 18c2efcb0ca3..b0e9fc3e9b6a 100644
--- a/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificTransactionTestBase.php
+++ b/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificTransactionTestBase.php
@@ -576,6 +576,30 @@ public function testQueryFailureInTransaction() {
$this->assertEquals('24', $saved_age);
}
+ /**
+ * Tests releasing a savepoint before last is safe.
+ */
+ public function testReleaseIntermediateSavepoint(): void {
+ $transaction = $this->connection->startTransaction();
+ $this->assertSame(1, $this->connection->transactionManager()->stackDepth());
+ $savepoint1 = $this->connection->startTransaction();
+ $this->assertSame(2, $this->connection->transactionManager()->stackDepth());
+ $savepoint2 = $this->connection->startTransaction();
+ $this->assertSame(3, $this->connection->transactionManager()->stackDepth());
+ $savepoint3 = $this->connection->startTransaction();
+ $this->assertSame(4, $this->connection->transactionManager()->stackDepth());
+ $savepoint4 = $this->connection->startTransaction();
+ $this->assertSame(5, $this->connection->transactionManager()->stackDepth());
+ $this->insertRow('row');
+ unset($savepoint2);
+ $this->assertSame(2, $this->connection->transactionManager()->stackDepth());
+ $this->assertRowPresent('row');
+ unset($savepoint1);
+ unset($transaction);
+ $this->assertFalse($this->connection->inTransaction());
+ $this->assertRowPresent('row');
+ }
+
/**
* Tests for transaction names.
*/
--
GitLab