Include translation operation in overviews for translatable entities

I agree with that. Should I give it a try? Where can I start?

I have been looking into the code to figure out how to modify the list:
node.admin.inc
node.api.php
translation.module
translation_entity.module
I am still learning, and i will get back to it tomorrow!

For nodes have a look at node_admin_nodes inside node.admin.inc around line 550

We should try to solve #1783964: Allow entity types to provide menu items before looking further into this.

I created a patch with ytsurk. We implemented a part of #1783964: Allow entity types to provide menu items to try to see if this approach makes sense. What do you guys think? Is this the right way to go?

the idea is using the getOperations of the EntityListController.
any entity can provide its own ListController.

if an operation needs to be added to a different entity, we introduced the hook_entity_operations.
there is a check missing so far, to recognize to what entity the operation should be added.
not sure if this would be the best solution ..

I have added a translation link form the drop down OPERATIONS list in admin overviews if the entity (node/user/comment/taxonomy) is translatable.

Gabor and I checked to see if TestEntity (test_entity) had a listing page to do the same thing there and there is no listing page for TestEntity, so that's ok.

Still needs a code review, a manual test and screenshots of the new screen.

did a Short review directly on the screen with @ancamp and found some minor changes.

Applied the changes given by Schnitzel.

uups, forgot to upload the patch.

Forgot to mark it as RTBC, good work

not a very generic solution - but definitely the intended behavior for the 4 included entities ..
or maybe - we wanted to go too far ;)

Great to see progress on this! ytsurk an me kind of wanted to demo the list controller to see how well it does work with content listings instead of settings listings. But let's in this case look at this later on.

Here is what the lists look like with translate added to the dropbuttons.

Node list

Term list

User list

Comment list

(Note: I could not find the comment list and had to have attiks tell me where it was. I didn't even know there was a comment list. It's nice!)

I think Gábor wanted to look at this first.

and I'm writing tests currently :)

Updated patch with Tests :)
Testing translation links for:
- Terms
- Users
- Nodes
- Comments
also testing that for bundles which are not translatable there are no translation links (expect User as there are no bundles)

There is more to do over all, but there is a small part of this that could be a novice task: uploading a version of the patch from #23 which is just the tests without the fix so the testbot can show it fails.

Updated patch after removing fixes so the testbox can show it fails

Adding interdiff

Just a quick note of something. I did not look through the tests in detail.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTranslationUITest.phpundefined
@@ -92,4 +95,21 @@ protected function getNewEntityValues($langcode) {
+  /**
+   * Tests translate link on comment content admin page.
+   */
+  function testTranslateLinkContentAdminPage() {

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.phpundefined
@@ -60,6 +60,23 @@ function getTranslatorPermissions() {
   /**
+   * Tests translate link on content admin page.
+   */
+  function testTranslateLinkContentAdminPage() {

Both of these say ContentAdminPage. I think the one for comments should say

function testTranslateLinkContentCommentAdminPage()

or maybe just
function testTranslateLinkCommentAdminPage()

These are questions I had about the patch (fix only) from #16. I dont think anything is really wrong, but @ancamp if you can answer the questions, that would be nice; I'm curious.

+++ b/core/modules/comment/comment.admin.incundefined
@@ -136,6 +136,14 @@ function comment_admin_overview($form, &$form_state, $arg) {
+    if (module_invoke('translation_entity', 'enabled', 'comment', $comment->bundle())) {
+      $links['translate'] = array(
+        'title' => t('translate'),
+        'href' => 'comment/' . $comment->cid . '/translations',
+        'query' => $destination,
+      );
+    }
+
     $options[$comment->cid]['operations']['data'] = array(

this is a picky whitespace thing, but in the comment.admin.inc is the only time the blank line is added between the new block and the $options line that follows. The node, taxonomy and user do not have that blank line. So I would suggest either take out this blank. Or, also add it in the other files if the blank line is helping readability of the code you are adding in by separating it a bit from near code.

+++ b/core/modules/node/node.admin.incundefined
@@ -560,6 +560,13 @@ function node_admin_nodes() {
+    if (module_invoke('translation_entity', 'enabled', 'node', $node->bundle())) {
+      $operations['translate'] = array(

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -285,9 +285,16 @@ function taxonomy_overview_terms($form, &$form_state, Vocabulary $vocabulary) {
+    if (module_invoke('translation_entity', 'enabled', 'taxonomy_term', $term->bundle())) {
+      $operations['translate'] = array(

+++ b/core/modules/user/user.admin.incundefined
@@ -214,9 +215,16 @@ function user_admin_account() {
+    if ($translation_enabled) {
+      $links['translate'] = array(

comments and users are adding a links['translate'] array, and nodes and terms are adding a operations['translate'] array.
The screenshots in comment #20 show translate being added to each "OPERATIONS" column.

And why is user the only one to use $translation_enabled instead of module_invoke()? Maybe because users do not use a bundle?

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -285,9 +285,16 @@ function taxonomy_overview_terms($form, &$form_state, Vocabulary $vocabulary) {
-      'edit' => array('title' => t('edit'), 'href' => 'taxonomy/term/' . $term->tid . '/edit', 'query' => drupal_get_destination()),
-      'delete' => array('title' => t('delete'), 'href' => 'taxonomy/term/' . $term->tid . '/delete', 'query' => drupal_get_destination()),
+      'edit' => array('title' => t('edit'), 'href' => 'taxonomy/term/' . $term->tid . '/edit', 'query' => $destination),
+      'delete' => array('title' => t('delete'), 'href' => 'taxonomy/term/' . $term->tid . '/delete', 'query' => $destination),

why the change from drupal_get_destination() to destination()? Just to make it consistant with the other tests?

+++ b/core/modules/user/user.admin.incundefined
@@ -214,9 +215,16 @@ function user_admin_account() {
-      'href' => "user/$account->uid/edit",
+      'href' => 'user/' . $account->uid . '/edit',

this seems like an unrelated change.

Oh, I see. You are making it consistant with the single quote pattern used in the other $operations arrays.

I remember we talked about this at badcamp with Schnitzel and xjm. They said if it was very close to a block of code you were adding and related, that it made sense to go ahead and fix it.

Still at needs review because @webchick wanted to see what Gabor had to say in general about the fix or the tests. But I wonder if he already said it in #11.

I'll do two little things (the test name from #27 and the white space from #29 since I complained about them. :)

I agree with Gabor: for now we don't really have a cleaner solution for this. We might want to revisit the implementation once we have a more generic and unified API to build on. Meanwhile this is a nice UX improvement that it's good to have in.

+++ b/core/modules/comment/comment.admin.inc
@@ -136,6 +136,14 @@ function comment_admin_overview($form, &$form_state, $arg) {
+    if (module_invoke('translation_entity', 'enabled', 'comment', $comment->bundle())) {

+++ b/core/modules/node/node.admin.inc
@@ -560,6 +560,13 @@ function node_admin_nodes() {
+    if (module_invoke('translation_entity', 'enabled', 'node', $node->bundle())) {

+++ b/core/modules/taxonomy/taxonomy.admin.inc
@@ -285,9 +285,16 @@ function taxonomy_overview_terms($form, &$form_state, Vocabulary $vocabulary) {
     );
+    if (module_invoke('translation_entity', 'enabled', 'taxonomy_term', $term->bundle())) {

Using module_invoke() is a more elegant solution than what we are doing now in the regular comment display (we could fix that one too), however we should be invoking translation_entity_translate_access() instead, otherwise we aren't checking that the user is actually allowed to visit the translation overview. Currently clicking the link may result in a 403.

+++ b/core/modules/user/user.admin.inc
@@ -192,6 +192,7 @@ function user_admin_account() {
   $destination = drupal_get_destination();
+  $translation_enabled = module_invoke('translation_entity', 'enabled', 'user');

Same as above, but why we need a variable here instead of just putting the call in the if test? This would be more consistent.

I have some answers to @YesCT and @plach

@plach:

Same as above, but why we need a variable here instead of just putting the call in the if test? This would be more consistent.

performance, calling it once is much much faster then all the time for each User. As there are no bundles for Users you can only enable translations once for Users and we don't have to check it every time a user is shown.

however we should be invoking translation_entity_translate_access() instead, otherwise we aren't checking that the user is actually allowed to visit the translation overview. Currently clicking the link may result in a 403.

oh, awesome found :) @YesCT when you are changin all the things already could you do this directly also? If not assign to me later then I will do this

@YesCT

And why is user the only one to use $translation_enabled instead of module_invoke()? Maybe because users do not use a bundle?

exactly same question from plach, reason: performance.

why the change from drupal_get_destination() to destination()? Just to make it consistant with the other tests?

Performance too, the destination is the same for every node, so why loading it for every node.

performance, calling it once is much much faster then all the time for each User. As there are no bundles for Users you can only enable translations once for Users and we don't have to check it every time a user is shown.

Sorry, I missed the loop :)

Not going to get to this tonight. So unassigning it form myself. I'll check in on it when I can come back to it, but if someone else addresses the reviewer comments first, that's ok. :)

- fixed things from YesCT in #27 and #29
- now using translation_entity_translate_access() as suggested in #32, which also means now that we have to run translation_entity_translate_access() for every single user. Als had to fix the tests as they did not have in: 'translate any entity'

One thing I really hate to do:

+++ b/core/modules/user/user.admin.incundefined
@@ -192,11 +192,11 @@ function user_admin_account() {
+    $account = user_load($account->uid);

but if not $entity->entityType(); in translation_entity_translate_access() does not work.

We should definitely use full Entities in user_admin_account(), but to change all this would be too much of work, so I would suggest to create follow ups for this.

sorry. cross post.

I'm reviewing this. Found some comment grammar nits I'll fix. I'll also post a patch and a just tests patch.

Here is the whole patch. (it passes locally). I have some questions about it... so I'll post a review of my own patch in a few.
There is also a tests only patch that should fail (it fails locally).
There is also an interdiff to 37: I added period to the end of // comment sentences, and blank likes to before the class closing that were removed when the tests for this were added, and I changed non_translatable to untranslatable to match what was used in the main et-ui issue.
There is also a just cleanup and fix file ... just for completeness in case someone wants to look at that.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.phpundefined
@@ -60,6 +60,23 @@ function getTranslatorPermissions() {
   /**
+   * Tests translate link on content admin page.
+   */
+  function testTranslateLinkContentAdminPage() {
+    $this->admin_user = $this->drupalCreateUser(array('access administration pages', 'access content overview', 'administer nodes', 'bypass node access'));
+    $this->drupalLogin($this->admin_user);
+
+    $page = $this->drupalCreateNode(array('type' => 'page'));
+    $article = $this->drupalCreateNode(array('type' => 'article'));
+
+    // Verify translation links.
+    $this->drupalGet('admin/content');
+    $this->assertResponse(200);
+    $this->assertLinkByHref('node/' . $article->nid . '/translations');
+    $this->assertNoLinkByHref('node/' . $page->nid . '/translations');
+  }

Just a question: Are pages by default in the et-ui tests not translatable and articles are translatable?

+++ b/core/modules/user/lib/Drupal/user/Tests/UserTranslationUITest.phpundefined
@@ -59,4 +59,19 @@ protected function getNewEntityValues($langcode) {
+  /**
+   * Tests translate link on user admin list.
+   */
+  function testTranslateLinkUserAdminPage() {
+    $this->admin_user = $this->drupalCreateUser(array('access administration pages', 'administer users', 'translate any entity'));
+    $this->drupalLogin($this->admin_user);
+
+    $uid = $this->createEntity(array('name' => $this->randomName()), $this->langcodes[0]);
+
+    // Verify translation links.
+    $this->drupalGet('admin/people');
+    $this->assertResponse(200);
+    $this->assertLinkByHref('user/' . $uid . '/translations');
+  }

Needs work because...

user is the only test added here that does not check that translations is not added when appropriate.

I think this is because users are either all translatable or all not. So, I think the test should also disable translation for users and test with assertNoLink that the translation link is not added to the user list.

Next

unassigning myself so @Schnitzel (or someone else) can respond or fix.

Just a question: Are pages by default in the et-ui tests not translatable and articles are translatable?

yes, Entity Translation UI Tests which are used define this by default (not drupal core default).

So, I think the test should also disable translation for users and test with assertNoLink that the translation link is not added to the user list.

Not my opinion: The Tests are testing if a new feature implemented (the translation links) is working correctly.
The Entity Translation UI Tests which are already in, also do not Test if Access is successfully denied if translation is not enabled to an Entity.
As we are here only adding links and not real functionality I would suggest that we make a follow up to create better test coverage for Entity Translation UI and add these Tests there.

back to needs review.

given my questions were answered, RTBC.

Here is a patch that fixes comment on #46

@rvilar (or other) an interdiff between #42 and #47 might help.

@YesCT I only remove the ';' character that Gabor shows in comment #46

recreated the patch of #42 and only changed the codestyle issues, see interdiff.

+++ b/.htaccessundefined
@@ -95,7 +95,7 @@ DirectoryIndex index.php index.html index.htm
-  # RewriteBase /
+  RewriteBase /

@rvilar Thanks a lot for jumping in here. It was a mystery why the bot had trouble. But, I think the RewriteBase might have been the cause.

(Now I had to roll back to Nov 10 to get the old patches to apply and I'm no git expert, so I might have made an error... but I tried to sort it out. See the history.)

I'll update the summary too.

in #21 webchick asked for Gabor to look at this.
Gabor addressed the approach in #30
plach and Schnitzel had a nice look and back and forth
then Gabor looked again #46

So I think it's been looked at well.

I did another manual test just to make sure the latest stuff is good.
More screen shots from before are in #20
But here is a quick reshot with patch from 54, first without translation enabled and then with translation enabled.

with translation enabled

Updated issue summary to show the recent screenshot and use the issue summary template to make it easy for a committer to see what the status is.

Looks good to me. Committed to 8.x. Thanks.

#1848164: Provide translate option for entities in contextual links. is kind of related