Laravel 10: DB::transactionLevel() shows incorrect level, if DDL operation was done

[mrckzgl]

Laravel Version

10.28.0

PHP Version

8.2.11

Database Driver & Version

10.10.2-MariaDB-1:10.10.2+maria~ubu220

Description

We encounter that DB::transactionLevel() shows an incorrect level after a DDL operation was done, which does commit implicitly. Consider this code snippet which is the current behaviour:

Log::debug(DB::transactionLevel()); // 0
DB::beginTransaction();
Log::debug(DB::transactionLevel()); // 1
DB::commit();
Log::debug(DB::transactionLevel()); // 0

DB::beginTransaction();
Log::debug(DB::transactionLevel()); // 1
// some DDL operation which closes the transaction
Log::debug(DB::transactionLevel()); // 1

This is problematic on different levels. First, getting the correct transaction level is important to be able to close all pending transactions. Second, due to a current change (see: #35380), DB::commit() and DB::rollback() both throw an exception if no active transaction is present. That means after beginTransaction and a DDL statement, this code:

if (DB::transactionLevel() > 0) {
  DB::commit();
}

will fail.

Steps To Reproduce

DB::beginTransaction();
Schema::dropIfExists('test');
Schema::create('test', function (Blueprint $table) {
    $table->id();
});
if (DB::transactionLevel() > 0) {
  DB::commit();
}

I just confirmed this behaviour in a minimal example, happy to provide a repo if needed.

[crynobone]

Hi there,

We encounter that DB::transactionLevel() shows an incorrect level after a DDL operation was done, which does commit implicitly

I don't believe commit should be done explicitly but you should be able to override db.transactions and handle it based on your requirements.

framework/src/Illuminate/Database/DatabaseServiceProvider.php

Lines 77 to 79 in a47df68

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

The current db.transactions is well-tested and working as intended.

[crynobone]

Also, this is documented here: https://laravel.com/docs/10.x/database#implicit-commits-in-transactions

[mrckzgl]

@crynobone Thank you very much for the documentation link, but I am unsure if there is a misunderstanding here. That documentation explicitly only applies to the statement and unprepared methods of the DB facade:

When using the DB facade's statement and unprepared methods within transactions you must be careful to avoid statements that cause implicit commits.

But the problem here occurs even if these methods are not used, but if Laravel methods like Schema::XXX are used. Please, I invite you to have again a look at the Step to Reproduce. I can understand that it is difficult (but probably not impossible) to track transaction level for raw SQL statements done by the user. However, in the example above we only use Laravel database abstraction methods, so the framework should very well be able to track the transaction level correctly internally there, and frankly this is what I would expect.

I don't believe commit should be done explicitly

I am not sure if I understand you correctly here. If we start a transaction manually via DB::beginTransaction() we have to explicitly commit at the end. This consumption of the Laravel API is officially documented here: https://laravel.com/docs/10.x/database#manually-using-transactions
I assume you refer to the other case, where transactions are run inside the callback of DB::transaction? I just reproduced, however that also in this case the internal transaction level is not updated correctly on a DDL operation done using Schema::XXXmethods. This example:

        DB::transaction(function () {
            Schema::dropIfExists('test');
            Schema::create('test', function (Blueprint $table) {
                $table->id();
            });
            $this->printTransactionLevel();
        });

Also fails with an "There is no active transaction" PDOException:

[2023-10-19 20:39:11] local.ERROR: There is no active transaction {"view":{"view":"/var/www/laravel_docker/resources/views/index.blade.php","data":{"errors":"<pre class=sf-dump id=sf-dump-142486146 data-indent-pad=\"  \"><span class=sf-dump-note>Illuminate\\Support\\ViewErrorBag</span> {<a class=sf-dump-ref>#277</a><samp data-depth=1 class=sf-dump-expanded>
  #<span class=sf-dump-protected title=\"Protected property\">bags</span>: []
</samp>}
</pre><script>Sfdump(\"sf-dump-142486146\", {\"maxDepth\":3,\"maxStringLength\":160})</script>
"}},"exception":"[object] (Spatie\\LaravelIgnition\\Exceptions\\ViewException(code: 0): There is no active transaction at /var/www/laravel_docker/vendor/laravel/framework/src/Illuminate/Database/Concerns/ManagesTransactions.php:194)
[stacktrace]
#0 /var/www/laravel_docker/vendor/laravel/framework/src/Illuminate/Database/Concerns/ManagesTransactions.php(194): PDO->commit()
#1 /var/www/laravel_docker/vendor/laravel/framework/src/Illuminate/Database/DatabaseManager.php(469): Illuminate\\Database\\Connection->commit()
#2 /var/www/laravel_docker/vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php(353): Illuminate\\Database\\DatabaseManager->__call('commit', Array)
#3 /var/www/laravel_docker/app/Http/Controllers/IndexController.php(27): Illuminate\\Support\\Facades\\Facade::__callStatic('commit', Array)
#4 /var/www/laravel_docker/resources/views/index.blade.php(3): App\\Http\\Controllers\\IndexController->run()

This is the same exception which occurs in the example code provided in the Steps to Reprocude section.
The transaction level at the end of the callback also is 1, instead of 0. This is an indication that the Schema::dropIfExists and Schema::create method do not correctly update the Laravel internal transaction level variable.

[crynobone]

If you refer to link from documentation (MySQL documentation) implicit commit include command used in Schema https://dev.mysql.com/doc/refman/8.0/en/implicit-commit.html

…

On Fri, 20 Oct 2023, 4:44 pm mrckzgl, ***@***.***> wrote: @crynobone <https://github.com/crynobone> Thank you very much for the documentation link, but I am unsure if there is a misunderstanding here. That documentation explicitly only applies to the statement and unprepared methods of the DB facade: When using the DB facade's statement and unprepared methods within transactions you must be careful to avoid statements that cause implicit commits <https://dev.mysql.com/doc/refman/8.0/en/implicit-commit.html>. But the problem here occurs even if these methods are not used, but if Laravel methods like Schema::XXX are used. Please, I invite you to have again a look at the Step to Reproduce. I can understand that it is difficult (but probably not impossible) to track transaction level for raw SQL statements done by the user. However, in the example above we only use Laravel database abstraction methods, so the framework should very well be able to track the transaction level correctly internally there, and frankly this is what I would expect. I don't believe commit should be done explicitly I am not sure if I understand you correctly here. If we start a transaction manually via DB::beginTransaction() we have to explicitly commit at the end. This consumption of the Laravel API is officially documented here: https://laravel.com/docs/10.x/database#manually-using-transactions I assume you refer to the other case, where transactions are run inside the callback of DB::transaction? I just reproduced, however that also in this case the internal transaction level is not updated correctly on a DDL operation done using Schema::XXXmethods. This example: DB::transaction(function () { Schema::dropIfExists('test'); Schema::create('test', function (Blueprint $table) { $table->id(); }); $this->printTransactionLevel(); }); Also fails with an "There is no active transaction" PDOException: [2023-10-19 20:39:11] local.ERROR: There is no active transaction {"view":{"view":"/var/www/laravel_docker/resources/views/index.blade.php","data":{"errors":"<pre class=sf-dump id=sf-dump-142486146 data-indent-pad=\" \"><span class=sf-dump-note>Illuminate\\Support\\ViewErrorBag</span> {<a class=sf-dump-ref>#277</a><samp data-depth=1 class=sf-dump-expanded> #<span class=sf-dump-protected title=\"Protected property\">bags</span>: [] </samp>} </pre><script>Sfdump(\"sf-dump-142486146\", {\"maxDepth\":3,\"maxStringLength\":160})</script> "}},"exception":"[object] (Spatie\\LaravelIgnition\\Exceptions\\ViewException(code: 0): There is no active transaction at /var/www/laravel_docker/vendor/laravel/framework/src/Illuminate/Database/Concerns/ManagesTransactions.php:194) [stacktrace] #0 /var/www/laravel_docker/vendor/laravel/framework/src/Illuminate/Database/Concerns/ManagesTransactions.php(194): PDO->commit() #1 /var/www/laravel_docker/vendor/laravel/framework/src/Illuminate/Database/DatabaseManager.php(469): Illuminate\\Database\\Connection->commit() #2 /var/www/laravel_docker/vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php(353): Illuminate\\Database\\DatabaseManager->__call('commit', Array) #3 /var/www/laravel_docker/app/Http/Controllers/IndexController.php(27): Illuminate\\Support\\Facades\\Facade::__callStatic('commit', Array) #4 /var/www/laravel_docker/resources/views/index.blade.php(3): App\\Http\\Controllers\\IndexController->run() This is the same exception which occurs in the example code provided in the Steps to Reprocude section. The transaction level at the end of the callback also is 1, instead of 0. This is an indication that the Schema::dropIfExists and Schema::create method do not correctly update the Laravel internal transaction level variable. — Reply to this email directly, view it on GitHub <#48777 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABKHJSXSO3BL7SPGT27EFTYAI2YDAVCNFSM6AAAAAA6HZ6K3WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZSGMZDQMRWGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mrckzgl]

@crynobone Yes, but that documentation is from dev.mysql.com. It does not apply to Laravel. As stated, the documentation for Laravel API you referred to only applies to DB::statement and DB::unprepared. It does not apply to other Laravel methods. I invite you to please completely and carefully read my last comment with all the points I presented. A response after just one minute from yours via email indicates that this might not have been done.

[crynobone]

If you have any suggestion to override implicit commit imposed by MySQL and PDO feel free to send in a PR.

[mrckzgl]

Frankly, this is not my task here. I am here to make you, the maintainers of Laravel, aware of a bug in your code, or well if you want to state that as limitation you do not want to address, as a problem in your documentation. I would assume, that you as maintainers have an interest in quality code, and / or quality documentation, but I might be wrong. If this bug turns out to be a limitation you do not want to address, I would kindly ask you to update your documentation so that it correctly reflects the problem I posted initially. Trying to argue that the documentation applies here, if it explicitly does not, is not very helpful or friendly to be honest.

So, to make it more direct: If you do not want to implement DB:transactionLevel() such that it tracks all transaction related state changes Laravel does, I would kindly ask you to include this in the documentation you already pointed to here: https://laravel.com/docs/10.x/database#implicit-commits-in-transactions and also in the actual API reference here: https://laravel.com/api/10.x/Illuminate/Database/ConnectionInterface.html#method_transactionLevel and here: https://laravel.com/api/10.x/Illuminate/Database/Concerns/ManagesTransactions.html#method_transactionLevel

thank you very much

[github-actions]

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

[mrckzgl]

@crynobone Thank you for reopening again. Just another idea to handle DDL statements: Why not open a transaction inside Schema::XXX after any possible implicit commit and of course only if a transaction was active beforehand? This way, transactionLevel stays the same. Of course, this behaviour then should ideally be documented for the respective Schema:XXX methods.

best

[ionutantohi]

I wrote a comment on antoher issue, about having the posibility to resolve a separate db connection when the user wants to issue implict commit statements. Not sure if this is possible/feasible though.

#35380 (comment)

DB::beginTransaction();

DB::isolatedConnection('mysql')->statement('CREATE TABLE ...')

DB::commit();

[tpetry]

Yes, but that documentation is from dev.mysql.com. It does not apply to Laravel. As stated, the documentation for Laravel API you referred to only applies to DB::statement and DB::unprepared.

The Laravel docs share a link to the MySQL resource listing all database command making an implicit commit. So yes, this is clearly documented and everything works as expected and there is no way Laravel can do anything different. It is how MySQL behaves.

The only possible change would be listing migrations as another reason for imilicit commits additionally to the DB::unprepared and DB::statement method.

[mrckzgl]

The Laravel docs share a link to the MySQL resource listing all database command making an implicit commit. So yes, this is clearly documented

I am a quite surprised to have to conduct such discussion. What you write is a wrong statement IMHO. Let's again cite the docs:

When using the DB facade's statement and unprepared methods within transactions you must be careful to avoid statements that cause implicit commits. These statements will cause the database engine to indirectly commit the entire transaction, leaving Laravel unaware of the database's transaction level.

Let's rephrase this: If users use DB::statement or DB::unprepared they have to be careful to avoid DDL statements. The docs do not state, that users need to be careful to avoid DDL statements in other Laravel methods, such as Schema::create and the like. In other words: That users need to be aware of other Laravel methods which might cause implicit commits is not stated in the paragraph above. That a link to the MySQL resource is given which lists all possible SQL statements, which, for example can be explicitly used as argument inside DB::unprepared and DB::statement, does not mean that users need to be aware of other Laravel methods which might internally use these listed SQL statements. That connection is not mentioned at all in the docs and also pretty far fetch IMHO. It is also far fetched, because users might not be aware that Schema:XXX will issue implicit commits as well, but in the first place it is far fetched, because the Laravel framework has all the information needed to know if their Schema:XXX methods will issue an implicit commit and could track the transaction level accordingly.

there is no way Laravel can do anything different.

From my point of view, here are some possible things Laravel could do different. Let's take Schema::create as example.
If a database backend and configuration is used which does an implicit commit after create table and if Schema::create actually issues create table Laravel could:

1. Decrease DB::transactionLevel (such that this method correctly reflects the transaction level), or
2. as suggested above: Start a new transaction if there was any beforehand. That way the code inside DB::transaction will be more consistent as at any point in user space exactly one transaction will be active.

A more generic approach might be to query the database driver if the transaction level has changed at beginning and end of Schema::create and act accordingly (for mariadb server variable in_transaction might be used). This approach might also catch all transaction changes in DB::statement and DB::unprepared. Also the approach of @ionutantohi #48777 (comment) might be interesting to evaluate. Though, I am not sure how mysql will behave in that case and to whom the schema changes in an isolated connection will be visible, or if mysql will commit all active transaction inside a DDL statement, independent of the used connection.

The only possible change would be listing migrations as another reason for imilicit commits additionally to the DB::unprepared and DB::statement method.

If for some reason a change in Laravel behaviour is not feasible or too expensive to implement at the moment: Of course, exactly! That is what I was referring to in #48777 (comment). More precise, not only migrations need to be listed in the docs, but all methods for which Laravel might cause an implicit commit internally. I am explicitly mentioning this, as for example Schema:table can be used outside of a migration, and some frameworks actually do this in production code.
EDIT: And, as stated above, it is not only https://laravel.com/docs/10.x/database which needs to be updated, but the API reference (https://laravel.com/api/10.x) as well.

In general, however, it would be nice if DB::transaction and friends would work regardless of the statements executed inside. Listing in the docs, that one need to be aware of certain Laravel methods to use is of course nice, but that is hard to follow in practise sometimes. As mentioned, we ran into a situation, where a framework we used, internally issued Schema::table inside a listener to some event. This is not so obvious for a user.

best

[crynobone]

Closing this for now, but anyone is still free to submit a PR to https://github.com/laravel/docs if you really want to pursue it.