Problem/Motivation

Follow up of #3398767: Allow returning explicitly to the prior nesting level in transactions (aka allow explicit COMMIT in Transaction objects) and #3406985: Convert all transactions in core to use explicit ::commitOrRelease().

Proposed resolution

In this issue :

Remaining tasks

User interface changes

No

API changes

No

Data model changes

No

Release notes snippet

Issue fork drupal-3584238

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes

mondrake opened merge request !15422

amateescu’s picture

amateescu commented
Status: Postponed » Needs review

No longer postponed :)

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Needs work
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Rebased

mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#3569127: Add new 11.3.x database dump fixtures, without modules deprecated for removal in 12.x

I think mysqli pipeline failure is due to #3569127-21: Add new 11.3.x database dump fixtures, without modules deprecated for removal in 12.x

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented

I reviewed the changes. The comment about not triggering deprecation errors in Database makes sense to me based on the comment sin the other issue.

The test run from a week ago did not seem to have any performance regressions.

I manually tested installing Standard profile locally and inspecting my query log using PostgreSQL 18, and did not find anything unexpected about the install process. Tables are being created dynamically after failure still.

I guess we need a rebase.

makertimswis’s picture

makertimswis
Primary language Dutch
commented
Related issues: +#3569316: Client connection ($clientConnection) must be of type object, null given when deleting a node

Since 11.3 we got a lot more caching issues combined with xdebug
https://www.drupal.org/project/drupal/issues/3569316 seems to fix these but it feels that it should touch this change

daffie’s picture

daffie commented
Status: Needs review » Needs work

It looks good to me.
The IS and the CR are in order.
Testing has been added for the new deprecation's.
I have a single remark on the PR, after that it is RTBC for me.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Needs work

Another commit-on-destruct has leaked in the code base.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

This one was tough. A try/catch was not intercepting an Error, so it ended up releasing a savepoint instead of rolling back. In the end, I see this really driving improvement on thoughtful transaction handling.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

I think every comment has been resolved and is RTBC now.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented

Do we need to split the change record so that #3583849: Deprecate PgSql Connection::*Savepoint() methods that's in 11.4 and this issue which needs to be 11.5 (or greater)? Or is it okay to leave the postgresql deprecations as tied to the draft change record?

mondrake’s picture

mondrake
Location 🇮🇹
commented

#16 it's probably not my call, but since anyway all of this is slated for removal in D13, I do not think it makes much difference. Also, one single CR is clearer IMHO.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented

Thanks for fixing whatever I messed up earlier. :-)

UnpackRecipeTest? Hmm, that almost seems related to #3608069: Restore (but don't apply) deprecated recipes to 11.4.x.

mondrake’s picture

mondrake
Location 🇮🇹
commented

#18 it passes locally for me and seems random on CI

mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#3608308: Composer 2.10 causing Drupal recipe unpacking tests to fail

related test failure

mondrake’s picture

mondrake
Location 🇮🇹
commented

Green on all dbs. PHP 8.6 warnings is another issue.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented

Still green and RTBC after the recent rebase.

mondrake’s picture

mondrake
Location 🇮🇹
commented

rebased and fixed TransactionTest

catch’s picture

catch
he/him
Primary language English
commented

One question on the MR.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Replied inline.

  • catch committed 14f2a5e0 on 11.x 
    task: #3584238 Deprecate implicit commit-on-destruct

By: mondrake
By:...

  • catch committed 9a20b9c4 on main 
    task: #3584238 Deprecate implicit commit-on-destruct

By: mondrake
By:...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Ah thanks that makes sense but I wouldn't have guessed just looking at the class so good to add the explicit note. This looks good to me, we have plenty of time before we actually remove support to flush out issues. Committed/pushed to main and 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.