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

TransactionCommitted event doesn’t contain transaction level I’d expect it to #30756

Closed
martinbean opened this issue Dec 4, 2019 · 10 comments
Labels

Comments

@martinbean
Copy link
Contributor

  • Laravel Version: 6.6.1
  • PHP Version: 7.3.11
  • Database Driver & Version: MySQL 5.7.27

Description:

When the TransactionCommitted event is dispatched, it seems to contain the transaction level after the transaction has been committed, rather than the transaction level the connection was at when the transaction was committed.

For example, if I begin a transaction, that dispatches a TransactionBeginning event and the transaction level on the connection is incremented to 1. When that transaction is committed, a TransactionCommitted event is dispatched but the transaction level is 0. I’d expect the transaction level for the corresponding TransactionCommitted event to be 1 as well, so I can listen for when a transaction is started and committed.

Steps To Reproduce:

  1. Start a transaction. Transaction level in TransactionBeginning event is 1.
  2. Commit transaction. Transaction level in TransactionCommitted event is 0 (one less than transaction level in TransactionBeginning event).

Test Code

I created a simple route that writes a row within a transaction, and set up listeners on the TransactionBeginning and TransactionCommitted events to log the transaction level:

Route::get('/', function () {
    DB::transaction(function () {
        App\User::create([
            'name' => 'John Doe',
            'email' => sprintf('john.doe+%s@example.com', uniqid()),
            'password' => Hash::make('password'),
        ]);
    });

    return response('Done.');
});
<?php

namespace App\Listeners;

use Illuminate\Database\Events\ConnectionEvent;
use Illuminate\Support\Facades\Log;

class LogTransactionLevel
{
    public function handle(ConnectionEvent $event)
    {
        Log::debug(
            sprintf('Transaction level for %s event is %d', get_class($event), $event->connection->transactionLevel())
        );
    }
}

Contents of log file after hitting route:

[2019-12-04 13:12:53] local.DEBUG: Transaction level for Illuminate\Database\Events\TransactionBeginning event is 1  
[2019-12-04 13:12:53] local.DEBUG: Transaction level for Illuminate\Database\Events\TransactionCommitted event is 0  
@martinbean martinbean changed the title TransactionCommitted event doesn’t contain level I’d expect it to TransactionCommitted event doesn’t contain transaction level I’d expect it to Dec 4, 2019
@driesvints
Copy link
Member

Hmm I can indeed see that as problematic. Appreciating a PR if you could whip up one. Thanks for reporting.

@driesvints driesvints added the bug label Dec 5, 2019
@martinbean
Copy link
Contributor Author

martinbean commented Dec 5, 2019

@driesvints Thanks for confirming! Will work on a PR 🙂 What branch should I open the PR against?

@driesvints
Copy link
Member

Probably master

@AReyes-cl
Copy link

@driesvints Thanks for confirming! Will work on a PR 🙂 What branch should I open the PR against?

Please read https://laravel.com/docs/6.x/contributions

@martinbean
Copy link
Contributor Author

@AReyes-cl Was that really necessary? @driesvints already gave an answer. A week ago.

I wasn’t sure whether this was a bug fix or a “major new feature”, which is why I asked.

@simoebenhida
Copy link
Contributor

@martinbean I think it makes sense that TransactionCommitted returns $transactions - 1 since it means that the previous transaction has been resolved or committed ?

@denvit
Copy link

denvit commented Dec 18, 2019

@martinbean according to the docs https://laravel.com/docs/6.x/contributions#which-branch:

All bug fixes should be sent to the latest stable branch or to the current LTS branch. Bug fixes should never be sent to the master branch unless they fix features that exist only in the upcoming release.

@driesvints
Copy link
Member

@denvit an answer was already provided for this and someone already noted this above.

I'm going to lock this as I believe there isn't really anything left to discuss here. Anyone's free to pr in a fix for this.

@laravel laravel locked as off-topic and limited conversation to collaborators Dec 19, 2019
@driesvints
Copy link
Member

PR was merged.

@driesvints
Copy link
Member

#30883

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants