Allow users to translate content they can edit

I think we should have a test for this.

This solution makes sense to me, as it brings back the ability to translate own content without complicating too much the translation permissions.

1. +++ b/core/modules/content_translation/content_translation.permissions.yml
   @@ -8,6 +8,8 @@ delete content translations:
   +translate editable content:
   

   What about translate editable entities for consistency with the global permission?

2. +++ b/core/modules/content_translation/content_translation.permissions.yml
   @@ -8,6 +8,8 @@ delete content translations:
   +  title: 'Translate any content user can edit'
   

   This permission is slightly different from the other "Translate ..." ones because it also includes create/edit/delete permission over translations (implied by the edit access), whereas the other ones alone only grant access to the translation overview page. I'm wondering whether we can differentiate its name a bit more to make this clearer. Maybe something like "Manage translations for any entity editable by the user"? Or maybe we could add a description along those lines?

3. +++ b/core/modules/node/tests/src/Functional/NodeTranslationAccessUITest.php
   @@ -0,0 +1,98 @@
   +class NodeTranslationAccessUITest extends ContentTranslationTestBase {
   

   I think this test should be merged with the ones in ContentTranslationWorkflowsTest to make sure we are not introducing regressions in the two workflows CT supports. I manually tested this patch and it seems it's all working fine, but it would be good to have automated tests proving that.

Btw, while testing this I found #2974146: Content translation overview operation links may lead to an access denied page.

This is looking good, thanks!

1. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationTestBase.php
   @@ -104,8 +104,10 @@ protected function setUp() {
   -    foreach ($this->langcodes as $langcode) {
   -      ConfigurableLanguage::createFromLangcode($langcode)->save();
   +    foreach ($this->langcodes as $index => $langcode) {
   +      ConfigurableLanguage::createFromLangcode($langcode)
   +        ->setWeight($index)
   +        ->save();
   
   +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationWorkflowsTest.php
   @@ -200,7 +248,7 @@ protected function doTestWorkflows(UserInterface $user, $expected_status) {
   -        $this->clickLink('Edit', 2);
   +        $this->clickLink('Edit', 1);
   
   @@ -228,7 +276,7 @@ protected function doTestWorkflows(UserInterface $user, $expected_status) {
   -        $this->clickLink('Delete', 2);
   +        $this->clickLink('Delete', 1);
   

   Why are these changes needed?

2. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationWorkflowsTest.php
   @@ -24,6 +24,13 @@ class ContentTranslationWorkflowsTest extends ContentTranslationTestBase {
   +   * The user translator account to be used to test multilingual entity editing.
   +   *
   +   * @var \Drupal\user\UserInterface
   +   */
   +  protected $userTranslator;
   

   User translator is a confusing name: I read the comments multiple times to figure out the difference with the regular translator with no luck. What about renaming it to entityOwner?

3. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationWorkflowsTest.php
   @@ -105,6 +130,29 @@ public function testWorkflows() {
   +    // Test workflows for the user translator with editable content.
   +    $this->setupEntity($this->userTranslator);
   +    $expected_status = [
   

   Can we move this block at the bottom of the test? This way all other roles are tested with the same entity, otherwise we might leave the door open to weird edge cases.

Looks good to me!

@berdir asked in Slack about this:

one thing that I was wondering about in #2972308: Allow users to translate content they can edit is why we seem to have two different places that are checking content translation access.. we have a function that is called in some places with call_user_func_array() and we have a method on the translation handler? they seem pretty similar?

First of all, I have to admit CT's access control implementation is definitely not straightforward and it might be possible to simplify it. I didn't follow the conversion to the new routing system and how the "old" access callbacks were migrated, however after looking at the code for a while, I think it's safe to assume this difference:

• The logic in CTH::translateAccess() is meant to check whether the user can perform a particular operation on a translation. This logic can be altered per entity type if a dedicated CTH is defined.
• The logic in content_translation_translate_access is meant to check whether the user has any translation permission, and is meant to control access to the translation overview. This logic can also be altered per entity type, however this is done via a legacy entity info key.

Would be good to get a second opinion on the permission name, so tagging for usability review. It would be nice to find something that's more self-explanatory (without the description) if we could, but apart from 'translate entities if editable' which is a mouthful and not necessarily better, I can't think of anything.

Setting NR for #12. Thanks!

Also I'm not sure this belongs in the release notes; there is no disruption from the change that site owners would need to be aware of as the current implementation simply adds a new permission. The change record should be sufficient. Thanks!

Great! Works very well with the content side of things. However, it didn't work with menu links. I have given a user to update menus and menu links with hook_entity_access(). I can see the "traduire" link in the contextual menus, but when I click on it, I get access denied.

Update: Right after I posted this comment, I found that things work well with menu links as well.

So, I realized that we are missing support for content_translation_language_fallback_candidates_entity_view_alter(). That has its own hardcoded access check and does *not* use either of the other two API's :-/.

I've started working on that and testing it, but realized again that we literally have inconsistencies in our inconsistencies:

* First there is the problem that viewing an entity on its own route doesn't trigger that hook anymore, see #2978048: Unpublished translations should fallback to the original language because that uses a different context. So we can only reproduce that by viewing the translation through another entity and the entity reference formatter.
* Second, entities that have their own status (and corresponding access logic) behave completely different behavior from entities that don't and only have a content translation status, which only affects translations. Specifically, the first result in an access denied without the other issue while the second will actually allow access to the translation. Both will be standardized/fixed with the other issue.

To test this, I access the translation both through the route as well as an entity reference field on another entity. With the other issue, we can update that to make the two checks consistent.

After that, I realized that we have some cacheability issues as well, because checking entity access can do anything, for example the edit own permission that we happen to be testing here, that means we need to vary the result by-user. Managed to test that by accessing the entity with two users with the same edit own permission but only one is the owner. And refactored the access checking and logic in the hook to pass that through. I also re-used the handler translation access API in there.

+++ b/core/modules/content_translation/content_translation.module
@@ -363,15 +364,14 @@ function content_translation_language_fallback_candidates_entity_view_alter(&$ca
+      $metadata = $manager->getTranslationMetadata($entity->getTranslation($langcode));
+      $access = $handler->getTranslationAccess($entity, 'update');
+      $entity->addCacheableDependency($access);
+      if (!$metadata->isPublished() && !$access->isAllowed()) {
+        unset($candidates[$langcode]);
       }

I realized that the access check should only be done in case the entity is not published.

Forgot that this is only done here, so I created #3016788: content_translation_language_fallback_candidates_entity_view_alter() should only check access if the entity is unpublished but now closed it again.

The same should also be done for config translation after this one gets merged. I tried setting up webforms and provide granular access to some roles - it was a nightmare! Currently, a person can either translate all config or they cannot translate anything. So a similar change in that sector will also be a huge leap for translations.

+++ b/core/modules/content_translation/content_translation.module
@@ -387,13 +388,14 @@ function content_translation_language_fallback_candidates_entity_view_alter(&$ca
   if ($manager->isEnabled($entity_type_id, $entity->bundle())) {
-    $entity_type = $entity->getEntityType();
-    $permission = $entity_type->getPermissionGranularity() == 'bundle' ? $permission = "translate {$entity->bundle()} $entity_type_id" : "translate $entity_type_id";
-    $current_user = \Drupal::currentuser();
-    if (!$current_user->hasPermission('translate any entity') && !$current_user->hasPermission($permission)) {
-      foreach ($entity->getTranslationLanguages() as $langcode => $language) {
-        $metadata = $manager->getTranslationMetadata($entity->getTranslation($langcode));
-        if (!$metadata->isPublished()) {
+    /* @var \Drupal\content_translation\ContentTranslationHandlerInterface $handler */
+    $handler = \Drupal::entityTypeManager()->getHandler($entity->getEntityTypeId(), 'translation');
+    foreach ($entity->getTranslationLanguages() as $langcode => $language) {
+      $metadata = $manager->getTranslationMetadata($entity->getTranslation($langcode));
+      if (!$metadata->isPublished()) {
+        $access = $handler->getTranslationAccess($entity, 'update');
+        $entity->addCacheableDependency($access);
+        if (!$access->isAllowed()) {
           unset($candidates[$langcode]);
         }

Both before and after, the logic is basically that you need *update* access to *view* an unpublished translation.

I'm wondering if this shouldn't just check $entity->access('view') instead, which will consider the language.

For example, we have a use case that allows access to an unpublished node through a special URL token, similar to https://www.drupal.org/project/preview_link for exmple.

That works fine for the default translation, which considers the view access operation, but it doesn't show any translation as they are filtered out by this logic.

It does come with a non-trivial overhead, checking access is expensive, especially when using node grants, but we can keep the logic to only do that if the node is unpublished.

Hm, so one problem is that unpublished access in core is very inconsistent, there's only a permission to access your own, and only the content_translation module offers an access all unpublished content permission.

I decided to extend it so that we are checking both, first translate and then fallback to entity view access of that specific translation.

Fixing the test by converting that test to use EntityTestMulRevPub and added a dedicated permission to view unpublished tranlsations, this allows to test the entity view access part and it also makes view_unpublished_translation vs. view_unpublished_translation_reference more consistent. In the referenced issue, we will then be able to change it to always 200 and the correct translation, so basically merge those two keys together again.

For the permission names, I suggest following the example of #2914486: Add granular permissions to the Layout Builder. For example,

'%entity_type: Configure layout overrides for @entity_type_plural that the user can edit'

This is closer to what I see in the issue summary here ("allow translating content that the user can edit") than what I see in the latest patch ("Manage translations for any entity editable by the user").

After quite a lot of discussion on that issue, I think it was decided to use fairly long titles instead of adding descriptions.

For final usability review, it will help to update the issue summary with the text used in the patch. I am adding a tag for that task.

Tested this manually on a distribution with group content permissions. It fixes the issue of group content editable but not translatable.

Greetings,

For who are having problem "own content's translating" , I have found a round way to do.

1-Make a "H" view for your "Z" content. (fictional example names)
2-Take permission to view of Z content from all of users.
3-Give permission to see "See own content for Z" to your users, "able to translate Z", "add translate", "edit translate", "delete translate".
4-For H view, disable SQL rewriting add contexual filter to your link to connecting url with right node (for example with unique naming).
5-Use rabbit hole to transfer Z content visitors to H view. So even they don't have rights to see Z content type, with disabled SQL rewriting, they may see it without able to reach translate page.

With this, you may give proper permissions for translation. Yes it gives you a lot of workings, no more layout or node display and increase view usage, but I reached total solution . We need a general update on all of permission page's details and some rights. I hope this solution would be helpful for some.

Have a nice day.

Tested this patch with drupal 8.7, so far it seems to work well without issues. Any chance this type of patch could be committed to the core in a reasonable timeframe? Looks like a key feature for a user based application.

Thank you for creating the #26 patch.
It is extremely disappointing that this issue has been postponed from 8.3 to 8.8.

I want to apply this #26 patch, but is it possible to revert?
Even if a patch is applied to my test site, I'm worried that it may affect the database.

Updated the issue summary to reflect what is currently going on in the patch as per #27.

Fixed deprecation notice.

@johndevman:

Thanks for updating the issue summary to match the current patch.

I still think that you should follow the example of the Layout Builder module (as I said in #27): long title, no description, "that the user can edit" rather than "editable by the user".

Reference: #2914486: Add granular permissions to the Layout Builder and the related change record Made translations permissions more granular (translate all and translate own).

From Comment #6, it looks as though the permission used to be closer to my suggestion. I do not see any discussion of the permission name after my comment in #27.

Even if you disagree with what the Usability team came up with for the Layout Builder, I hope you will agree that consistency is a good thing (for usability, maintainability, probably other reasons). Please use wording similar to what the Layout Builder settled on: something like

Manage translations for any content that the user can edit

Note that I replaced "entity" with "content". This is consistent with Word Choice (part of User interface standards > Interface text in the Develop guide):

• Use "Content" or "Piece of content"—not "Node".
• ...
• Use "Content types"—not "Content type entities". (Avoid using the word "entities".)

I think that is accurate. I have not looked at the code, but Comment #20 implies that this issue only affects content entities, not config.

Using entity is consistent with existing permissions in the content translation module though. You can see that in the context of this patch.

I think the use of content for entities is tricky, because not all entities are content and next to using content for nodes, you need a way to separate it, and be able to say this affects all (content) entities, and this other permission only affects nodes/content.

(I do agree with the wording otherwise)

In other words, it is too late to be consistent?

OK, I think we can agree on this:

Manage translations for any entity that the user can edit

with no description.

Thank you @benjifisher!

I've updated the permission title and removed the description.

The interdiff looks good. I am updating the issue description and removing the "needs usability review" tag.

I have not done any code review nor testing, so I will leave it to someone else to decide whether it is RTBC.

Had a look at the patch and it looks great!

Small nitpick on language but other than that RTBC++

+++ b/core/modules/content_translation/content_translation.permissions.yml
@@ -8,6 +8,8 @@ delete content translations:
+  title: 'Manage translations for any entity that the user can edit'

+ title: 'Manage translations for any entity the user can edit'

This should probably be filed against 9.1.x now.

Patch looks sensible to me, but it needs a re-roll against 9.1.x

Reroll has been done.

Rerolled.

#48 That patch contains a .orig file and is not correct.

Reroll looks good. Back to RTBC!

Fixed these on commit and pushed to 9.1.x, thanks!

FILE: ...slation/tests/src/Functional/ContentTranslationWorkflowsTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AND 2 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
   8 | WARNING | [x] Unused use statement
 152 | ERROR   | [x] Expected "UserInterface|null" but found
     |         |     "UserInterface|NULL" for parameter type
 152 | ERROR   | [x] Data types in @param tags need to be fully
     |         |     namespaced
 174 | WARNING | [x] A comma should follow the last multiline array
     |         |     item. Found: FALSE
----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Published the change record