node_access should cache by vid and not nid.

The problem exists in workflow systems, where the published node is different than the draft node, and where the node access for each might be different. For example, the published node might be viewable by all, but the draft node might only be viewable by document authors, editors, publishers, etc... This later group of people are not super-users, so they don't get 'bypass node access' permission, but through some hook_node_access(), they do have access to unpublished/draft revisions. In this use case, we can not cache by nid, but must cache by vid.

This problem is in node_access() and is not the same problem in #1064954: _node_revision_access() static usage, which is specific to _node_revision_access().

I think that this problem is already solved in Drupal 8 through the use of entity->uuid().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

douggreen’s picture

Title: node_access() static usage » node_access() should static cache by vid and not nid.

Status: Needs review » Needs work

The last submitted patch, core-node-access.patch, failed testing.

azinck’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
568 bytes

I agree 100%. The need for this is exemplified even by _node_revision_access() in core which contains the following line which calls node_access() twice: once each on different revisions of the same node as though the results could differ!

$access[$cid] = node_access($op, $node_current_revision, $account) && ($is_current_revision || node_access($op, $node, $account));

Here's a clean patch.

arknoll’s picture

This appears to still be a problem in D8. The node_access table still only has a nid column. It does not use vid or uuid.