Posted by fago on November 18, 2011 at 3:49pm
14 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | entity system |
| Category: | task |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | API change, Entity system, Needs change notification |
Issue Summary
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: (Tests needed) 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()
Comments
#1
I'll try working on this.
#2
Currently 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.
#3
Attached:
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.
#4
#5
The last submitted patch, predelete.patch, failed testing.
#6
Ahem.
#7
#8
I 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@seetoupload_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:<?php// Modules use hook_user_delete() to respond to deletion.
if ($method != 'user_cancel_delete') {
// Allow modules to add further sets to this batch.
module_invoke_all('user_cancel', $edit, $account, $method);
}
?>
#9
Okay, 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.#10
Note 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.
#11
+++ b/core/modules/entity/tests/entity_crud_hook_test.testundefined
@@ -103,6 +104,8 @@ class EntityCrudHookTestCase extends DrupalWebTestCase {
$_SESSION['entity_crud_hook_test'] = array();
comment_delete($comment->cid);
+ $this->assertHookMessage('entity_crud_hook_test_entity_predelete called for type comment');
+ $this->assertHookMessage('entity_crud_hook_test_comment_predelete called');
$this->assertHookMessage('entity_crud_hook_test_entity_delete called for type comment');
$this->assertHookMessage('entity_crud_hook_test_comment_delete called');
}
With 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.
#12
Ah, yeah, #11 seems like a really good idea to me. I'll work on adding tests for the hook ordering.
#13
In 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.#14
I 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.
#15
Attached simply replaces
assertHookMessage()withassertHookMessageOrder()(since the latter includes the functionality of the former).#16
Oh, 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.
#17
Rerolled for offset (plus a whitespace fix).
#18
All 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.
#19
Great work!
It's not. But I agree that should be fixed. (-> separate issue).
+++ b/core/modules/entity/entity.api.php@@ -286,10 +286,42 @@ function hook_entity_update($entity, $type) {
+ * This hook runs after the entity-specific delete hook.
Minor, but the hook is entity type specific, not entity specific. entity-specific should be clear although it's inaccurate. Thoughts?
+++ b/core/modules/entity/entity.api.php@@ -286,10 +286,42 @@ function hook_entity_update($entity, $type) {
+ * This hook runs after the entity-specific predelete hook.
same here.
#20
Oh, yeah, that could be confusing. :) Will fix.
#21
#22
Thanks, (doc) improvements look good -> back to rtbc.
#23
#24
#21: predelete-1346032-21.patch queued for re-testing.
#25
The last submitted patch, predelete-1346032-21.patch, failed testing.
#26
Needs to be rerolled now that the CRUD (curd) patch is in, plus we can add support for the predelete hook to the interface now. :)
#27
Looking at this will move it back if I fail :)
#28
And a patch (try).
#29
Made a mistake
#30
Manual interdiff ;)
+++ b/core/modules/entity/entity.controller.incundefined@@ -454,6 +454,9 @@
$this->preDelete($entities);
+ foreach ($entities as $id => $entity) {
+ $this->invokeHook('predelete', $entity);
+ }
Added this predelete hook stuff
+++ b/core/modules/entity/entity.controller.incundefined@@ -548,8 +551,8 @@
- * @param $op
- * One of 'presave', 'insert', 'update', or 'delete'.
+ * @param $hook
+ * One of 'presave', 'insert', 'update', 'predelete or 'delete'.
* @param $entity
Changed this docblock
21 days to next Drupal core point release.
#31
Re-roll looks good to me. Marking RTBC, I'll commit in 3-4 days unless there are objections.
#32
Here's a manual diff of #21 to #29.
#33
This needs a reroll, at least. I'm also checking on another hunch.
#34
+++ b/core/modules/entity/entity.controller.incundefined@@ -548,8 +551,8 @@
+ * @param $hook
+ * One of 'presave', 'insert', 'update', 'predelete or 'delete'.
Ah, there is also a typo here, which I'll fix.
#35
Alright, 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.
#36
#37
I read it carefully one more time, and thought that we should probably add
@seetoentity_delete_multiple()in the comment hook definitions now. Everything else looks correct. :)#38
The last submitted patch, predelete-1346032-37.patch, failed testing.
#39
Bot goof.
Opened #1371744: Entity load hooks are called in the wrong order as a followup.
#40
The 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.
#41
Tagging, since this is a significant API change. I also fleshed out the summary with more detail.
#42
And that hyphen was really bothering me. :P
#43
OK, 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.
#44
#45
Change notice added.
#46
#47
Automatically closed -- issue fixed for 2 weeks with no activity.