[10.x] Dispatch events based on a DB transaction result

[mateusjatenee]

This is another stab at #48631.

Right now, if we have event dispatching inside a database transaction, there are 2 ways to ensure events are not published:

1. By publishing all of them after the transaction:

DB::transaction(function () {
    $model->something();
    $model->somethingElse();
});

foreach ($model->flushEvents() as $event) {
    event($event);
}

#48631 aimed to provide syntatic sugar for that use case.

2. By making every listener queued and enable afterCommit.

I don't think option 2 is very good for a couple reasons:

• The event still gets published, so if you had a test to ensure nothing happens if the transaction fails, Event::assertNotDispatched() would fail.
• It only works if the listeners are queued
• You have to remember to enable afterCommit

What this PR aims to do is to make the event itself aware of transactions. So, if a transaction fails, the event doesn't even get published.
That way, it doesn't matter if the listeners are queued or not or if they have afterCommit enabled, and you can ensure, in the tests, that the event did not get published.

[mpyw]

It may be advised from fntneves/laravel-transactional-events's author @fntneves

[mfn]

Also it may be advised from fntneves/laravel-transactional-events's author @fntneves

Thanks for bringing this up, I'll also closely monitor this PR.

fntneves/laravel-transactional-events has been an integral part for years for the projects I'm working on and it's a great to not have to think about afterCommit or similar solution; interface-tagged events are a blessing and the best part is that you can just always tag them and without transaction they just work, too.

Also important to know is that laravel-transactional-events supports nested transaction very well. The bar is high 😁

[fntneves]

Hi @mateusjatenee,

It is good to see you bringing this concern into the core of Laravel!

I've thought about it in depth in the past, let me know if you need any help improving it.

[mateusjatenee]

@mfn the bar is most definitely very high!

I actually though of reaching out to @fntneves first but I wanted to see if this would get any sort of traction first. He's done fantastic work with the package, and I would love to see this feature baked into the framework.

The package's implementation is a bit more complex (and I think we could go that way in the future) — maybe even have the Dispatcher handoff the event to a SyncEventHandler or a TransactionalEventHandler (the name is just an idea since Dispatcher is already taken).

[mpyw]

Although it's not a BC, some people may prefer targeting for 11.x. It depends on maintainers' opinions

[taylorotwell]

@mateusjatenee what happens if no transaction is currently happening, or even doesn't happen during the entire request?

[mateusjatenee]

@mateusjatenee what happens if no transaction is currently happening, or even doesn't happen during the entire request?

If there is no transaction happening at the time the event is published, the callback is executed immediately (normal behavior). That behavior is guaranteed by DatabaseTransactionsManager::addCallback(). This test covers that.

We could also verify whether a transaction is happening on that if block, and skip if there's no active transaction.

I also added a test that covers nested transactions (if the parent one succeeds and a child transaction fails, an event published on the parent one should still succeed as well).

[taylorotwell]

I expanded the scope of this PR a bit 😅

I didn't like how after this PR we would have two ways of indicating "afterCommit" behavior - most existing support for this feature utilizes a $afterCommit = true property on the class while this PR uses a marker interface.

My updates continue to support the property based syntax but also introduce an interface based syntax to the other places in the framework that currently support "after commit" behavior (jobs, event listeners, mail, notifications, and model broadcasting).

This is accomplished using a new ShouldQueueAfterCommit interface which may be implemented on jobs, listeners, mail, and notifications instead of the plain ShouldQueue interface. A new BroadcastsEventsAfterCommit trait has been introduced for Eloquent to compliment the plain BroadcastsEvents trait. In addition, listeners may utilizes a ShouldHandleEventsAfterCommit interface.

class ExampleEvent implements ShouldDispatchAfterCommit
{
    // ...
}

class ExampleListener implements ShouldHandleEventsAfterCommit
{
    // ...
}

class ExampleJob implements ShouldQueueAfterCommit
{
    // ...
}

class SomeMailable extends Mailable implements ShouldQueueAfterCommit
{
    // ...
}

// etc...

[taylorotwell]

@fntneves would appreciate your final review on this PR and any thoughts you have since you have maintained a package to achieve a similar goal. ❤️

[fntneves]

One test case that comes to my mind:

• Dispatch all events, including ones registered within nested transactions, only after the root transaction commits.

Is this already handled somehow by $afterCommit implementation? It is important to do a sanity check, here.

[mateusjatenee]

That's a good catch. I'm adding some tests.
If you check DatabaseTransactionsManager. It only executes the callbacks once the all the transactions have been handled. This is the ManagesTransactions::commit() code:

    public function commit()
    {
        if ($this->transactionLevel() == 1) {
            $this->fireConnectionEvent('committing');
            $this->getPdo()->commit();
        }

        $this->transactions = max(0, $this->transactions - 1);

        if ($this->afterCommitCallbacksShouldBeExecuted()) {
            $this->transactionsManager?->commit($this->getName());
        }

        $this->fireConnectionEvent('committed');
    }

[mateusjatenee]

@fntneves Added the test here, let me know if that was in line with what you were thinking!

[fntneves]

