Contrib.social

Clarify comments mentioning fallback and default saving behavior for langcodes

Open on Drupal.org →
Created on 27 March 2013, over 12 years ago
Updated 23 January 2025, 6 months ago

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.

📌 Task
Status

Postponed: needs info

Version

11.0 🔥

Component

node system

Created by

🇺🇸United States yesct

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

  • D8MI

    (Drupal 8 Multilingual Initiative) is the tag used by the multilingual initiative to mark core issues (and some contributed module issues). For versions other than Drupal 8, use the i18n (Internationalization) tag on issues which involve or affect multilingual / multinational support. That is preferred over Translation.

  • stale-issue-cleanup

    To track issues in the developing policy for closing stale issues, [Policy, no patch] closing older issues

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment 6 months ago →
    🇺🇸United States smustgrave

    Thank 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.

  • Status changed to Closed: outdated 3 months ago
  • Comment 3 months ago →
    🇺🇸United States smustgrave

    Since there has been no follow up in 3 months going to close out. But don't worry this can always be re-opened!

    Thanks

  • Status changed to Active 21 days ago
  • Comment 21 days ago →
    🇺🇸United States xjm

    They're still there and haven't been clarified. :)

  • Comment 17 days ago →
    🇦🇺Australia acbramley

    We 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 10 days ago →
    🇺🇸United States xjm

    I 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). Thanks!

  • Comment 10 days ago →
    🇺🇸United States xjm

    Aside: This issue is what happens when we descope the docs gate from something!!

  • Comment 10 days ago →
    🇺🇸United States xjm

    One of the issues with the comments is grammar. They're not grammatically correct English.

    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).

    That said, the fact that this comment is repeated N times also hints at possible refactorings. (Out of scope, but 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.)

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024