PHP 8.0 PDO::rollback() now throws PDOException after implicit commit

[stemis]

• Laravel Version: 8.x
• PHP Version: 8.0RC5
• Database Driver & Version: MariaDB

Description:

As this is a difficult subject I will try my best to explain it properly, it is an issue that needs to be addressed properly.

PHP 8.0 changes

In PHP 8.0, the pdo transaction logic was rewritten to some extend.
When issuing a PDO::rollback(), this will now throw a PDOException if the transaction was already closed.

* I was not able to find any official documentation on this yet, but was able to confirm this with the different PHP versions

Background Implicit Commits with Transactions

MySQL has a thing called implicit commits, to demonstrate what this does I have created the following example. I have used MariaDB as this has access to the in_transaction session var to demonstrate the working.

create table Table1
(
    name varchar(10) null
);

START TRANSACTION;

SHOW VARIABLES WHERE Variable_name = 'in_transaction' # Returns 1

INSERT INTO Table1 (name) values ('before');

SHOW VARIABLES WHERE Variable_name = 'in_transaction' # Returns 1

create table Table2
(
    name varchar(10) null
);

SHOW VARIABLES WHERE Variable_name = 'in_transaction' # Returns 0

INSERT INTO Table1 (name) values ('after');

SHOW VARIABLES WHERE Variable_name = 'in_transaction' # Returns 0
ROLLBACK ;

# Table 1 now contains both rows.

So imagine a UnitTest executing any Implicit commit commands, then it commits the transaction. So when running Migration command in a test, or stored procedures, or seeders that have implicit commands in them, a rollback is no longer possible as the transaction was already closed.
In php <=7.4.12 no exception was thrown, so most of the times, you would have no idea why you have data leakage in your test
PHP 8.0 now throwns an exception.

Laravel

How does this affect Laravel?

1. Many applications use DatabaseTransactions or RefreshDatabase traits, which no longer work when implicit commits have been issued in a test.
2. When a rollback is happening with DB::transaction, a new PDOException is thrown.

Consider the following test file:

use Illuminate\Foundation\Testing\DatabaseTransactions;
use Illuminate\Support\Facades\DB;
use Tests\TestCase;

class MyTest extends TestCase
{
    use DatabaseTransactions;
    
    public function testMe()
    {
        DB::unprepared('CREATE TABLE a (col varchar(1) null)');
    }
}

This will generate the following error when running the test:

There was 1 error:

1) Tests\Unit\MyTest::testMe
PDOException: There is no active transaction

/app/vendor/laravel/framework/src/Illuminate/Database/Concerns/ManagesTransactions.php:258
/app/vendor/laravel/framework/src/Illuminate/Database/Concerns/ManagesTransactions.php:237
/app/vendor/laravel/framework/src/Illuminate/Foundation/Testing/DatabaseTransactions.php:31
/app/vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestCase.php:237
/app/vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestCase.php:153

Dump of the exception object:

PDOException {#4783
  #message: "There is no active transaction"
  #code: 0
  #file: "./vendor/laravel/framework/src/Illuminate/Database/Concerns/ManagesTransactions.php"
  #line: 258
  +errorInfo: null
  trace: {
    ./vendor/laravel/framework/src/Illuminate/Database/Concerns/ManagesTransactions.php:258 { …}
    ./vendor/laravel/framework/src/Illuminate/Database/Concerns/ManagesTransactions.php:237 { …}
    ./vendor/laravel/framework/src/Illuminate/Foundation/Testing/DatabaseTransactions.php:31 { …}
    ./vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestCase.php:237 { …}
    ./vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestCase.php:153 { …}
    ./vendor/phpunit/phpunit/src/Framework/TestCase.php:1188 { …}
    ./vendor/phpunit/phpunit/src/Framework/TestResult.php:730 { …}
    ./vendor/phpunit/phpunit/src/Framework/TestCase.php:883 { …}
    ./vendor/phpunit/phpunit/src/Framework/TestSuite.php:669 { …}
    ./vendor/phpunit/phpunit/src/Framework/TestSuite.php:669 { …}
    ./vendor/phpunit/phpunit/src/Framework/TestSuite.php:669 { …}
    ./vendor/phpunit/phpunit/src/TextUI/TestRunner.php:667 { …}
    ./vendor/phpunit/phpunit/src/TextUI/Command.php:148 { …}
    ./vendor/phpunit/phpunit/src/TextUI/Command.php:101 { …}
    ./vendor/phpunit/phpunit/phpunit:61 { …}
  }
}

Laravel code

Laravel calls the PHP's rollback command as follows:

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

Lines 236 to 240 in 6ecfdb5

	try {
	$this->performRollBack($toLevel);
	} catch (Throwable $e) {
	$this->handleRollBackException($e);
	}

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

Lines 274 to 281 in 6ecfdb5

	protected function handleRollBackException(Throwable $e)
	{
	if ($this->causedByLostConnection($e)) {
	$this->transactions = 0;
	}
	throw $e;
	}

