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

    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

    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.

    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

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

    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:

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

    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.

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

    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

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

    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
    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 ]
    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
    StatusFileSize
    new47.81 KB
    FAILED: [[SimpleTest]]: [MySQL] 34,312 pass(es), 2 fail(s), and 0 exception(es).
    [ View ]

    And a patch (try).

    aspilicious’s picture

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

    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

    StatusFileSize
    new10.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
    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.

    xjm’s picture

    Assigned:xjm» Unassigned
    xjm’s picture

    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.

    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.