DDL migrations produce errors on PHP 8.0 + MySQL in transactional mode

[lippok]

BC Break Report

Q	A
BC Break	yes
Version	3.0.2 (probably > 3.0)
PHP	8.0
DBAL-Driver	PDO
DB	mysql 5.7 and 8.0

Summary

When running the command migrations:migrate with a ddl sql statement (e.g. create table) on php 8.0 I got an error:

In Connection.php line 1761: There is no active transaction

Previous behavior

Bevore php 8.0 the migration produces no errors.

Current behavior

DDL statements on mysql result in an implicit commit (see mysql documentation). This causes the explicit commit in transactional mode to fail. But only starting with php 8.0 an error is raised (probably due to php/php-src@990bb34).

To bypass the error I can disable the transactional mode by overwriting isTransactional in my migration, but in my opinion it's a breaking change in the behaviour when using php 8.

I found the same issue in a bug report for the yii framework (yiisoft/yii2#18406) that helped me to figure out what happens. They fixed it by checking PDO::inTransaction before running the commit. If I'm not wrong this method is not wrapped by the dbal package and therefore not available here.

How to reproduce

1. Create a migration with a ddl statement.
2. Let the transactional mode active.
3. Run the migration.

[chriskapp]

It looks like this problem exists also at version 2.3.2. This occurred on our CI server, we are using Mysql and PHP 8:

Fatal error: Uncaught PDOException: There is no active transaction in /home/travis/build/apioo/fusio-impl/vendor/doctrine/dbal/lib/Doctrine/DBAL/Connection.php on line 1761
PDOException: There is no active transaction in /home/travis/build/apioo/fusio-impl/vendor/doctrine/dbal/lib/Doctrine/DBAL/Connection.php on line 1761
Call Stack:
    0.0001     414648   1. {main}() /home/travis/build/apioo/fusio-impl/vendor/phpunit/phpunit/phpunit:0
    0.0050    1361800   2. PHPUnit\TextUI\Command::main($exit = ???) /home/travis/build/apioo/fusio-impl/vendor/phpunit/phpunit/phpunit:61
    0.0050    1361928   3. PHPUnit\TextUI\Command->run($argv = [0 => 'vendor/bin/phpunit'], $exit = TRUE) /home/travis/build/apioo/fusio-impl/vendor/phpunit/phpunit/src/TextUI/Command.php:163
    0.0050    1361928   4. PHPUnit\TextUI\Command->handleArguments($argv = [0 => 'vendor/bin/phpunit']) /home/travis/build/apioo/fusio-impl/vendor/phpunit/phpunit/src/TextUI/Command.php:171
    0.0082    1553328   5. PHPUnit\TextUI\Command->handleBootstrap($filename = '/home/travis/build/apioo/fusio-impl/tests/bootstrap.php') /home/travis/build/apioo/fusio-impl/vendor/phpunit/phpunit/src/TextUI/Command.php:891
    0.0083    1559400   6. PHPUnit\Util\FileLoader::checkAndLoad($filename = '/home/travis/build/apioo/fusio-impl/tests/bootstrap.php') /home/travis/build/apioo/fusio-impl/vendor/phpunit/phpunit/src/TextUI/Command.php:1095
    0.0083    1559640   7. PHPUnit\Util\FileLoader::load($filename = '/home/travis/build/apioo/fusio-impl/tests/bootstrap.php') /home/travis/build/apioo/fusio-impl/vendor/phpunit/phpunit/src/Util/FileLoader.php:47
    0.0083    1563256   8. include_once('/home/travis/build/apioo/fusio-impl/tests/bootstrap.php') /home/travis/build/apioo/fusio-impl/vendor/phpunit/phpunit/src/Util/FileLoader.php:59
    0.0209    3591400   9. runMigrations() /home/travis/build/apioo/fusio-impl/tests/bootstrap.php:9
    0.0271    4526616  10. Doctrine\Migrations\Migrator->migrate($to = ???, $migratorConfiguration = ???) /home/travis/build/apioo/fusio-impl/tests/bootstrap.php:18
    0.0503    5141448  11. Doctrine\Migrations\Migrator->executeMigration($migrationsToExecute = [20200905081453 => class Doctrine\Migrations\Version\Version { private $configuration = class Doctrine\Migrations\Configuration\Configuration { ... }; private $outputWriter = class Doctrine\Migrations\OutputWriter { ... }; private $version = '20200905081453'; private $migration = class Fusio\Impl\Migrations\Version\Version20200905081453 { ... }; private $connection = class Doctrine\DBAL\Connection { ... }; private $class = 'Fusio\\Impl\\Migrations\\Version\\Version20200905081453'; private $state = 3; private $versionExecutor = class Doctrine\Migrations\Version\Executor { ... } }, 20200905191429 => class Doctrine\Migrations\Version\Version { private $configuration = class Doctrine\Migrations\Configuration\Configuration { ... }; private $outputWriter = class Doctrine\Migrations\OutputWriter { ... }; private $version = '20200905191429'; private $migration = class Fusio\Impl\Migrations\Version\Version20200905191429 { ... }; private $connection = class Doctrine\DBAL\Connection { ... }; private $class = 'Fusio\\Impl\\Migrations\\Version\\Version20200905191429'; private $state = 0; private $versionExecutor = class Doctrine\Migrations\Version\Executor { ... } }, 20200905191956 => class Doctrine\Migrations\Version\Version { private $configuration = class Doctrine\Migrations\Configuration\Configuration { ... }; private $outputWriter = class Doctrine\Migrations\OutputWriter { ... }; private $version = '20200905191956'; private $migration = class Fusio\Impl\Migrations\Version\Version20200905191956 { ... }; private $connection = class Doctrine\DBAL\Connection { ... }; private $class = 'Fusio\\Impl\\Migrations\\Version\\Version20200905191956'; private $state = 0; private $versionExecutor = class Doctrine\Migrations\Version\Executor { ... } }], $direction = 'up', $migratorConfiguration = class Doctrine\Migrations\MigratorConfiguration { private $dryRun = FALSE; private $timeAllQueries = FALSE; private $noMigrationException = FALSE; private $allOrNothing = FALSE; private $fromSchema = NULL }) /home/travis/build/apioo/fusio-impl/vendor/doctrine/migrations/lib/Doctrine/Migrations/Migrator.php:148

[nicklog]

Just do it like pierres as a workaround until its fixed.
Add following code into your migrations.

class Version**** extends AbstractMigration
{

    public function up(Schema $schema): void
    {
        // your migration
    }

    public function isTransactional(): bool
    {
        return false;
    }
}

[greg0ire]

I'm wondering if this issue should be moved to doctrine/dbal instead. It would make sense since the solution will mean dealing with a specifics of a particular platform. This could be done in a generic way though, with PDO::inTransaction(), as pointed out by OP.

@morozov , what do you think? Should this be handled in Connection::commit()?

[lippok]

An other option would be to catch the error here where he is thrown.

[greg0ire]

I think that's an anti-pattern (at least in PHP, I think in python it would be… well pythonic). IIRC that's because exceptions are costly but I'm not sure this is true, it might be cultural. Anyway, it's not something that we usually do I think.

Also, doctrine/migrations might not be the only package sending DDL statements through the DBAL, so I think it would make sense to handle this there.

[morozov]

@greg0ire as far as I understand, it's the migrations who starts the transaction, executes the DDL and then commits the transaction that already has been implicitly committed. Why and how should the DBAL solve this problem?

[greg0ire]

it's the migrations who starts the transaction

I did not spot this, and of course, you're correct. This issue is in the right repository 👍

[greg0ire]

I believe the fix could be a simple

-if ($migration->isTransactional()) {
+if ($migration->isTransactional() && (!$this->connection->getWrappedConnection() instanceof PDOConnection || $this->connection->getWrappedConnection()->inTransaction())) {

Can anybody try that?

[lippok]

I had to add use Doctrine\DBAL\Driver\PDOConnection;, afterwards your patch works! PDOConnection is marked as deprecated but I assume only for external use and it's ok to use it here.

[greg0ire]

A way to do the same without resorting to a deprecated name would be to use PDO instead of PDOConnection, since PDOConnection extends PDO.

[lippok]

Yes, instanceof \PDO is also working, just checked it. I think this approach is better.

[greg0ire]

@morozov I am not sure the DBAL could solve the issue, but it would definitely help to have inTransaction() added to the API of Connection. For systems that don't have an equivalent to PDO::inTransaction(), it could still be implemented by doing some bookkeeping regarding the number of calls to beginTransaction(), commit() and rollBack().

In v2, this package has 4 occurrences of commit(), all of which probably need to be complexified to account for this MySQL "feature". Also, there are 4 occurrences of rollBack(), and surely, calling rollBack() if there is no active transaction would fail, e.g. in this (unlikely, I concede that) scenario:

try {
    $connection->beginTransaction();
    $connection->executeSomeSQLWithDDLStatements(); // works fine and auto-commits
    callUnrelatedToDatabaseThatThrows();
} catch (Throwable $e) {
    $connection->rollBack();
}

Alternate solution to adding inTransaction to the API would be to add an argument to commit and rollBack that would make the check to inTransaction implicit, or to add a new API like maybeCommit / maybeRollback.

All of my proposals would constitue BC-breaks in the DBAL though, so we would still have to complexify code here as a first step I'm afraid.

Also, none of them feel really good to me, maybe you will have better ideas?

[morozov]

I believe the fix could be a simple

-if ($migration->isTransactional()) {
+if ($migration->isTransactional() && (!$this->connection->getWrappedConnection() instanceof PDOConnection || $this->connection->getWrappedConnection()->inTransaction())) {

Should it work with the non-PDO MySQL driver as well and only with the MySQL (PDO and non-PDO) drivers?

[greg0ire]

I overlooked mysqli, and the link to php-src given by @lippok above points to a change that is about mysqli and only it, so that's weird. It does mean my fix is probaby incomplete, the condition should be complexify to handle mysqli too.

[Wirone]

On the other hand inTransaction() could be introduced on interface's level and:

• each driver could provide bool result based on native code (PDO) or internal data
• return type could be ?bool where null could represent unknown state, to be handled differently per driver. This isn't state of the art solution but could be good enough for some transitional period.

I hit this issue too when upgraded to PHP8, right now I've used workaround with custom abstract migration class with @nicklog's isTransactional() returning always false. So basically I've added one layer of inheritance but thanks to that I just changed extended class in every migration file (instead of adding isTransactional() everywhere) and changed my migration template so my abstract class will be used in new migrations. When transaction support will be delivered, I will just change isTransactional() in abstract migration.

[derrabus]

Does mysqli complain if you COMMIT without an active transaction? I thought this was a PDO thing.

[greg0ire]

I thought so too, but OP linked to a commit that seems to be about mysqli (php/php-src@990bb34) @lippok, or any of the 15 people that upvoted this, please try with mysqli and report back.

[lippok]

When I'm using the driver mysqli instead of pdo_mysql, I cannot reproduce the transaction error at all. mysqli seems not to be affected but I don't understand why. Unfortunately I am not familiar with this project, thus I don't know which code is executed instead of the DbalExecutor.

I assume it has no influence, but I pass the db configuration with the option --db-configuration <configfile>.

[greg0ire]

Furthermore, we can see that php/php-src@990bb34 is also included in 7.4.13, so we probably would have had a lot of bug reports if there was an issue with mysqli indeed.

[morozov]

[…] maybe you will have better ideas?

What are the expectation of the users who use transactional migrations and MySQL that doesn't respect them?

[greg0ire]

Here, it's not end users that are asking for transactions, it's this package. Maybe not using transactions when MySQL is detected would be the best solution. We could fix this by having AbstractMigration::isTransactional return false in that case, which would make sense, because in the context of migrations, can we really call MySQL a transactional RDBMS when it behaves this badly with migrations?

[morozov]

This way, the behavior of the migration is not portable. If the user runs it on say PostgreSQL and MySQL, they will have to be prepared that the migration won't be transactional in the worst case. Then what's the point in having it transactional with PostgreSQL? IMO, it's the party who builds and runs the migration that needs to decide whether it has to be transactional or not. If it has to be transactional, it cannot be run on MySQL.

[greg0ire]

Sounds fair. This means:

• we should throw an exception with a helpful message in case there is an attempt to run a transactional migration under circumstances that we will lead to a crash (I gather it's PHP 8 + pdo_mysql?)
• we might complexify AbstractMigration::isTransactional() to have saner defaults in that case, so that people using MySQL don't have to always override that method. Alternatively, we may tweak the migration generator so that it generates the override in that case, but I feel the first solution is better.

[morozov]

we should throw an exception with a helpful message in case there is an attempt to run a transactional migration under circumstances that we will lead to a crash (I gather it's PHP 8 + pdo_mysql?)

Since it's the MySQL platform that doesn't support DDL in a transaction, why should the logic above depend on the PHP version and the driver?

[derrabus]

Keep in mind that a migration does not necessarily contain DDL statements. A migration that only consists of INSERT and UPDATE statements can very well be transactional.

[greg0ire]

@morozov because apparently the issue is swept under the rug when using other configurations. If we make the condition based on the platform, then we will make a lot more migrations that could run properly with the expected outcome no longer valid.

@derrabus ' remark makes me think that even this would not be possible, and that the only thing we can improve is the defaults, if we think the migrations he describes are the exception rather than the rule.

[yyaremenko]

@stingus regarding composer.lock - yes, my bad, agree; regarding composer require - why should I require the library outside of bundle?

[stingus]

@yyaremenko my bad this time :) I've meant doctrine/doctrine-migrations-bundle:^3.1.1. As of this moment, this should install doctrine/migrations:3.1.2.

[yyaremenko]

@stingus the problem here is since the lock file has an older requirement, upon new version of bundle installation you get an error

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - doctrine/doctrine-migrations-bundle[3.1.0, ..., 3.1.x-dev] require doctrine/migrations ^3.1 -> found doctrine/migrations[3.1.0, ..., 3.2.x-dev] but the package is fixed to 3.0.2 (lock file version) by a partial update and that version does not match. Make sure you list it as an argument for the update command.
    - Root composer.json requires doctrine/doctrine-migrations-bundle ^3.1 -> satisfiable by doctrine/doctrine-migrations-bundle[3.1.0, 3.1.1, 3.1.x-dev].

this can be fixed by running

composer require doctrine/doctrine-migrations-bundle:^3.1.1 --with-dependencies

but this updates quite a lot of dependencies aside from doctrine migrations.
I'm not sure if this is okay?...

[yyaremenko]

okay looks like I figured out the magic trick lol

composer update doctrine/migrations && composer require doctrine/doctrine-migrations-bundle:^3.1.1

looks like this one only updates the target library and the target bundle, leaving the rest of composer.lock intact.
If I'm doing it wrong, everybody is welcome to comment.

[NicoHaase]

If you only want to update that bundle, why not run composer update doctrine/doctrine-migrations-bundle?

[yyaremenko]

@NicoHaase good idea, thank you; but as I tried it, it updated the bundle from v3.0.2 to v3.0.3, and this is not what we want

[NicoHaase]

What does composer why-not doctrine/doctrine-migrations-bundle 3.1.2 tell you? Probably you need to update any other package too, but this is really out of scope for a bugtracker

[yyaremenko]

There is no installed package depending on "doctrine/doctrine-migrations-bundle" in versions not matching 3.1.2

(I've already updated to 3.1.1)

[ostrolucky]

See linked issues and pull requests. I don't know why do you guys comment on closed issue...

[yyaremenko]

@ostrolucky I wonder if you actually read the comments before making your remark. Because if you did, you would understand that 'linked issues and pull requests' had nothing to do with the problem discussed. Oh and probably you would also understand why we comment on a closed issue.

[nicwortel]

I had also updated doctrine/migrations and doctrine/doctrine-migrations-bundle to their latest versions, but started to receive this error again. Turned out that it was caused by a Connection wrapper which isn't handled correctly by Doctrine/Migrations/Tools/TransactionHelper.php. See getsentry/sentry-symfony#488, #1149 and doctrine/dbal#4616.

[RiseAndCry]

Still having this issue with 2.3.4

[greg0ire]

@RiseAndCry make sure you read https://www.doctrine-project.org/projects/doctrine-migrations/en/3.3/explanation/implicit-commits

[RiseAndCry]

So i guess i should either add 'isTransactional()) methods to all migrations if using <3.0 version or use configuration to specify the same with >3.0 ? (at least these seem to be the simplest solutions)

Thanks for the link

[greg0ire]

The configuration only affects new migrations, so you will have to do the first item regardless of the version. It would be cool if there was a way to do this with Rector

[povilas-oxylabs]

ok, thanks.

[jerru11]

Hi! The easiest solution is to provide needed version in composer.json

"doctrine/doctrine-migrations-bundle":"3.1.1", "doctrine/migrations": "3.2.1",

Transactions works correctly with php 8.0 + MySql 5.7

[RiseAndCry]

I'd rather not upgrade to 3 yet, it's not perfect when using multiple entityManagers (at least i haven't found a good way to make it work yet). But your solution might work for others. Thanks either way.

[ivaskad]

"doctrine/doctrine-migrations-bundle":"3.1.1", "doctrine/migrations": "3.2.1

This works for me,

downgrade doctrine/doctrine-migrations-bundle from 3.2.0 to 3.1.1
and doctrine/migrations from 3.3.0 to 3.2.1

on php8 & MySQL 8.0.20

[mariusjp]

"doctrine/doctrine-migrations-bundle":"3.1.1", "doctrine/migrations": "3.2.1

This works for me,

downgrade doctrine/doctrine-migrations-bundle from 3.2.0 to 3.1.1 and doctrine/migrations from 3.3.0 to 3.2.1

on php8 & MySQL 8.0.20

Same setup, can confirm, this works

[digitalkaoz]

@greg0ire im getting this error too (without using doctrine migrations) by using doctrine/dbal and php8. any chance this fix might be ported to dbal too? i guess it wont be changed on the PHP-SRC side

[greg0ire]

@digitalkaoz what fix? There is no fix, just don't attempt to use DDL + transactions with MySQL or Oracle, they do not support it.

[cavasinf]

Following the doc : https://www.doctrine-project.org/projects/doctrine-migrations/en/3.3/reference/configuration.html
Disable globally transactional by setting it to false for migrations solve that case.

---

If you are using Symfony and doctrine/doctrine-migrations-bundle 3.2.x, follow the doc : https://symfony.com/bundles/DoctrineMigrationsBundle/current/index.html
To disable transactional from config/packages/doctrine_migrations.yaml