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 (https://github.com/php/php-src/commit/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 https://github.com/php/php-src/commit/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.

[stof]

The issue I see is that disabling transactional behavior requires editing each generated migration. That would be painful for MySQL project. It would at least require a config setting to be able to generate non-transactional migrations by default (or disable transactional with a separate rule)

[greg0ire]

@stof that's what I proposed here, along with another solution:

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.

What's your opinion on the first solution? Too risky? It does change the behavior existing migrations, and would technically be a BC-break for that reason I suppose, although one could argue that the behavior was not the expected one in the first place.

[stof]

@greg0ire that first solution would still require my proposal if you don't want mysql users to hate the Doctrine maintainers by having to edit manually 100% of generated migrations to make them work.

[stof]

What's your opinion on the first solution? Too risky? It does change the behavior existing migrations, and would technically be a BC-break for that reason I suppose, although one could argue that the behavior was not the expected one in the first place.

I would propose to have AbstractMigration::isTransactional() defaulting to $platform->supportsTransactionalDDL() (adding that method in DBAL if it does not exist yet).

For platforms supporting transactional DDL, this is the best default (if you make a mistake in your migration, it avoids having to clean a state of a half-executed migration). But default to always transactional indeed does not look good anymore on mysql due to the magic it does...

[greg0ire]

Fully agree with your second comment, but I don't understand the first one: if we do what you describe in your second comment, isTransactional() will be inherited to all existing migrations, which means there will be nothing to edit manually, will there be?

[greg0ire]

I would propose to have AbstractMigration::isTransactional() defaulting to $platform->supportsTransactionalDDL() (adding that method in DBAL if it does not exist yet).

That method does not exist, and I just wrote it in a branch on my local repository, only to realize there is no 2.13 branch yet and such a new feature is supposed to go to 3.1 (since it's a new feature), which this package is not even allowing yet. I think we will probably resort to an instanceof check on the platform directly in this package instead.

[greg0ire]

Link to the 3.1 PR in case anyone is interested: https://github.com/doctrine/dbal/pull/4481

[alexandre-leng]

i have the same issue

[notice] Migrating up to DoctrineMigrations\Version20210125113154

In Connection.php line 1761:

  There is no active transaction  

Symfony LOG dev.log

i try on XAMP with php 8.0.1 and still i get the bug and i try on MAMP with php 7.4.1 and i still get the same bug

Other dev on the web have the same problem ENGLISH https://www.gitmemory.com/issue/doctrine/migrations/1104/757051253

FRENCH https://openclassrooms.com/forum/sujet/soucis-symphony-5-2-et-php-8-0-1

ANOTHER BUG RELATED TO https://github.com/yiisoft/yii2/issues/18406

[albe]

So AFAIS up to PHP 7.4 such a migration will execute roughly as if it were isTransactional() == false (only roughly because everything up to the first DDL will be within a transaction), because the failing commit was silently ignored. In PHP8 however the manual commit will now throw a PDOException, which totally breaks the transaction and stops following migrations in the execution plan from running. This is a major PITA for all projects using doctrine migrations and trying to support PHP 8, as it can not easily be handled by them automatically (catching the exception there will still have later migrations not executed). This means all existing migrations need to be changed and own users need to be advised to change their migrations.

In this case, I'm with the OP and the right solution IMO would be to create a BUGFIX that catches this very specific PDOException and ignores it to create b/c for now. Then a "sane" solution to solve this through proper API can be discussed with all the time needed.

        if ($migration->isTransactional()) {
            //commit only if running in transactional mode
            try {
                $this->connection->commit();
            } catch (\PDOException $e) {
                // FIXME: Workaround for PHP 8 and PDO mysql b/c, see https://github.com/doctrine/migrations/issues/1104
                if (PHP_MAJOR_VERSION < 8 || $e->getMessage() !== 'There is no active transaction') {
                    throw $e;
                }
            }
        }

Is this an anti-pattern? Pretty sure. Would I want to do something like this regularly? Nope. Is this solving a pain? Yep. Dirty as heck, but it is what it is.

[bpolaszek]

Is this an anti-pattern? Pretty sure. Would I want to do something like this regularly? Nope. Is this solving a pain? Yep. Dirty as heck, but it is what it is.

It's ugly, but I like it! 👍

[imreg]

The error still exists:

When package: "doctrine/migrations": "^3.1.0",

`Cannot load Xdebug - it was already loaded PHP 8.0.2 (cli) (built: Feb 5 2021 04:12:54) ( NTS ) Copyright (c) The PHP Group Zend Engine v4.0.2, Copyright (c) Zend Technologies with Xdebug v3.0.2, Copyright (c) 2002-2021, by Derick Rethans user@6ff7b32a5315:/var/www/html/frontend$ bin/console doctrine:migrations:migrate --no-interaction --all-or-nothing Cannot load Xdebug - it was already loaded [notice] Migrating up to DoctrineMigrations\Version20210311154750

In Connection.php line 1761:

There is no active transaction

doctrine:migrations:migrate [--write-sql [WRITE-SQL]] [--dry-run] [--query-time] [--allow-no-migration] [--all-or-nothing [ALL-OR-NOTHING]] [--configuration CONFIGURATION] [--em EM] [--conn CONN] [--] [] `

[ostrolucky]

Another workaround is to switch to mysqli driver instead of pdo_mysql, as only pdo_mysql triggers this exception, as can be seen in https://github.com/doctrine/dbal/pull/4544

[greg0ire]

@albe I think people already have found some reasonable workarounds IMO, see https://github.com/doctrine/migrations/issues/1104#issuecomment-757039024 for instance. There is no rush.

@imreg why would the issue no longer exist? The issue is still open. I'm hiding your answer as duplicate (of the original issue).

I think a good first step towards fixing this would be to add a configuration option defaulting to true and governing what is returned here: https://github.com/doctrine/migrations/blob/34b4f7fa0f8c1385d531841b1d451b736efcd9b0/lib/Doctrine/Migrations/AbstractMigration.php#L59-L62

That way, if the user wants to switch from PostgreSQL to MySQL, they can decide whether they want to keep using true or false depending on whether they mostly migrate their schema or their data. That option could be named transactional_by_default

[ostrolucky]

I have different opinion. Even though it's dirty, I think we need to go with https://github.com/doctrine/migrations/issues/1104#issuecomment-786311346 until it's fixed in DBAL. We shouldn't default to false even in MySQL, because migrations don't have to contain DDL changes only. Sometimes they contain things like data inserts or update only. MySQL transactions work perfectly fine there. This is why checking for platform, or changing some variable applicable for all transactions in project globally is not enough. We should keep defaulting to transactions whenever is it possible. But we don't know if user is doing DDL changes in particular transaction or not, that's why we have to still go through transaction, catch this particular error and ignore it.

[greg0ire]

until it's fixed in DBAL

Given the answer you got here, I'm starting to have some doubts about this.

Regarding https://github.com/doctrine/migrations/issues/1104#issuecomment-786311346 , it has the merit of not being coupled at all to the DBAL, unlike instanceof MySQLPlatform, so there's that. What's very wrong with it is that it hides the fact that the migration is not wrapped in a transaction to our users and that despite the fact isTransactional() returns true. That has always been the case, and it's only surfacing now that an exception is thrown, but that kind of shushing sure doesn't feel right.

Maybe it should be part of the configuration? Meaning possible values for transactional_by_default could become:

• true
• tentatively or mostly or silence_failures
• false

That way people that use MySQL + PDO or Oracle or whatever doesn't work can have migrations that works without deluding themselves. We could even print a message in the output indicating whether the migration ended up being transactional or not in this kind of case.

[albe]

Maybe it should be part of the configuration? Meaning possible values for transactional_by_default could become:

• true
• tentatively or mostly or silence_failures
• false

That way people that use MySQL + PDO or Oracle or whatever doesn't work can have migrations that works without deluding > themselves. We could even print a message in the output indicating whether the migration ended up being transactional or not in this kind of case.

Don't get me wrong. That sounds totally sensible and I like that idea. But that is still breaking for existing migrations/code and rather a new feature than a bugfix. I still think existing expectations on the behaviour should be kept up first through a patch level fix, then be dealt with new behaviour in a new minor version (at which point the dirty patch can be reverted again).

[ostrolucky]

Yeah one thing after another. People were delusioned for a long long time that they get some kind of transaction protection during DDL statements in MySQL, this is not the right ticket to solve it in. By the way, Oracle also doesn't support it and possibly other databases. For now we just need to make migrations work with MySQL again. That's why I plan to submit a patch today or tomorrow according solution in https://github.com/doctrine/migrations/issues/1104#issuecomment-786311346 so we could already have fix which makes migrations on PHP 8 with most popular RDBMS work again.

[albe]

@albe I think people already have found some reasonable workarounds IMO, see #1104 (comment) for instance. There is no rush.

Unfortunately, for us this is not possible. We have a Framework that relies on Doctrine migrations and our consumers all have built own migrations based on their application code. While we could create an intermediary layer for the framework level migrations, we would need to also change all those custom migration classes and without that they can not upgrade to PHP 8. Also, even if we introduce this intermediary layer and somehow update all those migrations through a code migration, would we need to remove that layer again at some point, because it becomes an empty class?

[greg0ire]

I see. If we are going to do this I think we should also fix this paragraph of the docs. We should probably add a warning similar to this one (but in proper english :wink:), and tell people to refer to the docs of their RDBMS to know what might actually be happening.

[greg0ire]

Since that documentation issue already exists for PHP < 8, I made a PR for that: https://github.com/doctrine/migrations/pull/1130

[ostrolucky]

And I have opened https://github.com/doctrine/migrations/pull/1131 for fixing PHP 8.0 + MySQL issue.

[abbul]

Hi. I had the same error, and this worked for me:

doctrine_migrations:
    migrations_paths:
        'DoctrineMigrations': '%kernel.project_dir%/migrations'

    all_or_nothing: true

[Zempheroth]

any fixes for php 8? weird things but all_or_nothing doesn't help still using isTransactional() return false

[greg0ire]

@Zempheroth yes there has been a fix, that is why the issue is closed.