[PHP8] PdoException with Transactions and MySQL implicit commits

[jon48]

Hello,

Still working on upgrading my custom modules, and now using the future 2.1 version as a base, I am facing an annoying issue when dealing with schema updates. I am actually surprised nobody has raised it, but this may be due to 2.1 not having any schema update yet.

Issue

Hopefully it should be fairly easy to reproduce (I have reproduced it both on my Windows & Ubuntu instances), as it only needs a DDL change on MySQL, for instance

DB::schema()->create('maj_transaction_test', function(Blueprint $builder) {
    $builder->integer('test_col');
});            

When running it as part of a standard webtrees page (i.e. going through all the middlewares, especially the UseTransaction one, it results in the exception below being raised:

There is no active transaction …\vendor\illuminate\database\Concerns\ManagesTransactions.php:45
#0 …\vendor\illuminate\database\Concerns\ManagesTransactions.php(45): PDO->commit()
#1 …\app\Http\Middleware\UseTransaction.php(46): Illuminate\Database\Connection->transaction()
#2 …\vendor\oscarotero\middleland\src\Dispatcher.php(136): Fisharebest\Webtrees\Http\Middleware\UseTransaction->process()
#3 …\app\Http\Middleware\DoHousekeeping.php(74): Middleland\Dispatcher->handle()
#4 …\vendor\oscarotero\middleland\src\Dispatcher.php(136): Fisharebest\Webtrees\Http\Middleware\DoHousekeeping->process()

The table is actually created, and (if no other error otherwise), in the context of a updateSchema , all the actions have completed and been committed, and on a refresh of the page, the error does not appear anymore.

Troubleshooting

With a quick Google search, it appears to be actually a well-known "new" behaviour of PDO in PHP 8.0 with MySQL implicit commits. As several discussions highlight, the behaviour is actually not really new, but PHP 8.0 now raise explicitly an exception when the transaction does not exist anymore, instead of hiding that fact and continuing, as it did before.

Basically, MySQL has a list of statement (https://dev.mysql.com/doc/refman/8.0/en/implicit-commit.html) -in particular any DDL change - which force a commit, whatever has been instructed beforehand by the code or driver (I have tried playing with PDO::ATTR_AUTOCOMMIT to no avail), so whatever transaction was previously open is forcibly committed and closed. The issue is that PDO does not have any notification of this change of status, and if not careful when calling the ->commit or '->rollback` methods on your transaction, the exception is now raised in PHP 8.0, as there is no transaction any more indeed. As an aside, IMHO, it is a correct thing for PHP/PDO to raise an exception to inform that the transaction is not one anymore, but this is a bit late in the process, as there is no notification in the abstraction between PDO and MySQL that the status has changed, so layers above PDO have to distrust the transaction abstraction layer, and take preventive or corrective actions to restore (or more realistically remediate) a more transactional mode.

Anyway, the consequence is that the exception bubbles up to webtrees. Addressing this issue is not easy, as it is usually too late, so rollback is not possible anymore, but some of the frameworks have taken on themselves to deal with the exception in their framework layers (with usually the approach of silently ignoring on a attempted commit, and raising a warning on a attempted rollback):

• Yii: https://github.com/yiisoft/yii2/issues/18406
• Drupal: https://www.drupal.org/project/drupal/issues/2736777
• Doctrine; https://github.com/doctrine/migrations/issues/1104

Laravel, on the other side, does not seem to want to address the issue, and leave it to a comment on MySQL in their documentation

• https://github.com/laravel/framework/issues/35380
• https://github.com/laravel/framework/issues/35161

I understand that the underlying issue is somewhat controversial and probably more a PDO or MySQL one, but I feel that as a framework, their position is one difficult to support, as this is their own component ManagesTransactions breaking the abstraction by exposing implementation details of the transaction method for instance.

Next steps

In the absence of a solution by Laravel in the near future. I think webtrees should still prevent the error from bubbling up to the end user, at least when everything has run successfully, and the commit is supposed to happen (a failed rollback is more problematic).

Unfortunately I have not found a neat way to address it, as most of the implementation details are hidden, so all options I have looked at have some drawbacks. But this part is clearly not my expertise, and somebody will probably have more clever ideas. I have created a commit on my fork with 2 suggestions (the code is not supposed to be clean, just for demonstration purpose):

• Capture the exception at the UseTransaction level (View diff): this allows not to bubble the issue to the front-end for that specific exception, but we do not have the information of whether the attempted action was a commit or a rollback (in the latter case, we probably still want to let the user know that something has gone wrong).
• Target specifically where we know that the transaction is likely to be broken (View diff): the most likely part of the code where implicit commit statements are going to be used are in the updateSchema of the MigrationService. So we make sure after the updates have been run if we are still in the transaction (some updates may not close the transaction), and reopen one if necessary. This still allows for some kind of transactional mechanism outside of the updateSchema methods, but it does not protect the overall transaction if somewhere else the transaction is forcibly closed (and it may be down to the developers to be careful if they use implicitly commit statements).

I have added a dummy module for testing : https://github.com/jon48/webtrees/blob/feature/php8-transaction/modules_v4/testpdo8/module.php that create/drop a table when called on /module/_testpdo8_/Test

[fisharebest]

Thanks for the thorough investigation and bug-report.

I think, perhaps, we could fix this for the core code by changing the order of the middlewares, so that the schema updates happen before we start the transaction.

(Any update that modifies data would now be responsible for its own transactions.)

This would effectively create two transactions - one for the migration, and one for the application.

But this would not help DDL statements issued by third-party modules...

[jon48]

I have not considered that option, will look a bit more at the details.

Would you create a transaction for the UpdateDatabaseSchema middleware though if you change the order? If so, at a first glance, I do not see much difference compared to suggestion 2, as the middleware is basically just a call to MigrationService->updateSchema, so if you protect that method directly, you would have protected both the core updates middleware and third-party modules using updateSchema calls (but then you don't even need to separate the transactions).

[jon48]

Forget my comment, that was really a first glance... :sweat_smile: Actually, the core schema update is already outside of the transaction scope (UpdateDatabaseSchema is called before UseTransaction), so is not affected by the issue (and the reason why nobody complained yet...)

So, this leaves only the case of the third-party modules' schema updates, and here I am taking my custom module developer hat. Maybe, based on your idea, an acceptable fix could be to slightly change the order from:

UseTransaction::class,
LoadRoutes::class,
BootModules::class,
Router::class,

to

LoadRoutes::class,
BootModules::class,
UseTransaction::class,
Router::class,

LoadRoutes should not have any interaction with the database, so should not be impacted. That would leave BootModules outside of the transactional scope, but I do not expect much database interaction at that stage except precisely schema updates.

After that, the transaction starts for the application runtime. This is not protected against implicit commits, but this should be a rarer use case, and maybe more of a case for the module developers to be aware of and to take care themselves (with the if(!DB::connection()->getPdo()->inTransaction()) { DB::connection()->beginTransaction(); } template. And hopefully, at some point, Laravel will review its position, and handle it cleanly within the transaction method.

[jon48]

Sorry to up this issue, but as an alpha of 2.1.0 is now out, what about my suggestion to have BootModules outside of the UseTransaction middleware? Laravel does not seem to offer any solution in the framework, despite further people commenting on the related issues.

[fisharebest]

That would leave BootModules outside of the transactional scope, but I do not expect much database interaction at that stage except precisely schema updates.

Another approach might be to split boot() into two functions: boot() and updateDatabaseSchema().

This might give us some flexibility in the future, as we'd be able to change how we handle this scenario - especially if laravel decides to make a change.

[fisharebest]

PS - I have also been looking at adding down() functions to the migrations - so users can more easily roll-back.

To do this, we'd need to provide a module interface for migrations. This might be a useful thing in any case.

[jon48]

Sounds good to me. It is a bit more involved than my initial suggestion, but adding a dedicated interface for DB changes offers more flexibility indeed.

[fisharebest]

Another solution to this problem is to have two database connections.

One for the application to make read/write queries - which uses a transaction.

One for migrations - which uses auto-commit.