change handling of return values from hook_flag_access()

Integration between Flag and OG sounds great!! :)

> NOTE: The existing hook_flag_access() is not appropriate here since implementing this hook could potentially completely bypass any additional non-role access checks that custom flags perform.

Could you explain more about this please?

We recently added hook_flag_validate(), so this would be a third 'can this flagging happen?' type hook, which is starting to feel like a lot.

This would also need to go on 3.x, not 2.x.

> This is because the $flag->access() function, which invokes hook_flag_access(), works on the basis of the last executed hook implementation has final say on visibility.

I don't think that's quite it, but clearly something of that ilk is going on:

    // Allow modules to disallow (or allow) access to flagging.
    $access_array = module_invoke_all('flag_access', $this, $entity_id, $action, $account);

    foreach ($access_array as $set_access) {
      if (isset($set_access)) {
        $access = $set_access;
      }
    }

I read that as the first module that either says 'Yes' or 'No' is the decider.
That's fine if it's a 'No', as we want a denial by a single module to deny totally.
But it's no good for granting access.

I would be more inclined to say this behaviour is incorrect and needs to be changed.

Compare with node_access()'s code:

  // We grant access to the node if both of the following conditions are met:
  // - No modules say to deny access.
  // - At least one module says to grant access.
  // If no module specified either allow or deny, we fall back to the
  // node_access table.
  $access = module_invoke_all('node_access', $node, $op, $account);
  if (in_array(NODE_ACCESS_DENY, $access, TRUE)) {
    $rights[$account->uid][$cid][$op] = FALSE;
    return FALSE;
  }
  elseif (in_array(NODE_ACCESS_ALLOW, $access, TRUE)) {
    $rights[$account->uid][$cid][$op] = TRUE;
    return TRUE;
  }

> since this is an existing API and this type of change potentially

That's what the new major version is for! :)

Changing title accordingly.

Changes look good, but we don't need to have new constants here:

+++ b/flag.api.php
@@ -175,8 +175,13 @@ function hook_flag_validate($action, $flag, $entity_id, $account, $skip_permissi
+ *    - FLAG_ACCESS_ALLOW : User has access to the flag.
+ *    - FLAG_ACCESS_DENY : User does not have access to the flag.
+ *    - FLAG_ACCESS_IGNORE : This module does not perform checks on this flag/action.

The standard in other parts of Drupal is:

TRUE: grant access
FALSE: explicitly deny access
NULL: say nothing

I'm tagging this as Novice as the patch doesn't need much more work.

Would be great to get this in for 3.0, as it's an API change -- if it doesn't make it in soon, it's postponed till D8!

Great! Look forward to it.

After that, I'll roll a beta and hopefully a 3.0 soon after that!

Here is a simplified patch from #8.

EDIT: After I committed it I noticed that @shawn_smiley wrote a very similar patch few minutes before me ;)
Well, at least we're closer to get Flag 7.x-3.x stable release out now.

Thanks -- both of you! :)
And tests, wow!

Committing this patch, which is #13 with a few tweaks to comments and a change to this assertion text:

> $this->assertNoLink('Flag this item', 0, 'The flag link does not appear on the page.');

I assume the message should say 'does not appear' rather than 'appears', since we're asserting it's not there.

git commit -m "Issue #1965040 by shawn_smiley: Changed handling of return values from hook_flag_user_access() to allow all implementing modules to be considered."

And with that, I'm rolling a beta release tonight!

Change record: http://drupal.org/node/1994208