Skip to content

[10.x] Fix DB::afterCommit() broken in tests using DatabaseTransactions - #50068

Merged
taylorotwell merged 2 commits into
laravel:10.xfrom
oprypkhantc:after-commit-in-tests
Feb 13, 2024
Merged

[10.x] Fix DB::afterCommit() broken in tests using DatabaseTransactions#50068
taylorotwell merged 2 commits into
laravel:10.xfrom
oprypkhantc:after-commit-in-tests

Conversation

@oprypkhantc

@oprypkhantc oprypkhantc commented Feb 13, 2024

Copy link
Copy Markdown
Contributor

Closes #50066

Currently, committing the "root" transaction causes DatabaseTransactionsManager to remove all $pendingTransactions. This tells the DatabaseTransactionsManager that it's outside of the transaction and makes it immediately execute callbacks on calls to DB::afterCommit():

class Something {
	public function something(): void {
		// DatabaseTransactionsManager::$pendingTransactions->count() === 0
		
		DB::transaction(function () {
			// DatabaseTransactionsManager::$pendingTransactions->count() === 1
			
			// Not executed immediately
			DB::afterCommit(fn () => dump(123));
		});

		// DatabaseTransactionsManager::$pendingTransactions->count() === 0
		//
		// dump(123) executed at this point
		
		// Executed immediately because of $pendingTransactions->count() === 0
		DB::afterCommit(fn () => dump(456));
	}
}

Outside of tests, this works as expected.

In tests that use DatabaseTransactions trait however, this causes an issue whenever you have a commit inside of your test:

class SomeTest extends TestCase {
	use DatabaseTransactions;

	public function testSomething(): void
	{
		// DatabaseTransactionsManager::$pendingTransactions->count() === 1
		
		DB::transaction(function () {
			// DatabaseTransactionsManager::$pendingTransactions->count() === 2
			
			// This will only execute after the transaction commit, as expected
			DB::afterCommit(fn () => dump("after commit"));
		});
		
		// DatabaseTransactionsManager::$pendingTransactions->count() === 0
		//
		// Here the $pendingTransactions no longer contains the "root" transaction, 
		// causing subsequent `afterCommit()` calls to immediately execute:

		DB::transaction(function () {
			// DatabaseTransactionsManager::$pendingTransactions->count() === 1
			
			// Unlike the DB::afterCommit() a few lines above, this one is executed immediately,
			// without waiting for the transaction to commit, which is not expected
			DB::afterCommit(fn () => dd(123));
		});
	}
}

This PR fixes this by not letting commit() remove a pending transaction of a lower level than what's being committed. The result is that committing a nested transaction in tests leaves the root $pendingTransactions in place.

Please let me know if we need more tests or explanations for this. Feel free to make changes on the branch.

@driesvints driesvints changed the title Fix DB::afterCommit() broken in tests using DatabaseTransactions [10.x] Fix DB::afterCommit() broken in tests using DatabaseTransactions Feb 13, 2024
@taylorotwell
taylorotwell merged commit af816e5 into laravel:10.x Feb 13, 2024
@driesvints

Copy link
Copy Markdown
Member

Thanks @oprypkhantc. Appreciate the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DB::afterCommit() callbacks and after commit jobs are incorrectly executed in the transaction in tests

3 participants