Follow-up from #1184944: Make entities classed objects, introduce CRUD support:

Problem

Proposed resolution

  • Add hook_entity_predelete() (analogous to hook_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 with assertHookMessageOrder().

Remaining tasks

Patch in #37 includes all of the above changes.

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()
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Assigned: Unassigned » xjm

I'll try working on this.

xjm’s picture

Currently we have:

  • hook_entity_delete()
  • hook_user_delete()
  • hook_node_delete()
  • hook_file_delete()
  • hook_comment_delete()
  • hook_taxonomy_term_delete()
  • hook_taxonomy_vocabulary_delete()

The following run before entity deletion (with hook_entity_delete() also invoked after entity deletion):

  • hook_user_delete()
  • hook_node_delete()
  • hook_file_delete()

So these three hooks and all their implementations need to be renamed to _predelete(), and any logic relating to them in hook_entity_delete() implementations also needs to be moved to hook_entity_predelete(). Then, delete hooks need to be invoked after their deletion.

Edit: Thankfully, entity_crud_hook_test_entity_delete() is the only implementation of hook_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):

  • hook_taxonomy_term_delete()
  • hook_taxonomy_vocabulary_delete()
  • hook_comment_delete()

So these don't need to be modified, but the predelete hooks need to be invoked before deletion for each of these entity types.

xjm’s picture

FileSize
34.68 KB

Attached:

  1. Invokes hook_entity_predelete() and hook_entity_delete() for all core entity types.
  2. Adds hook_taxonomy_term_predelete(), hook_taxonomy_vocabulary_predelete(), and hook_comment_predelete().
  3. Renames hook_node_delete(), hook_user_delete(), hook_file_delete(), and all their implementations to *_predelete()
  4. Adds replacement hook_node_delete(), hook_user_delete(), and hook_file_delete() that are invoked after the entity is deleted.
  5. Adds test coverage for all predelete hooks.
  6. Adds consistent API documentation for all entity delete and predelete hooks.

I haven't proofread it yet, but sending to testbot.

xjm’s picture

Status: Active » Needs review
Issue tags: +API change

Status: Needs review » Needs work

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

xjm’s picture

FileSize
34.74 KB

Ahem.

xjm’s picture

Status: Needs work » Needs review
xjm’s picture

Status: Needs review » Needs work

I double-checked just now for remaining references to the old hook names for node, user, and file. It appears I may need to additionally correct a couple @see to upload_file_delete() in system.api.php.

There's also a (confusing) docblock reference to hook_user_delete() in the API documentation for hook_user_cancel(), plus this inline comment in user_cancel() that really does not make much sense:

  // 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);
  }
xjm’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
35.76 KB

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 to hook_user_predelete() there.

xjm’s picture

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.

Dave Reid’s picture

+++ 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.

xjm’s picture

Status: Needs review » Needs work

Ah, yeah, #11 seems like a really good idea to me. I'll work on adding tests for the hook ordering.

xjm’s picture

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.

xjm’s picture

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.

xjm’s picture

Status: Needs work » Needs review
FileSize
18.53 KB
49.88 KB

Attached simply replaces assertHookMessage() with assertHookMessageOrder() (since the latter includes the functionality of the former).

xjm’s picture

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.

xjm’s picture

FileSize
49.88 KB

Rerolled for offset (plus a whitespace fix).

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

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.

fago’s picture

Great work!

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.

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.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Oh, yeah, that could be confusing. :) Will fix.

xjm’s picture

Status: Needs work » Needs review
FileSize
838 bytes
49.89 KB
fago’s picture

Status: Needs review » Fixed

Thanks, (doc) improvements look good -> back to rtbc.

fago’s picture

Status: Fixed » Reviewed & tested by the community
catch’s picture

Issue tags: -API change

#21: predelete-1346032-21.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +API change

The last submitted patch, predelete-1346032-21.patch, failed testing.

xjm’s picture

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. :)

aspilicious’s picture

Assigned: xjm » aspilicious

Looking at this will move it back if I fail :)

aspilicious’s picture

Assigned: aspilicious » Unassigned
Status: Needs work » Needs review
FileSize
47.81 KB

And a patch (try).

aspilicious’s picture

FileSize
47.54 KB

Made a mistake

aspilicious’s picture

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.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Re-roll looks good to me. Marking RTBC, I'll commit in 3-4 days unless there are objections.

xjm’s picture

FileSize
10.03 KB

Here's a manual diff of #21 to #29.

xjm’s picture

Assigned: Unassigned » xjm
Status: Reviewed & tested by the community » Needs work

This needs a reroll, at least. I'm also checking on another hunch.

xjm’s picture

+++ 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.

xjm’s picture

Status: Needs work » Needs review
FileSize
49.62 KB
594 bytes

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:

  1. Renaming taxonomy_node_delete() to taxonomy_node_predelete() (because of #1050466: The taxonomy index should be maintained in a node hook, not a field hook).
  2. Adding API documentation for hook_file_predelete() and updating hook_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.

xjm’s picture

Assigned: xjm » Unassigned
xjm’s picture

FileSize
814 bytes
49.69 KB

I read it carefully one more time, and thought that we should probably add @see to entity_delete_multiple() in the comment hook definitions now. Everything else looks correct. :)

Status: Needs review » Needs work

The last submitted patch, predelete-1346032-37.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
BTMash’s picture

Status: Needs review » Reviewed & tested by the community

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.

BTMash’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue tags: +Needs change record

Tagging, since this is a significant API change. I also fleshed out the summary with more detail.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Title: ordering of hook_entity_delete() is in-consistent » Ordering of hook_entity_delete() is inconsistent

And that hyphen was really bothering me. :P

xjm’s picture

Issue summary: View changes

Updated issue summary.

catch’s picture

Title: Ordering of hook_entity_delete() is inconsistent » Change notification for: Ordering of hook_entity_delete() is inconsistent
Category: bug » task
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

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.

sun’s picture

Issue tags: +Entity system
xjm’s picture

Priority: Critical » Major
Status: Active » Fixed
xjm’s picture

Title: Change notification for: Ordering of hook_entity_delete() is inconsistent » Ordering of hook_entity_delete() is inconsistent

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -Needs change record

Hey xjm, take off that issue tag.

xjm’s picture

Issue summary: View changes

Updated issue summary.