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.
Follow-up from #1184944: Make entities classed objects, introduce CRUD support:
Problem
- Comments, terms, and vocabularies invoke
hook_entity_delete()
after the entity has been already deleted. - Nodes, users, and files invoke the hook first, which is inconsistent.
- In #1184944: Make entities classed objects, introduce CRUD support,
hook_entity_delete()
is invoked after deletion, as this is consistent with load/insert/update hooks. - Comments need to be deleted before node deletion. See #890790: deleting nodes does not delete their comments..
Proposed resolution
- Add
hook_entity_predelete()
(analogous tohook_entity_presave()
), so that modules can act before entity deletion. - Add the predelete hook to comments, terms, and vocabularies.
- For users, files, and nodes:
- Rename the delete hooks and all their implementations to be predelete hooks
- Invoke the delete hook separately after entity deletion.
(This ensures the existing code execution order is not changed, as that could have unexpected side effects.)
- Add test coverage to ensure the proper hook ordering for entity hooks. (Replace the
assertHookMessage()
method withassertHookMessageOrder()
.
Remaining tasks
Patch in #37 includes all of the above changes.
- Consider converting deletion hooks to work on multiple entities at once, similar to
hook_entity_load()
. This would increase performance for mass-deletions. - Fix the hook ordering for
hook_entity_load()
: #1371744: Entity load hooks are called in the wrong order.
API changes
- The following hooks are added:
- hook_entity_predelete()
- hook_taxonomy_term_predelete()
- hook_taxonomy_vocabulary_predelete()
- hook_comment_predelete()
- hook_node_predelete()
- hook_user_predelete()
- hook_file_predelete()
- All core implementations of the following hooks are changed:
- hook_node_delete() to hook_node_predelete()
- hook_user_delete() to hook_user_predelete()
- hook_file_delete() to hook_file_predelete()
Comment | File | Size | Author |
---|---|---|---|
#37 | predelete-1346032-37.patch | 49.69 KB | xjm |
#37 | interdiff.txt | 814 bytes | xjm |
#35 | interdiff.txt | 594 bytes | xjm |
#35 | predelete-1346032-35.patch | 49.62 KB | xjm |
#32 | diff.txt | 10.03 KB | xjm |
Comments
Comment #1
xjmI'll try working on this.
Comment #2
xjmCurrently we have:
The following run before entity deletion (with
hook_entity_delete()
also invoked after entity deletion):So these three hooks and all their implementations need to be renamed to
_predelete()
, and any logic relating to them inhook_entity_delete()
implementations also needs to be moved tohook_entity_predelete()
. Then, delete hooks need to be invoked after their deletion.Edit: Thankfully,
entity_crud_hook_test_entity_delete()
is the only implementation ofhook_entity_delete()
in core, so that's not too bad.The following run after entity deletion (with
hook_entity_delete()
also invoked before entity deletion):So these don't need to be modified, but the predelete hooks need to be invoked before deletion for each of these entity types.
Comment #3
xjmAttached:
hook_entity_predelete()
andhook_entity_delete()
for all core entity types.hook_taxonomy_term_predelete()
,hook_taxonomy_vocabulary_predelete()
, andhook_comment_predelete()
.hook_node_delete()
,hook_user_delete()
,hook_file_delete()
, and all their implementations to*_predelete()
hook_node_delete()
,hook_user_delete()
, andhook_file_delete()
that are invoked after the entity is deleted.I haven't proofread it yet, but sending to testbot.
Comment #4
xjmComment #6
xjmAhem.
Comment #7
xjmComment #8
xjmI double-checked just now for remaining references to the old hook names for
node
,user
, andfile
. It appears I may need to additionally correct a couple@see
toupload_file_delete()
insystem.api.php
.There's also a (confusing) docblock reference to
hook_user_delete()
in the API documentation forhook_user_cancel()
, plus this inline comment inuser_cancel()
that really does not make much sense:Comment #9
xjmOkay, actually,
upload_file_delete()
doesn't actually exist in D8--nor does it in D7 or D6 that I can find. I opened #1347812: Remove/replace documentation references to upload_file_load() and upload_file_delete() for that.Attached clarifies the comments that referenced
hook_user_delete()
and adds references tohook_user_predelete()
there.Comment #10
xjmNote that, since this change already touches 26 files, I haven't done any refactoring. I think that would be best as a separate followup once both this and #1184944: Make entities classed objects, introduce CRUD support are in.
Comment #11
Dave ReidWith the test messages, this doesn't actually confirm the proper order of the hooks? It just tests if the message appears? Maybe we should add assertHookMessageOrder()?
Not exactly excited about changing all the hook implementations to predelete, but I get why we're doing that.
7 days to next Drupal core point release.
Comment #12
xjmAh, yeah, #11 seems like a really good idea to me. I'll work on adding tests for the hook ordering.
Comment #13
xjmIn the process of looking at this, I noticed a couple things about the
assertHookMessage()
method: it has two optional parameters and a return value that are never used anywhere.Comment #14
xjmI also discovered an inconsistency with load hook ordering: For most operations, the entity-specific hook is called first and the generic entity hook is second. In entity_load(), however, the generic entity hook is first. See DrupalDefaultEntityController::attachLoad().
Edit: This may be fixed by #1184944: Make entities classed objects, introduce CRUD support, but I'm not sure.
Comment #15
xjmAttached simply replaces
assertHookMessage()
withassertHookMessageOrder()
(since the latter includes the functionality of the former).Comment #16
xjmOh, and a clarification of @Dave Reid's comment in #11: the delete hook implementations for node, user, and file entities are "changed" (really just renamed) to predelete because, currently, they precede deletion. So, by renaming them, we ensure that the existing hook execution order is not changed by this patch. Some of the renamed implementations could probably be moved in part or full to delete hooks, but I think that would also be better in case-by-case followups, in order to limit the scope/impact of this patch.
Comment #17
xjmRerolled for offset (plus a whitespace fix).
Comment #18
tim.plunkettAll those lines and not a single thing for me to nitpick.
I like the idea of assertHookMessageOrder().
Tests pass, docs are good. Awesome stuff!
Note, xjm mentioned that this should go in after #1184944: Make entities classed objects, introduce CRUD support.
Comment #19
fagoGreat work!
It's not. But I agree that should be fixed. (-> separate issue).
Minor, but the hook is entity type specific, not entity specific. entity-specific should be clear although it's inaccurate. Thoughts?
same here.
Comment #20
xjmOh, yeah, that could be confusing. :) Will fix.
Comment #21
xjmComment #22
fagoThanks, (doc) improvements look good -> back to rtbc.
Comment #23
fagoComment #24
catch#21: predelete-1346032-21.patch queued for re-testing.
Comment #26
xjmNeeds to be rerolled now that the CRUD (curd) patch is in, plus we can add support for the predelete hook to the interface now. :)
Comment #27
aspilicious CreditAttribution: aspilicious commentedLooking at this will move it back if I fail :)
Comment #28
aspilicious CreditAttribution: aspilicious commentedAnd a patch (try).
Comment #29
aspilicious CreditAttribution: aspilicious commentedMade a mistake
Comment #30
aspilicious CreditAttribution: aspilicious commentedManual interdiff ;)
Added this predelete hook stuff
Changed this docblock
21 days to next Drupal core point release.
Comment #31
catchRe-roll looks good to me. Marking RTBC, I'll commit in 3-4 days unless there are objections.
Comment #32
xjmHere's a manual diff of #21 to #29.
Comment #33
xjmThis needs a reroll, at least. I'm also checking on another hunch.
Comment #34
xjmAh, there is also a typo here, which I'll fix.
Comment #35
xjmAlright, the thing I was concerned about turned out to be me misinterpreting an illusion in the diff. ;) I also reran the tests locally and checked the assertions to be sure. So the patch from #29 looks fine.
Attached reapplies two hunks manually:
taxonomy_node_delete()
totaxonomy_node_predelete()
(because of #1050466: The taxonomy index should be maintained in a node hook, not a field hook).hook_file_predelete()
and updatinghook_file_delete()
(because of #1347812: Remove/replace documentation references to upload_file_load() and upload_file_delete().)I additionally made the comment correction shown in the interdiff.
Comment #36
xjmComment #37
xjmI read it carefully one more time, and thought that we should probably add
@see
toentity_delete_multiple()
in the comment hook definitions now. Everything else looks correct. :)Comment #39
xjmBot goof.
Opened #1371744: Entity load hooks are called in the wrong order as a followup.
Comment #40
BTMash CreditAttribution: BTMash commentedThe patch looks very good. I was initially confused about why the non delete-related hook tests were also revised since that seemed to be outside the issue. Talked about it with xjm on irc and the tests for the order the other hooks is a (good) side-effect of this issue. So there will be an updated summary so it relates correctly with #39 and any other cleanups of this type.
Comment #40.0
BTMash CreditAttribution: BTMash commentedUpdated issue summary.
Comment #40.1
xjmUpdated issue summary.
Comment #41
xjmTagging, since this is a significant API change. I also fleshed out the summary with more detail.
Comment #41.0
xjmUpdated issue summary.
Comment #41.1
xjmUpdated issue summary.
Comment #41.2
xjmUpdated issue summary.
Comment #41.3
xjmUpdated issue summary.
Comment #41.4
xjmUpdated issue summary.
Comment #41.5
xjmUpdated issue summary.
Comment #41.6
xjmUpdated issue summary.
Comment #42
xjmAnd that hyphen was really bothering me. :P
Comment #42.0
xjmUpdated issue summary.
Comment #43
catchOK, gave this one more look through, and I've committed/pushed to 8.x.
There's a lot of more or less copy/paste code in here, but fortunately that'll go away once #1368394: Convert all core entities to classed objects is done and everything's using the controllers.
Needs a change notification for the API addition.
Comment #44
sunComment #45
xjmChange notice added.
Comment #46
xjmComment #48
xjmHey xjm, take off that issue tag.
Comment #48.0
xjmUpdated issue summary.