hook_flag_unflag() should be invoked before the flagging is deleted [#1720180]

        // Note the order: We delete the entity before calling _unflag() to
        // delete the {flagging} record.
        $this->_delete_flagging($existing_flagging_id);
        $this->_unflag($entity_id, $uid, $sid);
        module_invoke_all('flag', 'unflag', $this, $entity_id, $account, $existing_flagging_id);

I suspect the core pattern is usually to invoke hooks *before* data is deleted, so implementations can act on the imminently deleted data.

Comment	File	Size	Author
#7	1720180.flag_.invoke-hook-flag-unflag-before-deletion.patch	3.44 KB	joachim
#7

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

joachim CreditAttribution: joachim commented 9 August 2012 at 08:07

Either way, the order of operations needs to be documented.

Comment #2

joachim CreditAttribution: joachim commented 9 August 2012 at 08:07

And if we changed this, the fix at #1719942: RulesEvaluationException with node unflagged event will need to be reverted.

Comment #3

joachim CreditAttribution: joachim commented 10 August 2012 at 10:16

Title:	should hook_flag() $op unflag be invoke after things have been deleted or not?	» hook_flag_unflag() should be invoked before the flagging is deleted
Category:	task	» bug

Compare:

http://api.drupal.org/api/drupal/modules!node!node.api.php/function/hook_node_delete/7
http://api.drupal.org/api/drupal/modules!system!system.api.php/function/hook_entity_delete/7

This is sufficiently against pattern to be called a bug.

Comment #4

joachim CreditAttribution: joachim commented 11 August 2012 at 14:24

Issue tags:

+Needs change record

Just a note to say that all changes to hook_flag() and its successors should go on the same change notification.

Comment #5

joachim CreditAttribution: joachim commented 17 August 2012 at 08:35

The tricky thing here is that hook_flag_unflag() implementors currently have access to the updated flag counts.

If we reverse the order, then the count they have access to is the OLD count from before the unflagging action, which is one out.

We thus probably have to rearrange things like this:

1. decrease the count
2. invoke hook and rules
3. delete the entity

Which then runs us into potential problems of data integrity if something in the hook or rule causes a failure -- hence we need to start a DB transation (like http://api.drupal.org/api/drupal/modules!node!node.module/function/node_... does).

Comment #6

joachim CreditAttribution: joachim commented 17 August 2012 at 09:27

Splitting the count change and the entity delete means splitting this:

  function _unflag($entity_id, $flagging_id) {
    db_delete('flagging')->condition('flagging_id', $flagging_id)->execute();
    $this->_decrease_count($entity_id);
  }

I think in the long term we should merge _(un)flag() with the flagging CRUD methods, but in the short term to avoid DX WTF, both _(un)flag() methods should lose the nested call to _(inc|dec)rease_count() and instead require callers to call both sequentially (which is just flag() -- these are all private methods anyway). Filed #1736280: unnest call to _(inc|dec)rease_count() in _(un)flag() as a helper issue.