Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Transaction aware code execution #35373

Merged
merged 7 commits into from Nov 27, 2020
Merged

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented Nov 26, 2020

This PR is a followup on #35266 and the functionality introduced here, if merged, will be used in the other PR.

So, this PR introduces a transaction manager class that records transactions, commits, and rollbacks. It also records code execution that should be transaction aware. This code will only be executed after transactions are committed and will be discarded if transactions were rolled back.

DB::transaction(function () {
    $user = User::create([...]);

    $user->teams()->create([...]);
});

Now imagine there's a listener registered on the user.created eloquent event that sends a welcome email. Inside the listener, you can do this:

public function handle(){
    DB::afterCommit(function (){
        Mail::send(...);
    });
}

The callback passed to afterCommit will be stored in a local cache and will be executed once the transaction is committed. If the transaction was rolledback, the callback will be discarded.

@ibrunotome
Copy link
Contributor

@Gummibeer
Copy link
Contributor

Gummibeer commented Nov 26, 2020

Wouldn't it be more like an afterTransaction()? 🤔
The withinTransaction() sounds more like "run this code only if you are within a transaction" or "run this code in the same transaction as the event is fired in".

PS: the afterTransactions() would also match the proposed method on a pending job.

Solved: 04df2e1

@TitasGailius
Copy link
Contributor

What if we dispatched the user.created event only after the transaction is committed?

I know this would be a breaking change but it always felt strange to me that certain eloquent events are dispatched even if the data is not actually persisted yet.

@Gummibeer
Copy link
Contributor

@TitasGailius and how would you run logic that has to be done after the model is created but inside the same transaction?
For example sending a verification email for a user. The user shouldn't be created when dispatching the mail failed as it would be impossible for the user to login.

@TitasGailius
Copy link
Contributor

@Gummibeer oh, I see. Good point. 👍

@taylorotwell
Copy link
Member

Just so I understand, this means the listener code like in your example must be aware that it is run within a transaction?

@gilbertorussi
Copy link
Contributor

gilbertorussi commented Nov 26, 2020

Maybe UserCommitted/UserPersisted event that runs when the transaction the user is created is committed/persisted?

@themsaid
Copy link
Member Author

@taylorotwell this PR introduces a generic function to make code execution transaction-ware. In future PRs, we can add config values to make listeners, jobs, mail, etc... transaction-aware by default.

@stevebauman
Copy link
Contributor

stevebauman commented Nov 26, 2020

What about being able to assign a name to database transactions inside of the DatabaseTransactionManager?

For example (pseudo API):

DB::transaction('user.creation', $callback)
DB::afterCommit('user.creation', $callback)

Then it is immediately clear that the callback is associated with a specific transaction. Thoughts?

@negoziator
Copy link

I find I really strange to have it as a function. Personally I would like to ser this as a trait or something similar.

@ibrunotome
Copy link
Contributor

I find I really strange to have it as a function. Personally I would like to ser this as a trait or something similar.

This guy made this https://github.com/fntneves/laravel-transactional-events, I've been using for years and this maybe is the opportunity to make it native in the core of Laravel.

@paras-malhotra
Copy link
Contributor

I believe it's better to just leave it at the callbacks (after commit / rollback) and provide an out-of-the-box functionality for jobs/queue dispatches to be transaction-aware. Extending the same for events (for example) may not be very intuitive. Many events may want to run within the same transaction.

And if the events/mail, etc. are queueable, they would automatically be handled by the queue component.

@mfn
Copy link
Contributor

mfn commented Nov 26, 2020

public function handle(){
    DB::afterCommit(function (){
        Mail::send(...);
    });
}

What happens if this is running without an enclosing transaction?

@themsaid
Copy link
Member Author

@mfn the code will be executed rightaway if there's no transaction.

@KieronWiltshire
Copy link

KieronWiltshire commented Jan 13, 2021

I would just like some clarification on this. What would be the outcome in this scenario:

DB::transaction(function() {
    $user = User::create();

    $profile = DB::transaction(function() use ($user) {
        $model = Profile::create(['user_id' => $user->id]);

        DB::afterCommit(function() {
            echo 1;
        });

        return $model;
    });

    DB::afterCommit(function() {
        echo 2;
    });

    return $user;
});

I'm assuming the whole transaction would commit, then it would echo 1 then 2.

@nmfzone
Copy link
Contributor

nmfzone commented Feb 17, 2022

@themsaid Sorry for bothering, I just need a confirmation.

Why listener that implements ShouldQueue and use Sync driver is not dispatched after transaction commit, even when I set $afterCommit = true?

But, when the listener doesn't implement ShouldQueue, it would dispatched after transaction commit. Any reason?

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.

None yet