Flag status incorrect when flagging another flag as a reaction to unflagging (Javascript)

Are you sure you exported the rules correctly? I am getting following error:

Integrity check for the imported configuration failed. Error message: Data selector flagged-node for parameter node is invalid..

I think I've got a broken version of Rules. Fixed it and tested it out. You are right, I've got the same problem.

Hmmm, for some reason, when we click unflag, the following bit:

$flag->is_flagged($entity_id)

is still returning 1 the first time we click 'Don't notify' when it should be 0.

Finally, got some time to troubleshoot this further. The problem lies in caching in the function, flag_get_user_flags(). The notify flag is not removed from the $flagged_content cache like it should have been the first time we unflag. In function flag, we are resetting our caches like this:

// Clear various caches; We don't want code running after us to report
// wrong counts or false flaggings.
drupal_static_reset('flag_get_counts');
drupal_static_reset('flag_get_user_flags');
drupal_static_reset('flag_get_entity_flags');

@joachim, does this need to be so early up in this function? If we move, the line:

drupal_static_reset('flag_get_user_flags');

further down, it prevents this error. Basically, the old flag is removed from the cache like it should be. However, I am not sure what the repercussions of moving it further down are. Hope you can share your thoughts on this.

I'm really wary of rearranging the order of this sort of thing. It could have all sorts of repercussions. For starters, it would mean that any hook implementations relying on the data from those functions would get stale data.

I'll try and find time to look at this in more depth over the next week or two.

Thanks Joachim.

If we move, the line:

drupal_static_reset('flag_get_user_flags');

further down, it prevents this error

Just to be a bit more clear, I actually meant if we move that line further down in the same function, like the last thing before the return.

I presume you mean move it down to the end of that function. In which case it would come after all the hook invocations. So any implementations of the hooks would get stale data from those API functions.

> The above example uses rules, but the problem also exists when using hook_flag_unflag()

I can reproduce this with the hook:

/**
 * Implements hook_flag_unflag().
 */
function MYMODULE_flag_unflag($flag, $entity_id, $account, $flagging) {
  if ($flag->name == 'test_2154251_active') {
    $flag_passive = flag_get_flag('test_2154251_passive');
    $flag_passive->flag('flag', $entity_id);
  }
}

(The two flags I have are called 'active' and 'passive': the active one controls the passive one.)

Here's what is happening, AFAICT:

- User unflags the active flag
- flag_page() is called via ajax
- flag_page() calls $flag->flag()
- static caches are cleared [A]
- hook_flag_unflag() is invoked
- our hook_flag_unflag() is called, and programmatically flags the passive flag by calling $flag->flag()
- static caches are cleared [B]
- hook_flag_unflag() is invoked for the passive flag
- $flag->flag for the the passive flag is complete
- the active flag's Flagging entity is deleted [C]
- $flag->flag for the the active flag is complete
- flag_page() calls flag_build_javascript_info() to collect the info to pass back over AJAX to create the updated link
- flag_build_javascript_info() calls $flag->is_flagged to figure out what type of link to send back
- $flag->is_flagged calls flag_get_user_flags() [D]

Here's where I think the problem is:

- At point A, the static cache is cleared. That should mean that at point D, flag_get_user_flags() has no static data and goes to query the database.

However, the fact that the passive flag has its own trip through the flag workflow means that at some point, the static cache is set. Because we haven't yet reached point C, the database still has the flagging for the active flag, and so the static cache is set with the OLD value for the active flag. In effect, the effect of the cache clear the active flag did is undone.

