Problem/Motivation
The node access system is great for having granular node access controls with different realms and grants, however it is not yet compatible with the entity translation system (that was introduced in Drupal 7). The regular node translation system uses separate node copies which is naturally compatible with node access (due to the keys being the node IDs), however with the entity translation system, a node can have many language variants and it is not possible to have separate access values for them.
Trying to work around the problem by introducing language specific grants and realms was discussed and discarded, because the grant/realm system is additive, so if a grant/realm grants access to a node, other grants/realms are not considered. So if access is provided to a node via any language, the rest of the languages would not be inaccessible either. That is not a good fit for the granularity needed.
The legacy translation module, which is hidden, cannot be removed until this issue is fixed.
#1952044: Migration path for legacy content translation module data and #1952062: Remove legacy translation module in favor of content translation postponed on this issue.
#1860522: [META] support (basis for a contrib) Translation Editorial Workflow (support for revisions for translations of entities) lists this issue as a use case.
Proposed solution
We propose the stored node access system to introduce a langcode column to serve as part of a compound key for node access, allowing more granular access control. Records for all language variants of a node should be saved, and the original language version of the entity should be marked as a fallback item, like so:
The langcode column is used to be able to pick out specific grants and realms only for specific language versions. The fallback value is used when no language condition is provided/available, so as to serve as the master access value in that case. The specific access and node display fallback logic takes care of displaying the right language values later. There is not much better we can do if there is no specific language condition set.
API changes
The proposed patch changes hook_node_grants() so the return value can now contain langcode specific grants, like so:
function example_node_access_records(Drupal\node\Node $node) {
// ...
$grants[] = array(
'realm' => 'example',
'gid' => 1,
'grant_view' => 1,
'grant_update' => 0,
'grant_delete' => 0,
'langcode' => 'it',
);
// ..
return $grants;
}
If a langcode is not provided than the node's original submission language ($node->langcode) is assumed. If the langcode equals the node's original submission language, a fallback = 1 value is automatically added, otherwise the fallback flag is 0.
This is a followup to #1658814: Make node_access() language-aware.
Issues blocked by this one
- #1952044: Migration path for legacy content translation module data
- #1952062: Remove legacy translation module in favor of content translation
Follow-up Issues
- #1953892: Move language node access logic into NodeAccessController
- #1953898: Refactor Core Node Access Tests to not only test for denies, also test for public grants
- #1953900: Add a test for a hook implementation that defines a langcode but language module is not enabled.
- #1953904: Clarify comments mentioning fallback and default saving behavior for langcodes
- #1953916: Make tense of verb in inline comment be Test .. instead of Tests ... (node access, maybe more places in core)
- (need to open?) #210 check of adding langcodes, using custom (xx) instead, to speedup testbot
Related Issues
- #1738368: Not possible to use the entity getter to retrieve non-translatable field values (See #17)
- #1739994: Use the Language class universally instead of stdObj instances (See #30)
- #1658814: Make node_access() language-aware (This issue is a follow-up of that one.)
- #1814402: Convert 'node_access_test_private' variable to state() system (See #82)
- #1860522: [META] support (basis for a contrib) Translation Editorial Workflow (support for revisions for translations of entities) (This issue is a use case there. See #112)
- #1862750: Implement entity access API for nodes (See #197)
Comment | File | Size | Author |
---|---|---|---|
#210 | language-grants-1658846-210.patch | 62.2 KB | YesCT |
#210 | interdiff-207-210.txt | 24.61 KB | YesCT |
#207 | language-grants-1658846-205-207.interdiff.txt | 7.16 KB | penyaskito |
#207 | language-grants-1658846-207.patch | 62.03 KB | penyaskito |
#205 | language-grants-1658846-205.patch | 61.35 KB | effulgentsia |
Comments
Comment #1
Gábor HojtsyUnpostpone, since that was committed.
Comment #2
kalman.hosszu CreditAttribution: kalman.hosszu commentedI can dedicate time for this issue
Comment #3
kalman.hosszu CreditAttribution: kalman.hosszu commentedI attached the first version of the patch. Pls review it!
Kalman
Comment #4
Gábor HojtsyLooks like a good start IMHO. Quick code review.
"during database storage" is likely better, but I'm sure others will have nicer ideas :)
Its not the people who are in Catalan but the node variant. So this would eventually deny access to the Catalan node variant. The comment as-is sounds like it refers to the Catalan people :)
Theoretically the node langcode should always be set, its part of the schema + loading/saving.
Also databse => database.
which *is* set automatically
Comment #5
kalman.hosszu CreditAttribution: kalman.hosszu commentedThanks Gabor, I attached a new one.
Comment #6
Gábor HojtsyThanks, looks like it it time to move on to updating the queries where the data is used, and then add some tests.
Comment #7
Gábor HojtsyAnybody up to help with that?
Comment #8
disasm CreditAttribution: disasm commentedComment #9
disasm CreditAttribution: disasm commentedattached is a non-functioning tests and patch of what I was working on. I basically copied assertNodeAccess function, and testNodeAccess, and started reworking the tests to work with node_access_grants function. I'm leaving for vacation this weekend for 2 weeks, so I won't be able to work on this anymore until I get back. Un-assigning in case someone else wants to pick up where I left off.
Comment #11
kalman.hosszu CreditAttribution: kalman.hosszu commentedI'm back from my holiday.
Comment #12
Gábor Hojtsy@kalman.hosszu: can you help review the patch proposed above and bring it forward then?
Comment #13
kalman.hosszu CreditAttribution: kalman.hosszu commentedI will able to continue the work on this after wednesday.
Comment #14
Schnitzel CreditAttribution: Schnitzel commentedworking on this
Comment #15
Schnitzel CreditAttribution: Schnitzel commentedhere a first version which was discussed with Gabor:
- There is a new column in the node_access called "fallback", this defines which node_access record should be used if no language metatag is defined in db_select or elsewhere.
- The record with the same language as the original node language ($node->langcode) will be the fallback.
- There is a new Metadata for db_select called "langcode", if this is set the node_access table will use the records with this language. If langcode is not set it uses the fallback records
Open Todos
- right now the fallback column is only used with db_select, have to check if the node_access table is queried elsewhere
- is "langcode" the right Metadata key? Or should we use "access_langcode"?
- It does not test yet what happens if there are two node_access records with the same Nid but different languages. How should we test this? Because as far as I know this is not yet possible via the UI and also not really via code.
Comment #16
Gábor HojtsyLooks like an unrelated fix?! Should be its own issue AFAIS.
I would document here, that "used as a fallback if a language condition is not provided". To explain what does this mean.
Merge these into one update function IMHO.
Also need upgrade path for existing records. Sounds like we could get away by asking for rebuilding the node access records in the update (unless it already happens, which it might in fact be the case).
I'd also add simple upgrade path tests, to see if the data is properly updated after update.
debug :)
Comment #17
Schnitzel CreditAttribution: Schnitzel commentednew patch with fixes from @gabors mentions, thanks for review!
the LANGUAGE_NOT_SPECIFIED issue is here: #1738368: Not possible to use the entity getter to retrieve non-translatable field values
Still needs upgrade path tests
Comment #18
Schnitzel CreditAttribution: Schnitzel commentedComment #19
Schnitzel CreditAttribution: Schnitzel commentedjust did my own review and found this:
I would add fallback = 1, but what are we using for language? because if we add something (like default language) and a query is made with Metadata langcode, it could return no results, where actually this record is made for return all nodes all the time (eg: no node_access enabled)
--> own conclusion: I changed the default of the fallback column to be 1, so then the default record is set as fallback automatically.
I also checked node_access and I changed it to this:
so actually I'm only adding the langcode check for specific nid checks, not for the global nid=0 check.
attached new patch with node_access() change and fallback default
Comment #21
Schnitzel CreditAttribution: Schnitzel commented#19: drupal-1658846-node_access_grants-19.patch queued for re-testing.
Comment #23
Schnitzel CreditAttribution: Schnitzel commentedbecause I updated node_access() this was obviously that it failed.
So the reason is, that for the requested languages there is no node_access record, so the user gets now an access denied.
which @gabor and I both agree that this is the expected behavior.
attached patch changes the test
Comment #24
agentrickardI really dislike the use of a separate "fallback" column. Is it not possible to store that information in the langcode column?
If it is not, do we have a better, more consistent column name to use? (e.g. "default" or "is_default")
Comment #25
agentrickardNote #2: The use of "langcode" is consistent with the rest of core. No need to change the name of that column (reference comment 15).
Comment #26
agentrickardLooked at this more in-depth today, and I still don't understand the "fallback" column. In my tests, all node access records validate as "fallback". This is due to the logic used for determining fallback, which is:
I fail to see any cases where this might be not be true, which means that every {node_access} row is marked as 'fallback.' Instead, I am experimenting with setting the original node as the fallback. That code looks like so:
Here's a revised patch with that approach, plus an update to HEAD, and a fix for some if/else code format errors.
However, I also fail to see any cases where a record with a duplicate nid but a unique langcode can be saved. Is there something I'm missing regarding the translation system that would allow for that (say, selecting Multiple languages for a node)?
I see the benefit to adding langcode regardless, since it allows language-based access to site alongside other access control rules. (I have a module for testing that I need to sandbox to help others with this.)
Comment #27
agentrickardSee https://drupal.org/sandbox/agentrickard/1747908 for a sandbox module that implements node access for testing.
Comment #28
Gábor Hojtsy@agentrickard: Thanks for the review! The problem is if there is a specific language provided by the node access query alter (by added metadata on the query), it can filter down to one item per node easily. That is "trivial". But then if no language is provided (which is a very-very common use case, like a view without language filtering), then you get multiple entries per node. Then you need *some* way to pick from those entries. That is the hard part of this issue really to crack. The idea of the fallback language was to 'cache' the "original submission language of the node" "flagged" there. So we'd check the permission for that version. The main issue is that the language selection happens later in the rendering phase, so this way even though you might get a French version of the node displayed later, if you don't have access to the original version (let's say German), then you'll not see the node. We need to figure this out somehow.
Any great ideas?
Comment #29
Schnitzel CreditAttribution: Schnitzel commented@agentrickard
yes you are perfectly right, right now I'm did not manage yet to write a test which would test against this, I mentioned this in http://drupal.org/node/1658846#comment-6365354 so I first have to check with fago how to create via the entity api another language for the node so that there are two records in node_access.
this will only work for content translation, but not with entity translation (and we try to get rid of content translation).
Comment #30
pp CreditAttribution: pp commentedBe carefully, if #1739994: Use the Language class universally instead of stdObj instances issue will be committed you must correct this lines use "new Language(array(..))" instead of "(object) array(..)"
Comment #31
pp CreditAttribution: pp commentedsorry...
Comment #32
tstoecklerWell, for completeness' sake, you might as well make the change anyway, since it is the more correct way. Not marking "needs work", though.
Comment #33
Schnitzel CreditAttribution: Schnitzel commentedyes, agree, we should do this, added the new Language object and also fixxed codestyle.
but did not add the changes from #26
Comment #34
agentrickard@Schnitzel Yes. Gabor explained that to me in Munich. Backing away from this patch for now...
Comment #35
Gábor Hojtsy@agentrickard: @plach is hard at work to bring the entity translation UI into core, in fact the sandbox at http://drupal.org/sandbox/plach/1719670 has some pretty well working setup now (where you can submit and translate nodes :). Hope we can clean this up and include in core soon. However there is no cross-relation between this issue and that one, so we should not stop here in any way. :)
Comment #36
agentrickardRight, but it is hard to test the desired behavior given that we don't want to use translation nodes.
Comment #37
Gábor HojtsyDo we want to admit defeat here or anybody has cleverer ideas to query this data? If we don't do this, the D8 permission system looks like will have language support at some places (eg. runtime checking), but NOT on the stored per node access level. That might or might not be a DX issue as well :/
Comment #38
Gábor HojtsyOk, admitting defeat then. I have little hope this will happen in Drupal 8 given the above lack of interest/concentrated focus. Sounds like language support in node access will be a half beast, only in runtime (that already landed), not in stored node access (that this patch was supposed to achieve) :/
Comment #39
plach@Gabor:
Do you think this can be achieved in contrib?
Comment #40
Schnitzel CreditAttribution: Schnitzel commented@Plach:
Yes this can be achieved in contrib, it will probably not as fast as in core (because additional joins from the node_access table), but it's possible yes.
Comment #41
plachWell, it's not critical then. And we might be able to classify this as clean-up/security improvement and work on it after feature freeze.
Comment #42
Gábor HojtsyUhm, as per my understanding of the node access system, it is not possible. Not sure what Schnitzel is seeing, would love to get an example :)
1. http://api.drupal.org/api/drupal/modules%21node%21node.api.php/function/... provides the records. You can have custom realm's and grant ids (gids). A realm + gid combination is used to give permission based on things like 'you are a member of this organic group' (member = realm, this organic group = gid). So you could *theoretically* bastardise this to be used for language + langcode (however gids can only take integers). Then however, the realm + gid used for a lookup is determined via http://api.drupal.org/api/drupal/modules%21node%21node.api.php/function/... which only gets a user and a node operation. It does not get metadata about the node being viewed/edited/etc. so you cannot tell which language would apply. Finally, even if you could tell, this realm + gid system would not be compatible with other systems. Eg. if an organic group or any other realm based access gives access to the node, it would not care about the language specific access anymore. The node access system grants access I believe if any grant+gid worked out. So
- realm + gid could be bastardized for language, but need to encode langcode as a number
- no way currently to tell which gid to pick, when is a language realm relevant
- even then it is not compatible with other node access systems
Schnitzel, do you see something that I don't or am I in a misunderstanding?
Comment #43
plachOk, and what about
?
Comment #44
Gábor HojtsyI consider this as a pretty big API change / new feature, I'm not sure it would fit under cleanup after December 1st. You can try, but I'd not say I see it likely it would succeed. Also given lack of interest so far anyway, I don't see what would magically change anyway.
Comment #45
plachI am interested, it's just that I have more critical issues to work on :)
Comment #46
Gábor HojtsyYeah, that is how what gets into Drupal 8 is turning out.
Comment #47
agentrickardI'm interested in making this happen, but my understanding is that it is blocked because we can't write proper tests because the new translation system -- field based rather than node based -- isn't in yet.
Comment #48
Gábor Hojtsy@agentrickard: no, entity language variance is a feature in **Drupal 7** already. We are merely catching up with other subsystems to the previous Drupal version. For example, we added support in core search a month and a half ago: http://drupal.org/node/1737832
The main question remains the data querying. The system currently (without patch) provides node access permissions per node id per operation. It cannot go to under the node level to support language variance (although path aliases, core search, etc. can do that). So the underlying idea is to make the node access system be able to relate operation permissions to a node id + language pair, right. This concept is already in Drupal 7.
However, then there are different type of nodes. There are untranslated nodes (only in one language), there are untranslatable nodes (always language neutral) and there are translated nodes. So a node can be in one or more languages and the one language can be language neutral as well. So the node access table would have a varied number of items for each node (according to the current patch suggested).
So then the problem of querying comes in. If you want a listing filtered to French nodes, we can filter the node access data to French only and only display items which are French. If you want French and language neutral or French and German and Italian (all of which are configurable in a view), then we need to filter for multiple items per node some of which might be contradictory. Eg. if you want either French or German, and a node would not give you access to the French variant but would to the German variant, we need to filter it down to the German variant or deny access?
Finally, then there is the language-free listings, where no language is provided. How should we decide which nodes to give access to? The actual language chosen for display is decide later in the render phase, so even if we decide that the node should get access, what happens if it turns out later it should not have access due to permission restrictions.
So as you might be able to see, this need a lot more information about the language of the query to be passed on to the node access system and possibly change the whole approach as to how node access is stored and retrieved. Its not really a "language people" question as it upsets the whole node access layer. Given that only a minimal number of "language people" were interested in this so far, without a long hard look (and likely extensive work) on this, I don't see it being feasible in Drupal 8.
Comment #49
salvisPlease help me understand: without node access, if you ask for French or German and a node has both, would you see the node twice?
With node access, if only German were accessible, the node access system would have to return a list of accessible languages (with only 'de' in this case) rather than just a Boolean, right?
What about NA priority? Should it apply per nid or per nid+lang_code? Either one could be completely wrong, depending on your use case...
The case where German is accessible but French is not (even though a French version of the node exists) — is that really relevant in practice? What is the justification for that? Editorial workflow? That's not node access.
published
already exists as a language-specific property.Is the German language protected, too? What keeps the user from simply switching to German? If he can do that, then node access is not the proper layer to implement this. Even if you could protect the German language, why would you want to do that?
You will really alienate users on a site if they find out that some content is available in one language but blocked in another (even though it exists!), meaning they have to constantly switch around languages or even maintain multiple accounts in order to get the whole picture.
I've re-read the summary of #1658814: Make node_access() language-aware, but I'm unconvinced that any site would really want to use language-specific node access, especially given its costs in overhead, complexity and maintenance. It's hard enough to keep track of which users have access to which nodes; who would want to multiply that burden and do this per language? and what for?
Sorry if this has been discussed before, but at the point where we are right now, it makes sense to take a step back and reconsider what we're trying to achieve.
Comment #50
Gábor HojtsyNot by default definitely. If you want to display a node, you give a language and it will attempted to be displayed in that language (and if you don't give a language, the page negotiated content language will be used). If you invoke multiple node displays for the same node with different languages, then yes, that would possibly display the node multiple times, but you asked for that :)
I don't know. Either the node access system would need to change or the trust in the return value. If you asked for a French node you would not get back the nid. If you asked for a German node and it was accessible, you'd get back that nid. If you did not ask for any specific language, its hard to tell which one the rendering layer would pick later, so we might need to carry around the node knowing we might not display it. That breaks assumptions in the node access system, eg. you would not be able to build a pager system on top of it with predictable page sizes.
If you mean "NA priority" = "no language provided in node access lookup" then yeah, that is the most challenging question.
As per my understanding (and the node access docs), once you use node level access, published is not taken into account and should be implemented as part of your node access. See http://api.drupal.org/api/drupal/modules%21node%21node.api.php/function/...
Note that the system makes no distinction between published and unpublished nodes. It is the module's responsibility to provide appropriate realms to limit access to unpublished content.
It is still part of the primary D8 goals to implement per-language published state for nodes, so if that happens but this does not then sites needing to use per node access will not be able to have different languages in different publication states. Yes, that is a workflow question, not a node access one, but as demonstrated, the node access system stumps on the workflow so it needs to be integrated in the node access system as well.
I don't want us to decide for the site builder up front, what is going to alienate their users. As said above, this would be needed for a compatible pure workflow to not stump on per language published states. It could also be useful for other things, but my main goal was to make workflow make sense on this level if needed. It is fine if you don't see value in implementing this, the goal was obviously to be able to ignore this if not needed.
As said, if we don't have this, we make multilingual sites needing to pick from per language publication states (which is a very core workflow requirement for sites being translated) and implementing node access. You argue this would be the best we should do. Looks like we might not have resources anyway to do anything else.
Comment #51
agentrickardGabor-
The problem I ran into was that the old model of "Content Translation", wherein a translation of a node is, in fact, a new node, is something we want to discard. However, last I checked, there was no UI and no test framework for running checks for entity-based translations. That was the status we discussed in Denver (see comments 26-29 above).
If that framework is in place, then we have a query / data storage issue that we should be able to solve. But if we can't test entity translation using core UIs, then this can't move forward.
Comment #52
Gábor HojtsyNo, we do not currently have a UI. We have extensive test coverage for creating multilingual entities, such as in the search tests. There are APIs to create these, and testing access through the API should in fact be enough / identical in terms of feature set, so I don't see a reason for this to wait on an actual UI to land. As for an actual entity translation UI, there is a patch at #1188388: Entity translation UI in core, but as said, we should not need it to do any progress here.
The node access system is about assigning a boolean to a combination of a number and an operation string and this issue proposed to make that combination include a language code as well (optionally).
Comment #53
agentrickardOK. This would unblock me. I'm traveling this week but hope to take a look this weekend. At the least, I can make some time at BadCamp to try to knock this out.
Comment #54
agentrickardStraight re-roll of #34 to get moving again.
Comment #55
agentrickardHave to rename the update hook.
Comment #57
agentrickardThe idea of 'fallback' needs serious consideration. Now that we have per-field translation, the access system doesn't quite know what to do with each field translation.
In my tests, the only workaround is as follows:
* Make the hook_node_access_records() return value sensitive to all active translations, using
$node->getTranslationLanguages()
* Check for "fallback" status in the cases where:
** The node type is not translatable.
** The node langcode is "multiple"
** The node langcode is "not applicable"
** The node langcode is "not specified"
Here's the fugly code for that check:
Comment #58
Gábor HojtsyComment #60
Schnitzel CreditAttribution: Schnitzel commentedas discussed with xjm and Gabor at badcamp: I will update my patch to provide a new test node access module which is language aware to not change the current existing test node access module as most of the contrib node access module will not be language aware.
Comment #61
xjmSo I think we need to add test coverage for various combinations of languages (nodes that have no language, or that have one or multiple translations with different default languages) with:
@Gábor Hojtsy and @Schnitzel and I talked about this awhile and we concluded that it's not possible to use grant realms to implement this functionality, but not for the reasons described in #42. While it would be possible to define grant realms per language and write grant records per language, it would only solve the problem of allowing a role granted the realm (e.g.)
translator_de
to edit any node that has a German translation, not the problem of allowing that role access to edit only the German translation of that node (and not the French).Comment #62
Schnitzel CreditAttribution: Schnitzel commentedImplemented everything as discussed with @xjm and @gabor:
- fixed wrong node_access Table Primary key and added it with langcode column
- added code that node_access() function does correctly load the langcode from the content negotiation
- added new node_access_test_language.module which is a new node_access module which is language aware, so it creates different grants for each language the node is translated to
- new Test NodeAccessLanguageAwareTest.php which tests node access with node_access_test_language.module
Comment #64
Schnitzel CreditAttribution: Schnitzel commented#62: interdiff-55-62.patch queued for re-testing.
Comment #65
Schnitzel CreditAttribution: Schnitzel commented#62: drupal-1658846-node_access_grants-62.patch queued for re-testing.
Comment #67
Schnitzel CreditAttribution: Schnitzel commentedchecked with @xjm and decided that we will also have to write some combined tests, so where a non language aware module and a language aware node_access module is installed, did this in NodeAccessLanguageAwareCombinationTest.
Also updated NodeAccessLanguageTest to test when actually a private node is created.
And also updated NodeAccessLanguageTest with tests when a node is created without a language defined (so as when no language module is installed).
Comment #69
Schnitzel CreditAttribution: Schnitzel commentedoh, node install has changed.
Comment #70
YesCT CreditAttribution: YesCT commentedadding tag for needs issue summary since this seems to be pretty involved at this point. :)
Comment #71
Gábor HojtsyComment #72
Gábor HojtsyCreated this figure for layout out the changes. Updating issue summary with explanation.
Comment #73
Gábor HojtsyComment #74
Gábor HojtsyComment #75
xjmRerolling to clean up some spinach. :)
Comment #76
xjm@GaborHojtsy and @Schnitzel took the time to go over this issue with me in detail during BADCamp, and I'm fairly confident now that this is a good solution given the current node access implementation. The updated summary explains it pretty clearly.
The attached reroll adds the following fixes:
This method is repeated in all three tests classes, so I've moved it into the base class.
The logic here was a bit convoluted, so I've rewritten it somewhat (see interdiff).
The test coverage is excellent. One thing we might want to do is simplify the new node access test module a bit so that it doesn't rely on a field and a magic
variable_set()
flag. (The new module is modeled on the existingnode_access_test.module
, which could use its own cleanup anyway. As I recall, I started to do that once and ended up spending four hours just documenting how it works. Maybe a followup.)I'll take another look at this patch tomorrow; been staring at it too long today. :)
Comment #78
xjmMissed one.
Comment #79
xjmCouple other cleanups I missed:
db_select()
.Comment #81
xjmHm, the same fails from #78 occur with #69 rebased. Either something went funky with the rebase, or an intermediate commit has broken this patch.
Comment #82
xjmThis is some combination of "Ask and ye shall receive" and "be careful what you wish for": #1814402: Convert 'node_access_test_private' variable to state() system
Comment #83
xjmComment #85
xjmComment #86
xjmSome search-and-replace cleanups (adding articles and periods, fixing capitalization, removing
t()
from assertion message texts, etc.).Comment #87
agentrickardComment #88
xjmI'm confused by this comment. In this patch, the API takes care of it. Modules don't need to unless they want to. The fact that the new module here does is just part of its dummy implementation; the existing node access test module is unchanged, demonstrating that non-language-aware node access modules can just ignore it.
Edit: Maybe we need to change the test module so we have better coverage for the fallback functionality (and avoid this confusion).
Comment #89
xjmYes probably. I thought it did but I guess not.
Comment #90
xjmRefactoring the tests a bit.
Comment #91
agentrickardI don't see that part of the API in the patch. I would expect to see a loop in node_access_acquire_grants() or _node_access_write_grants(), but I don't see one.
I think my confusion is that I expect all node access modules to be language aware. But I think what you're saying is that if the $grants don't specify a language, then we just assert a fallback and default langcode. This was one of the issues we discussed at BadCamp, and I think you have solved this problem.
Comment #93
xjmYep, the default value for the fallback column is 1, so
thingsnode access records are a fallback unless they aren't.Comment #94
xjmI simplified the test module significantly and refactored the tests for clarity. Not bothering with an interdiff because pretty much the whole tests have changed, though the same assertions are present.
A couple of the assertions demonstrate behavior I'm not sure about. I'll follow up more later.
Edit: #89 also still needs to be addressed.
Comment #95
xjmSo this is the part that seems suspect to me. In this case, the comment actually contradicts the assertions below it. (It's the same in the earlier versions of the patch.) The normal behavior for node access would be additive, such that when the non-language-aware node access module grants access without specifying a langcode, access is granted. Here, the fact that the langcode is not specified is resulting in language-aware node access modules always overriding non-language-aware ones, regardless of circumstance. (The node access query tag shows the same behavior.) This seems like a bug to me.
Comment #96
Gábor Hojtsy@xjm: That seems to be due to the non-language aware node access saving only one fallback entry while the language aware node access saving entries for each language, so it would obviously trump the fallback-only entry. Do you think it would make sense for the non-language-aware entry to be saved for all languages applicable to the node then?
Comment #97
xjmYeah, I think that's what @agentrickard was getting at in #91 and at BADCamp. If a particular record in
hook_node_access_records()
does not specify a language code, I think we can assume it applies to all translations. So we'd need to either save a record for each language when no specific language is specified, or alter the logic in bothnode_access()
andnode_query_node_access_alter()
somehow such that both records get selected.I'd also like to add some additional coverage to see what happens when a language-neutral node is saved with one or both of the access control modules active.
Comment #98
Gábor HojtsyI think writing all records in that case would be better, so that we clearly have all records on the same level as intended instead of some records being "more equal" than others.
Comment #99
agentrickardThat sounds like the loop I was talking about, and the query parameters I think I added in my last patch.
Comment #100
Gábor HojtsyAll, right, looks like that is the direction then, in short: save records in all language codes if none was provided (instead of the current behaviour of saving only one in the original language code). @Schnitzel can you help implement that so we can do a quick round of reviews and then get this in? :)
Comment #101
Schnitzel CreditAttribution: Schnitzel commentedyes, will do!
Comment #102
Schnitzel CreditAttribution: Schnitzel commentedSo I tried to change it, but actually I'm more confused then before ^^
I agree that this:
looks confusing, especially because of this:
Short info:
$this->nodes['public_hu_private']
has this definitions:- non-language aware node_access module: public
- language aware node_access module: hu private, ca: public
- Default language: hu <-- important!
$this->nodes['public_ca_private']
has this definitions:- non-language aware node_access module: public
- language aware node_access module: hu public, ca: private
- Default language: hu <-- important!
So in the first case:
$this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_hu_private'], $this->web_user);
is denied for no language code, because the default language (which will be used as fallback) is denied.
in the second case:
$this->assertNodeAccess($expected_node_access, $this->nodes['public_ca_private'], $this->web_user);
is granted for no language code, because the default language is granted and this is used for as fallback.
So one thing we figured out during the D8MI Meeting in IRC today: the non-language aware node access module creates grantes only when the node is specifically private. If it is set to public then it returns no grants and depends on the Core behavior of creating an "all 1 1 1" grant. Whereas the language aware node access module creates grants for the node no mater if the node itself actually has one translation private.
So I could imagine three things:
--> this would be a change from the current behavior so not the right thing to do
-->this would then fit with the non-language aware node access module but would not change something from the cases on top
-->this could maybe be a good case and actually also my opinion, BUT: It would not change any of the cases below, the first case:
$this->assertNodeAccess($expected_node_access_no_access, $this->nodes['public_hu_private'], $this->web_user);
would still be denied access because there is an grant for this.so you see I don't really see a case where changing this:
would actually make a difference? :/
Comment #103
Gábor HojtsyI don't think it is safe to assume in a node access module that if there are no grants returned, then the node will be accessible. What is safe to assume is that the module is hands-off in terms of that node and other modules decide. The tests should work with that approach as well, if not already.
A node access module should return grants based on its thinking of access. If it wants to grant or revoke access specifically, it should return so. It should not assume any other environmental setup :)
Comment #104
Schnitzel CreditAttribution: Schnitzel commentedokay, so for me this sounds like, these two things are fine, but should we now still change the core behavior to: "save records in all language codes if none was provided" ?
Comment #105
salvisDoubling the number of records if any language (besides English) is installed is not so great, especially because it's unclear what percentage of sites really need language-specific node access.
Say you want to run a site in German — you're automatically punished, even though you have no English content and you couldn't care less about language-specific node access.
Can we find a better solution?
Comment #106
Gábor Hojtsy@salvis: the proposal is to add records in all languages that the node is available in. So if you are on a German only site, that will not have English records. Also, in Drupal 8, you can remove English, so you'd only have German as a configured language, so the situation is even better :)
Comment #107
salvisIn the absence of complete and sensible UI translations, removing English is probably not a viable option, but it's a nice idea.
What about LANGUAGE_ALL, LANGUAGE_DEFAULT, LANGUAGE_NOT_APPLICABLE, LANGUAGE_NOT_SPECIFIED, LANGUAGE_SITE_DEFAULT?
Comment #108
Gábor Hojtsy@salvis: you can remove English even though the missing translations would show up as English. I'd avoid further discussing this unrelated issue here. Let's focus on getting this change in!
Comment #109
salvis@Gábor Hojtsy:
Yes, definitely, I didn't intend to do that.
Have you considered the special language codes? Even if we have nothing but German, we probably need a way to answer queries with LANGUAGE_ALL, for example. And if we have content with LANGUAGE_NOT_APPLICABLE, we need to answer to queries with 'de'.
Comment #110
Gábor Hojtsy@salvis: I believe that is already possible.
Comment #111
Gábor HojtsyBack on topic, the work that seems to be left is that if no langcode is provided with a grant, core would save the grant for all langcodes applicable to the node not just the original langcode for the node. @Schnitzel: can you still help with doing that?
Comment #112
YesCT CreditAttribution: YesCT commentedthis issue is listed as part of an example of a concrete use case meta issue #1860522: [META] support (basis for a contrib) Translation Editorial Workflow (support for revisions for translations of entities)
Comment #113
Gábor HojtsyNeeds someone to implement the final pieces, we are very close. Need a volunteer!
Comment #114
plachI can work on this in a few days if none wants to pick this up before.
Comment #115
Gábor Hojtsy@plach/@Schnitzel: any possibility that you might be available or I should go look for others. It feels pretty awkward we got this close to implementing this and then stop before the (hopeful) finish line :/
Comment #116
plachSorry, I don't think I'll be able to work on this earlier than a week. If someone else wishes to step-up feel free.
Comment #117
penyaskitoRerolled patch from #94 "as is".
Comment #118
penyaskitoAdd a grant for each language this node can be translated to if a specific language is not given in grants.
Comment #120
penyaskitoQuoting #111:
If I understood, we want to add every langcode this node can be translated to.
My previous patch added a reference to translation_entity_enabled in _node_access_write_grants and this is not a valid call, because translation_entity could be disabled.
So maybe this functionality should be moved to translation_entity by implementing hook_node_access_records_alter and adding a new grant for each available language. Is this true of I'm missing the point?
Added a patch for showing my view, but I'm sure that more simpletests should be implemented if this is the right path to follow.
Interdiff is with #117, not #118.
Comment #122
Schnitzel CreditAttribution: Schnitzel commentedsorry for my way too long non answering :/
But unfortunately everybody needs something from me right now, as I only have time to write at 0100...
But yes @penyaskito the idea is, that core adds a grant for each language of a node where any other module does not create a grant.
A really good point is what you mentioned in #120, but actually getTranslationLanguages() is a function of Entity and not of entity_translation. So why not just use getTranslationLanguages() and never check translation_entity_enabled()?
Because Multilanguage is something which Entity is capable of, Entity Translation only enables you better handling of this Multilanguage system.
Comment #123
penyaskitoThanks for the feedback @Schnitzel!
Attached patch should work then, let's see if its back to green.
Comment #125
penyaskitoOops, failures based on using the language object instead of langcode.
Comment #127
penyaskitoWe should check that the entry for that node-grant-language does not already exist, or we get duplicate key errors as in those tests.
I'll work on it ASAP if no-one tries it before.
Comment #128
penyaskitoAdded check and reworded comment.
Comment #130
Schnitzel CreditAttribution: Schnitzel commentedmhh wait, why can it even happen that there are two grants for the same nodeID and the same translation in there?
All Grants for a Node should be deleted before the new Grants are created, so this should no happen. I think there is something else wrong with the array merge or so.
Comment #131
Gábor HojtsyRefiling as a task given not solving this would mean a regression in terms of content access features (due to entity translation taking place of node-copy translation).
Also agreed with Schnitzel on the duplication question.
Comment #132
Gábor Hojtsy@penyaskito: any chance you could take a look at this soon? It would be pretty important to get this in sooner than later.
Comment #133
penyaskitoNew patch for moving forward.
I were looping over the languages without looping in the grants first. Locally I have some test errors, but they look unrelated. Let's what the testbot has to say.
Comment #134
penyaskitoRemoved forgotten debug statement.
Comment #135
Gábor HojtsyReviewed, I don't see anything wrong with this patch, asked @xjm and @Schnitzel to take a look.
Comment #136
xjmThanks @penyaskito. That looks like the solution we were discussing.
The comment here still contradicts the assertions in the tests. I expected some of these assertions to change as part of the "write grants for all languages" change. It might just be a result of the existing test node access module having unintuitive behavior, as @Schnitzel points out in #102. (I'd almost consider that a bug in that module.)
@Gábor Hojtsy is definitely right that the language access should not make any assumptions about what other modules do, but the result is that we're missing test coverage for the case where there are actually explicit "public" grant records written from another module, because the existing test module doesn't do that. The solution to this problem is to either add another test module, or to change the behavior of the existing module. That's a lot of overhead for this already-gnarly issue, so I'm inclined change the comment so it is at least accurate for the current behavior, smack on a
@todo
, add more coverage in a followup, and open a new bug if it turns out there's a bug for that case.We also still need this test coverage, I think. This scenario seems more likely to me than the complicated situation above, so I'm less comfortable with deferring that to a followup, but please let me know if you feel differently.
Finally, I should have brought this up previously, but there's a lot of obscure, weird query logic here and no inline comments explaining it. (It's a pre-existing problem, but it becomes worse now that the query and query alter are more complicated.) Can we add some inline comments explaining what's going on in these two functions?
Thanks everyone for your perseverance on this issue. Node access is complicated...
Comment #137
penyaskitoI'm not a node grants expert, so I don't fully understand yet what is needed.
Pending (from my understanding):
1. Change assertions from NodeAccessLanguageAwareCombinationTest or improve comments.
2. Add more coverage for the case where there are actually explicit "public" grant records written from another module or write a follow-up issue.
3. Add coverage to see what happens when a language-neutral node is saved with one or both of the access control modules active.
4. Document obscure, weird query logic in node access checks.
This patch only covers 4) since I need more time for understanding the other points (I'll ping @GaborHojtsy or @xjm in IRC this week if needed).
Comment #139
vijaycs85Re-rolling...
Comment #140
Gábor Hojtsy@penyaskito: I think (1) would need a documentation update indeed, the code comments do seem to contradict the test as pointed out by @xjm. For (2) I think adding yet another test module to test combinations with public grants would be good in a followup and agree (3) would need test coverage (not sure that would need to be here too, but I agree it is closer to introducing the multilingual functionality) . The added comments for (4) look good but none of them wrap at 80 chars. Please wrap them.
Comment #141
Gábor Hojtsy@penyaskito: can you work on fixing these points?
Comment #142
Gábor Hojtsy#139: 1658846-language-node-139.patch queued for re-testing.
Comment #144
vijaycs85Re-rolling with current code base and comment wrapping for (4).
Comment #145
penyaskitoAddressing 1).
Comment #146
Schnitzel CreditAttribution: Schnitzel commentedw0000t Schnitzel is back!
I'm super sorry for not taking care about this :/
unfortunately I was super busy during Nov-Jan to finish customer projects and letting my employees into holidays, but I finally found time to check my node access issue again!
So I read through all the things still open:
I think @penyaskito did a good Job with this in #145, so nothing to do here.
Fully agree, but as this is not yet covered currently I think we can make a follow up Issue: "Refactor Core Node Access Tests to not only test for denies, also test for public grants"
Done! I added more nodes to both Tests: "Language Aware" and "Lanuage Aware Combination", as there where already tests for language-neutral (eq: no specific language").
Checked @penyaskito work here, rewrote it a bit and also fixed the 80chars length issue.
So from my point of view this is ready to go, and we need a follow up.
What do you think @xjm @gabor?
Comment #147
Gábor HojtsyChanges look good to me, fix @xjm's concerns :)
Comment #148
penyaskitoLGTM if a second pair of eyes are needed :-)
Comment #149
catchWhere's user 1 set up? I don't see any difference here from the assertions above in terms of which user is current during the queries - also isn't it possible to use addMetaData() to specify the account, that'd be more explicit.
s/occr/occur.
Is that really necessary to rebuild node access, wouldn't it happen on module_enable() anyway?
This too - what default permissions are messing up the test?
This changes the meaning of the comment quite a bit, I'd add a new hunk I think?
Have any explains been run on this?
Also why change the query regardless of whether multiple languages are enabled?
At first glance this looks like a lot of overhead for sites that may never use this feature, it ought to be possible to only add that if it's likely to be used - i.e. if there's more than one language on the site. Alternatively ET module could alter the query?
Comment #150
Gábor Hojtsy@catch: only responding to the query building question since that seems to be a major item. I think how that should work depends on what indexes are available. The patch changes this only on indexes:
There are likely easy to find people who know better about database performance, but I believe if we query for parts of the index that will only work performantly if the query and the index are set up properly (ie. we use the first part of the index?). So if there is no proper index for the language-less case then querying for that might not end up being better. However if the site only has one language then I'd assume the database optimises out the langcode since there is no variance.
Comment #151
catchRealm is always added to the query - at least if the user has any grants, so possibly language could be put after in key order? Or add another index?
Comment #152
Schnitzel CreditAttribution: Schnitzel commentedThanks @catch for the review!
Here a new Version of the Patch, Changelog:
node_access_rebuild();
is necessary, as this does not happen during module_enabledb_delete('role_permission')
is really not necessary. I copied it from the already existing core node access test. Removed it now in all places. Lets see if testbot likes this :)('nid', 'gid', 'langcode', 'realm')
to('nid', 'gid', 'realm', 'langcode')
Here some EXPLAIN on the db_select(), looks good to me? If this needs more performance testing, I'm happy to explain a person which knows more about performance what the whole thing does.
Current Drupal 8 Core:
With Patch:
Comment #153
Schnitzel CreditAttribution: Schnitzel commentedComment #154
Gábor Hojtsy@Schnitzel, thanks! What about the last concern raised by @catch? That is where the order of indexes is significant.
Comment #155
Schnitzel CreditAttribution: Schnitzel commented@gabor
I changed the order of the keys, but honestely I don't know if this really fixes what catch ment. Any advises here?
Comment #156
Gábor HojtsyWell, the question/suggestion was to not add any langauge condition if there is definitely no language in node access. What do you think?
Comment #157
Gábor Hojtsy@catch/@Schnitzel: I think tying the node permissions hook extension to entity translation being enabled might be a mistake. While entity translation provides a user interface to configure and enter translations, it is possible to disable the module and still view the entity translations (entity display controllers are independent of data entry). So I can imagine people on live sites might disable the module (or even use a separate module to enter translation if they wish so), which would otherwise be perfectly valid in the system as-is now. I've asked @YesCT to try and reproduce the independence of display vs. entry.
Comment #158
YesCT CreditAttribution: YesCT commentedre the possibility of having ET UI modify the query, mentioned by @catch in #149:
Gabor suggested testing to see if content displays in the translated languages without et enabled.
Tested (with and without the patch, behavior the same) by:
so, putting the query in ET would not be advised.
[edit, added below]
maybe not common, but that situation might happen if content is translated not on the live site.
Possible for deployment, where you want to optimise performance on live and not edit content directly there.
Comment #159
Gábor HojtsySounds like the above resolves @catch's concerns then and demonstrates that entity translation module is not the right place to put a query alter for this.
Comment #160
catchI don't think the query alter running when Entity Translation module is disabled is a good reason to bake this into node module, that's an extreme edge case vs. additional complexity for sites that will never use this at all.
Comment #161
YesCT CreditAttribution: YesCT commentedI'm not clear on the concern.
What about this situation:
No ET ever enabled, just languages.
Some content in some languages, some content in other languages. Would this be useful in that case? [edit: or maybe that case is covered without this patch]
Perhaps the issue summary needs some more explanation.
Comment #162
Gábor Hojtsy@catch: so in short, the display logic and fallback is within the entity system, entry and configuration is with the entity translation module. If the node permissions query altering (display/entry access logic) happens in the module, then if I create nodes with varying node access values for translations, then disable the module, I also possibly leak access to those translations, since the translations will be there but the code will not be there anymore to limit access. You are proposing we make it work like this and ignore the unauthorized access possibility that ensues?
Comment #163
catchI'd be OK with language module holding the query alter.
Comment #164
Gábor HojtsyAll right, let's alter that in from language module then (I assume if there are multiple user entered languages, because otherwise this is really an optimization only for English sites, not for single language foreign language sites).
Comment #165
xjmI think this hunk was mostly there to ensure there weren't any false positives down the line.
Comment #166
Schnitzel CreditAttribution: Schnitzel commentedspoke with @xjm and @catch about the performance issue in node_query_node_access_alter(), moved now the langcode check into language module, as it really makes only sense here.
Comment #167
Schnitzel CreditAttribution: Schnitzel commentedoh, btw not 100% if the naming "node_access_tag_subquery" for the alter is correct.
I thought about node_access_subquery first, but actually in node_access() there is no subquery, and the alter is called in node_query_node_access_alter() which is the node_access TAG function. If anybody has a better idea, I'm open to change it ;)
Comment #168
Schnitzel CreditAttribution: Schnitzel commentednew alter, so needs documentation in case this is how we want to do it.
Comment #169
xjmFirst of all, I checked with @webchick and she agrees that this is appropriate as a task (meaning, not subject to the Feb. 18 deadline).
Moving this alter into language.module makes more sense to me, and actually helps address a concern I've had for awhile about the node module including all this functionality itself. However, now we have support for this functionality split between Node and Language. I'm wondering if it would make more sense to move more of the functionality to the Language module?
I think we can drop the word "tag" from the tag name; it's a bit redundant.
node_access_subquery
probably works, but there might be something that better describes its purpose.This comment needs to be updated now that there are six nodes.
This would probably be the place to add documentation about the subquery alter. Also, do we need to specify here that the functionality is dependent on the Language module?
{node_access}
should be in curlies here.We probably need a comment for this line.
What happens when the Language module is not enabled but someone tries to set a langcode in their hook implementation? Hopefully node access gets rebuilt when language is disabled in this case, and any modules that have langcode-aware node access would specify Language as a dependency? (That also adds credence to this functionality belonging to Language instead of Node.)
There are also a number of comments that could use some clarification. I can help with that once we decide what else (if anything) we want to move over to Language. If we do update the patch, let's also be sure to post the test-only patch alongside the full one to be sure that our tests are still providing the expected coverage. (I'm also still not 100% sold on removing the permissions cleanup.)
Leaving at NR for feedback.
Comment #170
Gábor Hojtsy@xjm:
It would be great to know what do you mean in more concrete terms. In Drupal 8, the direction of moving code was much more from language (locale) module to the individual modules, so the individual modules are language aware and its not language module implementing things on behalf of other modules, to set up a pattern, which contrib modules would need to follow anyway. For example, in Drupal 7, the node language property on nodes, as well as the language filtering on the admin interface was in the node module while the language property UI was in locale module altered in for the node form. That was pretty odd, because the other parts could not be just altered in, so that part of the logic was here and the other was there made it pretty confusing.
I think we now have the same "confusing" setup with altering from language module but otherwise core functionality with language support in node module. It is clear the schema and the needed understanding of per-language access records would need to live in node module because that handles the data storage. So I'm not sure we are cleaning up code and making things easier to work with / understand by separating these logic pieces.
The general philosophy otherwise in Drupal 8 is that language is not a feature that is injected from the outside but natively understood in modules. Eg. search module indexes nodes with multiple languages natively, field API handles multilingual properties natively.
Comment #171
plachI completely agree with Gabor: Language as well as ET are providing their features in an entity-type agnostic way. Hence it would be architecturally wrong to force them to add code explictly dealing with a particular entity.
If we want to reduce the overhead for node access on monolingual sites, which is a sensible and desirable goal, we should just do so and have Node natively make its node acess support language-aware when multiple language are available.
Comment #172
catchThat's also fine with me..
Comment #173
Gábor HojtsyAll right, here is an update. It moves back the language condition to the node module as discussed above. Also made all changes suggested by @xjm (apart from moving more things out of node module that is).
1. Moved back the logic and made it conditional on language_multilingual() in the query.
2. No need to document the query alter or name it differently anymore.
3. Updated the comment for the 6 nodes instead of the 4.
4. Updated the comment on langcode in hook_node_access_records() that it should only be provided if multilingual content is possible.
5. Fixed missing {node_access} curlies.
6. Made node access reindex itself also if language module is disabled.
Please review!
Comment #175
Gábor HojtsyBad invisible whitespace...
Comment #177
Gábor HojtsyFailed with
Drupal\Core\Config\StorageException' with message 'Failed to write configuration file...
and friends. Should be a fluke.Comment #178
Gábor Hojtsy#175: language-grants-1658846-176.patch queued for re-testing.
Comment #179
attiks CreditAttribution: attiks commentedPatch looks good, and lots of tests ++
Some minor comment issues
':' at the end?
dot missing at the end
for = four? but assert uses 3
.. as the default.
Module -> module
access is granted
access is not granted
Module -> module
.. is not granted ...
Module -> module
why " and not '?
Comment #180
penyaskitoComment #181
penyaskitoHandled @attiks comments.
Comment #182
Gábor Hojtsy@attiks, @xjm, @catch, others? Any more concerns?
Comment #183
Gábor HojtsyBack to RTBC with all concerns resolved from above.
Comment #184
catchDoesn't this also need to be wrapped in a language_multilingual() check? If not I think it could use a comment - I'm not sure from reading the code what happens if langcode is NULL.
Comment #185
Gábor Hojtsy@catch: the code above in the patch initializes the $langcode to the node's language and then further clarifies that to be the translation $langcode of the node for the page's content language if present. So it makes all attempt to get to the proper langcode. I'm not sure we should summarize part of the code of the same function from above. We usually don't do this.
Comment #186
webchickMoshe wanted to do a review of this prior to commit (and xjm too) so taking out of the RTBC queue for now.
Comment #187
xjmSorry that I let this slide for a month again. (In my defense, there was a vacation and a DrupalCon and a new job and a cold in there.) ;) I reviewed the patch again, and all the recent changes seem correct to me. I withdraw my earlier concerns, and I'm confident this is ready. I'm really satisfied with the robust test coverage and the overall architecture, but it does look like a couple of my earlier questions haven't been addressed.
Let's file this and link it here.
"Allow modules to alter..." and the usecase? Also, I think we need to document this alter hook?
I know that catch asked for this, but why do we need to do it? Maybe putting the why in the comment would be good. Even after all this time I am still hazy on what's up with user 1 in simpletest.
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 novalues
, does it simply insert no records, or does it fail? Can someone test this?Then, some things I noticed reading through the patch (hopefully) one last time are documentation nitpicks that can be addressed in a novice followup issue. (These were the things I referred to as "comments need clarification" before I dropped off the map again up in #136. Please file that issue as well and link it here.
"Node" does not need to be capitalized.
Missing a period.
"of the node_access_test module"
These three comments are a little unclear (same for the similar comments in the other tests).
These can all be "Test" or "Verify" or something rather than "Tests". (Ditto for similar comments elsewhere.)
"Add the langcode and fallback fields to the {node_access} table," or maybe "Add language support to the {node_access} table"?
For this comment, I'm more interested in why we initialize it as an empty string, and what happens if it stays that way. (I don't need the comment to tell me what is clear on the next line.) :)
I don't quite understand this comment.
"If the node is published, also take the default grant into account. The default is saved with a node ID of 0."
This comment is not wrapped correctly.
langcode-based. Also I'd replace "if on" with "if this is a".
"as the fallback"
"If a specific langcode is given"
I think this comment is not wrapped properly.
language-specific
Comment #188
Gábor HojtsyIt is pointless to pour more time into this, if Dries does not commit to possibly having this functionality in core. I will not ask anybody to spend more time on this unless we have confirmation it makes sense. I can easily encourage people to work on other useful/important things. Assigning to Dries for review.
Comment #189
xjmThanks @Gábor Hojtsy, makes sense.
Comment #190
moshe weitzman CreditAttribution: moshe weitzman commentedI was mainly worried about scaling but this only creates additional records for those nodes which have translations. Back to RTBC
Comment #191
xjmYay! Do we want to move #187 to a followup then? Edit: One part of it (about the hook docs) is sort of docs gate, though, unless I'm mistaken.
Comment #192
Gábor Hojtsy#181: language-grants-1658846-181.patch queued for re-testing.
Comment #194
Gábor HojtsySent for a re-test. Also I agree at least the invocation of hook_node_access_tag_subquery_alter() should be removed, since there are no implementations. It was for the prior state when language module implemented some query altering, but after that we agreed that was a wrong approach.
Comment #195
penyaskitoRerolled, still needing work for the comments (set to NR for knowing if it applies OK)
Comment #197
xjmWe broke this more today with #1862750: Implement entity access API for nodes. :(
Comment #198
effulgentsia CreditAttribution: effulgentsia commentedStraight reroll for #197.
Comment #199
effulgentsia CreditAttribution: effulgentsia commentedThis logic probably should be moved into NodeAccessController, but didn't do so in #198 in order to keep that a straight reroll. I think that can be follow up material though, unless someone is inspired to do it here.
Comment #201
Gábor HojtsyFails with fatal errors like this among others:
Comment #202
Gábor HojtsyAlso note that if this is not solved, we'll need to throw away node access records in the process with #1952044: Migration path for legacy content translation module data (ie. we will not be able to provide an upgrade path for node access).
Comment #203
effulgentsia CreditAttribution: effulgentsia commentedThe failures in #198 are the same ones that also existed prior to #197. Here's a fix for all but the private file test. Will look at that one next.
Note that I only joined this issue after #197, so I don't know where this issue is in terms of #191, #194, or other feedback.
Comment #205
effulgentsia CreditAttribution: effulgentsia commentedI copied a hack that already exists in HEAD's ImageItem::setValue() to FileItem::setValue(). I didn't track down exactly why the private file test doesn't fail the same way in HEAD, but only surfaces with the code in this issue, but frankly, I don't think it matters. It's something we'll need to clean up in both ImageItem and FileItem, and not really this issue's concern.
Comment #206
Gábor HojtsyAs per @penyaskito this still needs to resolve comments from @xjm, at least this one:
This should be removed. Not used in the patch anymore. @xjm requested it be documented, but we have no use for it.
Comment #207
penyaskitoThanks Alex for taking care on this.
I removed the unnecessary new drupal_alter, and addressed some of the IMHO more important comments from @xjm (#1658846-187: Add language support to node access grants and records). We can afford them in followups if others agree.
#1658846-199: Add language support to node access grants and records still pending too, we can create a followup too.
Removed the @todo, because this epic is hopefully addressed now, yay!
Comment #208
YesCT CreditAttribution: YesCT commentedfollow-up for #199 #1953892: Move language node access logic into NodeAccessController
I'll work on figuring out what others are needed and opening them next.
Comment #209
YesCT CreditAttribution: YesCT commentedNoting what was done about each point in #187
1.
#1953898: Refactor Core Node Access Tests to not only test for denies, also test for public grants
2.
drupal_alter already removed. See: #206 and #207
3.
I dont understand what documentation should be added. Later for each check it explains that it's using user 1 since it has permissions to get everything.
Is there really something todo here? I dont think so.
4.
#1953900: Add a test for a hook implementation that defines a langcode but language module is not enabled.
Other things from xjm addressed in #207 (or coming right away in a patch) except:
unclear comments
#1953904: Clarify comments mentioning fallback and default saving behavior for langcodes
clean up tense verb for statements inside the tests to be "Test that" instead of "Tests that". This is probably in many places in core. Lets see how wide to make the cleanup in the follow-up
#1953916: Make tense of verb in inline comment be Test .. instead of Tests ... (node access, maybe more places in core)
Next steps:
patch coming for some things I saw need to be fixed.
Also, Need to:
update issue summary with a follow-ups section.
update motivation to include this is needed for removing legacy translation module.
add a related issues section. Specify which issues are blocked and waiting on this.
Comment #210
YesCT CreditAttribution: YesCT commentedAttached patch addresses these things, and a bunch of wrap to 80 chars for comments.
This could still use another read through of the whole thing.
grammar fix needed still.
since we started on this, 1354 has been updated to clarify that namespaces in comments should start with \.
http://drupal.org/node/325974
public function setUp()
adding a language causes the ui .po file to be imported automatically. In other issues, we are using xx or some langcode that does not have a translation file, in order to not slow down testing. Should we do that here? Maybe this is bypassing the importation, since it's creating the language object and not going through the add language ui.
This could be a followup to find places where using xx in core instead of adding a language could speed up testing (and speed up the testbot).
changing this to clarify that it's just convienient to use as an admin with all permissions who should be able to see everything.
this and other comment multiline comments should wrap at 80 chars.
occr should be occur
does this mean other things from catch's review in #149 are not done? I checked and other things look to be addressed, or were explained in comment on the issue.
this was a correction from #187 of:
"Add the langcode and fallback fields to the {node_access} table," or maybe "Add language support to the {node_access} table"?
But now it is missing the period.
Comment #210.0
YesCT CreditAttribution: YesCT commentedSignificantly updated summary.
Comment #211
YesCT CreditAttribution: YesCT commentedthe alter mentioned needing documentation in #168 and #187 was removed (#206). So removing need documentation tag.
Also, this has tests, and follow-ups are open for any additional ones needed. so removing needs tests tag too.
Comment #215
Gábor HojtsyThe changes in docs comments from @YesCT and @penyaskito look good! I'm sure we can find as much to improve as we like, and all recent comments evolved around comments needing more cleanup. YesCT opened several followups to track these further. Given the extent of reviews this has gotten as well as the prestigious and very hard working set of people on the issue, not to mention the repeated changes around this area of code in core, I think its about time to get the foot in core.
Just looking at what would dreditor generate for the issue, the list of key contributors in summary: penyaskito, Schnitzel, xjm, agentrickard, effulgentsia, kalman.hosszu, vijaycs85, Gábor Hojtsy, disasm, YesCT Impressive!
Comment #216
Dries CreditAttribution: Dries commentedThanks for re-rolling this patch. Committed it to 8.x. Thanks!
Comment #217
Gábor HojtsyAmazing, thanks, now for the change notice.
Comment #218
plachWhoo-hoo!
(hell of reroll ahead ;)
Comment #219
plachEr, too much enthusiasm, I guess
Comment #220
Schnitzel CreditAttribution: Schnitzel commentedwow, nice! thanks for all which worked on this :)
Comment #221
Gábor HojtsyAdded an extensive change record at http://drupal.org/node/1959668 with code examples, figures, etc. Please reopen if needs improvement. Also cross-linked with the original node access runtime language support change notice from http://drupal.org/node/1667616.
Comment #222
plachAwesome work, thanks!
Comment #223
agentrickard@Gábor Hojtsy
A code sample explaining this line would be useful as well: "Node access grant providers should use the same logic to return multilingual grants only on multilingual sites."
Comment #224
Gábor Hojtsy@agentrickard: added this whole new section, hopefully this helps :) Please verify.
Comment #225
agentrickardWhat I meant was that I need an example of hook_node_grants(). Currently, the grants array returned makes no account of language. Is that handled automatically by the system?
Comment #226.0
(not verified) CreditAttribution: commentedadded issues this is blocking, updated motivation to clarify cannot remove legacy module, added follow-up section, added related issues.
Comment #227
penyaskito