Have a $flag->access() method

This mathod will call the current two access methods, and then, once $op is known, will continue to do more more access checks.

I don't mean to say that this issue, #322034, deals also with the access checks described in #285237: Ability to disallow a flag/unflag operation. No. That would entail schema and UI changes and we'll never finish it.

My intention here is to replace the existing user_access()+applies_to_content_object() with a single access().

Benjamin, that's very good!

Here's a re-roll with minor changes.

- The patch was adjusted to accomodate #320271: [API] Possibility to skip user permission check, which has just got committed.

- I added some comments with pseudo code to the end of access() to demonstrate what kind of code we should expect to see there in the future. This is the rationale for this patch.

I haven't yet tested the patch, though (that's why I'm not yet marking this 'needs review', it's in the brainstorming stage yet, I think).

Nathan, what are you thoughts on this plan?

Things to do:

1.

function access($content_id, $account = NULL, $action = NULL) {

We should rename $content_id to $content_id_or_object because sometimes we already have an object at our hands. The method will have to do is_object($content_id_or_object) to detect what it got.

2. 'applies_to_content_id_array()' is the SQL counterpart of our access checking method. We should rename it to, say, 'array_access()'.

Things to do:

1. [...]
2. [...]

Here's a patch with these things done.

I ran into an obvious problem though, how could we check flag access when displaying in Views?

First, I shall give an overview:

We provide two ways to do access checking:

1. At the "PHP level" (that's the $flag->access() method).
2. At the "SQL level" (that's the $flag->access_array() method).

We're mimicing Drupal itself here. Drupal too has two ways to do access checking:

1. At the "PHP level" (that's the node_access() function, and hook_access).
2. At the "SQL level" (that's the node_db_rewrite_sql hook).

The "SQL level" is used when displaying lists of nodes; it allows us not to load every node prior to determining if we have access to flag/unflag it. It's useful for views that are styled as "table" or "fields".

Of course, the comparison I make here between our module and Drupal's Node module isn't accurate. (Our $flag->access_array() doesn't really work in the SQL level: it uses PHP code.) My intention here is only to explain why we need both access() and access_array().

how could we check flag access when displaying in Views?

Views will be using $flag->access_array().

(It already does this, in the patch.)

I mentioned in #285237: Ability to disallow a flag/unflag operation that:

A reminder: All access checks need to be duplicated in the access_array() method, where, for efficiency reasons, we don't load the nodes. (This is used for 'table' Views, for example [...]

Note that one important thing this patch does is to pass to access_array() the flagging state for each node. Future patches (to implement the various access controls in #285237) will use this flagging state to determine if a flag/unflag action is permitted.

(For example: when a node is flagged, the "Roles that are permitted to unflag an item" setting will be consulted. When a node is unflagged, the "Roles that are permited to flag an item" setting will be consulted.)

how could we check flag access when displaying in Views?

Do you mean "How can we call hook_flag_access in access_array()"? Well, access_array() isn't really working in the SQL level, so there's no problem calling there the hook for every node. (Implementations of this hook might load the node --they'll most probably do this-- and there's an efficiency issue here. That's why I don't think hook_flag_access's signature should follow that of Drupal's Node module).

Here's a re-roll, a little more fleshed out in terms of how a hook_flag_access could be implemented.

(Yes, I've noticed your hook_flag_access can either allow or disallow an operation, whereas my proposal could only disallow an operation.)

I switched the $flag->access method to always accept an object [...] to help match hook_access() provided by node module.

I don't think doing something merely to match an existing signature in Drupal is a good thing.

("signature" is the types and order of the parameters passed to a function. But I don't include the name of the function in this ad-hoc definition.)

For example, fetch_content() could be avoided in some scenarios, e.g. if the node is of type 'page' and the flag only applies to the 'story' type. Addmittedly, Drupal's node_load() has an internal cache, and user_load()'s lack of cache can be compensated by MySQL's query cache, and furthermore I don't have a good example for demonstrating performance loss by always doing fetch_content(). You may be right (that we should pass an object to access() and that's it.) It's just that I think we need to think about this a little bit and not do things just because Drupal's hook_access() has a certain signature.

I think that hook_flag_access() should get an array of content IDs. Programmers wishing to write efficient code would base their code mostly on DB queries. Lazier programmers would do a loop that fetch_content()'s each node. We'll leave this choice to the programmer.

It's worth iterating that the purpose of access_array() is only to make "table" views not load the nodes when they show the flag/unflag link.

The alternative, the access check would need to be provided in some other way, possibly through db_rewrite_sql or hook_views_data_alter(),

No, things aren't that complicated. For example, if we'd have a "Users can flag only content they have created" setting, then we need our access_array() to SELECT the UID column as well, and then inspect it.

(I'll comment shortly.)

Oops, I replied in the wrong issue.

But, I also wanted to comment in this issue.

Instead of doing the whole plan in one step, we could, as a first step, just commit a flag::access() method. A very very minimal one (one that doesn't call further hooks). We can postpone deciding on the other issues. Having a flag::access() for awhile perhaps will make us a bit wiser (and make our code more elegant).

(Sure, I see your last commits. Great work! To finaly get rid of all those nagging issues.)

(As for a minimal access(), it's better not to do this before '1.0'. We know the current code doesn't have bugs. It's seems needlessly risky to introduce even the most minimal code factorization right now.)