Follow up for confusing comments in #1658846-187: Add language support to node access grants and records
Problem/Motivation
Comments are unclear.
Proposed resolution
Improve comments. Maybe by adding a general sentence explaining fallback and defaults that deal with langcodes. Maybe by adding a @see to link to the api documentation that explains the fallback behavior.
Remaining tasks
- Discuss why those tests/statements/assertions are there.
- Propose what the comments should say.
- Implement resolution. See contributor task document for creating a patch: http://drupal.org/node/1424598
User interface changes
No UI changes.
API changes
No API changes.
Original report by @xjm
in #1658846-187: Add language support to node access grants and records
+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessLanguageTest.phpundefined
@@ -61,31 +47,196 @@ function testNodeAccess() {
+ // Creating a public node with langcode Hungarian, will be saved as
+ // the fallback in node access table.
...
+ // Creating a private node with langcode Hungarian, will be saved as
+ // the fallback in node access table.
...
+ // Creating a private node with no special langcode, like when no language
+ // module enabled.
...
+ // Creating a private node with langcode Hungarian, will be saved as
+ // the fallback in node access table.
...
+ // Creating a public node with langcode Hungarian, will be saved as
+ // the fallback in node access table.
...
+ // Creating a public node with no special langcode, like when no language
+ // module enabled.
These three comments are a little unclear (same for the similar comments in the other tests).
--
Also:
--- a/core/modules/node/lib/Drupal/node/NodeAccessController.php
+++ b/core/modules/node/lib/Drupal/node/NodeAccessController.phpundefined
@@ -109,11 +109,19 @@ protected function accessGrants(EntityInterface $node, $operation, $langcode = L
// Check the database for potential access grants.
$query = db_select('node_access');
$query->addExpression('1');
+ // Only interested for granting in the current operation.
$query->condition('grant_' . $operation, 1, '>=');
I don't quite understand this comment.
Comments
Comment #1
Gábor HojtsyWell, "// Only interested for granting in the current operation." just documents what the code trivially does below it. We can remove it if that is better. It is not new code added by the patch, just new docs. It documents the existing behavior. It could also have been "Filter only for elements for the requested operation". But again, looks like not adding a comment is cleaner than adding one that people might not grok? The condition is pretty self documenting.
As for the comments cited pior to that, as far as I see, they say "we save a node with X language, so X will be marked as the fallback langcode for node access" which is just repeatedly documenting how the node's original language and node access fallback language are tied together. Any suggestions for improving that text?
Comment #1.0
Gábor Hojtsyadded more comment clarification stuff.