Module Grants does not entirely override node_access

Well we can introduce very very simple core patch, something like inserting module_grants_node_access() call at the beginning of the node_access() function. It would be reliable enough, and not hard to maintain.
If I started to work on module like Module Grants, I would start with core patch and it would also then remove the need for overriding access callback.
Yes core patches are generally considered bad, but when you need fix faulty behavior of the core it's worth the hassle.

Node access misbehaving is critical.

Should we patch _node_revision_access() too ? Module Grants implements replacement for it too, but it's private function (starting with underscore) and other modules shouldn't call it directly. Do you know any cases of modules directly calling _node_revision_access() ?
I would say, since it's private function, calling it directly would be wrong use so we shouldn't support wrong use (and general Drupal policy is the same).

Also I worry about this comment in node module:

 * In node listings, the process above is followed except that
 * hook_access() is not called on each node for performance reasons and for
 * proper functioning of the pager system. When adding a node listing to your
 * module, be sure to use db_rewrite_sql() to add
 * the appropriate clauses to your query for access checks.

It seems Module Grants doesn't work for example for Views-generated lists, because Views queries are altered using core node access logics (in db_rewrite_sql() ).
So Module Grants is badly broken because of inconsistent behaviour between single node view and node listing view.

Regarding db_rewrite_sql() see http://www.advomatic.com/blogs/marco-carbone/using-multiple-node-access-...
Module Grants is incomplete in this area indeed..

For db_rewrite_sql() I created another issue, cause it's different problem: #601064: Module Grants behavior is inconsistent for node listings

Agree with all of the above. In particular:

Node access misbehaving is critical.

But more than anything this is critical to core. If core D6 didn't misbehave the way it does, then Module Grants wouldn't have to exist.

Re #1:
"If I started to work on module like Module Grants, I would start with core patch and it would also then remove the need for overriding access callback."

Yes that's the trade-off: make everyone install a core-patch (with possibly far-reaching side-effects), which they may need to re-patch when new versions of core come out, or... keep it clean with a contrib module, which is the path that was chosen with Module Grants.
The latter brings with it the almost impossible task of "fixing the engine (i.e. core) of a car without lifting its bonnet".

As far as "several contributed modules, and indeed core itself, directly call node_access to check for access before taking certain actions" goes, thanks for the suggestion, I will put a warning on the Module Grants project page. Marco can you supply us with some contrib modules and/or core operations that are known to naughtily call node_access() directly?

Rik

Well, maybe core behavior is wrong, but atleast it is consistent. With some workarounds it's quite usable, and being consistent means it can be trusted.
Module Grants, otoh, is not consistent and getting different node access logics in different places is much much worse.

As for example of node access module...well, take any module. node_access() is public function for checking access. Module developers are expected to use it. Disclaimer won't do much, it should just be consistent, and that means core patch is a must.

update: huh, mcarbone already said everything.

As per crea's suggestion in comment #1... Updated the Module Grants documentation page: http://drupal.org/node/408816

Subscribing....