Committing a transaction while there are still active savepoints leaves the stack in inconsistent state

We’d need voidClientTransaction() to be public in ✨ [PP-1] Introduce a Connection::executeDdlStatement method Active , better make it so now so we can also add it to the interface.

Looks good. A couple of remarks on the MR.

Thanks for review @daffie, addressed all your points.

@daffie if we add @internal, then I think we should remove the method from the interface.

Funnily, I tried to find guidance on use of @internal in Drupal, but I could not find any in the coding standards.

To me, @internal means 'hey developer do not use this method in your code, it may change without notice'. But in this case, we will be actually recommending to use this method to database driver developers when they need to void the transaction stack. So I'm not sure adding @internal is actually the right thing to do.

BTW, we already have the same issue with ::unpile() and ::rollback(), and we did this in the docblock:

  /**
   * Removes a Drupal transaction from the stack.
   *
   * The unpiled item does not necessarily need to be the last on the stack.
   * This method should only be called by a Transaction object going out of
   * scope.
   *
   * This method should only be called internally by a database driver.
   *
   * @param string $name
   *   The name of the transaction.
   * @param string $id
   *   The id of the transaction.
   *
   * @throws \Drupal\Core\Database\TransactionOutOfOrderException
   *   If a Drupal Transaction with the specified name does not exist.
   * @throws \Drupal\Core\Database\TransactionCommitFailedException
   *   If the commit of the root transaction failed.
   */
  public function unpile(string $name, string $id): void;

if we just add

   * This method should only be called internally by a database driver.

also to the new method will it be enough?

Done #13 in the last commit to the MR.

Not adding the new method to the interface is for me the right decision.
All code changes look good to me.
For me it is RTBC.

I am changing the priority to critical, as without this change the result can be data loss.

In #15 @daffie writes that not adding the method to the interface is the right change, but the MR does in fact add the new method to the interface.

I think the documentation addition with the note is probably fine, we have a general problem of 'interfaces, but only supposed to be called by a tiny subset of code' which it is not for this issue to resolve. However, it would be good to confirm whether this was a mis-type on the comment or a mistaken RTBC, so moving back to needs review.

Ah sorry, I commented in Slack but did not copy here:

@daffie
that was not the change... the change was to add the same text to the docbloc about not to call the method outside db drivers. Personally I stand in favour of adding the method to the interface - mysqli driver will need to call it, as it will Connection::executeDdlStatement when we get to it.

@catch the

general problem of 'interfaces, but only supposed to be called by a tiny subset of code'

is a nice one, and one that one may argue is a language limitation of PHP. I was thinking about an assert on the backtrace and looking for the namespace of the caller... but yes, it's not for here.

I can live with the new method being on the interface.
Back to RTBC.

I think our only other option would be just having a public method on the class and calling it, which might be valid, but it's a bit of an existential issue for Drupal core to sort out more widely, and this is a critical bugfix.

Committed/pushed to 11.x, thanks!

mondrake → closed merge request !4818

Automatically closed - issue fixed for 2 weeks with no activity.