pushTransaction is doing $this->transactionLayers[$name] = $name; and this function checks for if (isset($this->transactionLayers[$name])) yes rollback uses array_search. Slow, confusing (what's the array structure, again...?), ugly. Edit: no test needed as no functionality has changed and transactions have tests already.

CommentFileSizeAuthor
#4 transactionLayers_stores_keys-1186582-4.patch647 bytesAlbert Volkman
transactionLayers_stores_keys.patch539 byteschx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs review » Reviewed & tested by the community

There is no small cleanup.

chx’s picture

chx CreditAttribution: chx commented

You mean there is no too small cleanup? I agree. Let's find a few whitespaces to delete :D

Status: Reviewed & tested by the community » Needs work

The last submitted patch, transactionLayers_stores_keys.patch, failed testing.

Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented
Status: Needs work » Needs review
FileSize
transactionLayers_stores_keys-1186582-4.patch647 bytes

Looks good in core/lib/Drupal/Core/Database/Connection.php, but not in core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php. This fixes that :)

Status: Needs review » Needs work

The last submitted patch, transactionLayers_stores_keys-1186582-4.patch, failed testing.

Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented

Odd, that test didn't fail locally. Do I have to have my local site set up with sqlite to properly test?

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Needs review

#4: transactionLayers_stores_keys-1186582-4.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, transactionLayers_stores_keys-1186582-4.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

This test failure is probably:
#1783656: Intermittent test failure in testBulkImportUpdateExisting

Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented
Status: Needs work » Needs review
Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented

#4: transactionLayers_stores_keys-1186582-4.patch queued for re-testing.

Albert Volkman’s picture

Albert Volkman CreditAttribution: Albert Volkman commented

Not completely up-to-snuff with OOP inheritance, so would this simply need to be removed from the sqlite implementation altogether?

jhedstrom queued 4: transactionLayers_stores_keys-1186582-4.patch for re-testing.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Patch still applies and makes sense.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 0dfd0ca and pushed to 8.0.x. Thanks!

  • alexpott committed 0dfd0ca on 8.0.x 
    Issue #1186582 by Albert Volkman, chx: Fixed rollback and and...

Status: Fixed » Closed (fixed)

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