DB::afterCommit callbacks aren't run in tests when RefreshDatabase trait is used

[mariomka]

• Laravel Version: v8.21.0
• PHP Version: 7.4.11
• Database Driver & Version: psql (PostgreSQL) 12.4 (Ubuntu 12.4-1.pgdg20.04+1) - It doesn't depend on DB driver.

Description:

DB::afterCommit callbacks aren't run when RefreshDatabase trait is used because ManagesTransactions trait only commits when all transactions are executed, but RefreshDatabase uses a transaction. Then the callbacks aren't called.

Steps To Reproduce:

I've created a reproduction repo: https://github.com/mariomka/laravel-after-commit-repro

There are two tests, one with RefreshDatabase (it fails) and one without RefreshDatabase (it passes): https://github.com/mariomka/laravel-after-commit-repro/tree/master/tests

Both tests run the same "action": https://github.com/mariomka/laravel-after-commit-repro/blob/master/app/AfterCommitAction.php

Step by step reproduction:

git clone [email protected]:mariomka/laravel-after-commit-repro.git
cd laravel-after-commit-repro
cp .env.example .env
composer install
php vendor/bin/homestead make
# Enable postgresql in Homestead.yaml
vagrant up
vagrant ssh
cd code
php vendor/bin/phpunit

[driesvints]

I've installed your repo locally and both tests pass for me. Can you first please try one of the support channels below? If you can actually identify this as a bug, feel free to report back and I'll gladly help you out and re-open this issue.

• Laracasts Forums
• Laravel.io Forums
• StackOverflow
• Discord
• Larachat
• IRC

Thanks!

[mariomka]

@driesvints have you used MySQL or Postgres driver? with SQLite runs because I think it doesn't run transactions

[driesvints]

Ah, you said:

It doesn't depend on DB driver.

So I tried with SQLite indeed.

But still, can you maybe try a support channel first? DB::afterCommit isn't documented atm and I suspect it might just be a misplacement. I'm not 100% sure myself.

[mariomka]

@driesvints I'm Sorry, I didn't think about SQLite.

I'm sure that this is not intended behavior, it works perfectly outside the tests. The issue only occurs in tests.

Checkout this fragment:

framework/src/Illuminate/Database/Concerns/ManagesTransactions.php

Line 43 in 374e956

