diff --git a/composer.json b/composer.json index ce24f8536bd3..e062b0e03b8a 100644 --- a/composer.json +++ b/composer.json @@ -104,7 +104,7 @@ "league/flysystem-read-only": "^3.3", "league/flysystem-sftp-v3": "^3.0", "mockery/mockery": "^1.5.1", - "orchestra/testbench-core": "^8.10", + "orchestra/testbench-core": "^8.11.3", "pda/pheanstalk": "^4.0", "phpstan/phpstan": "^1.4.7", "phpunit/phpunit": "^10.0.7", diff --git a/src/Illuminate/Database/Concerns/ManagesTransactions.php b/src/Illuminate/Database/Concerns/ManagesTransactions.php index 14661cc76ebf..04209df61afd 100644 --- a/src/Illuminate/Database/Concerns/ManagesTransactions.php +++ b/src/Illuminate/Database/Concerns/ManagesTransactions.php @@ -47,11 +47,9 @@ public function transaction(Closure $callback, $attempts = 1) $this->getPdo()->commit(); } - $this->transactions = max(0, $this->transactions - 1); + $this->transactionsManager?->commit($this->getName(), $this->transactions); - if ($this->afterCommitCallbacksShouldBeExecuted()) { - $this->transactionsManager?->commit($this->getName()); - } + $this->transactions = max(0, $this->transactions - 1); } catch (Throwable $e) { $this->handleCommitTransactionException( $e, $currentAttempt, $attempts @@ -194,27 +192,13 @@ public function commit() $this->getPdo()->commit(); } - $this->transactions = max(0, $this->transactions - 1); + $this->transactionsManager?->commit($this->getName(), $this->transactions); - if ($this->afterCommitCallbacksShouldBeExecuted()) { - $this->transactionsManager?->commit($this->getName()); - } + $this->transactions = max(0, $this->transactions - 1); $this->fireConnectionEvent('committed'); } - /** - * Determine if after commit callbacks should be executed. - * - * @return bool - */ - protected function afterCommitCallbacksShouldBeExecuted() - { - return $this->transactions == 0 || - ($this->transactionsManager && - $this->transactionsManager->callbackApplicableTransactions()->count() === 1); - } - /** * Handle an exception encountered when committing a transaction. * diff --git a/src/Illuminate/Database/DatabaseTransactionsManager.php b/src/Illuminate/Database/DatabaseTransactionsManager.php index 8d145188f065..37797ad9e502 100755 --- a/src/Illuminate/Database/DatabaseTransactionsManager.php +++ b/src/Illuminate/Database/DatabaseTransactionsManager.php @@ -12,11 +12,11 @@ class DatabaseTransactionsManager protected $transactions; /** - * The database transaction that should be ignored by callbacks. + * When in test mode, we'll run the after commit callbacks on the top-level transaction. * - * @var \Illuminate\Database\DatabaseTransactionRecord + * @var bool */ - protected $callbacksShouldIgnore; + protected $afterCommitCallbacksRunningInTestTransaction = false; /** * Create a new database transactions manager instance. @@ -26,6 +26,19 @@ class DatabaseTransactionsManager public function __construct() { $this->transactions = collect(); + $this->afterCommitCallbacksRunningInTestTransaction = false; + } + + /** + * Sets the transaction manager to test mode. + * + * @return self + */ + public function withAfterCommitCallbacksInTestTransactionAwareMode() + { + $this->afterCommitCallbacksRunningInTestTransaction = true; + + return $this; } /** @@ -54,30 +67,28 @@ public function rollback($connection, $level) $this->transactions = $this->transactions->reject( fn ($transaction) => $transaction->connection == $connection && $transaction->level > $level )->values(); - - if ($this->transactions->isEmpty()) { - $this->callbacksShouldIgnore = null; - } } /** * Commit the active database transaction. * * @param string $connection + * @param int $level * @return void */ - public function commit($connection) + public function commit($connection, $level = 1) { - [$forThisConnection, $forOtherConnections] = $this->transactions->partition( - fn ($transaction) => $transaction->connection == $connection - ); - - $this->transactions = $forOtherConnections->values(); + // If the transaction level being commited reaches 1 (meaning it was the root + // transaction), we'll run the callbacks. In test mode, since we wrap each + // test in a transaction, we'll run the callbacks when reaching level 2. + if ($level == 1 || ($this->afterCommitCallbacksRunningInTestTransaction && $level == 2)) { + [$forThisConnection, $forOtherConnections] = $this->transactions->partition( + fn ($transaction) => $transaction->connection == $connection + ); - $forThisConnection->map->executeCallbacks(); + $this->transactions = $forOtherConnections->values(); - if ($this->transactions->isEmpty()) { - $this->callbacksShouldIgnore = null; + $forThisConnection->map->executeCallbacks(); } } @@ -89,11 +100,14 @@ public function commit($connection) */ public function addCallback($callback) { - if ($current = $this->callbackApplicableTransactions()->last()) { - return $current->addCallback($callback); + // If there are no transactions, we'll run the callbacks right away. Also, we'll run it + // right away when we're in test mode and we only have the wrapping transaction. For + // every other case, we'll queue up the callback to run after the commit happens. + if ($this->transactions->isEmpty() || ($this->afterCommitCallbacksRunningInTestTransaction && $this->transactions->count() == 1)) { + return $callback(); } - $callback(); + return $this->transactions->last()->addCallback($callback); } /** @@ -101,24 +115,27 @@ public function addCallback($callback) * * @param \Illuminate\Database\DatabaseTransactionRecord $transaction * @return $this + * + * @deprecated Will be removed in a future Laravel version. Use withAfterCommitCallbacksInTestTransactionAwareMode() instead. */ public function callbacksShouldIgnore(DatabaseTransactionRecord $transaction) { - $this->callbacksShouldIgnore = $transaction; - - return $this; + // This method was meant for testing only, so we're forwarding the call to the new method... + return $this->withAfterCommitCallbacksInTestTransactionAwareMode(); } /** * Get the transactions that are applicable to callbacks. * * @return \Illuminate\Support\Collection + * + * @deprecated Will be removed in a future Laravel version. */ public function callbackApplicableTransactions() { - return $this->transactions->reject(function ($transaction) { - return $transaction === $this->callbacksShouldIgnore; - })->values(); + return $this->transactions + ->when($this->afterCommitCallbacksRunningInTestTransaction, fn ($transactions) => $transactions->skip(1)) + ->values(); } /** diff --git a/src/Illuminate/Foundation/Testing/DatabaseTransactions.php b/src/Illuminate/Foundation/Testing/DatabaseTransactions.php index d0e0bafc52de..f9cab95fe62a 100644 --- a/src/Illuminate/Foundation/Testing/DatabaseTransactions.php +++ b/src/Illuminate/Foundation/Testing/DatabaseTransactions.php @@ -22,9 +22,7 @@ public function beginDatabaseTransaction() $connection->setEventDispatcher($dispatcher); if ($this->app->resolved('db.transactions')) { - $this->app->make('db.transactions')->callbacksShouldIgnore( - $this->app->make('db.transactions')->getTransactions()->first() - ); + $this->app->make('db.transactions')->withAfterCommitCallbacksInTestTransactionAwareMode(); } } diff --git a/src/Illuminate/Foundation/Testing/RefreshDatabase.php b/src/Illuminate/Foundation/Testing/RefreshDatabase.php index b486cbb0ac91..07a64dc98b2f 100644 --- a/src/Illuminate/Foundation/Testing/RefreshDatabase.php +++ b/src/Illuminate/Foundation/Testing/RefreshDatabase.php @@ -98,9 +98,7 @@ public function beginDatabaseTransaction() $connection->setEventDispatcher($dispatcher); if ($this->app->resolved('db.transactions')) { - $this->app->make('db.transactions')->callbacksShouldIgnore( - $this->app->make('db.transactions')->getTransactions()->first() - ); + $this->app->make('db.transactions')->withAfterCommitCallbacksInTestTransactionAwareMode(); } } diff --git a/tests/Database/DatabaseTransactionsManagerTest.php b/tests/Database/DatabaseTransactionsManagerTest.php index e8d82e048720..dbfa21f5b1f1 100755 --- a/tests/Database/DatabaseTransactionsManagerTest.php +++ b/tests/Database/DatabaseTransactionsManagerTest.php @@ -67,7 +67,7 @@ public function testCommittingTransactions() $manager->begin('default', 2); $manager->begin('admin', 1); - $manager->commit('default'); + $manager->commit('default', 1); $this->assertCount(1, $manager->getTransactions()); @@ -118,7 +118,11 @@ public function testCommittingTransactionsExecutesCallbacks() $manager->begin('admin', 1); - $manager->commit('default'); + $manager->commit('default', 2); + + $this->assertEmpty($callbacks, 'Should not have ran the callbacks.'); + + $manager->commit('default', 1); $this->assertCount(2, $callbacks); $this->assertEquals(['default', 1], $callbacks[0]); @@ -144,7 +148,11 @@ public function testCommittingExecutesOnlyCallbacksOfTheConnection() $callbacks[] = ['admin', 1]; }); - $manager->commit('default'); + $manager->commit('default', 2); + + $this->assertEmpty($callbacks, 'Should not have run the callbacks.'); + + $manager->commit('default', 1); $this->assertCount(1, $callbacks); $this->assertEquals(['default', 1], $callbacks[0]); diff --git a/tests/Database/DatabaseTransactionsTest.php b/tests/Database/DatabaseTransactionsTest.php index ac9587419eeb..8c5166506147 100644 --- a/tests/Database/DatabaseTransactionsTest.php +++ b/tests/Database/DatabaseTransactionsTest.php @@ -64,7 +64,7 @@ public function testTransactionIsRecordedAndCommitted() { $transactionManager = m::mock(new DatabaseTransactionsManager); $transactionManager->shouldReceive('begin')->once()->with('default', 1); - $transactionManager->shouldReceive('commit')->once()->with('default'); + $transactionManager->shouldReceive('commit')->once()->with('default', 1); $this->connection()->setTransactionManager($transactionManager); @@ -83,7 +83,7 @@ public function testTransactionIsRecordedAndCommittedUsingTheSeparateMethods() { $transactionManager = m::mock(new DatabaseTransactionsManager); $transactionManager->shouldReceive('begin')->once()->with('default', 1); - $transactionManager->shouldReceive('commit')->once()->with('default'); + $transactionManager->shouldReceive('commit')->once()->with('default', 1); $this->connection()->setTransactionManager($transactionManager); @@ -103,7 +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('commit')->once()->with('default'); + $transactionManager->shouldReceive('commit')->once()->with('default', 2); + $transactionManager->shouldReceive('commit')->once()->with('default', 1); $this->connection()->setTransactionManager($transactionManager); @@ -130,8 +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('commit')->once()->with('default'); - $transactionManager->shouldReceive('commit')->once()->with('second_connection'); + $transactionManager->shouldReceive('commit')->once()->with('second_connection', 2); + $transactionManager->shouldReceive('commit')->once()->with('second_connection', 1); + $transactionManager->shouldReceive('commit')->once()->with('default', 1); $this->connection()->setTransactionManager($transactionManager); $this->connection('second_connection')->setTransactionManager($transactionManager); diff --git a/tests/Integration/Database/EloquentTransactionUsingRefreshDatabaseTest.php b/tests/Integration/Database/EloquentTransactionUsingRefreshDatabaseTest.php new file mode 100644 index 000000000000..d167816534c2 --- /dev/null +++ b/tests/Integration/Database/EloquentTransactionUsingRefreshDatabaseTest.php @@ -0,0 +1,101 @@ +afterApplicationCreated(fn () => User::unguard()); + $this->beforeApplicationDestroyed(fn () => User::reguard()); + + parent::setUp(); + } + + public function testObserverIsCalledOnTestsWithAfterCommit() + { + User::observe($observer = EloquentTransactionUsingRefreshDatabaseUserObserver::resetting()); + + $user1 = User::create(UserFactory::new()->raw()); + + $this->assertTrue($user1->exists); + $this->assertEquals(1, $observer::$calledTimes, 'Failed to assert the observer was called once.'); + } + + public function testObserverCalledWithAfterCommitWhenInsideTransaction() + { + User::observe($observer = EloquentTransactionUsingRefreshDatabaseUserObserver::resetting()); + + $user1 = DB::transaction(fn () => User::create(UserFactory::new()->raw())); + + $this->assertTrue($user1->exists); + $this->assertEquals(1, $observer::$calledTimes, 'Failed to assert the observer was called once.'); + } + + public function testObserverIsCalledOnTestsWithAfterCommitWhenUsingSavepoint() + { + User::observe($observer = EloquentTransactionUsingRefreshDatabaseUserObserver::resetting()); + + $user1 = User::createOrFirst(UserFactory::new()->raw()); + + $this->assertTrue($user1->exists); + $this->assertEquals(1, $observer::$calledTimes, 'Failed to assert the observer was called once.'); + } + + public function testObserverIsCalledOnTestsWithAfterCommitWhenUsingSavepointAndInsideTransaction() + { + User::observe($observer = EloquentTransactionUsingRefreshDatabaseUserObserver::resetting()); + + $user1 = DB::transaction(fn () => User::createOrFirst(UserFactory::new()->raw())); + + $this->assertTrue($user1->exists); + $this->assertEquals(1, $observer::$calledTimes, 'Failed to assert the observer was called once.'); + } + + public function testObserverIsCalledEvenWhenDeeplyNestingTransactions() + { + User::observe($observer = EloquentTransactionUsingRefreshDatabaseUserObserver::resetting()); + + $user1 = DB::transaction(function () use ($observer) { + return tap(DB::transaction(function () use ($observer) { + return tap(DB::transaction(function () { + return User::createOrFirst(UserFactory::new()->raw()); + }), function () use ($observer) { + $this->assertEquals(0, $observer::$calledTimes, 'Should not have been called'); + }); + }), function () use ($observer) { + $this->assertEquals(0, $observer::$calledTimes, 'Should not have been called'); + }); + }); + + $this->assertTrue($user1->exists); + $this->assertEquals(1, $observer::$calledTimes, 'Failed to assert the observer was called once.'); + } +} + +class EloquentTransactionUsingRefreshDatabaseUserObserver +{ + public static $calledTimes = 0; + + public $afterCommit = true; + + public static function resetting() + { + static::$calledTimes = 0; + + return new static(); + } + + public function created($user) + { + static::$calledTimes++; + } +}