So if any hook implementations or field modules throw an exception, you can end up with the records deleted from {node} but still in the other tables.

CommentFileSizeAuthor
#24 trans.diff12.06 KBmoshe weitzman
#22 trans.patch12.11 KBmoshe weitzman
#18 trans.diff11.68 KBmoshe weitzman
#17 trans.diff17.36 KBmoshe weitzman
#16 923826-16-transactions-delete.patch12.07 KBcarlos8f
#12 transactions.patch12.04 KBcatch
#7 transactions.patch11.37 KBcatch
#4 transactions.patch11.37 KBcatch
#2 transactions.patch11.14 KBcatch
node_delete_transaction.patch2.63 KBcatch

Comments

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

simple enough

catch’s picture

catch
he/him
Primary language English
commented
Component: node system » base system
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new transactions.patch11.14 KB

Went through and added transactions to all core entity delete functions.

catch’s picture

catch
he/him
Primary language English
commented

Patch also removes cache_clear_all() from taxonomy_term_delete() and taxonomy_vocabulary_delete() and puts it in the submit handlers instead, this has already been done for all other core entities and it's exactly the same lines of code.

catch’s picture

catch
he/him
Primary language English
commented
Title: node_delete() doesn't run in a transaction » $entity_delete() should use transactions
StatusFileSize
new transactions.patch11.37 KB

Even better, the node_save() transaction tries to rollback with $transaction->rollback('node');, but starts the transaction with db_transaction(); (no argument), afaik that'll mean the rollback fails in HEAD anyway. Added that to the patch here, retitling.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Love that you moved those cache_clear_all(). Bot is happy.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ modules/node/node.module	28 Sep 2010 05:36:18 -0000
@@ -1100,7 +1100,7 @@ function node_save($node) {
-    $transaction->rollback('node');
+    $transaction->rollback('');

Stray empty string here.

Powered by Dreditor.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new transactions.patch11.37 KB

argh, fixed.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 7.x-dev » 8.x-dev

Although badly needed, this is D8 material according to the rules (I had to learn today).

catch’s picture

catch
he/him
Primary language English
commented
Version: 8.x-dev » 7.x-dev

No it's not, we already added transactions to $entity_save(), this is just filling in a gap. I have an actual live Drupal 7 site with nodes that exist in some tables but not {node}, this would likely have prevented that data integrity issue from arising. In other words it's a real Drupal 7 bug, not a theoretical improvement.

catch’s picture

catch
he/him
Primary language English
commented

#7: transactions.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

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

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new transactions.patch12.04 KB

Re-rolled.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

back to rtbc

Status: Reviewed & tested by the community » Needs work

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

dawehner’s picture

dawehner
Primary language German
commented 
-    db_delete('node')
-      ->condition('nid', $nids, 'IN')
-      ->execute();
-    db_delete('node_revision')
-      ->condition('nid', $nids, 'IN')
-      ->execute();
-    db_delete('history')
-      ->condition('nid', $nids, 'IN')
-      ->execute();
-    db_delete('node_access')
-      ->condition('nid', $nids, 'IN')
-      ->execute();
+      // Delete after calling hooks so that they can query node tables as needed.
+      db_delete('node')
+        ->condition('nid', $nids, 'IN')
+        ->execute();
+      db_delete('node_revision')
+        ->condition('nid', $nids, 'IN')
+        ->execute();
+      db_delete('history')
+        ->condition('nid', $nids, 'IN')
+        ->execute();

Is there a reason why db_delete('node_access') is removed totally?

carlos8f’s picture

carlos8f commented
Status: Needs work » Needs review
StatusFileSize
new 923826-16-transactions-delete.patch12.07 KB

A missing $tids = $orphans; in taxonomy_term_delete() caused it to run forever. Oops!

moshe weitzman’s picture

moshe weitzman commented
StatusFileSize
new trans.diff17.36 KB

Put back that node_access delete. It was removed by mistake during catch's reroll. I think this is ready.

moshe weitzman’s picture

moshe weitzman commented
StatusFileSize
new trans.diff11.68 KB

Oops. Mixed up two patches there.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

bot is happy again. this was rtbc and we just rerolled so back to rtbc.

moshe weitzman’s picture

moshe weitzman commented

#18: trans.diff queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, trans.diff, failed testing.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs work » Needs review
StatusFileSize
new trans.patch12.11 KB

Fixed stale hunk in taxo delete

Status: Needs review » Needs work

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

moshe weitzman’s picture

moshe weitzman commented
Status: Needs work » Needs review
StatusFileSize
new trans.diff12.06 KB

Fix braces

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC after successful reroll.

Anonymous’s picture

Anonymous (not verified) commented
Issue tags: +Needs tests

don't think we should block on it, but this needs tests. i'll write some.

Anonymous’s picture

Anonymous (not verified) commented
Issue tags: -Needs tests

hmm, well, seems we can't do this. we can't test anything at all that uses exceptions to say "something went wrong".

see #301005: Add "expected fail" functionality to simpletest, last comment in October 2009 :-(

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

This seems like a sensible hardening improvement that shouldn't break anything.

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)

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