Investigate need for unused variables in DriverSpecificTransactionTestBase

What's the sniff doing? Checking if variable is unused after having been declared?

If that's the case, then the problem is clear - transactions are instantiated and then only committed while destructing a variable that is going out of scope. So you declare it, but then you just exit the method to get it committed - no explicit action on the variable. You may want to get it descoped explicitly via unset(), but then some of the tests would no longer make sense.

This current approach already created a lot of problems and this is another one coming. I think everyone agrees we should not do database operations while destructing a variable; #3406985: Convert all transactions in core to use explicit ::commitOrRelease() is the next step to move away from that.

I ran the experiment this issue asks for, against mysql.

The assignment is what keeps the transaction open. A Transaction commits or releases itself when it is destructed, so the variable holds it alive until the test method ends. Call the helper without assigning the result and the object is destroyed immediately, ending the transaction or savepoint there and then.

Dropping the assignment in testRollbackRootWithActiveSavepoint() so it reads $this->createFirstSavepointTransaction(); makes the test fail:

Failed asserting that exception of type "Drupal\Core\Database\TransactionOutOfOrderException" is thrown.

That is the mechanism @mondrake describes in #3: once the savepoint has been released, rolling back the root is no longer out of order, so nothing is raised. With the assignment in place the same test passes with 12 assertions.

The issue asks whether they can be removed and the ignores dropped. They cannot, as these tests are currently written, which leaves documenting them.

What I would suggest:

• State the contract on the two helpers that hand out the objects, createRootTransaction() and createFirstSavepointTransaction(), since that is where a reader meets the return value.
• Put a one-line reason above each of the seven suppressions.

I tried the inline phpcs:ignore ... -- reason form first. It suppresses correctly, but it pushes those lines well past 80 characters and core does not use that syntax anywhere, so a comment line above looked like the better fit.

Two things to watch if someone picks this up. phpcs:ignore applies to the line that follows it, so a comment added above must not detach the suppression. It does not; the sniff still reports the file clean. And to confirm that clean result was real rather than the sniff skipping the file, removing a single suppression produces exactly one warning.

Happy to open an MR along those lines if the direction looks right. The wider point @mondrake raises in #3, whether these tests should stop depending on destruct-time behaviour at all now that #3406985 has landed, is a bigger call than this issue and I would rather not pre-empt it.

@smustgrave Thanks for calling that out. Fair point, that was AI-drafted and I posted it without trimming it. Shortened the comments and just pushed. Good lesson for me, I'll write these myself from now on instead of posting AI output straight into core issues. Appreciate you taking the time to flag it.

Many Thanks

It took us a while to wrap our heads (we were 3 with alberto56 and ulysse33) around it (in a Drupal meetup), but the comments make sense to us.

Committed and pushed 4c0717bbd4f to main and 21f8590d92d to 12.0.x. Thanks!