What has changed?

This exception was not thrown on PHP7.4.12, it is thrown on PHP 8.0RC5

I encountered this issue myself when trying to run my application on 8.0RC5, I can imagine a lot of others will encounter this exception.

Next steps?

Now the question arises what to do with the new Exception that is thrown? As it is currently quite vague as to why it is happening by just looking at the error message, and it cost me a lot of research to find out the cause of it.

I think we need to do at least the following:

• Warn users that their test contains implicit commits leading to leaked data during test runs which greatly reduces their isolation.
• Still have a way to "ignore" the exception to make sure there will be a smooth transition when moving to 8.0.
• Think carefully about the effect of this new exception, how will this affect DB::transaction() when a rollback is initiated?

Please let me know if I can help you out in any way with this issue.

Steps to reproduce

Run the following on PHP 8:

use Illuminate\Foundation\Testing\DatabaseTransactions;
use Illuminate\Support\Facades\DB;
use Tests\TestCase;

class MyTest extends TestCase
{
    use DatabaseTransactions;
    
    public function testMe()
    {
        DB::unprepared('CREATE TABLE a (col varchar(1) null)');
    }
}

[driesvints]

I'm investigating this.

[driesvints]

So, I've investigated this with the team and want to reply to some of your proposals:

Warn users that their test contains implicit commits leading to leaked data during test runs which greatly reduces their isolation.

We'd need to list every possible implicit commit trigger for MySQL/MariaDB. This is unfeasible and a lot of work. It's probably also quirky since we'd not be relying on the behavior of the engine directly.

Still have a way to "ignore" the exception to make sure there will be a smooth transition when moving to 8.0.

Your code was already broken on PHP <8.0. It's just more explicit right now. I think the exception being exposed right now is a good thing.

Think carefully about the effect of this new exception, how will this affect DB::transaction() when a rollback is initiated?

The effect of the exception is correct. It throws a fatal error because you're ending a transaction prematurely with an implicit commit.

I think atm I'm more leaning towards doing nothing since it's good that PHP 8 is finally exposing this explicitly. We can't catch the exception and modify it because we don't know what caused the transaction to be closed. It could be the unprepared statement from above or something else. We'd have no way of knowing.

If we'd really want to solve this codewise then listing implicit statements and checking those when a transaction is active and then throw an exception is the best way to go but that would be a lot of work and get cumbrsome.

The best course of action here, I believe, would be to just document the use of DB::unprepared better with an explicit note about transactions and a link to the MySQL page you shared.

I'll let @taylorotwell decide on this.

[stemis]

Thanks for looking into this @driesvints, I completely agree, it is actually good it is throwing an explicit exception since the code was already broken.

The best course of action here, I believe, would be to just document the use of DB::unprepared better with an explicit note about transactions and a link to the MySQL page you shared.

I used unprepared as it's good for PoC's in issue descriptions because it goes straight to the DB, however, it can also be a migration command or some other thing that triggers an implicit command.

Personally, I am fine with your proposal, I agree this should be properly documented on all transaction-related documentation, for example:

• DatabaseTransactions trait
• DB::transaction, DB::beginTransaction DB::rollback etc.

If we'd really want to solve this codewise then listing implicit statements and checking those when a transaction is active and then throw an exception is the best way to go but that would be a lot of work and get cumbrsome.

Yes, it will be almost impossible to do this, as you will never be able to 100% cover all cases(ie stored procedures)

Note
The effect of the above is that packages that use migrations inside a transaction ie orchestral/testbench/RefreshDatabaseTest.php and the testbench implementation in for example: laravel-excel/FromQueryTest.php can no longer do this. This has to be handled outside of the transaction now, correct?

[crynobone]

It shouldn't affect RefreshDatabase since migration occured before calling $this->beginDatabaseTransaction();

[stemis]

It shouldn't affect RefreshDatabase since migration occured before calling $this->beginDatabaseTransaction();

Oops, you are correct. It only affects the DatabaseTransactions trait.

[driesvints]

Yes, this only affects the DatabaseTransactions trait indeed.

Thanks for your help here @stemis. I'll try to send in a PR to the docs shortly.

[driesvints]

Here it is: laravel/docs#6599

[lk77]

@driesvints so the only solution to this is to close the transaction before any implicit commit query ?

like :

// commit previous transaction
\DB::commit();
// run implicit commit query
Model::truncate();
// open a new transaction
\DB::beginTransaction();

[ionutantohi]

Sorry for resurrecting this, but this can not be avoided if users will have a way to run statements on a separate PDO session/connection?

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

// or

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

I am thinking that running the above statement on a separate PDO connection it will not interfere with transactions started on default/resolved connection.

Even the migrate command might use this. As in, to run migrations always on a separate connection because it executes statements which trigger implicit commit

But i am not sure if this is possible in the framework