Make node_access() language-aware

Well, there is also a major architecture question about whether the individual rules applied via hook_node_access() should continue to be supported. The main problem with that access hook is that it makes generating lists of content virtually impossible.

It's related because it changes the nature of this patch if we ditch hook_node_access().

The problem I have with this approach -- like many others when touching on a language context -- is that it treats language as a special case. I understand that, due to the limits of node_access, we cannot consider language to be a realm (since the permissive OR nature of grants enforcement won't allow it).

It would be cleaner, perhaps, to allow AND grants to be enforced (something I've advocated since Drupal 6).

As for hook_node_access(), having $langcode should be irrelevant, since the $node object should already have a language property.

Scratch the last part of that. I can see a use-case for asking whether a node is visible in a different $langcode.

Agreed on that point.

Gabor and I discussed this on IRC. The move in Drupal 8 is to make $langcode a part of the entity_id key. E.g., instead of just having $node->nid as a foreign key, the key will be $node->nid + $node->langcode.

This patch just pushes that direction forward, so it looks good conceptually.

Minor changes:

+  // to an empty langcode if a node type was requested. The later is purely

Use "latter" not "later" here.

The following review is just from reading the patch; I will take a closer look in context later.

1. We definitely need to add test coverage for this.

2. +++ b/core/modules/node/node.api.phpundefined
   @@ -604,7 +606,7 @@ function hook_node_load($nodes, $types) {
   -function hook_node_access($node, $op, $account) {
   +function hook_node_access($node, $op, $account, $langcode) {
   
   +++ b/core/modules/node/node.moduleundefined
   @@ -2848,13 +2848,17 @@ function node_form_system_themes_admin_form_submit($form, &$form_state) {
   -function node_access($op, $node, $account = NULL) {
   +function node_access($op, $node, $account = NULL, $langcode = NULL) {
   

   I noticed that the parameter for the hook is non-optional despite that the argument to node_access() is optional. Gábor and I discussed this in IRC, and he pointed out that the same is true of the $account parameter.

   In reality the hook definition is merely documentation--an module can declare a hook implementation with fewer than the recommended arguments and it will work just fine--so it doesn't matter too much either way.

3. +++ b/core/modules/node/node.moduleundefined
   @@ -2872,18 +2876,25 @@ function node_access($op, $node, $account = NULL) {
   -  if (isset($rights[$account->uid][$cid][$op])) {
   -    return $rights[$account->uid][$cid][$op];
   +  if (isset($rights[$account->uid][$cid][$langcode][$op])) {
   +    return $rights[$account->uid][$cid][$langcode][$op];
   ...
   -    $rights[$account->uid][$cid][$op] = TRUE;
   +    $rights[$account->uid][$cid][$langcode][$op] = TRUE;
   ...
   -    $rights[$account->uid][$cid][$op] = FALSE;
   +    $rights[$account->uid][$cid][$langcode][$op] = FALSE;
   
   @@ -2892,24 +2903,25 @@ function node_access($op, $node, $account = NULL) {
   -    $rights[$account->uid][$cid][$op] = FALSE;
   +    $rights[$account->uid][$cid][$langcode][$op] = FALSE;
   ...
   -    $rights[$account->uid][$cid][$op] = TRUE;
   +    $rights[$account->uid][$cid][$langcode][$op] = TRUE;
   ...
   +    $rights[$account->uid][$cid][$langcode][$op] = TRUE;
   
   @@ -2937,13 +2949,13 @@ function node_access($op, $node, $account = NULL) {
   -      $rights[$account->uid][$cid][$op] = $result;
   +      $rights[$account->uid][$cid][$langcode][$op] = $result;
   ...
   -      $rights[$account->uid][$cid][$op] = TRUE;
   +      $rights[$account->uid][$cid][$langcode][$op] = TRUE;
   

   For these it would perhaps be better to make the $langcode a part of the $cid rather than a separate level of nesting?

4. +++ b/core/modules/node/node.moduleundefined
   @@ -2892,24 +2903,25 @@ function node_access($op, $node, $account = NULL) {
   +  // @todo: add langcode support to node_access schema / queries.
   

   Presumably this refers to #1658846: Add language support to node access grants and records, right?

   There should be no colon after @todo, and Add should be capitalized. I would actually also like to see this @todo at the end of the function docblock.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageTest.php
@@ -0,0 +1,86 @@
+    $base_node_access = array('view' => TRUE, 'update' => FALSE, 'delete' => FALSE);

I think $expected_node_access would be a clearer variable name here.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageTest.php
@@ -0,0 +1,86 @@
+    // Tests that Catalan provided specifically results in the same. The node
+    // has no Catalan property support yet, but all properties are returned
+    // regardless of language with their original values.
+    $this->assertNodeAccess($base_node_access, $node, $web_user, 'ca');
+
+    // Tests that Croatian provided specifically results in the same. The setup
+    // has no Croatian language configured, but all properties are returned
+    // regardless of language with their original values.
+    $this->assertNodeAccess($base_node_access, $node, $web_user, 'hr');

I don't get either of these 2 comments. It looks like all we're asserting is that when we haven't yet set 'node_access_test_secret_catalan' that special Catalan logic isn't invoked, but the comments seem unrelated to that.

+++ b/core/modules/node/node.api.php
@@ -596,6 +596,8 @@ function hook_node_load($nodes, $types) {
+ * @param object $langcode
+ *   The language code to perform the access check operation on.

I think this doc can be improved, but I don't have a specific suggestion at this time, and I'm okay with letting it go until a follow-up.

+++ b/core/modules/node/node.module
@@ -2872,18 +2880,25 @@ function node_access($op, $node, $account = NULL) {
+  // If no language code was provided, default to the node's langcode or
+  // to an empty langcode if a node type was requested. The latter is purely
+  // for caching purposes.

This is awkward, and so I agree with xjm that we should instead do something like

if (is_object($node)) {
  if (!isset($langcode)) {
    $langcode = $node->langcode;
  }
  $cid = $node->nid . ':' . $langcode;
}
else {
  $cid = $node;
}

I don't see that as causing inconsistency, because $account is who we're determining access for, and $op is what kind of access, but $node->nid and $langcode jointly make up the thing we're checking access to.

:)

This seems to be a pretty straight-forward change, and has approval from a couple of the node access system maintainers.

Committed and pushed to 8.x. Seems like we will need a change notice.

If I'm not mistaken the added test classes were not committed. Adding that to the title.

Oops! Added the missing file and committed and pushed it to 8.x. Sorry. :(

I updated Devel Node Access to this level, see #1727532: Update DNA for language-aware node access

Add revsions followup issue.