Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#24 | trans.diff | 12.06 KB | moshe weitzman |
#22 | trans.patch | 12.11 KB | moshe weitzman |
#18 | trans.diff | 11.68 KB | moshe weitzman |
#17 | trans.diff | 17.36 KB | moshe weitzman |
#16 | 923826-16-transactions-delete.patch | 12.07 KB | carlos8f |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedsimple enough
Comment #2
catchWent through and added transactions to all core entity delete functions.
Comment #3
catchPatch 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.
Comment #4
catchEven 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.
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commentedLove that you moved those cache_clear_all(). Bot is happy.
Comment #6
sunStray empty string here.
Powered by Dreditor.
Comment #7
catchargh, fixed.
Comment #8
sunAlthough badly needed, this is D8 material according to the rules (I had to learn today).
Comment #9
catchNo 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.
Comment #10
catch#7: transactions.patch queued for re-testing.
Comment #12
catchRe-rolled.
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedback to rtbc
Comment #15
dawehnerIs there a reason why db_delete('node_access') is removed totally?
Comment #16
carlos8f CreditAttribution: carlos8f commentedA missing $tids = $orphans; in taxonomy_term_delete() caused it to run forever. Oops!
Comment #17
moshe weitzman CreditAttribution: moshe weitzman commentedPut back that node_access delete. It was removed by mistake during catch's reroll. I think this is ready.
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedOops. Mixed up two patches there.
Comment #19
moshe weitzman CreditAttribution: moshe weitzman commentedbot is happy again. this was rtbc and we just rerolled so back to rtbc.
Comment #20
moshe weitzman CreditAttribution: moshe weitzman commented#18: trans.diff queued for re-testing.
Comment #22
moshe weitzman CreditAttribution: moshe weitzman commentedFixed stale hunk in taxo delete
Comment #24
moshe weitzman CreditAttribution: moshe weitzman commentedFix braces
Comment #25
moshe weitzman CreditAttribution: moshe weitzman commentedBack to RTBC after successful reroll.
Comment #26
Anonymous (not verified) CreditAttribution: Anonymous commenteddon't think we should block on it, but this needs tests. i'll write some.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedhmm, 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 :-(
Comment #28
webchickThis seems like a sensible hardening improvement that shouldn't break anything.
Committed to HEAD. Thanks!