And here is the problem with that static cache:

  if (isset($entity_id)) {
    if (!isset($flagged_content[$uid][$sid][$entity_type][$entity_id])) {

The static cache is only checked per-entity. So if the cache has been set for the one node we are working on, subsequent calls will take this cache.

I'm still not sure how we can fix this. flag_get_user_flags() purposely does this:

   * Thanks to using a cache, inquiring several different flags about the same
   * item results in only one SQL query.

Yeah, this is a tricky one. The whole point of the function is so that we only need to run one SQL query for the node that we are trying to access, but I am not sure how we can fix this and still limit it to just one query.

Will need to put some thought into this...

Actually, now I've slept on it, I think your original fix may be right.

At the start of flag(), the flag_get_user_flags cache will be incorrect, because it has data from before the (un)flagging was begun.

Once you clear the flag_get_user_flags cache, any subsequent call will STILL be incorrect, because the flagging entity in the database has not yet been created/deleted. The cache will be empty, the query will run, and the cache will be populated with the identical data that was cleared from it earlier!

It's only after the actual database operation on the {flagging} table that the query in flag_get_user_flags() will get correct information.

Therefore:

- anyone who has been calling flag_get_user_flags() during the flag() process (ie in a hook implementation) has been getting bad data anyway
- it makes sense to clear a cache as close as possible to the change that invalidates it. I'd consider whether that might be _insert_flagging() / _delete_flagging().

We should look at the other calls to drupal_static_reset() in follow-on issues, and see if they should be moved too.

it makes sense to clear a cache as close as possible to the change that invalidates it

That's precisely why I thought it should be after the database changes that are made in the function. So, each time a flag() process is done, flag_get_user_flags() will have data that reflects the db.

However, I was kind of apprehensive about this change as I wasn't sure what the ramifications of this would be.

it makes sense to clear a cache as close as possible to the change that invalidates it. I'd consider whether that might be _insert_flagging() / _delete_flagging().

Couldn't we just do this as the last thing before the return as both a flagging and an unflagging affects the flagging table and consequently, invalidates the cache.

We should look at the other calls to drupal_static_reset() in follow-on issues, and see if they should be moved too.

The only other places drupal_static_reset() is called are in these functions:
_flag_clear_cache() - flag.admin.inc
revert - flag_flag.inc

Both of them reset flag_get_flags(). But their placement don't seem problematic as _flag_clear_cache() is the last thing that we do after making changes.

I'm concerned that if we move the cache clearing this far down, it now happens after rules_invoke_event_by_args(): Rules events will get stale data, whereas before they happened after the cache clear and after the database change.

I think we have to clear caches as near as possible to the actual database changes, so that anything invoked after that gets fresh data.

I also think these deep parts of our code are really crusty and need a good clean up, but that's another issue.

Okay, I have got a question. Why is it when $action == 'flag' we are making the db changes and then invoking the Rules event?

        // The _flag() method is a plain DB record writer, so it's a bit
        // antiquated. We have to explicitly get our new ID out.
        $flagging->flagging_id = $flagging_id;
        $this->_increase_count($entity_id);
        // We're writing out a flagging entity even when we aren't passed one
        // (e.g., when flagging via JavaScript toggle links); in this case
        // Field API will assign the fields their default values.
        $this->_insert_flagging($flagging);
        module_invoke_all('flag_flag', $this, $entity_id, $account, $flagging);
        // Invoke Rules event.
        if (module_exists('rules')) {

Whereas, when $action == 'unflag', we are invoking the Rules event, and then making the db changes.

          // Invoke Rules event.
          if (module_exists('rules')) {
            $event_name = 'flag_unflagged_' . $this->name;
            // We only support flags on entities.
            if (entity_get_info($this->entity_type)) {
              $variables = array(
                'flag' => $this,
                'flagged_' . $this->entity_type => $entity_id,
                'flagging_user' => $account,
                'flagging' => $flagging,
              );
              rules_invoke_event_by_args($event_name, $variables);
            }
          }
          $this->_delete_flagging($flagging);
          $this->_unflag($entity_id, $flagging->flagging_id);

For both types of flaggings, if we just move the db changes before the Rules event, then, do the cache clearing, and then, invoke the Rules events, wouldn't that make more sense and give all functions accurate data? Like the attached patch.

That does seem like a problem, yes. I think it's one we should fix, though it'll affect users using Rules as the behaviour of their rules may change. Does it affect this issue, or can it be tackled separately?

It also affects this issue as, once we move the db changes before the Rules invocations and clear cache right after, this issue will be fixed. We need to perform the same set of steps for both flagging and unflagging to make the module more coherent and standardized.

Not sure why the earlier patch failed. Attaching the patch again.

I'm adding tests for this sort of thing, starting with #2193421: add tests for hook invocation during flag/unflag.

The tests I wrote make it clearer what's going on:

    // Flag:
    $expected_hook_order = array(
      'hook_entity_presave',
      'hook_entity_insert',
      'hook_flag_flag',
      'rules_event',
    );

    // Unflag:
    $expected_hook_order = array(
      'hook_flag_unflag',
      'rules_event',
      'hook_entity_delete',
    );

The reason for having hook_entity_delete() go last is that other hooks may want to actually have the entity in question to look at or work with in some way. You may want to load things that depend on it, inspect its fields, and so on.

Here's my go at it -- I think it's best to move the cache clears to be as close as possible to the database changes that invalidate them. This also helps with #2202969: clean up flag() and its low-level helpers.

I'm hoping this fixes the problem. Here's the old workflow from my comment above:

- User unflags the active flag
- flag_page() is called via ajax
- flag_page() calls $flag->flag()
- static caches are cleared [A]
- hook_flag_unflag() is invoked
- our hook_flag_unflag() is called, and programmatically flags the passive flag by calling $flag->flag()
- static caches are cleared [B]
- hook_flag_unflag() is invoked for the passive flag
- $flag->flag for the the passive flag is complete
- the active flag's Flagging entity is deleted [C]
- $flag->flag for the the active flag is complete
- flag_page() calls flag_build_javascript_info() to collect the info to pass back over AJAX to create the updated link
- flag_build_javascript_info() calls $flag->is_flagged to figure out what type of link to send back
- $flag->is_flagged calls flag_get_user_flags() [D]

Here's how it goes now:

- User unflags the active flag
- flag_page() is called via ajax
- flag_page() calls $flag->flag()
- hook_flag_unflag() is invoked
- our hook_flag_unflag() is called, and programmatically flags the passive flag by calling $flag->flag()
- hook_flag_unflag() is invoked for the passive flag
- static caches are cleared [B]
- $flag->flag for the the passive flag is complete
- the active flag's Flagging entity is deleted [C]
- static caches are cleared [A]
- $flag->flag for the the active flag is complete
- flag_page() calls flag_build_javascript_info() to collect the info to pass back over AJAX to create the updated link
- flag_build_javascript_info() calls $flag->is_flagged to figure out what type of link to send back
- $flag->is_flagged calls flag_get_user_flags() [D]

This means that when we get to point D, the most recent static cache clear is now the one done by the outer flag, at A. That happens after BOTH flaggings were deleted.

Therefore, D should get correct data to build its flag link.

Yeah, I like this patch a lot better and it fixes this issue as we are getting the correct data now.

Thanks for testing! I accidentally deleted the flags I had made to test this the other day! :)
And thanks for all your work debugging this one and figuring out where the problem was.

git commit -m "Issue #2154251 by Shabana Blackborder, joachim: Fixed flags that react to other flags getting an incorrectly formed link when unflagging with the Javascript link; moved cache clears to be beside the database changes." --author="ShabanaBlackborder "

Oh what still needs figuring out is whether any hook invocations or rules events now get a different value for the count if they request it -- and write up a change record for that.

Yeah, will need to look into that. This will require a bit more time as we need to investigate further, hoping to get around to it as soon as I finish a couple of things.

I was thinking we should expand the hook tests to have each hook call the API functions and so we can test that the counts & flag data is correct (or at least know for definite it's not!).

So the simplest thing to do would be to add that to the tests, then locally revert the patch from this issue, run the tests again and compare the debug output to see exactly how the API function's data has changed.

Change record: https://drupal.org/node/2210055.

Wow, awesome work, Joachim. You got through all these issues really quickly!

I've had a lot of time sitting on trains lately :D

-

Hehee, lucky for Flags! Wished I had some free time like that :)

:)

Follow-up: #2208847: flag_get_FOO_counts() API functions do not have their static caches reset when (un)flagging.