Stop triggering a warning in MySQL's TransactionManager rollback

Seems straight forward. Random failure seems to be javascript related.

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

Actually imagine we want to keep that right?

Addressed @larowlan's input.

Feedback from larowlan was addressed and answered. While not my ares this all seems reasonable to me so setting to RTBC.

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.

I think that was supposed to NW for comments.

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.

Believe #11 answers #9

Updated title to better reflect what's being changed.

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.

Personally I’ll wait for 🐛 Ensure post transaction callbacks are always executed ONLY at the end of the root Drupal transaction Needs review before working on #13.

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.

#11

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.

#16 lots of discussion in ✨ Introduce a Connection::executeDdlStatement method RTBC .

Given that trying to unwind this fixes another bug I think this is a good solution and we should go with it.

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!

catch → closed merge request !7430