That's it! Thank you, you're nailing it!

[mateusjatenee]

@fntneves @taylorotwell I've added two nested transactions tests that are almost the same — the difference is that in one, the parent transaction event is dispatched in the beginning, and on the second it, it's dispatched at the end.
I think it's important to keep both because of how DatabaseTransactionsManager works.

addCallback always assumes that you're on the last applicable transaction. So when you do this:

        DB::transaction(function () { // this adds a transaction at level 1
            DB::transaction(function () { // this adds a transaction at level 2
                Event::dispatch(new ShouldDispatchAfterCommitTestEvent); // this pushes a callback to the level 2 object
            });

            // This also pushes a callback to the level 2 object, because the level 2 object is only removed if the level 2 transaction is rolled back.
            Event::dispatch(new AnotherShouldDispatchAfterCommitTestEvent);
        });

As you can see, both callbacks get added to transaction 2 because it's the last object in the array.
This, however, works differently:

        DB::transaction(function () { // Adds transaction at level 1
            Event::dispatch(new AnotherShouldDispatchAfterCommitTestEvent); // Adds a callback to the level 1 object

            DB::transaction(function () { // Adds transaction at level 2
                Event::dispatch(new ShouldDispatchAfterCommitTestEvent); // Adds a callback to the level 2 object
            });

            // Although the child transaction has been concluded, the parent transaction has not.
            // The event dispatched on the child transaction should not have been dispatched.
            $this->assertFalse(ShouldDispatchAfterCommitTestEvent::$ran);
            $this->assertFalse(AnotherShouldDispatchAfterCommitTestEvent::$ran);
        });

I don't think this could cause any bugs right now because even if you have 2 DatabaseTransactionRecord objects, and a callback from level 1 gets added to level 2, they'd still be executed correctly, but it might be good to keep this since the behavior differs in case something changes in the future.
The only bug I could think of is — what if you have 2 callbacks from different levels (e.g 1 and 2) incorrectly placed in the 2nd level, and that transaction happens to fail? Then the callback from level 1 would be mistakenly removed, but that shouldn't be possible due to temporal constraints. If transaction 2 were to fail, it'd be removed from the array and callback 2 would be added to level 1 as intended.

I'm not sure I worded this properly, let me know if that makes sense.

[crynobone]

Doesn't this make Illuminate Events depends on Illuminate Database package?

[fntneves]

According to the current implementation, if there is no instance bound (in this case, TransactionManager), the events are immediately dispatched (the afterCommit logic is the responsibility of the projects using it).

https://github.com/laravel/framework/pull/48705/files#diff-260e35f57adc10558ea232b625f9143bab31003c4fa1b7a998ce6fae381667ebR563

Therefore, it does not depend on the Illuminate DB package.

[fntneves]

I don't think this could cause any bugs right now because even if you have 2 DatabaseTransactionRecord objects, and a callback from level 1 gets added to level 2, they'd still be executed correctly, but it might be good to keep this since the behavior differs in case something changes in the future. The only bug I could think of is — what if you have 2 callbacks from different levels (e.g 1 and 2) incorrectly placed in the 2nd level, and that transaction happens to fail? Then the callback from level 1 would be mistakenly removed, but that shouldn't be possible due to temporal constraints. If transaction 2 were to fail, it'd be removed from the array and callback 2 would be added to level 1 as intended.

I'm following you. And I agree that that behaviour should not cause bugs. At least, in your example.

What happens, though, if we have the following?

DB::transaction(function () { // Adds transaction at level 1
            DB::transaction(function () { // Adds transaction at level 2
                // Callback #1
                Event::dispatch(new ShouldDispatchAfterCommitTestEvent); // Adds a callback to the level 2 object
            });
            
            // Callback #2
            Event::dispatch(new AnotherShouldDispatchAfterCommitTestEvent); // Adds a callback to the level 1 object
            
            try {
                DB::transaction(function () { // ### Adds transaction to the level XXXXX object ?????
                    // Callback #3
                    Event::dispatch(new ShouldDispatchAfterCommitTestEvent); // Adds a callback to the level XXXXX object
                    throws new \Exception();
                });
            } catch (Exception $e) {}
        });

In this case, at the end of the root transaction, I expect the following callbacks to be triggered: #1 and #2.

My doubts are about which level will Callback #3 be attached to and what happens to that level's object when the second nested transaction fails. If it is the same as the first nested transaction (level 2), I suspect that Callback #1 might never be called.

While we can consider this might not happen, one reason I believe we should check it is when third-party packages already have transactions that combined with the ones in the main project, might cause such behaviour without one notice. This would be a nightmare to debug.

[taylorotwell]

@fntneves we are going to work on that as a separate bug / issue

[woodspire]

Since this has been merged, adding documentation to laravel/docs should be done.

[mateusjatenee]

Working on docs here: https://github.com/laravel/docs/pull/9106/files

[driesvints]

Hey @mateusjatenee. This PR was merged with failing tests. Please only mark a PR as ready when it has passing tests.

[mateusjatenee]

My bad @driesvints. I'll pay more attention next time — added a PR here #48858