From 464c7fa05b640a7a2e02818fa84e45c1d5ffce4d Mon Sep 17 00:00:00 2001 From: Taylor Otwell Date: Thu, 23 Nov 2023 11:05:15 -0600 Subject: [PATCH 1/5] better object design --- .../Database/Concerns/ManagesTransactions.php | 40 ++++++------- .../Database/DatabaseTransactionsManager.php | 56 +++++++++++-------- tests/Database/DatabaseConnectionTest.php | 14 ----- .../DatabaseTransactionsManagerTest.php | 32 +++++++---- tests/Database/DatabaseTransactionsTest.php | 21 +++---- .../DatabaseTransactionsManagerTest.php | 4 +- 6 files changed, 80 insertions(+), 87 deletions(-) diff --git a/src/Illuminate/Database/Concerns/ManagesTransactions.php b/src/Illuminate/Database/Concerns/ManagesTransactions.php index 32bd999b1726..7e58aa376b18 100644 --- a/src/Illuminate/Database/Concerns/ManagesTransactions.php +++ b/src/Illuminate/Database/Concerns/ManagesTransactions.php @@ -47,13 +47,16 @@ public function transaction(Closure $callback, $attempts = 1) $this->getPdo()->commit(); } - $this->transactionsManager?->stageTransactions($this->getName(), $this->transactions); - - $this->transactions = max(0, $this->transactions - 1); - - if ($this->afterCommitCallbacksShouldBeExecuted()) { - $this->transactionsManager?->commit($this->getName()); - } + [$levelBeingCommitted, $this->transactions] = [ + $this->transactions, + max(0, $this->transactions - 1), + ]; + + $this->transactionsManager?->commit( + $this->getName(), + $levelBeingCommitted, + $this->transactions + ); } catch (Throwable $e) { $this->handleCommitTransactionException( $e, $currentAttempt, $attempts @@ -196,27 +199,18 @@ public function commit() $this->getPdo()->commit(); } - $this->transactionsManager?->stageTransactions($this->getName(), $this->transactions); + [$levelBeingCommitted, $this->transactions] = [ + $this->transactions, + max(0, $this->transactions - 1) + ]; - $this->transactions = max(0, $this->transactions - 1); - - if ($this->afterCommitCallbacksShouldBeExecuted()) { - $this->transactionsManager?->commit($this->getName()); - } + $this->transactionsManager?->commit( + $this->getName(), $levelBeingCommitted, $this->transactions + ); $this->fireConnectionEvent('committed'); } - /** - * Determine if after commit callbacks should be executed. - * - * @return bool - */ - protected function afterCommitCallbacksShouldBeExecuted() - { - return $this->transactionsManager?->afterCommitCallbacksShouldBeExecuted($this->transactions) || $this->transactions == 0; - } - /** * Handle an exception encountered when committing a transaction. * diff --git a/src/Illuminate/Database/DatabaseTransactionsManager.php b/src/Illuminate/Database/DatabaseTransactionsManager.php index a8e634e42ddf..f9ed0d1b40bf 100755 --- a/src/Illuminate/Database/DatabaseTransactionsManager.php +++ b/src/Illuminate/Database/DatabaseTransactionsManager.php @@ -59,39 +59,25 @@ public function begin($connection, $level) } /** - * Move relevant pending transactions to a committed state. + * Commit the root database transaction and execute callbacks. * * @param string $connection * @param int $levelBeingCommitted - * @return void + * @param int $newTransactionLevel + * @return array */ - public function stageTransactions($connection, $levelBeingCommitted) + public function commit($connection, $levelBeingCommitted, $newTransactionLevel) { - $this->committedTransactions = $this->committedTransactions->merge( - $this->pendingTransactions->filter( - fn ($transaction) => $transaction->connection === $connection && - $transaction->level >= $levelBeingCommitted - ) - ); - - $this->pendingTransactions = $this->pendingTransactions->reject( - fn ($transaction) => $transaction->connection === $connection && - $transaction->level >= $levelBeingCommitted - ); + $this->stageTransactions($connection, $levelBeingCommitted); if (isset($this->currentTransaction[$connection])) { $this->currentTransaction[$connection] = $this->currentTransaction[$connection]->parent; } - } - /** - * Commit the root database transaction and execute callbacks. - * - * @param string $connection - * @return void - */ - public function commit($connection) - { + if ($newTransactionLevel > 0) { + return []; + } + // This method is only called when the root database transaction is committed so there // shouldn't be any pending transactions, but going to clear them here anyways just // in case. This method could be refactored to receive a level in the future too. @@ -106,6 +92,30 @@ public function commit($connection) $this->committedTransactions = $forOtherConnections->values(); $forThisConnection->map->executeCallbacks(); + + return $forThisConnection; + } + + /** + * Move relevant pending transactions to a committed state. + * + * @param string $connection + * @param int $levelBeingCommitted + * @return void + */ + public function stageTransactions($connection, $levelBeingCommitted) + { + $this->committedTransactions = $this->committedTransactions->merge( + $this->pendingTransactions->filter( + fn ($transaction) => $transaction->connection === $connection && + $transaction->level >= $levelBeingCommitted + ) + ); + + $this->pendingTransactions = $this->pendingTransactions->reject( + fn ($transaction) => $transaction->connection === $connection && + $transaction->level >= $levelBeingCommitted + ); } /** diff --git a/tests/Database/DatabaseConnectionTest.php b/tests/Database/DatabaseConnectionTest.php index b91db617a9b5..5afafb05b4af 100755 --- a/tests/Database/DatabaseConnectionTest.php +++ b/tests/Database/DatabaseConnectionTest.php @@ -290,20 +290,6 @@ public function testCommittingFiresEventsIfSet() $connection->commit(); } - public function testAfterCommitIsExecutedOnFinalCommit() - { - $pdo = $this->getMockBuilder(DatabaseConnectionTestMockPDO::class)->onlyMethods(['beginTransaction', 'commit'])->getMock(); - $transactionsManager = $this->getMockBuilder(DatabaseTransactionsManager::class)->onlyMethods(['afterCommitCallbacksShouldBeExecuted'])->getMock(); - $transactionsManager->expects($this->once())->method('afterCommitCallbacksShouldBeExecuted')->with(0)->willReturn(true); - - $connection = $this->getMockConnection([], $pdo); - $connection->setTransactionManager($transactionsManager); - - $connection->transaction(function () { - // do nothing - }); - } - public function testRollBackedFiresEventsIfSet() { $pdo = $this->createMock(DatabaseConnectionTestMockPDO::class); diff --git a/tests/Database/DatabaseTransactionsManagerTest.php b/tests/Database/DatabaseTransactionsManagerTest.php index a62c03dfb5ef..6edc932bcf29 100755 --- a/tests/Database/DatabaseTransactionsManagerTest.php +++ b/tests/Database/DatabaseTransactionsManagerTest.php @@ -66,16 +66,24 @@ public function testCommittingTransactions() $manager->begin('default', 1); $manager->begin('default', 2); $manager->begin('admin', 1); + $manager->begin('admin', 2); - $manager->stageTransactions('default', 1); - $manager->stageTransactions('admin', 1); - $manager->commit('default'); + $manager->commit('default', 2, 1); + $executedTransactions = $manager->commit('default', 1, 0); - $this->assertCount(0, $manager->getPendingTransactions()); - $this->assertCount(1, $manager->getCommittedTransactions()); + $executedAdminTransactions = $manager->commit('admin', 2, 1); + + $this->assertCount(1, $manager->getPendingTransactions()); // One pending "admin" transaction left... + $this->assertCount(2, $executedTransactions); // Two committed tranasctions on "default" + $this->assertCount(0, $executedAdminTransactions); // Zero executed committed tranasctions on "default" + // Level 2 "admin" callback has been staged... $this->assertSame('admin', $manager->getCommittedTransactions()[0]->connection); - $this->assertEquals(1, $manager->getCommittedTransactions()[0]->level); + $this->assertEquals(2, $manager->getCommittedTransactions()[0]->level); + + // Level 1 "admin" callback still pending... + $this->assertSame('admin', $manager->getPendingTransactions()[0]->connection); + $this->assertEquals(1, $manager->getPendingTransactions()[0]->level); } public function testCallbacksAreAddedToTheCurrentTransaction() @@ -121,12 +129,12 @@ public function testCommittingTransactionsExecutesCallbacks() $manager->begin('admin', 1); - $manager->stageTransactions('default', 1); - $manager->commit('default'); + $manager->commit('default', 2, 1); + $manager->commit('default', 1, 0); $this->assertCount(2, $callbacks); - $this->assertEquals(['default', 1], $callbacks[0]); - $this->assertEquals(['default', 2], $callbacks[1]); + $this->assertEquals(['default', 2], $callbacks[0]); + $this->assertEquals(['default', 1], $callbacks[1]); } public function testCommittingExecutesOnlyCallbacksOfTheConnection() @@ -148,8 +156,8 @@ public function testCommittingExecutesOnlyCallbacksOfTheConnection() $callbacks[] = ['admin', 1]; }); - $manager->stageTransactions('default', 1); - $manager->commit('default'); + $manager->commit('default', 2, 1); + $manager->commit('default', 1, 0); $this->assertCount(1, $callbacks); $this->assertEquals(['default', 1], $callbacks[0]); diff --git a/tests/Database/DatabaseTransactionsTest.php b/tests/Database/DatabaseTransactionsTest.php index 384f4223fa81..3affe52a8a00 100644 --- a/tests/Database/DatabaseTransactionsTest.php +++ b/tests/Database/DatabaseTransactionsTest.php @@ -64,8 +64,7 @@ public function testTransactionIsRecordedAndCommitted() { $transactionManager = m::mock(new DatabaseTransactionsManager); $transactionManager->shouldReceive('begin')->once()->with('default', 1); - $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 1); - $transactionManager->shouldReceive('commit')->once()->with('default'); + $transactionManager->shouldReceive('commit')->once()->with('default', 1, 0); $this->connection()->setTransactionManager($transactionManager); @@ -84,8 +83,7 @@ public function testTransactionIsRecordedAndCommittedUsingTheSeparateMethods() { $transactionManager = m::mock(new DatabaseTransactionsManager); $transactionManager->shouldReceive('begin')->once()->with('default', 1); - $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 1); - $transactionManager->shouldReceive('commit')->once()->with('default'); + $transactionManager->shouldReceive('commit')->once()->with('default', 1, 0); $this->connection()->setTransactionManager($transactionManager); @@ -105,9 +103,8 @@ public function testNestedTransactionIsRecordedAndCommitted() $transactionManager = m::mock(new DatabaseTransactionsManager); $transactionManager->shouldReceive('begin')->once()->with('default', 1); $transactionManager->shouldReceive('begin')->once()->with('default', 2); - $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 1); - $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 2); - $transactionManager->shouldReceive('commit')->once()->with('default'); + $transactionManager->shouldReceive('commit')->once()->with('default', 2, 1); + $transactionManager->shouldReceive('commit')->once()->with('default', 1, 0); $this->connection()->setTransactionManager($transactionManager); @@ -134,11 +131,9 @@ public function testNestedTransactionIsRecordeForDifferentConnectionsdAndCommitt $transactionManager->shouldReceive('begin')->once()->with('default', 1); $transactionManager->shouldReceive('begin')->once()->with('second_connection', 1); $transactionManager->shouldReceive('begin')->once()->with('second_connection', 2); - $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 1); - $transactionManager->shouldReceive('stageTransactions')->once()->with('second_connection', 1); - $transactionManager->shouldReceive('stageTransactions')->once()->with('second_connection', 2); - $transactionManager->shouldReceive('commit')->once()->with('default'); - $transactionManager->shouldReceive('commit')->once()->with('second_connection'); + $transactionManager->shouldReceive('commit')->once()->with('default', 1, 0); + $transactionManager->shouldReceive('commit')->once()->with('second_connection', 2, 1); + $transactionManager->shouldReceive('commit')->once()->with('second_connection', 1, 0); $this->connection()->setTransactionManager($transactionManager); $this->connection('second_connection')->setTransactionManager($transactionManager); @@ -196,7 +191,7 @@ public function testTransactionIsRolledBackUsingSeparateMethods() $transactionManager = m::mock(new DatabaseTransactionsManager); $transactionManager->shouldReceive('begin')->once()->with('default', 1); $transactionManager->shouldReceive('rollback')->once()->with('default', 0); - $transactionManager->shouldNotReceive('commit'); + $transactionManager->shouldNotReceive('commit', 1, 0); $this->connection()->setTransactionManager($transactionManager); diff --git a/tests/Foundation/Testing/DatabaseTransactionsManagerTest.php b/tests/Foundation/Testing/DatabaseTransactionsManagerTest.php index f5655ebc3862..feac68b8e5bc 100644 --- a/tests/Foundation/Testing/DatabaseTransactionsManagerTest.php +++ b/tests/Foundation/Testing/DatabaseTransactionsManagerTest.php @@ -42,8 +42,8 @@ public function testItExecutesCallbacksForTheSecondTransaction() $this->assertFalse($testObject->ran); - $manager->stageTransactions('foo', 1); - $manager->commit('foo'); + $manager->commit('foo', 2, 1); + $manager->commit('foo', 1, 0); $this->assertTrue($testObject->ran); $this->assertEquals(1, $testObject->runs); } From 057c42cbdb9331a9c9c3c5b6e9eba6b79b5d15a2 Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Thu, 23 Nov 2023 17:05:34 +0000 Subject: [PATCH 2/5] Apply fixes from StyleCI --- src/Illuminate/Database/Concerns/ManagesTransactions.php | 2 +- tests/Database/DatabaseConnectionTest.php | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Illuminate/Database/Concerns/ManagesTransactions.php b/src/Illuminate/Database/Concerns/ManagesTransactions.php index 7e58aa376b18..99670cf0949c 100644 --- a/src/Illuminate/Database/Concerns/ManagesTransactions.php +++ b/src/Illuminate/Database/Concerns/ManagesTransactions.php @@ -201,7 +201,7 @@ public function commit() [$levelBeingCommitted, $this->transactions] = [ $this->transactions, - max(0, $this->transactions - 1) + max(0, $this->transactions - 1), ]; $this->transactionsManager?->commit( diff --git a/tests/Database/DatabaseConnectionTest.php b/tests/Database/DatabaseConnectionTest.php index 5afafb05b4af..8709e339c226 100755 --- a/tests/Database/DatabaseConnectionTest.php +++ b/tests/Database/DatabaseConnectionTest.php @@ -7,7 +7,6 @@ use Exception; use Illuminate\Contracts\Events\Dispatcher; use Illuminate\Database\Connection; -use Illuminate\Database\DatabaseTransactionsManager; use Illuminate\Database\Events\QueryExecuted; use Illuminate\Database\Events\TransactionBeginning; use Illuminate\Database\Events\TransactionCommitted; From 7f728df500bd338a1ee5336f412e11a472b02825 Mon Sep 17 00:00:00 2001 From: Taylor Otwell Date: Thu, 23 Nov 2023 11:20:51 -0600 Subject: [PATCH 3/5] use method for testing --- src/Illuminate/Database/DatabaseTransactionsManager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Illuminate/Database/DatabaseTransactionsManager.php b/src/Illuminate/Database/DatabaseTransactionsManager.php index f9ed0d1b40bf..871dfc715937 100755 --- a/src/Illuminate/Database/DatabaseTransactionsManager.php +++ b/src/Illuminate/Database/DatabaseTransactionsManager.php @@ -74,7 +74,7 @@ public function commit($connection, $levelBeingCommitted, $newTransactionLevel) $this->currentTransaction[$connection] = $this->currentTransaction[$connection]->parent; } - if ($newTransactionLevel > 0) { + if (! $this->afterCommitCallbacksShouldBeExecuted($newTransactionLevel)) { return []; } From 695e1d8953a0300fa518126437f3bf32874a902b Mon Sep 17 00:00:00 2001 From: Taylor Otwell Date: Thu, 23 Nov 2023 11:26:01 -0600 Subject: [PATCH 4/5] add or --- src/Illuminate/Database/DatabaseTransactionsManager.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Illuminate/Database/DatabaseTransactionsManager.php b/src/Illuminate/Database/DatabaseTransactionsManager.php index 871dfc715937..1858acf64d74 100755 --- a/src/Illuminate/Database/DatabaseTransactionsManager.php +++ b/src/Illuminate/Database/DatabaseTransactionsManager.php @@ -74,7 +74,8 @@ public function commit($connection, $levelBeingCommitted, $newTransactionLevel) $this->currentTransaction[$connection] = $this->currentTransaction[$connection]->parent; } - if (! $this->afterCommitCallbacksShouldBeExecuted($newTransactionLevel)) { + if (! $this->afterCommitCallbacksShouldBeExecuted($newTransactionLevel) || + $newTransactionLevel === 0) { return []; } From 6d47ef9eca743f42128836b6322ebe5c08e75582 Mon Sep 17 00:00:00 2001 From: Taylor Otwell Date: Thu, 23 Nov 2023 11:45:18 -0600 Subject: [PATCH 5/5] fix condition --- src/Illuminate/Database/DatabaseTransactionsManager.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Illuminate/Database/DatabaseTransactionsManager.php b/src/Illuminate/Database/DatabaseTransactionsManager.php index 1858acf64d74..c730dc503ac2 100755 --- a/src/Illuminate/Database/DatabaseTransactionsManager.php +++ b/src/Illuminate/Database/DatabaseTransactionsManager.php @@ -74,8 +74,8 @@ public function commit($connection, $levelBeingCommitted, $newTransactionLevel) $this->currentTransaction[$connection] = $this->currentTransaction[$connection]->parent; } - if (! $this->afterCommitCallbacksShouldBeExecuted($newTransactionLevel) || - $newTransactionLevel === 0) { + if (! $this->afterCommitCallbacksShouldBeExecuted($newTransactionLevel) && + $newTransactionLevel !== 0) { return []; }