This is a followup to #1658814: Make node_access() language-aware to apply the language additions to node revisions access as well. Node revisions access checking is implemented in http://api.drupal.org/api/drupal/modules%21node%21node.module/function/_... and reuses node_access(), so mostly passing on of the arguments is required as well as adding $langcode as part of the internal static cache key. Should be straightforward.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +Entity system

Tag for entity system too.

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
2.11 KB

Just reusing the node access language feature.

agentrickard’s picture

Looks fine. One question: can different node revisions have different languages? For instance, could the first draft be in English and the next revision be moved to Magyar?

If that were possible, then would $node->langcode be trustworthy?

Gábor Hojtsy’s picture

@agentrickard: It is possible that $node->language changes, however _node_revision_access() checks for a specific revision, and if the language is not provided, then it looks intuitive to fall back on the language of that revision AFAIS. What do you think?

agentrickard’s picture

As long as the $node object here is loaded from that revision, I think it's fine. Just checking.

We don't want to do anything wild like rename $node to $version here, do we?

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/node.module
@@ -1784,8 +1788,13 @@ function _node_revision_access(Node $node, $op = 'view', $account = NULL) {
   // Statically cache access by revision ID, user account ID, and operation.
-  $cid = $node->vid . ':' . $account->uid . ':' . $op;
+  $cid = $node->vid . ':' . $langcode . ':' . $account->uid . ':' . $op;

The comment should be updated.

Also, it seems weird that _node_revision_access() uses a unique $cid, while node_access() uses a multi-dimensional array. I'm not suggesting we fix that in this issue, just noting here for the record.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.21 KB

@agentrickard: I don't think its related to introducing language support to change variable naming or of the node. Also, it was recently updated when the node class was introduced as it seems, and @xjm noted it was updated to use this flat cache structure, and in neither case it was seen necessary to rename looks like. I'm fine with renaming it, but then maybe $node_version or $node_revision would be better to signify that it is indeed a node.

@tstoeckler: updated the comment. @xjm noted the difference in cache keys before and indicated that in fact node_access() would need to be changed to use the flat static cache keys, not this function to use the nested cache array. So I don't think that means any work to do here either.

tstoeckler’s picture

Yes, I agree that flat arrays make sense and are more consistent with what we do elsewhere.

Anyway, this looks good. I'm pretty sure this doesn't need to be tested manually, but can someone verify that, or else state what specifically does need to be tested?

agentrickard’s picture

@Gabor : agreed. Thanks.

One other question that spins off from the revision state question: Does this patch accidentally introduce the concept of node access keyed by version id instead of node id? This is a serious architecture and UX question if we intend to support it.

Here's what I mean by "accidentally":

* Node 1 revision 1 has language 'en'
* Node access table entry is nid 1 lang en etc.
* The node nid has 1 record in {node_access}
* Node 1 revision 2 is saved as language 'ca'
* Node access table entry is nid 1 lang ca etc.
* The node nid has 2 records in {node_access}

I think is does not introduce a vid to {node_access}, but since {node_access} also stores data for unpublished nodes, we have to be careful about which revision's data is used to populate {node_access}.

Gábor Hojtsy’s picture

Well, node access does not have per-revision support either way I guess. Once node access records get language support too, node revisions having different languages could make more node access records "creep" into the system but those would happen regardless of revisions anyway the same way, if the node is available in different languages (and would possibly need different language permissions). I think how that is implemented is the most complex / interesting part of this discussion, and should be implemented in #1658846: Add language support to node access grants and records. Without that this does not have any side effects like that, and with that, it depends on how we implement that and what certain data stored will mean when applied.

agentrickard’s picture

I'm not worried about row count creep so much as suddenly finding ourselves in a position where the primary key switches from 'nid' to 'nid-langcode' (which we have agreed to do) to 'nid-vid-langcode' without deliberation.

Not a blocker on this issue.

Gábor Hojtsy’s picture

I have no intention of it changing to nid-vid-langcode, and I don't think the explained scenario would mean that. Since the node access system does not have vid level details, it will just look for the high level nid-language combination regardless of vid.

agentrickard’s picture

What I'm suggesting is that a new language per revision could force us to use vid, even though we don't want to. (This is part of a larger issue that I never saw addressed, which is "do we want access controls based on entity id or entity revision id", which I don't think has ever been addressed.)

Like I said, though, not a blocker here.

Gábor Hojtsy’s picture

Issue tags: +Needs tests

Ok, anything else to do here then? :) Apart from expanding test coverage in core/modules/node/lib/Drupal/node/Tests/NodeRevisionPermissionsTest.php that is :)

