`RefreshDatabase` + double wrapping in `DB::transaction()` breaks `$afterCommit` functionality

[ryanldy]

Laravel Version

10.23.1

PHP Version

8.2.10

Database Driver & Version

8.0.33

Description

In tests, model observer created does not run if all condition applies:

1. Test uses mysql DB connection.
2. Test uses RefreshDatabase trait.
3. Using createOrFirst() or firstOrCreate() method and the method is inside DB::transaction().

This issue started to appear in laravel/framework 10.21. I think it was introduced in #48144

cc: @tonysm / @mpyw

Steps To Reproduce

1. Clone TestDBTransaction sample project.
2. Make sure to update mysql database connection in your .env file.
3. Run vendor/bin/phpunit. Test should pass but it fails.
4. Try to comment the lines under Failing Test (line 27 and 30) section and uncomment a line from the Passing Tests (line 35 or 38) in NoteObserverTest. Test now passes.

[mpyw]

Confirmed. Thanks for reporting

[mpyw]

It appears to be a problem related to the RefreshDatabase trait. Both succeed when no trait is used

[mpyw]

I have disabled the savepoint fixes and double-wrap in DB::transaction() in the test case, but the test still fails. In short, I don't think the bug is caused by the createOrFirst() related changes, but by the incompatibility of the RefreshDatabase trace and the $afterCommit feature.

     public function withSavepointIfNeeded(Closure $scope): mixed
     {
+         return $scope();
-         return $this->getQuery()->getConnection()->transactionLevel() > 0
-             ? $this->getQuery()->getConnection()->transaction($scope)
-             : $scope();
     }

DB::transaction(fn () => DB::transaction(fn () => $user->notes()->createOrFirst(['notes' => 'abc'])));

This is a more serious problem than we thought.

Related to:

• [8.x] Delay pushing jobs to queue until database transactions are committed #35422 (@themsaid)
• DB::afterCommit callbacks aren't run in tests when RefreshDatabase trait is used #35857 (@mariomka)
• [9.x] Fix afterCommit and RefreshDatabase #41782 (@taylorotwell)

[mpyw]

@ryanldy Would you rename the issue into the following?

`RefreshDatabase` + double wrapping in `DB::transaction()` breaks `$afterCommit` functionality

[mpyw]

@pabloelcolombiano has also mentioned this problem: #35422 (comment)

[ryanldy]

@mpyw thank you. Done renaming the issue.

[mpyw]

@taylorotwell already tried to fix this issue in #41782, but it was not enough when transactions were nested. Would you review the changes?

[tonysm]

@mpyw do you think #48466 fixes this issue?

[mpyw]

@tonysm I'm not sure... At least, I don't think that dealing specifically for withSavePointIfNeeded() is the best solution, since this problem is reproduced even if DB::transaction() is simply nested on the user code side.

[mpyw]

Perhaps we should pass the following test with RefreshDatabase.

DB::transaction(fn () => DB::transaction(fn () => $user->notes()->create(['notes' => 'abc'])));

[tonysm]

I don't think it's a problem in real apps. It's only when testing (because the tests run in a transaction), so I think this should be a good fix for now.