In node:

        node_invoke($node, 'delete');
        module_invoke_all('node_delete', $node);
        module_invoke_all('entity_delete', $node, 'node');
        field_attach_delete('node', $node);

In flag:

    field_attach_delete('flagging', $flagging);
    // Remove from the cache.
    entity_get_controller('flagging')->resetCache();
    // Invoke hook_entity_delete().
    module_invoke_all('entity_delete', $flagging, 'flagging');

Data should still be available when hook_entity_delete() is invoked, so implementations can make use of it.

CommentFileSizeAuthor
#8 flag-2196085-8.patch1.05 KBcs_shadow
#6 flag-2196085-6.patch1.12 KBcs_shadow
#4 flag-2196085-4.patch1.08 KBcs_shadow
#2 flag-2196085-2.patch705 bytescs_shadow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

joachim CreditAttribution: joachim commented
Parent issue: » #2202969: clean up flag() and its low-level helpers
cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Status: Active » Needs review
FileSize
flag-2196085-2.patch705 bytes

Attaching a quick patch where hook_entity_delete() is invoked before deleting field data. Also added a short comment for field_attach_delete, if that helps to understand the code better.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Needs work

This is going to need a reroll due to the refactoring that took place in #2202969: clean up flag() and its low-level helpers. I should have marked this as postponed until that one was fixed -- sorry!

cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Status: Needs work » Needs review
FileSize
flag-2196085-4.patch1.08 KB

Rerolled the patch against latest HEAD.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Needs work

Thanks for working on a patch!

+++ b/includes/flag/flag_flag.inc
@@ -920,17 +920,15 @@ class flag_flag {
+      db_delete('flagging')->condition('flagging_id', $flagging->flagging_id)->execute();

I'm not sure the order is right. The entity should still exist in the DB when field API and hooks are invoked.

Look at node_delete() -- we should do the same as that, for consistency.

cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Status: Needs work » Needs review
FileSize
flag-2196085-6.patch1.12 KB

Changed the order as in node_delete().

Order in node_delete:

  db_delete('node_type')
    ->condition('type', $type)
    ->execute();
  field_attach_delete_bundle('node', $type);
  module_invoke_all('node_type_delete', $info);

  // Clear the node type cache.
  node_type_cache_reset();

New order in flag:

      // Delete the flagging entity.
      db_delete('flagging')->condition('flagging_id', $flagging->flagging_id)->execute();
      // Delete field data.
      field_attach_delete('flagging', $flagging);
      // Invoke hook_entity_delete().
      module_invoke_all('entity_delete', $flagging, 'flagging');

      // Remove from the cache.
      entity_get_controller('flagging')->resetCache();
joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Needs work

That's not node_delete(), that's the code for deleting a node *type*.

Though node_delete() just calls https://api.drupal.org/api/drupal/modules!node!node.module/function/node.... But that definitely deletes the record after invoking hooks.

cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Status: Needs work » Needs review
FileSize
flag-2196085-8.patch1.05 KB

Sorry, looked at the wrong function earlier. This one follows the order of deleting nodes. Now the order is:

      // Invoke hook_entity_delete().
      module_invoke_all('entity_delete', $flagging, 'flagging');
      // Delete field data.
      field_attach_delete('flagging', $flagging);
      
      // Delete the flagging entity.
      db_delete('flagging')->condition('flagging_id', $flagging->flagging_id)->execute();
      
      // Remove from the cache.
      entity_get_controller('flagging')->resetCache();
joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Fixed

Perfect! Thanks for working on this.

  • Commit f0fcc38 on 7.x-3.x authored by cs_shadow, committed by joachim: 
    Issue #2196085 by cs_shadow: Fixed hook_entity_delete() getting invoked...
cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented

@joachim thanks! Finally got it right.

Status: Fixed » Closed (fixed)

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