- Issue created by @mondrake
- Status changed to Needs review
9 months ago 8:00pm 10 April 2024 - Status changed to RTBC
9 months ago 1:58pm 11 April 2024 - 🇺🇸United States smustgrave
Seems straight forward. Random failure seems to be javascript related.
- Status changed to Needs review
9 months ago 9:17pm 11 April 2024 - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Just a question on the MR. Most likely down to my lack of understanding, but keen to learn if so.
Feel free to self RTBC if appropriate - Status changed to Needs work
8 months ago 10:50pm 14 April 2024 - Status changed to Needs review
8 months ago 7:14am 15 April 2024 - Status changed to RTBC
8 months ago 9:30am 15 April 2024 - 🇳🇿New Zealand quietone
Feedback from larowlan was addressed and answered. While not my ares this all seems reasonable to me so setting to RTBC.
- Status changed to Needs review
8 months ago 10:29am 15 April 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
This needs an explanation somewhere of why we're changing from a recoverable error to an exception and why that's okay. Warnings don't stop code - they log and move on... now we're throwing an exception - so unless it is caught this is going to halt execution.
- Status changed to Needs work
8 months ago 11:33am 15 April 2024 - Status changed to Needs review
8 months ago 10:14pm 16 April 2024 - 🇬🇧United Kingdom longwave UK
Looked into git history for this one. The warning was added in #2736777: MySQL on PHP 8 now errors when committing or rolling back when there is no active transaction → where PHP 7 and PHP 8 had different behaviour and PHP 7 would not fail, so we added a warning for PHP 8 users to ensure the behaviour was the same. Now we only support PHP 8 I think we can change the behaviour to always throw an exception? To me, if someone is attempting a rollback when there is no active transaction, this is exceptional behaviour - I see no reason why this should happen in normal operation - and if we are currently warning "this may cause data integrity issues" then it is worth halting execution so this is more likely to be picked up if it happens during development.
- Status changed to RTBC
8 months ago 4:56pm 19 April 2024 - 🇺🇸United States smustgrave
Believe #11 answers #9
Updated title to better reflect what's being changed.
- Status changed to Needs work
8 months ago 9:12am 20 April 2024 - 🇮🇹Italy mondrake 🇮🇹
Actually #11 made me realize this would throw the exception only for PDO MySql, and we di not know what would happen with other drivers.
We probably need to think something that would work consistently across drivers.
Anyway, this is really just protective code, would trigger only when the client connection does not exist AND the TransactionManager still believe it exists.
- 🇮🇹Italy mondrake 🇮🇹
Personally I’ll wait for 🐛 Ensure post transaction callbacks are only at the end of the root Drupal transaction Fixed before working on #13.
- Status changed to Needs review
8 months ago 12:44pm 23 April 2024 - 🇮🇹Italy mondrake 🇮🇹
Given #11 and #13, maybe we can be bold here and just stop triggering a warning? In the end this is now triggered at a much lower level then it was in #2736777: MySQL on PHP 8 now errors when committing or rolling back when there is no active transaction → , since the transaction manager stack prevents the client rollback be called in most cases. Then in a follow up we may want to review how a rollback failure is handled across all drivers - and not for MySQL only.
- 🇮🇹Italy mondrake 🇮🇹
To me, if someone is attempting a rollback when there is no active transaction, this is exceptional behaviour - I see no reason why this should happen in normal operation - and if we are currently warning "this may cause data integrity issues" then it is worth halting execution so this is more likely to be picked up if it happens during development.
It's actually twice exceptional, with the TransactionManager stack behind the scenes: you cannot rollback a transaction that you haven't started; simply because the Transaction object does not exist. As soon as a Transaction is started, the manager will take care of keeping the stack of Drupal transaction objects in sync with the client transaction on the db driver, and will not allow you to rollback an object that does not exists. You will get much ealier a
TransactionOutOfOrderException
thrown by the manager checking the stack, not by checking if the client connection is active. (Unless you do very hacky things like instantiating the Transaction objects outside of the manager, or using reflection, or overrideing the manager, but then everything goes...)And, by the way, you should not call the rollback in the manager either.
So here, in HEAD, this warning is only triggered if the stack state is fooled by the actual client connection state - the stack legitimatley believes the transaction is active, but matter-of-fact it isn't. DDL in MySQL is a case when this can happen.
- 🇮🇹Italy mondrake 🇮🇹
#16 lots of discussion in ✨ [PP-1] Introduce a Connection::executeDdlStatement method Active .
- Status changed to RTBC
8 months ago 4:27pm 26 April 2024 - 🇬🇧United Kingdom longwave UK
Given that trying to unwind this fixes another bug I think this is a good solution and we should go with it.
- 🇬🇧United Kingdom catch
I'm constantly flip flopping between being annoyed that phpunit removed so many things we rely on at once, vs. being very glad we're catching bugs like this as a result. This looks a lot happier than throwing the exception from where we did the warning, nice find.
Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!
- Status changed to Fixed
8 months ago 12:04pm 27 April 2024