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()
    Files: 
    CommentFileSizeAuthor
    #37 predelete-1346032-37.patch49.69 KBxjm
    PASSED: [[SimpleTest]]: [MySQL] 34,207 pass(es).
    [ View ]
    #37 interdiff.txt814 bytesxjm
    #35 interdiff.txt594 bytesxjm
    #35 predelete-1346032-35.patch49.62 KBxjm
    PASSED: [[SimpleTest]]: [MySQL] 34,215 pass(es).
    [ View ]
    #32 diff.txt10.03 KBxjm
    #29 predelete-1346032-22.patch47.54 KBaspilicious
    PASSED: [[SimpleTest]]: [MySQL] 34,296 pass(es).
    [ View ]
    #28 predelete-1346032-21.patch47.81 KBaspilicious
    FAILED: [[SimpleTest]]: [MySQL] 34,312 pass(es), 2 fail(s), and 0 exception(es).
    [ View ]
    #21 predelete-1346032-21.patch49.89 KBxjm
    FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch predelete-1346032-21.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
    [ View ]
    #21 interdiff-17-21.txt838 bytesxjm
    #17 predelete-1346032-17.patch49.88 KBxjm
    PASSED: [[SimpleTest]]: [MySQL] 34,138 pass(es).
    [ View ]
    #15 predelete-1346032-15.patch49.88 KBxjm
    PASSED: [[SimpleTest]]: [MySQL] 34,120 pass(es).
    [ View ]
    #15 interdiff-8-15.txt18.53 KBxjm
    #9 predelete-1346032-8.patch35.76 KBxjm
    PASSED: [[SimpleTest]]: [MySQL] 33,999 pass(es).
    [ View ]
    #9 interdiff-6-8.txt1.37 KBxjm
    #6 predelete-1346032-6.patch34.74 KBxjm
    PASSED: [[SimpleTest]]: [MySQL] 34,006 pass(es).
    [ View ]
    #3 predelete.patch34.68 KBxjm
    FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/node.api.php.
    [ View ]

    Comments

    Assigned:Unassigned» xjm

    I'll try working on this.

    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.

    StatusFileSize
    new34.68 KB
    FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/node.api.php.
    [ View ]

    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.

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

    Status:Needs review» Needs work

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

    StatusFileSize
    new34.74 KB
    PASSED: [[SimpleTest]]: [MySQL] 34,006 pass(es).
    [ View ]

    Ahem.

    Status:Needs work» Needs review

    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:

    <?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);
      }
    ?>

    Status:Needs work» Needs review
    StatusFileSize
    new1.37 KB
    new35.76 KB
    PASSED: [[SimpleTest]]: [MySQL] 33,999 pass(es).
    [ View ]

    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.

    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.

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

    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.

    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.

    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.

    Status:Needs work» Needs review
    StatusFileSize
    new18.53 KB
    new49.88 KB
    PASSED: [[SimpleTest]]: [MySQL] 34,120 pass(es).
    [ View ]

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

    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.

    StatusFileSize
    new49.88 KB
    PASSED: [[SimpleTest]]: [MySQL] 34,138 pass(es).
    [ View ]

    Rerolled for offset (plus a whitespace fix).

    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.

    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.

    Status:Reviewed & tested by the community» Needs work

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

    Status:Needs work» Needs review
    StatusFileSize
    new838 bytes
    new49.89 KB
    FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch predelete-1346032-21.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
    [ View ]

    Status:Needs review» Fixed

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

    Status:Fixed» Reviewed & tested by the community

    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.

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

    Assigned:xjm» aspilicious

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

    Assigned:aspilicious» Unassigned
    Status:Needs work» Needs review
    StatusFileSize
    new47.81 KB
    FAILED: [[SimpleTest]]: [MySQL] 34,312 pass(es), 2 fail(s), and 0 exception(es).
    [ View ]

    And a patch (try).

    StatusFileSize
    new47.54 KB
    PASSED: [[SimpleTest]]: [MySQL] 34,296 pass(es).
    [ View ]

    Made a mistake

    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.

    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.

    StatusFileSize
    new10.03 KB

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

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

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

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

    Status:Needs work» Needs review
    StatusFileSize
    new49.62 KB
    PASSED: [[SimpleTest]]: [MySQL] 34,215 pass(es).
    [ View ]
    new594 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.

    Assigned:xjm» Unassigned

    StatusFileSize
    new814 bytes
    new49.69 KB
    PASSED: [[SimpleTest]]: [MySQL] 34,207 pass(es).
    [ View ]

    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.

    Status:Needs work» Needs review

    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.

    Issue summary:View changes

    Updated issue summary.

    Issue summary:View changes

    Updated issue summary.

    Issue tags:+Needs change record

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

    Issue summary:View changes

    Updated issue summary.

    Issue summary:View changes

    Updated issue summary.

    Issue summary:View changes

    Updated issue summary.

    Issue summary:View changes

    Updated issue summary.

    Issue summary:View changes

    Updated issue summary.

    Issue summary:View changes

    Updated issue summary.

    Issue summary:View changes

    Updated issue summary.

    Title:ordering of hook_entity_delete() is in-consistentOrdering of hook_entity_delete() is inconsistent

    And that hyphen was really bothering me. :P

    Issue summary:View changes

    Updated issue summary.

    Title:Ordering of hook_entity_delete() is inconsistentChange 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.

    Issue tags:+Entity system

    Priority:Critical» Major
    Status:Active» Fixed

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

    Status:Fixed» Closed (fixed)

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

    Issue tags:-Needs change record

    Hey xjm, take off that issue tag.

    Issue summary:View changes

    Updated issue summary.