Active
Project:
Drupal core
Version:
main
Component:
node system
Priority:
Minor
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
27 Mar 2013 at 03:49 UTC
Updated:
16 Jul 2025 at 17:31 UTC
Jump to comment: Most recent
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.
Comment #15
smustgrave commentedThank you for creating this issue to improve Drupal.
We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.
Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Comment #16
smustgrave commentedSince there has been no follow up in 3 months going to close out. But don't worry this can always be re-opened!
Thanks
Comment #17
xjmThey're still there and haven't been clarified. :)
Comment #18
acbramley commentedWe still need more information, I agree with #1 that the comments are a bit superfluous as they're just stating what happens when you save a node with a langcode. Unless we want to heavily document what a fallback is in the test (I don't think we should) maybe we should just remove the entire comment, or just make it "Creating a public node with langcode Hungarian"
We should also update the IS with the exact comments we want to change (remove the original report) and update the proposed resolution.
Comment #19
xjmI don't think PMNMI is the correct status? We just need proposals for better comments. (Another node-access-not-actually-node issue.) It's tagged "Needs IS update", which corresponds to "Active" when there's no MR (or NW when there is one).
I also would not put every single version of these comments in the IS; it's obvious which need fixing when you actually read the test. Someone should just create an MR and we start from there.
Thanks!
Comment #20
xjmAside: This issue is what happens when we descope the docs gate from something!
Comment #21
xjmOne of the issues with the comments is grammar. They're not grammatically correct English, which exacerbates the problem.
I think it is relevant to document which langcode behavior we expect to use as that is the biggest gotcha of the whole API (and also exactly what is under test). See also: https://www.drupal.org/sa-core-2018-001
That said, the fact that this comment is repeated N times also hints at possible refactorings. (Out of scope currently, but a refactor might make the test easier to follow and reduce the need for boilerplate docs since helper methods or data providers would be more self-documenting.) If someone wanted, they could propose an alternative to this issue that refactors the test instead, and this might end up as a duplicate.