Gábor Hojtsy’s picture

Here come tests. Reusing the same "secret Catalan" logic used in the higher level node_access() tests. I took the basics of the existing node revisions access tests and topped it off with variance in testing for the plain scenario and the "secret Catalan" scenario for all of langcode NULL, 'hu' and 'ca'. Very similar to the base node access language tests, but probably even more variations of things tested :)

Please review! Any findings of things to improve?

tstoeckler’s picture

Status: Needs review » Needs work

Very nifty test! Nice!
I found a few things to complain about, though. Mostly minor / code-style stuff.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeRevisionLanguagePermissionsTest.php
@@ -0,0 +1,106 @@
+    $language = (object) array(
+      'langcode' => 'hu',
+    );
...
+    $language = (object) array(
+      'langcode' => 'ca',
+    );

This is super minor, but this should use new Language(...)

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeRevisionLanguagePermissionsTest.php
@@ -0,0 +1,106 @@
+    for ($i = 0; $i < 3; $i++) {
...
+      $this->node_revisions[] = $revision;
+    }
...
+    $revision = $this->node_revisions[1];

Why are we creating 3 revisions if we're only using one in the test?

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeRevisionLanguagePermissionsTest.php
@@ -0,0 +1,106 @@
+      $account->op = $op;

Since $account is a classed object, it feels really awkward to tack a random variable onto it. Off the top of my head, I could not find a way to avoid that, though. Since it's only tests, I guess we can get away with it.

Nobody tell Crell about this issue, though... :)

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeRevisionLanguagePermissionsTest.php
@@ -0,0 +1,106 @@
+      drupal_static_reset('node_access');
+      drupal_static_reset('_node_revision_access');

Is this needed for any module who dynamically determines access to nodes/revisions? If so, that would be a cool thing to document. I looked briefly at http://api.drupal.org/api/drupal/core!modules!node!node.module/group/nod... and http://api.drupal.org/api/drupal/core!modules!node!node.api.php/function... but couldn't find anything.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeRevisionLanguagePermissionsTest.php
@@ -0,0 +1,106 @@
+          if ((!empty($case['account']->is_admin) || $case['op'] == $case['account']->op) && ($langcode != 'ca' || !$node_access_test_secret_catalan)) {

The !empty($case['account']->is_admin seems strange here. It is only needed, because the users we created above might be user 1, right? If so, can we not simply check for that above and then kill this condition here? I might be missing something.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeRevisionLanguagePermissionsTest.php
@@ -0,0 +1,106 @@
+            $this->assertTrue(_node_revision_access($revision, $case['op'], $case['account'], $langcode), "{$this->map[$case['op']]} granted.");
...
+            $this->assertFalse(_node_revision_access($revision, $case['op'], $case['account'], $langcode), "{$this->map[$case['op']]} not granted.");

The assertion messages look very sparse. The look something like "view granted." and "delete not granted." right? Maybe at least something like "view operation granted for user 2." or whatever.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5.86 KB
3.2 KB

@tstoeckler: Thanks for the review. Improved patch attached. Notes:

1. All right, changed to instantiate Language.
2. We were in fact creating 4 revisions (including the original). I think it makes sense to pick a revision from multiples to test with, but I decreased the number of total revisions created to 3 instead of 4 (and still picking the middle one for the test). Expanded comments as well to reflect this. Also see later (*)
3. See later (*)
4. The static resets are of course needed because we change the node permissions setup in the middle of the HTTP request by turning on the node_access_test module's logic to make Catalan nodes inaccessible. That is why the code will behave differently when that switch is turned on.
5. The is_admin property is not even used in this test, removed. Also see later (*).
6. See later (*)
7. (You did not mention, but I could have put the langcodes to the permutation generator, did that now. I did not include the 0/1 node access switch because it needs these static clears and not doing for all permutations individually seemed like a better idea).

Finally, all points marked above with (*) are a result of this test being derived (copied and edited) from the NodeRevisionsPermission.php test case. It does create 4 revisions and only uses 1 (point 2), it tacks the op on the $account (point 3), it tacks on an is_admin property (point 5) and has short assert messages (point 6). I've updated the code where it was evident (points 2 and 5), but left the code as-is for consistency where it was doing the same (points 3 and 6).

Any other comments / suggestions for improvement? :)

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Yes, I agree if the other test does the same (which I checked) that it makes sense to keep it this way here. Especially since I couldn't come with a better solution anyway. :-)
Looks RTBC to me.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Superb, thanks.

Automatically closed -- issue fixed for 2 weeks with no activity.