try {

It only commits when there is only one transaction, if I run the action directly without tests it runs because there is only one transaction but for testing, I need to use RefreshDatabase and it uses a transaction.

There is the line where DatabaseTransactionsManager runs callbacks when a transaction is committed:

framework/src/Illuminate/Database/DatabaseTransactionsManager.php

Line 59 in 374e956

public function commit($connection)

[driesvints]

@mariomka can you maybe try a support channel first? If you can confirm the bug feel free to report back.

[mariomka]

But still, can you maybe try a support channel first? DB::afterCommit isn't documented atm and I suspect it might just be a misplacement. I'm not 100% sure myself.

@driesvints Thank you for your time but I don't know what you want me to ask the community. Seeing some comments on #35373 (#35373 (comment), #35373 (comment), #35373 (comment)) DB::afterCommit should be inside a transaction, then when the transaction is committed callback is called. In fact, it works fine. However, it is untestable when the test needs the RefreshDatabase trait (common when you are using a DB in tests).

[osteel]

Hi,

I've got the same issue. I've just discovered $afterCommit, applied it to one of my observers and now the related tests fail, because yeah, the database transaction is never committed as a result (using a MySQL instance to run my tests too, not SQLite). I guess we'd need a way to ignore $afterCommit in the context of a test suite

[mfn]

I guess we'd need a way to ignore $afterCommit in the context of a test suite

This feels wrong, then you do not know whether something was supposed to be done or not?

[osteel]

This feels wrong, then you do not know whether something was supposed to be done or not?

Not entirely sure what you mean, but since $afterCommit = true needs all transactions to be committed, but Laravel won’t let said transactions get committed in the context of a test suite using MySQL, the only way to test whether what’s supposed to happen once they are committed does happen, would seem to be able to ignore $afterCommit = true in the context of the test suite, somehow.

[mfn]

Wait.

Doesn't it support nested transactions?

Apologies, because I use https://github.com/fntneves/laravel-transactional-events and the transaction opened via the trait (to wrap a test in transactions) does not count as influencing "transactionable ware" code triggered in nested transactions.

(btw I use this together with Postgres with great success).

It would be irritating to me to test a part of an application, which makes use of such a feature and then not knowing whether it was supposed to be executed or not just because the test is wrapped in a transaction. 🤔

[phuclh]

@mariomka I have the same issue when using DB::transaction(), but not when handling the transaction manually with DB::beginTransaction() and DB::commit(). Do you have the answer for this?

[simonmaass]

anyone find a solution for this? i am trying to test a job that creates some models in an DB transaction... and i am trying to test that some jobs are dispatched in the model observer... we are using afterCommit = true in the observer... and they are not being fired

[tylernathanreed]

@simonmaass

I just found this thread, and I didn't see an answer either. I went ahead and solved it myself. For everyone else coming here in the future, here's how I resolved this issue.

The way Laravel handles the "after commit" functionality is by the new DatabaseTransactionsManager introduced in Laravel 8.x. All forms of "after commit" logic funnel into the addCallback method:

public function addCallback($callback)
{
    if ($current = $this->transactions->last()) {
        return $current->addCallback($callback);
    }

    call_user_func($callback);
}

As seen above, if the connection currently has a transaction, all of the callbacks are bound to the ending of that transaction (which in the case of testing, has the scenario where it may never commit).

I overrode the DatabaseTransactionsManager like so:

<?php

namespace App\Providers;

use App\Testing\DatabaseTransactionsManager;
use Illuminate\Support\ServiceProvider;

class TestServiceProvider extends ServiceProvider
{
    /**
     * Register the service provider.
     *
     * @return void
     */
    public function register()
    {
        if (! $this->app->runningUnitTests()) {
            return;
        }

        $this->registerDatabaseTransactionsManager();
    }

    /**
     * Registers the database transaction manager.
     */
    protected function registerDatabaseTransactionsManager()
    {
        // Our automated tests wrap everything inside of a database
        // transactions, which means the "afterCommit" behavior
        // is never invoked. We'll need to tweak this a bit.

        $this->app->singleton('db.transactions', function () {
            return new DatabaseTransactionsManager;
        });
    }
}

This binds my own DatabaseTransactionsManager in place of the original one. Here's what that looks like:

<?php

namespace App\Testing;

use Illuminate\Database\DatabaseTransactionsManager as Manager;

class DatabaseTransactionsManager extends Manager
{
    /**
     * The baseline transaction depth.
     */
    protected $baselineDepth = 0;

    /**
     * Register a transaction callback.
     *
     * @param  callable  $callback
     *
     * @return void
     */
    public function addCallback($callback)
    {
        if ($this->transactions->count() > $this->baselineDepth) {
            return parent::addCallback($callback);
        }

        call_user_func($callback);
    }

    /**
     * Updates the baseline transaction depth to the current transaction depth.
     *
     * @return $this
     */
    public function captureTransactionDepth()
    {
        $this->baselineDepth = $this->transactions->count();

        return $this;
    }
}

The new manager allows you to capture a "baseline depth", of which, upon reaching it, all callbacks are immediately fired, rather than deferred to a transaction committal.

Now you just need to capture the transaction depth in the correct place, which can be done inside of your test:

public function setUp(): void
{
    parent::setUp();

    $this->app->make('db.transactions')->captureTransactionDepth();
}

And with that all in place, any "after commit" calls on top of the refresh transactions will be called immediately. If you have a transaction within your test, the callback will be deferred until the end of that specific transaction, but will still be called at some point in your test.

[pyrou]

@tylernathanreed your solution only work if only one transaction is perform in your application code. I implemented your solution, and with following code in my controller :

                DB::transaction(function () {
                   DB::afterCommit(function () { dump('This is executed'); });
                });
                DB::transaction(function () {});
                DB::transaction(function () {});
                DB::transaction(function () {
                    DB::afterCommit(function () { dd('This is not'); });
                });

it doesn't dd()

Because, each time you start a transaction with DB::transaction(function () {}); it increment the $this->transactions->count().
But, there is no code that can decrement it. Transaction cleaning is done when calling commit() on the DatabaseTransactionsManager which is only called when the very first level transaction is committed. (Check code here :

framework/src/Illuminate/Database/Concerns/ManagesTransactions.php

Lines 48 to 52 in b9203fc

	$this->transactions = max(0, $this->transactions - 1);
	if ($this->transactions == 0) {
	$this->transactionsManager?->commit($this->getName());
	}

) (That transaction in our concerns would be the one that hold the database refresh. or here

framework/src/Illuminate/Database/Concerns/ManagesTransactions.php

Lines 194 to 198 in b9203fc

	$this->transactions = max(0, $this->transactions - 1);
	if ($this->transactions == 0) {
	$this->transactionsManager?->commit($this->getName());
	}

[pyrou]

@driesvints Can you reconsider re-opening this issue ? It looks we are several people (including me) experiencing this issue with non-sqlite db driver.

[driesvints]

Sure. Appreciating PR's to solve this.

[simonmaass]

@driesvints could you please also re-open this issue?

[aromka]

+1 subscribing for the solution.

For now this workaround works:

public bool $afterCommit = true;

public function __construct()
{
        if (app()->runningUnitTests()) {
            $this->afterCommit = false;
        }
}

[taylorotwell]

Potential fix #41782

Would appreciate feedback / testing.

Of course, the other fix / existing work around is to use the DatabaseMigrations trait instead of RefreshDatabase for your test cases that test things using afterCommit.

[shadoWalker89]

Personally I very much appreciate everything @taylorotwell and his team are doing on managing a free framework that makes our life easier.
@driesvints is making an excellent job at handling the laravel repos.
I totally agree that the issues isn't a forum and is reserved specifically to framework bugs reporting, and issues that are usage questions, or asking for help with something should be closed and the author should try the forum ...
But when it comes to issues that are not usage questions, but issues that seem to be an issue shouldn't be closed as fast as it currently happens, because it could be a real bug, closing the issue very fast will not let it be visible and no one will attempt to fix it.
Having an open issue that we are not 100% sure is a bug and allow the community to see it, discuss and come up with a solution is better then closing it right away.
Maybe when an issue that is not a confirmed bug could be labeled and after some period let's say 20 days if no one showed an interest in that particular issue, then it could be closed. Maybe also you can use a bot to do that.
I hope you will take this into consideration.
Hope all the best for you guys.

[driesvints]

I truly appreciate the kind words @shadoWalker89 but we're not going to change the way we work, sorry.

[BrandonSurowiec]

Hey @driesvints I just ran into this DB::afterCommit callback testing bug in 8.x for one of my projects. (8.x is also the version in which the bug was reported.) When Taylor fixed it, he sent it to 9.x in Apr 2022 a few months before the bug support policy ran out for 8.x. Could we get the fix in #41782 backported? I can send in a PR, but I worry about it getting closed.

[driesvints]

I'm sorry @BrandonSurowiec but 8.x is closed for bug fixes. You should upgrade as soon as you can.