-
Notifications
You must be signed in to change notification settings - Fork 299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PHP8] PdoException with Transactions and MySQL implicit commits #3856
Comments
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... |
I have not considered that option, will look a bit more at the details. Would you create a transaction for the |
Forget my comment, that was really a first glance... 😅 So, this leaves only the case of the third-party modules' schema updates, and here I am taking my custom module developer hat. UseTransaction::class,
LoadRoutes::class,
BootModules::class,
Router::class, to LoadRoutes::class,
BootModules::class,
UseTransaction::class,
Router::class,
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 |
Sorry to up this issue, but as an alpha of 2.1.0 is now out, what about my suggestion to have |
Another approach might be to split 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. |
PS - I have also been looking at adding To do this, we'd need to provide a module interface for migrations. This might be a useful thing in any case. |
Sounds good to me. |
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. |
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
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: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):
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
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 thetransaction
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):
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).updateSchema
of theMigrationService
. 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 theupdateSchema
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
The text was updated successfully, but these errors were encountered: