Support both simple and editorial workflows for translating entities

As commented over in #1188388: Entity translation UI in core, I think this feature should be optional. The easiest way that I can think of for this is to have a global checkbox (could be per entity type too, but that's not that important) that is disabled by default. And only if you check it, then it will actually check this permission. You still have to configure it, but then at least you know it and only have to do it because you do want to prevent some users from editing the original content. Or an optional module and instead make the permission check pluggable, if there's more functionality that's specific to that workflow?

Because I assume that most sites will not use this feature.

Trying to automatically assign permissions sounds problematic, what happens when you add an additional role?

Also, I think if you need a feature like this, you're probably not far away from requiring something like our TMGMT project (with the still-not finished local translator :() that allows you to manage the translation progress, assign jobs to translators, review their work and so on. Not sure how far core should and can go in that regard.

cross-post.

TBH I did not think deeply about whether we would actually need this feature in core or not: you are totally right that it supports workflow use cases that generally will require complex functionalities like the ones provided by TMGMT.

If we want to stick close to what core provides right now, we do not need this feature. In core you create translations and mark them outdated. You can access the translation overview but you need the edit permission to actually manipulate node values.

One functionality that we would missing, though, is that with just core translators can own a node translation created by someone else beforehand and work only on that one. This could be replaced by per-language ownership, hopefully.
Instead the 'edit original values' permission would allow for a '(just) translate any entity or a specific entity-type' workflow.

However both are ways to provide a basic workflow capability to plain core. I guess we need to retain some basic stuff working.

Edit: obviously we still need the 'edit shared fields' permission to turn a language-aware entity form into a translation form. I guess we can apply a similar argument for this and decide we just want an option (or maybe not even that) to hide form elements dealing with non-multilingual stuff, leaving permissions out of this.

Btw, I guess this is the kind of product-oriented discussion that could benefit from a feedback from the Usability team.

I had to rewrite my comment on this a few times :) Its a though call.

I definitely don't think we should do this by default, as it introduces quite a hairy UX. However I also see it as a valuable addition to core, if you do need to get this workflow. Especially considering the many "institutions" that use Drupal, they often require very formal workflows where editing the original is definitely not allowed. In the end introducing this feature, will simplify the workflow significantly for content creators who do live with those restrictions.

I could buy-in to a checkbox, I generally don't like *magic functionality* checkboxes. But here its to support a important use case, and doesn't necessarily have to sound crazy (e.g. Add per-language content editing permissions).

I could buy-in to a checkbox, I generally don't like *magic functionality* checkboxes. But here its to support a important use case, and doesn't necessarily have to sound crazy (e.g. Add per-language content editing permissions).

IIUC we would have a checkbox in the bundle settings saying something like "Enable translation workflow" that would make permissions be applied and suddenly appear in the permissions list, right?

We decided to go on with this one and making it optional (defaulting to FALSE), since this is bound to feature freeze. After proper UX testing we should be able to understand whether we still want this one.

I will work on this.

I'm planning to post a patch for this during the upcoming weekend. @bforchhammer will help us with code reviews and coding, if needed.

We are discussing the very same topic for D7 in #1829630: Improve workflow permissions (match D8 solution) and we should be able to forward-port the approach we agree upon here.

After studying bforchhammer's analysis concerning ET permissions and discussing this in IRC, I radically changed my mind about this issue.

We identified a set of desirable aspects the ET access control system will need to satisfy:

1. We want to support both a simple translation model, where translations are "normal" content and are handled the same way no matter which language they are assigned, and an editorial translation model where content admins would have full powers on content, while translators would enter translations only for a (possibily strict) subset of the entity fields (see the analysis cited above for details).
2. We need to keep the edit and translate actions separate at access control level because this is the only way to provide a straightforward permissions set allowing site builders to easily implement the translation models described above.
3. We need more granularity in defining translate access, ideally all the CRUD operations should have a dedicated permission.
4. We want to be able to provide per-bundle translation access control where needed.
5. We want to keep the permissions page clean and avoid bloating it by adding per-bundle translate permissions for each operation for any defined entity type, as many of them might not need those. Moreover this might generate confusion as a site builder might ask herself why she has not the ability to assign edit permissions per-bundle for a certain entity type, while it's possible to limit translation access per-bundle for the same type.

After having this set of goals in front, the way to go appears more clear to my eyes :)

To address the first two points we just need to provide different routes, associated with different access callbacks (or whatever they are called today), for editing and translating an entity. Both of them would exploit the entity form controllers to present the user an entity form, which as usual would show only the elements the user has access to.
A user with edit permissions will be able to access the edit router path in any language, thus retaining the contextual editing, which is on of the major wins of multilingual entity forms on the UX side. A user with translate permissions would instead have access to the translation overview and to the translation form, a stripped down entity form where only the multilingual elements are displayed, only the ones the user has access to obviously. In this scenario edit permissions would take precedence over translate ones, hence a user having both would access the edit form also from the translation overview, instead of the translation form.
This approach would be fairly transparent to the user (actually this is already happening in core, as translation creation is performed on a different route than editing), and would allow to choose between the two models described above by just assigning the correct permissions to the various roles.

Wrt to the permission set exposed in the permission page I'd suggest to provide just the following: access|create|edit|delete translation and delegate a more granular distinction to the single entity types. For instance Node may expose a single generic 'translate' permission for each content type, which, combined with the generic ET ones, would allow to set all combinations of access to the CRUD operations for translations with bundle granularity. OTOH User might not provide any translation-related permission and just rely on the ET ones. From the implementation perspective we would just need to adjust the logic in the EntityTranslationControllerInterface::getAccess() method. The base implementation would rely only on the ET permissions while subclasses might extend its logic as needed.

I think this approach would nicely satisfy all the goals depicted above, while still keeping things easy and familiar for site builders.

I could work on the implementation sunday afternoon if I get positive feedback about this proposal. Comments?

That means we neither need the edit original permission nor a checkbox to enable a more complex workflow, right? Sounds great to me, my main problem here was that enabling a module messes with the required permissions of something that worked before. That's not something you expect (unless it's a module that's supposed to do exactly that, like node permission modules ;))

Having to give users new permissions to use new features from a new module on the other side is the normal and expected workflow, so that's fine.

@plach and I just discussed some of my remaining concerns:

1) Having different routes means that there will be different pages on which the same entity values can be edited. My concern (or question rather) was whether this will make it any more difficult for developers to "override things". Most likely this won't be a problem, because both pages would still use the same entity form, which can be changed via hook_form_alter().

2) If we think about "access to entity values" being the resource which we are trying to restrict access to, then the proposed setup will cause an overlap: capabilities of the "translate" permission will be completely contained within the ones of the "edit permission" (as far as "entity values" are concerned). I think, it's generally better to have non-overlapping permissions, because they are easier to comprehend, explain, debug, extend. On the other hand, it does make sense for 'edit' to grant access to translation values, considering the two workflows which we want to support. Also considering that (with the new router system allowing multiple access callbacks) the behavior could be easily changed in contrib, it's more important to support our two main use-cases well.

So I think the proposal above is a good way forward!

Here is a first draft. Tests needs to be updated.

#16: et-workflows-1807776-15.patch queued for re-testing.

+++ b/core/modules/comment/comment.module
@@ -409,6 +409,14 @@ function comment_permission() {
+  return $permissons;

@Berdir found out what is failing :D

I definitely need some sleep :(

wtf

Let's try this.

(Hopefully) Fixed the last failing test.

I like the approach a lot. The entity type specific permissions make a lot of sense to me, although it's kinda ugly to have these module exists checks in hook_permissions(). Not sure if we could come with a pattern that would make this a bit nicer, like a method on the translation handler that is called in translation_entity_permission()? Could also have a default translate $entity_type permission...

We should also add descriptions to the entity type specific permissions to say that users also need the global translation permissions. And maybe a description to those noting that you might need the specific ones :)

Speaking of entities, I think this module (and now also Entity Reference which is close to being commited) are the first to actually use the term entity in the UI, which I assume might be quite confusing. Are there any plans/issues about that? For example the module name, I think Gabor always talked about a "Content translation" module (blocked by the existing one, of course), or something along those lines. Maybe we can also use that for permission and other UI's? We actually do have the concept of ContentEntity in core as well now, so it's not even that far off. There's the problem that we already use content for nodes in the UI, though :)

Not something that belongs here anyway, the entity is already in the permission name, we're just renaming them a bit around that.

Not sure if this is a feature request or a task but I hope we can get this in after dec 1 either way :)

I just tested the patch with node and comment translation, and the permissions seem to do their job beautifully :)

Attached patch contains the following changes:

• Implemented CommentTranslationController::getAccess() and TermTranslationController::getAccess() to fix visibility of links on comment translation overview page
• Added a few missing access checks to translation_entity_overview().
• Fixed Warning: Missing argument 2 for Drupal\translation_entity\EntityTranslationController::entityFormSharedElements() (appeared when translating a comment)
• Fixed translation_entity_edit_access() and translation_entity_delete_access(), so they cannot be accessed for the original entity language.
• Added a translation check for the "delete translation" button on entity forms.

Remaining issues:

1. Shall we rename translation_entity_translate_access to translation_entity_overview_access to align it with the naming of other access functions?
2. Also, the respective "access_callback" in hook_entity_info_alter() is now only used for the "$path/translations" router item. So can we move it into hook_menu or am I missing something?
3. The "view translation overview" permission does currently not respect the module-specific "translate XY" permissions... Shall we maybe add "overview" to the list of access operations on getTranslationAccess)?

4. +++ b/core/modules/translation_entity/translation_entity.module
   @@ -376,46 +432,45 @@ function translation_entity_form_controller(array $form_state) {
   +    'view entity translations' => array(
   +      'title' => t('Access translations'),
   +    ),
   

   Is this one actually used anywhere at the moment?

5. What the patch completely ignores at the moment is the issue with shared fields; at the moment, a user who has just the "create translation" permission can edit shared fields when creating a new translation. My suggestion would be to only allow access to shared fields, when the user has edit access to the original entity... Is this something we can do in this patch, or is this still blocked by #1498724: Introduce a #multilingual key (or similar) for form element?

Sounds like we're missing test coverage, not surprisingly :) Any chance we can easily add some test coverage for the things you found?

Note: I usually use -interdiff.txt for my interdiff's, much shorter than interdiff-do-not-test.patch :) Dreditor works fine with .txt files.

Sounds like we're missing test coverage, not surprisingly :) Any chance we can easily add some test coverage for the things you found?

Maybe we can start with some tests for the "pure translator" role (i.e. testing the new route). It's going to be hard to cover all combinations ;-)

Note: I usually use -interdiff.txt for my interdiff's, much shorter than interdiff-do-not-test.patch :) Dreditor works fine with .txt files.

I'm just following @plach's file-naming convention there :)

Here is a new one fixing #28/5 and providing test coverage. We should be more or less ready to go.

This should fix failing tests and improve test coverage.

Shall we rename translation_entity_translate_access to translation_entity_overview_access to align it with the naming of other access functions?

No more necessary, now the access check in there is way more generic.

Also, the respective "access_callback" in hook_entity_info_alter() is now only used for the "$path/translations" router item. So can we move it into hook_menu or am I missing something?

I think we want to retain the ultimate ability to swap it with something different. However we can revisit this when the access system for the new routing is in place for the relevant routes. I think at that point we could even limit edit access for non-default languages to users having the 'update translations' permission. This would cleanly separate altering entity values in the original language and in other languages.

The "view translation overview" permission does currently not respect the module-specific "translate XY" permissions... Shall we maybe add "overview" to the list of access operations on getTranslationAccess)?

That one has been dropped in the latest iterations.

Is this one actually used anywhere at the moment?

I think we need multiple access callbacks for this, i.e. waiting for all the routes to be converted to the new system.

What the patch completely ignores at the moment is the issue with shared fields; at the moment, a user who has just the "create translation" permission can edit shared fields when creating a new translation.

Just an oversight. This should now be fixed and test-covered.

Updated issue summary. added direct link to comment 107

Updated issue summary.

Updated issue summary.

Updated issue summary.

#35: et-workflows-1807776-35.patch queued for re-testing.

Looks nice, yay for more test coverage!

I hope we can get rid of the non-translate access stuff on the translation controllers soon now that we have the entity access controller. Not sure about the translate related access stuff, not sure how extendable the controller is.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -414,12 +414,14 @@ protected function drupalCompareFiles($file1, $file2) {
    */
-  protected function drupalCreateUser(array $permissions = array()) {
+  protected function drupalCreateUser(array $permissions = array(), $name = NULL) {
     // Create a role with the given permission set, if any.
     $rid = FALSE;
     if ($permissions) {
@@ -431,7 +433,7 @@ protected function drupalCreateUser(array $permissions = array()) {

@@ -431,7 +433,7 @@ protected function drupalCreateUser(array $permissions = array()) {
 
     // Create a user assigned to that role.
     $edit = array();
-    $edit['name']   = $this->randomName();
+    $edit['name']   = !empty($name) ? $name : $this->randomName();
     $edit['mail']   = $edit['name'] . '@example.com';

What is this for exactly? Why does it matter how those users are called?

I hope we can get rid of the non-translate access stuff on the translation controllers soon now that we have the entity access controller.

I think we'll have to wait until all the entity types are actualy implementing the access controllers, I tried to use plain access controllers but almost nothing worked. I'd like to avoid postponing this on that one.

Not sure about the translate related access stuff, not sure how extendable the controller is.

I think it can serve as good foundation to implement the feature, once we understand whether entity translations will be full-fledged entity objects we can rely on those and remove the special casing here or just have a separate translation access controller provided by ET that modules can extend if the y wish to do so. I'd totally prefer the former approach.

What is this for exactly? Why does it matter how those users are called?

It's not a crucial change, but it's handy for assert messages. It makes more clear which "role" the users are covering in the various assertions. I think it's a harmless change :)

Any chance to get an RTBC or a NW, here? ;)

Updated issue summary.

I've scheduled some drupal time for wednesday, so hopefully I'll manage to do a review then. :)

I just tested the patch again with different combinations of permissions (and all core-entities), and I have to say I really like the approach. It supports the mentioned models quite well :)

Attached patch fixes a number of documentation issues, and adds descriptions for translate permissions as suggested by Bojhan in #27.

Overall code and tests look good to me. The only remaining issue I found was with node previews: translators (i.e. users without editing capability) do not get previews for translations because of node_access() checks in node_preview(). (There also was a fatal error before, which I fixed by adding a module_load_include statement to NodeFormController::preview()).

Edit: the "do_no_test.patch" is the interdiff.

NW based on #46 (node preview issue).

I think we'll have to wait until all the entity types are actually implementing the access controllers, I tried to use plain access controllers but almost nothing worked. I'd like to avoid postponing this on that one.

For reference, here's the respective meta issue: #1818580: [Meta] Convert all entity types to the new Entity Field API
For reference, these are the respective issues:
#1862758: [Followup] Implement entity access API for terms and vocabularies
#1862750: Implement entity access API for nodes
#1862752: Implement entity access API for users
#1862754: Implement entity access API for comments

And we already have a follow-up issue as well: #1810320: Remove EntityTranslationControllerInterface::getAccess() once have entity access

Edit: updated the issue links above.

Actually, Entity Field API isn't a requirement here. The issues mentioned in #1696660-117: Add an entity access API for single entity access are.

The descriptions wording could be improved a bit, I think. Maybe Bohjan has some suggestions?

Can I have an image?

Can I have an image?

Sure :-)

I don't follow, why we are even adding these descriptions. We don't do that for others things either, e.g. saying on "Article: Delete any content" that you will need "View published content" and/or "View own unpublished content", it sounds like we are adding circular descriptions.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -346,6 +346,7 @@ public function submit(array $form, array &$form_state) {
+    module_load_include('inc', 'node', 'node.pages');

It seems that should be form_load_include()?

Because they are permissions from completely different modules/groups. E.g, to translate Articles, you will need both "Article: translate any content" and "* translations" from the translation_entity module. As discussed above, this is a flexible model but it's not very intuitive when you look at that page and just see "Article: translate any content".

Why don't we just have the proper fix, which is to have tity translation Create/edit/delete per content type - rather than a generic toggles and a "do everything" permission for a specific content type :)

However as that might be out of scope, I would honestly just remove them and possibly move this workflow issue over to #1810386: Create workflow to setup multilingual for entity types, bundles and fields. This is a off pattern, and to me is more confusing than helpful as you are left scanning all over the page for each permission. I haven't see us do this for other "across categories".

#54, i.e. have access/create/edit/delete permissions per entity type, sounds like a good idea to me and also makes the set-up more flexible, i.e. you trust your users enough that they can delete comment translations but not node translations, but they can still create and edit node translations.
That would make the permissions page more unwieldy as it already is, but that page needs unwieldification anyway, so that shouldn't be a blocker IMO.

Benedikt and I deeply discussed this in IRC after his analysis, and we explored also the solution @Bojhan is suggesting. The problem with it is that we would be splitting access control configuration between the page which is dedicated to it (the permissions page) and a regular configuration page, which IMHO can be even more confusing than having combined permissions.

have access/create/edit/delete permissions per entity type, sounds like a good idea to me

The problem with this approach, as explained in the OP, is that for some entity types we want bundle granularity, but defining 4 permissions for each defined bundle in the site will easily make the permission page explode. Moreover we would easily end-up with entity types that provide per-entity-type edit permissions and per-bundle translation permissions, which would be inconsistent and confusing.

and also makes the set-up more flexible, i.e. you trust your users enough that they can delete comment translations but not node translations, but they can still create and edit node translations.

This is possible also with the current proposal, it would just be matter of creating two roles: comment translator and node translator.

Edit: it's true, but this looks a fairly advanced use case that could be solved by installing a module expanding on the Entity Access API.

Rerolled, let's whether the bot is happy (this is stil NW per #46, however).

This should fix failing tests and the preview issue. If we could have a feedback about #56 it would be great.

fef4eb3 Issue #1807776: Fixed taxonomy integration.
aa734ab Issue #1807776: Fixed node tests.
e138acf Issue #1807776: Fixed node preview access.

Put on D8MI sprint board.

Updated issue summary remaining tasks

#59: et-workflows-1807776-59.patch queued for re-testing.

Rerolled

#63: et-workflows-1807776-63.patch queued for re-testing.

The changes look fine to me, the user role name feature is a bit off-context but small enough and certainly useful.

The main thing I'm a bit sad about is that we are implementing access checks here although we have a new system for this, issues to convert the existing functions and we should instead push those, not improve the workaround that we don't have one :)

I agree having the global access/create/delete/edit permissions and only one "translate X" permissions to couple them for entity bundles is good. It keeps the permissions under a reasonable number. While it does not allow for crazy use cases where you can delete comment translations but not node translations, and it needs some cross-permission allocation, I think its overall cleaner than having 4 permissions for each bundle.

What I'm not comfortable with is the access permission that is added but then not used anywhere. Adding that without using it would definitely set up false expectations (eg. you could hide all translations by not granting that), so I'd not go into adding it without using it. Only add it if we use it.

Otherwise looks good to me :) Extensive tests too :)

@Berdir:

The main thing I'm a bit sad about is that we are implementing access checks here although we have a new system for this, issues to convert the existing functions and we should instead push those, not improve the workaround that we don't have one :)

I agree, but this is a feature, those are tasks: this is subject to a deadline. I'll be happy to reroll this if those issues are fixed before this one.

@Gabor:

Great, I'll drop the "view translation" permission ASAP.

Removed the view permission as per #68, improved the permission descriptions a bit and fixed a couple of issues I found reviewing my own code.

@Berdir:

About removing all those ugly module_exists(): #1446382: Need a reliable way to determine if a specific bundle for an entity type is translatable.

#69: et-workflows-1807776-69.patch queued for re-testing.

@Gabor:

Are we missing anything to RTBC this?

Looks all good to me :)

:)

I don't have time to review this just now, but I'd like to loop this feature in under the "pre-thresholds explosion that is blocks as plugins fall-out" feature RTBC exception because plach did an AMAZING job getting us under thresholds the past couple weeks, and don't want his pet patch to suffer as a result.

@webchick:

I really appreciate it but to be honest this went RTBC today so I ain't sure it deserves this treatment :)

Updated issue summary.

I see my concerns where never addressed. All these messages sound way to excessive.

I tried to address your concerns in the latest patch, if the messages aren't good enough please provide suggestions, otherwise I am afraid this will be stuck in NW state indefinitely.

I dont know, I just dont get at all why we are adding this. We don't do it a whole lot for any other permissions, that require other permissions.

Exactly, and I am proposing to not go down that road and just rely on documentation for this.

All right, let's drop the dependency descriptions from this patch and then either do a user test on whether people get it this way to prove it works and/or figure out a way to display the dependencies more like the modules' page as @YesCT says. Looks like no other complaints with this patch :)

Just a clarification: should I drop all the descriptions or just the enty-type-specifc ones?

Removed all descriptions.

Looking at interdiff, looks like "Configure translatability of entities and fields." provides extra info for the permission and does not go under Bojhan's concerns for cross-permission descriptions, so I think that should still stay. The rest are explaining permission dependencies / combinations that Bojhan was concerned about. So I'd put back "Configure translatability of entities and fields." assuming the resulting patch would satisfy Bojhan's concerns.

Here it is

I believe this satisfies Bojhan's concerns fully as well. So back to RTBC then.

Jumping into this patch blind (er. as blind as someone who's reviewed probably 70-80% of the multilingual issue can be :P~) without reading the issue (though I did scan the summary) so I can replicate a new user experience (as I try to do). Sorry if I say things that were already said. Sorry too for the sort of "stream of consciousness" review.

First, I had a momentary freak-out that I couldn't find the multilingual modules, but there they are at the bottom of the page now. :D I enabled all of them, accidentally coming across #1884936: Modules page looks silly in Bartik in the process. Found myself wishing once again for #1851128: Leave the details of just-enabled modules expanded in modules UI, because the help / perms / configure links are totally undiscoverable without it. :(

Anyway. :P Nothing to do with this patch. :P

I go to the language page and add French (unrelated side note: why does this not auto-download French translations like the installer patch does?). I also go to the People page and add a user named Françoise in the "French Translator" role (which I also create) and a user called peon with no special role.

Onto the permissions page.

I check the following under Content Translation:

- Administrer translation settings: Administrator
- Create translations: Administrator, French Translator
- Edit translations: Administrator, French Translator
- Delete translations: Administrator

And also give French Translator the ability to create and edit Articles.

Go to Content language settings. I allow translations of Articles' Body and Tags fields. (Side note: I checked Taxonomy but there was nothing in the table. I'm guessing that's not the fault of this patch, though...)

I reload the permissions page, but there doesn't seem to be any new functionality. Oh, but I LIE. On further inspection, there's a very subtle new permission under Article for "Translate any content."

PROBLEM: Don't think I would've found that permission there, I not been somewhat expecting it. :\ I only discovered it because of the stupid bug at #1153072: New content types (and other dynamic) permissions are not auto-selected for admin role—always give it all perms. There's probably been discussion about this up above, so I'll look later.

Add an article as admin with a title, body, tags, and an image. Log out, log back in as Françoise. See the translations tab, and I click it, and click "Add" next to the French translation.

Neat! Only see the Tags and Body field, and then a workflow box for marking a translation as out-dated. Translate those fields, leave the checkbox alone. Save. The fields show as translated on page load. Yay.

PROBLEM?: The big disadvantage to this approach is that I lose the context of the other fields, which might be handy for translation. For example, if this was a product, chances are seeing the product image might be useful when describing its attributes in French. However, I don't want to make the image itself translatable. Is this behaviour configurable in any way that I missed?

Now this starts to give me all sorts of evil ideas, such as: how does this "lite" form react when I do things like make the content type require revisions? Let's see!

Log out as Françoise, log back in as admin. (Side note: The french context stuck. Not sure that was intended. I manually hacked the URL and got back to the English version.)

First, I go to the article content type and turn on revisions. Save. Then I go back to the node and edit it from "I like cats" to "I like dogs", which is of course a huge difference, and thus requires me to check my "Flag other translations as outdated" checkbox.

PROBLEM?: That fieldset's real easy to miss if you're an admin, cos it's crammed in with other stuff. Though I suppose so is the revision information one...

PROBLEM?: There's no summary on the vertical tab, which makes it look weird compared to the others. Is this intentional?

I save and get some weird flash of content(?) and then see my node. Go to translations tab, and see "Outdated" next to French. Nice!

Log back in as Françoise. She sees the translation is outdated, and goes to edit it.

PROBLEM?: How, apart from obsessive tab clicking and very keen observation skills, does Françoise know any of her translations are out of date? Is there a block/view of out-dated translations that the site owner could put somewhere?

PROBLEM: On the edit form, my body field still says "J'aime les chats." How am I supposed to know what the English text is and more importantly how it changed? Keep two tabs open and read very carefully? Heck, she doesn't even know who marked it out-dated to contact them about it and ask what they changed, and there's no log message associated with it either to provide a hint.

PROBLEM: The revision checkbox/log is not on this form.

Going through the motions, I change "chats" to "chiens" and uncheck the translation box.

PROBLEM? It seems pretty weird to have to do the negative of something. It feels more natural to have a positive checkbox like "This translation is now up to date."

Click the Translations tab again, and no more outdated stuff.

====

Overall, this still feels very similar to the UX (with most of the same problems) that I reviewed back in October or whenever that was for the original Entity Translation UI. So I'm not sure exactly what part of that this patch is changing/improving. I need to go read the patch/issue now, but in the meantime maybe we have things to discuss...

Oh, one thing I can see right off is I was supposed to test translating with just admin too, for the "simple" workflow.

PROBLEM One thing I notice right off is I see that silly language selector on node/1/edit that has only "English" in it.

PROBLEM When clicking "edit" on the French translation as user 1, I get:

Notice: Trying to get property of non-object in node_help() (line 135 of core/modules/node/node.module).

A hunch is that it's related to the URL being /fr/node/1/edit instead of node/1/edit? Not sure.

When submitting the translation, I also got more notices (they disappeared too fast for me to see them, but watchdog says they are the same error.

So we need some test coverage for that, or else I have a PEBCAK error.

Also, I guess this answers one of my questions above, which is if the extra context is important, then give the translators... uh... which permission? "administer translations" access? That doesn't seem right. "Administer content"? That's not ideal because that'll show them ALL of the fields on the form that the universe ever invented, including confounding vertical tabs for crazy-advanced contrib modules, and these people are typically not that kind of admin.

PROBLEM: I'm not sure which permission triggers "simple" vs. "complex" workflow. I can dig through the module to figure this out, but I ought to be able to figure this out from the permissions list without digging through the code.

That's probably enough to be "needs work" at this point.

Yes, please, have a look to the issue
summary now. If I understood your review correctly, some of the (valid) problems you pointed out are unrelated to this patch. Some of them should already have an issue, btw. I'll review your review more carefully ASAP :)

@webchick: this patch adds permissions to allow people to slice and dice what do they allow for translators to do. 95% of the problems you identified exist with or without this patch.

RELATED:

1. You did not find the permissions: Well, the permissions list just grows which each module, especially cross-functional modules like this make permissions appear at all kinds of places. Is is better to list the permissions under the content translation module instead of right alongside editing permissions at other entities?

11. Permissions triggering complex/simple workflow: well, you see all the permissions. The combinations of them allow for covering simple and complex translation scenarios :)

UNRELATED:

2. Losing context of other fields like product image: not made worse or better in this patch, it is how per-field translation works now.

3. Outdated flag easy to miss: also predates this patch pretty much. The node form reorganization deals with surfacing such things much better.

4. No issue summary on vertical tab: bug, not introduced or changed by this patch.

5. Block/view of outdated translations: Again, the outdated flag predates this patch and is even in the D7 node-copy translation module. D7 does not have an outdated translation view either. Contrib modules deal with this. This is something we could do when the admin node listing is redone in views.

6. How does the translator know what is changed: Again, this is the same D7 experience with *both* node copy translation and field translation. I agree it sucks, but again it has nothing to do with this patch. It could definitely use improvement, not in this patch.

7. Revisions checkbox/log not on this form: is the revision log translatable? :)

8. Negative flag: predates this patch again, also the same in D7 node-copy translation in core (also in D6).

9. Silly language selector: well, it's the node's language. Not affected by this patch.

10. node_help() errors: bug predates this patch (and even entity translation altogether, it is bug with how routing was introduced in core): #1831846: Help block is broken with language path prefixes

crosspost

Starting to read the comments, only at the beginning, but I agree with that "Mark outdated" thing being possibly removed from core and moved to contrib, since there are a some issues with it atm. We should have a follow-up to discuss it.

However, skipping to the bottom again...

In general I like the permission structure of having global CUD permissions around translating, and then having distinct "yes you can translate things on this entity" permissions. It's a good way to simplify what might otherwise be a confusing and convoluted form. :) I still would put the "translate BLAH ENTITY" permissions along with the rest of the Content Translation permissions though, since those two are tightly linked and pretty much useless without each other.

I'm really not a fan though of this dark magical "If you have edit translations you get a simplified form BUT if you have edit content type perms you get the whole thing" behaviour. That definitely was not a choice I knew I could make that way, and I would've left Drupal with the impression it couldn't do what I was seeking in the simplified way.

It seems like configuring the translation workflow you want should be a separate concern from CRUD permissions around translations and entities. How I would personally approach this is just another setting in the "Configuration" table column at admin/config/regional/content-language that's something like (pretend these are radios :D):

Translation form workflow
( o ) Entire form is shown, with non-translatable fields indicated with "(All languages)" (default)
( ) Minimized translation form that shows only those fields which can be translated

YesCT mentioned that plach was opposed to this idea because it meant having to introduce translation CRUD on a per-entity basis, and I'm not sure I understand that concern. Going back to reading the issue again.

Done with the issue. I agree with #56 that CRUD translation permissions per entity and/or a separate place to configure permissions for translations from the main permissions page would make me want to shoot myself in the face, but sounds like a nice contrib module. ;)

So I guess the main reason not to do #97 is because if the user doesn't have edit permissions to the node, what do you do about non-translatbale fields that are being shown to them? My initial impression would be #disabled = TRUE, but I'm sure there's a hole there...

Can you point me to where I'm sure this discussion probably already happened?

So I guess the main reason not to do #97 is because if the user doesn't have edit permissions to the node, what do you do about non-translatbale fields that are being shown to them? My initial impression would be #disabled = TRUE, but I'm sure there's a hole there...

Can you point me to where I'm sure this discussion probably already happened?

I'm not sure this was written down somewhere, but it was certainly discussed at various in-person meetings at least. Usually the entity form does not work so it displays things you cannot edit if you don't have permission. Such as if you have permissions to submit forum posts, you see the title and body and forum category, but not the revisions fieldset or path alias, unless you have administer nodes. The same concept is applied to translation forms. It is indeed arguable that the fields not shown might have a much bigger effect on the translation experience vs. the content creation/editing experience.

The general technical hurdle AFAIK is that you cannot just #disable => TRUE a widget. How does a disabled calendar date picker or unlimited file upload widget (which has previews, drag and drop and label editing as well a new file upload portion) look? I don't think the general problem of displaying an arbitrary disabled field widget is solved yet. This concept can be introduced in the widget API, that would again need a sizable side track and is not really in the scope of this issue.

A disabled widget could also resort to rendering the frontend version of its output. That is tricky. Eg. revisions information have no frontend display mode, and the file list looks dramatically different on the frontend (even depending on whether you wanted files to display on the front end or not), so not sure that is pluggable into an editing/translation form as-is either.

I'm really not a fan though of this dark magical "If you have edit translations you get a simplified form BUT if you have edit content type perms you get the whole thing" behaviour. That definitely was not a choice I knew I could make that way, and I would've left Drupal with the impression it couldn't do what I was seeking in the simplified way.

Well, IMO the beauty of this solution is exactly in its responsiveness: you don't need to go and fiddle with yet another obscure configuration which you might not even know the existence of. I'm pretty sure that if we provided such a form someone would come up with it being unintuitive and obscure and "I don't know what an editorial workflow is" complaints would arise all the time, since apparently it's not a commonly understood terminology.

So I guess the main reason not to do #97 is because if the user doesn't have edit permissions to the node, what do you do about non-translatbale fields that are being shown to them? My initial impression would be #disabled = TRUE, but I'm sure there's a hole there...

Well that would work, but there is nothing like an absolute "simple workflow" or "editorial workflow", they are just two names to indicate two extremes of a range of workflows that vary based on the permission set you apply. And this is exactly how entity forms have always worked in Drupal, showing only what you are allowed to see.
And btw, this has never been exactly the esasiest stuff to set up in Drupal. For instance you didn't see the revision log with the French translator because you didn't provide the related permission, not because of this patch ;)

To be honest I just think most the advanced stuff you can do with permissions (in all areas) cannot simply be groked without some experience.

I still would put the "translate BLAH ENTITY" permissions along with the rest of the Content Translation permissions though, since those two are tightly linked and pretty much useless without each other.

Well, per-entity permissions are provided by the single entity-defining modules. To move them under the "Content translation" section we'd need to define some entity info keys ET could exploit to generate permission for each translatable entity. Then we would be back to a situation where you would have a mix of CUD permissions of different granularity under the same section (per-bundle node permissions, per-entity comment permissions and so on).

I agree with #56 that CRUD translation permissions per entity [...] would make me want to shoot myself in the face

Actually per-bundle ;)

Can you point me to where I'm sure this discussion probably already happened?

Everything we discussed on this topic that is not in this issue is in an IRC log and in #1829630: Improve workflow permissions (match D8 solution).

Updated issue summary remaining tasks and added up to date screenshot.

Well that would work,

Sorry, I misunderstood: I thought we were talking about #access = FALSE. #101 is the proper answer to this.

I added a couple of pointers to additional background information in the issue summary, not sure we discussed exactly #97 but most of the relevant stuff should be in there.

Added background information

Updated issue summary.

No work to be done yet.

Sorry, but I don't agree with that follow-up: I'd like at least that the concern I raised about mixing permissions with different granularities was answered.

If that's a strong pain point I'd prefer to address it here, every new issue is a new patch with its overhead. I see no blocker to address it here if we have to. That said I'd prefer to leve this as they are now.

That probally would make sense, however its hard to visualize for me, could you show it?

Here is a reroll and a screenshot of how the Content Translation section could look like (I'm still not convinced about it).

@webchick, @Bojhan: what do you think?

So #117 is exactly what I had in mind, and looks great. Bojhan also said he liked it in IRC.

However, we had a loonnnnnngggg discussion in IRC today about this. There are a couple of challenges:

1) That granular list is not just per-content type, but per-entity and *sometimes* per-bundle. For example, "contact message" is per-entity, but "Article" is per-bundle, but "comments" is per-entity even though you could also reasonably do per-bundle, since there's a bundle per content type. And all of those are good UX decisions, to keep the list from being overwhelming/non-sensical. So we would need some kind of way of programmatically determining how granular the permissions should be (entity or bundle), if we don't make each module in charge of defining its own permissions in a way that makes sense for that module. That might be tricky.

2) Similarly, the actual names of those permissions are going to be difficult to programmatically define. We don't track plural names of entities, for one.

However, overall I think this is a better approach since it scales better to contrib (in the current patch, all entity module authors need to include a module_exists('entity_translation') check in their hook_permission() which is a bit ick). It also resonates with the feedback from http://groups.drupal.org/node/271918 of people being generally ecstatic of only having to go one place to configure their multilingual features.

plach was going to look into this and see how hard it was to do. I would say that centralizing the permissions is #1, making the names all nicely formatted could be a follow-up, as far as I'm concerned, as long as the initial pass isn't completely un-shippable.

And regarding the workflow changes based on permission, plach actually brought me around on that. He noted that it's very similar to the way node form permissions work right now. For example, if you have Menu module enabled, and your user has permissions to edit menus, you'll see a vertical tab with menu settings below the main form. But if you don't have edit menu permissions, you won't. So really, this is doing the same thing. Admins will see the whole form, translators without edit perms will only see the parts of the form they need be concerned with.

I would love if we could add a sentence to hook_help() about this behaviour, but otherwise I think I'm good on that concern.

Let's rock! :)

Talking about it more, we're not sure if that behaviour will be as much of a WTF as it was for me or not, so we're going to defer the help text writing to a follow-up during CMH, and let one of the newer contributors both test the behaviour as a "real" user and then also write help text that could've helped them figure it out, if they find it confusing. :)

"comments" is per-entity even though you could also reasonably do per-bundle, since there's a bundle per content type. And all of those are good UX decisions, to keep the list from being overwhelming/non-sensical.

I don't actually think that's a particularly good decision. For any real Drupal site (i.e. with a bunch of modules) the permissions page is crazy-long anyway. And I don't consider the screenshot in #117 to be super user-friendly either. Our permissions page just doesn't scale anymore and hasn't for quite some time. That is a separate issue, though, albeit one we should definitely fix. Therefore I don't understand the reasoning for removing another 5-10 permissions from that list (the comment *bundle* translation permissions) because that supposedly makes the list that much more user-friendly. It is a marginal improvement at best, it introduces inconsistencies between entity types and it hides a feature that we already support.

The main reason for having a single permission for comments is *not* to keep the number of permissions down. It is to stay consistent within comment permissions. There is only a single edit comments permission, so why should there be a 26 different translate permissions?

Ahh, I didn't realize that. Thanks for clearing that up. I take #122 back :-)

In other words, the translation permission granularity is unified with the permission granularity of the entity module so it looks odd to be different when listed under the entity translation module but it looks naturally fitting when shown among the other permissions for the entity. One more reason to list the permissions with the other entity permissions? :)

I really think we should test both ways, but we can't. The only indication from users we have right now is that they like things grouped together (the content language settings page). This approach will let us do things in cleaner way in code, this is why I agreed with @webchick to try this route. I think we can still revert back to permissions split per-entity/bundle if the current approach performs badly on user tests.

@plach: good plan!

Rather off-topic, but in response to #122, another reason not to do per-content type comment permissions is because "Comment" comes alphabetically before "Content type" (or "Node") so people see it first, and then get confused thinking they're setting content type permissions. That happened to someone in the usability testing at http://groups.drupal.org/node/271918.

Here is a plain reroll. Setting to needs review to have a testbot pass, I'm working on the discussed changes right now.

Here is a patch implementing #117 and fixing failing tests.

Silly me :(

This looks great from the code perspective. We will see works out in terms of UX but we need to get it in to test that.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -162,6 +162,9 @@
  * \Drupal\Core\Entity\EntityManager::defaults.
@@ -224,6 +227,7 @@ class EntityManager extends PluginManagerBase {

@@ -224,6 +227,7 @@ class EntityManager extends PluginManagerBase {
     'translation' => array(),
     'bundles' => array(),
     'view_modes' => array(),
+    'bundle_permission_granularity' => FALSE,

I am still unhappy about all that stuff being in the entity info definition because of two things:

a) Parsing annotations is slow and having it in there means it needs to be parsed and cached even for sites that do not use entity translation.

b) translation_entity.module is cheating because, despite being an optional module, it is in core and can alter the core EntityManager + documentation of that for it's own optional stuff.

There is an issue to put bundle and view mode information into separate hooks. I would suggest to do the same with the entity translation information.

Of course, in a follow-up issue but I wanted to write it down :)

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -544,6 +544,29 @@ function translation_entity_permission() {
+          $t_args['%bundle_label'] = isset($info['bundles'][$bundle]['label']) ? $info['bundles'][$bundle]['label'] : '';

I guess it's unlikely that there is no bundle label, but I'd suggest to fall back to the machine name in that case instead of an empty string. Otherwise we end up with identical labels..

Also, wondering if there should be a way to disable the automatic permission definition, maybe because an entity type wants to do something completely different. Not sure if that should be a separate flag or if we should use a string or constant to separate between none/entity/bundle..

a) Parsing annotations is slow and having it in there means it needs to be parsed and cached even for sites that do not use entity translation.
[...]
There is an issue to put bundle and view mode information into separate hooks. I would suggest to do the same with the entity translation information.

We could also provide those info through hook_entity_info() implementations, since many of them are not ET specific in the end. No strong preference here.

b) translation_entity.module is cheating because, despite being an optional module, it is in core and can alter the core EntityManager + documentation of that for it's own optional stuff.

I wholeheartedly agree: in the first iterations of the initial ET patch the documentation for the ET specifc keys was in a translation_entity.api.php file, but apparently this was not the right way to do it. However I'm also disturbed by this situation, as it clearly breaks the dependency chain by referring to ET in a place where there should be no knowledge about it.

Also, wondering if there should be a way to disable the automatic permission definition, maybe because an entity type wants to do something completely different. Not sure if that should be a separate flag or if we should use a string or constant to separate between none/entity/bundle..

I picked the second option as this was my initial approach yesterday, before going for a simpler one. Anyway the idea is this entity info could be resued by any module needing to implement entity permissions in a generic fashion, but now we have a global killswitch which means that every module relying on it won't provide permissions if the permission_granularity key is empty.

@Berdir:

Any other concern?

Rerolled + screenshot :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityAccessTest.phpundefined
@@ -68,12 +68,12 @@ function testEntityAccess() {
-    // The custom user is not allowed to view test entities.
+    // The custom user is not allowed to perform any operation on test entities.
     $custom_user = $this->drupalCreateUser();
     $this->assertEntityAccess(array(
-      'create' => TRUE,
-      'update' => TRUE,
-      'delete' => TRUE,
+      'create' => FALSE,
+      'update' => FALSE,
+      'delete' => FALSE,
       'view' => FALSE,
     ), $entity, $custom_user);

This change proves that the first part of this test is in fact relying on the user 1 and not the one that was created because the created user would not have access to create. The drupalLogin() isn't doing anything. Not a problem of this patch, just confirms my confusion in the entity DUBT issue.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationControllerNG.phpundefined
@@ -15,7 +15,14 @@
   /**
-   * Overrides EntityTranslationController::removeTranslation().
+   * Overrides \Drupal\translation_entity\EntityTranslationController::getAccess().
+   */
+  public function getAccess(EntityInterface $entity, $op) {
+    return entity_access_controller($entity->entityType())->{$op . 'Access'}($entity);

Can't you call $entity->access($op) here? That's how this is used usually, this is too ugly :)

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationTestBase.phpundefined
@@ -0,0 +1,215 @@
+ * Definition of Drupal\entity\Tests\EntityTranslationWorkflowsTest.

Should be Contains \Drupal\...

Otherwise this looks good to me. Can someone do a quick re-roll for the last two points and then I can RTBC this so that webchick can have a look at the UI/permissions.

Here it is!

That's so much nicer :)

The requested permission changes were made, back to RTBC.

Thanks!

Updated screenshot is in #141

In EntityInterface:

-  public function getTranslationAccess(EntityInterface $entity, $langcode);
+  public function getTranslationAccess(EntityInterface $entity, $op);

I might be completely on the wrong track, but wasn't the intent of passing $langcode the ability to allow/restrict permissions per-language? I.e. I want to give user "tstoeckler" permission to translate to German and user "plach" to translate into Italian, but I definitely don't want to give "tstoeckler" permission to translate into Italian. :-)
I totally don't think we should solve that in core, but if we leave it in the interface, we will be passing the langcode around everywhere and therefore allow contrib to solve that.
Thoughts?

@tstoeckler:

You are making a very valid point: I've been thinking about this for a while and actually tried to add the new $langcode parameter and see whether it made sense given the current status. The result was honestly confusing and unhelpful. The main reason is that atm we have pretty inconsisent access control wrt language: the entity access controller has a langcode parameter in all its access methods, but the AccessibleInterface provided by the Typed Data API, which is implemented by all entities, has no langcode parameter. For this reason in the translation overview code we would end-up with $entity->access($op) calls next to $controller->getTranslationAccess($entity, $op, $langcode) calls, which would be very inconsistent. Moreover the langcode parameter would be ignored in the current implementation, which would add even more confusion to this mess.

Another thing we need to take into account is that our current translation access control is hardcoded into the translation controller and, as such, cannot actually be replaced by a language-aware access control system.

All of this considered, I decided to punt on this part and just add a todo, since I think we need a far more solid API to rely on to support the (totally valid) use case mentioned in #148. Specifically I think we need an entity translation access controller, providing the canonical access checks the current entity access controller provides, which in turn means being able to treat entity translations as standalone entities (see #1810370: Entity Translation API improvements). In this scenario we would be able to do something like the following:

<?php>
drupal_container()->get('plugin.manager.entity')
  ->getAccessController('entity_translation')
  ->updateAccess($entity, $langcode, $account);
?>

The langcode parameter could be dealt with or ignored in core, but this way the access controller could be replaced by a language-aware version.

To solve the inconsistency between the access controller and the language-unaware Entity::access() method we could easily add a langcode parameter to AccessibleInterface, but I think this is indicating us we may need a cleaner solution. I've been thinking about this for some month now and I think that if the EntityTranslation class implemented the EntityInterface and was refactored to be just a simple wrapper around the parent entity object, we could rely on its language() method to get the language whenever we want instead of passing it around all the time, we would be actually injecting it. In this scenario, the access checks for the edit and translate operations would be:

<?php
$translation = $entity->getTranslation($langcode);

// Determine whether the user can update *the entity* in the given language:
$access = $translation->access('update', $account);

// Determine whether the user can update *the translation* in the given language:
$access = drupal_container()->get('plugin.manager.entity')
  ->getAccessController('entity_translation')
  ->updateAccess($translation, $account);

// or the equivalent with a wrapper method:
$access = $translation->translationAccess('update', $account);
?>

By the way, in this scenario the $translation object could be passed around and used as the wrapped entity. IMO this would greatly simplify the Entity API DX, because we would be introducing the concept of "active language" without introducing a state in the entity object.

Unintentional status change :)

Wow, thanks for the detailed explanation. It all makes complete sense and I have nothing to add. RTBC++

Rerolled after #1822458: Move dynamic parts (view modes, bundles) out of entity_info().

ehm

Better reroll.

Rerolled

Rerolled the patch to adjust the new views integration tests per #1896268-23: Add Views integration for translation_entity.

I'm also recategorizing this as task, since it's mostly refactoring an already existing functionality more than introducing a new one: the only additions are three new permissions in the pemissions section + bundle permission granularity.

Since this requires a big refactoring of the ET tests, which will make writing new ones easier and cleaner (see the attached interdiff), I'm requesting @webchick to evaulate the possibility to consider this as a UX improvement and thus commit it despite us being over thresholds. In fact rerolling this will become more and more painful the more tests we add to ET.

I don't think this is an unfair request, since in this case the limit between feature and task is very blurred and I think this could go in even after feature completion.