The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Follow up for 4. in #1658846-187: Add language support to node access grants and records
Problem/Motivation
If some hook implementation defines a langcode, but language is off or the language is not enabled, it looks like this would blow up.
Proposed resolution
If some hook implementation defines a langcode, but language is off or the language is not enabled, it looks like this would blow up. Did I say that already and ask for a test for it? Edit: No, I think I said maybe it was on the contrib module to add language to its requirements. However, let's make sure we document that somehow, somewhere. If you execute a db_insert() with no values, does it simply insert no records, or does it fail? Can someone test this?
Remaining tasks
- Find out what happens when execute a db_insert() with no values. Does it simply insert no records, or does it fail?
- Add comments to the code to document what happens.
- Add the test for if a module has some hook implementation defining a langcode when language is not enabled. See contributor task document for writing tests: http://drupal.org/node/1468170
User interface changes
No UI changes.
API changes
No API changes.
Original report by @xjm
In 4. in #1658846-187: Add language support to node access grants and records
+++ b/core/modules/node/node.moduleundefined
@@ -3036,18 +3077,35 @@ function _node_access_write_grants(Node $node, $grants, $realm = NULL, $delete =
+ if (isset($grant['langcode'])) {
+ $grant_languages = array($grant['langcode'] => language_load($grant['langcode']));
+ }
+ else {
+ $grant_languages = $node->getTranslationLanguages(TRUE);
+ }
+ foreach ($grant_languages as $grant_langcode => $grant_language) {
+ // Only write grants; denies are implicit.
+ if ($grant['grant_view'] || $grant['grant_update'] || $grant['grant_delete']) {
+ $grant['nid'] = $node->nid;
+ $grant['langcode'] = $grant_langcode;
+ // The record with the original langcode is used as the fallback.
+ if ($grant['langcode'] == $node->langcode) {
+ $grant['fallback'] = 1;
+ }
+ else {
+ $grant['fallback'] = 0;
+ }
+ $query->values($grant);
+ }If some hook implementation defines a langcode, but language is off or the language is not enabled, it looks like this would blow up. Did I say that already and ask for a test for it? Edit: No, I think I said maybe it was on the contrib module to add language to its requirements. However, let's make sure we document that somehow, somewhere. If you execute a db_insert() with no values, does it simply insert no records, or does it fail? Can someone test this?
Comments
Comment #14
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.
Thanks!
Comment #15
acbramley commentedI've looked at the code (now lives in NodeGrantDatabaseStorage::write) and I can't see how it would "blow up". Given the age of this issue and lack of activity I'm going to close it for now.
Comment #16
xjmThe code is still essentially the same, so I should probably try to determine what I was talking about in my review of #1658846-187: Add language support to node access grants and records twelve years ago. The problem that I believe I was talking about was the following codepath:
...in the scenario where the Language module or the language in question was not currently available on the site.
language_load()not defined ➡️ fatal. This potential problem may no longer be relevant with post-release Drupal 8 now that the language manager might always be available? At a minimum, it's no longer going to fatal.Language not currently available on site ➡️ an array
[$langcode => NULL].Then the foreach would set it to a null langcode, instead of what it started as.Edit: Never mind, mis-read the part of the code; it doesn't care about the actual language object. In fact, it looks like I might also have mis-read it in the same way in 2013 at BADCamp or Sydney or wherever I was when I first reviewed that part of the patch. Really it should just do aforeach(array_keys()), honestly.Anyway, that's the part I was asking for test coverage for, because this could potentially cause strange things for site owner expectations if translation is disabled and then re-enabled, for example, if the contrib or custom node access implementation provides or alters in langcodes. Edit: including resiliency and not-data-loss-or-access-bypasses if it wasn't properly declaring dependencies on the language. (See for example SA-CORE-2018-001 for why we have to be careful about stuff like that.)
Comment #17
xjmWas going to assign it to myself to explain later but then ended up explaining it now.