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.

[mpyw]

I think it is not bad as a temporary fix, but not enough as a permanent solution. Let @taylorotwell decide on that.

[mpyw]

@tonysm How about leaving FIXME: comment on your code? That would be perfect as temporary fixes.

[fuwasegu]

Hmmm...
Of course, this may solve the problem, but it seems to be a temporary solution.
And isn't this destructive?
Without deep consideration of various patterns, inconsistencies may occur in transaction management.
I think there are many projects that take advantage of the ease of transaction nesting in Laravel, and nest transactions rather easily (like putting them in the Repository layer, but also in the UseCase layer just to be safe).

I don't have any concrete ideas, but I think it would be better if RefreshDatabase Trait can do something about it.

[crynobone]

A better solution is not to use RefreshDatabase for such usage as it's always going to include a database transaction.

[mpyw]

A better solution is not to use RefreshDatabase for such usage as it's always going to include a database transaction.

Fair enough, but it's still just a work-around. I think this is clearly a bug and would be happy to see a fundamental fix.

[mpyw]

Commented on the PR: #48466 (comment)

[mpyw]

While working on this issue I found a new bug. It appears that multiple connections are not being handled correctly.

• afterCommit() callbacks do not fire when RefreshDatabase handles multiple $connectionsToTransact #48472

[mpyw]

@tonysm According to #48472, the multiple $callbacksShouldIgnore support in PR #48466 itself is probably useful. However, I don't like the idea of only focusing the support to createOrFirst(), and think it should be more in line with PR #48471.