While working on #1851086: Replace admin/people with a View, I discovered that user_admin_account() has a new call to if (module_invoke('translation_entity', 'translate_access', $account)) {.
That now makes for one of my last failing tests, because there is no field for a translation link.

The entire module has no integration, and it's out of scope for the other issue to add it. (If it was the case of just adding a field handler, I'd have done that already :))

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

Issue tags: +D8MI
plach’s picture

Yes, we need this. I think the main problem right now is that we have not decided yet how to deal with the storage of entity translations, I am not even sure we will store them. As a consequence I'm not even sure the {translation_entity} table, which is what the D7 views integration revolves around, will stay. See #1807800-41: Add status and authoring information as generic entity translation metadata.

I think this should be postponed to #1810370: Entity Translation API improvements.

tim.plunkett’s picture

Well, if we want to postpone full integration on that, we can make this a meta, but we could easily add a field handler for entities that does

$uri = $entity->uri();
return l($uri['path'] . '/translations', $uri['options']);

Since that's all we need for the admin/people and admin/content tables.

plach’s picture

@tim.plunkett:

As you think it's better, no strong perference about this.

tim.plunkett’s picture

Status: Active » Needs review
FileSize
4.54 KB

Here's what I had in mind.

plach’s picture

Status: Needs review » Needs work

Thanks!

The hook_views_data() stuff should go into the respective modules: ET has no dependencies, not even soft ones, on entity-defining modules.

tim.plunkett’s picture

I was surprised to see module_exists('translation_entity') sprinkled throughout core, so it seems I have the exact opposite viewpoint: why should I as an entity type provider have to care about translation_entity, when the handler is generic and could/should be handled elsewhere? It basically means that contrib modules won't bother.

xjm’s picture

So I went back and forth about which module(s) should be providing the data integration. In this case I think hook_views_data() in each entity module is the right choice, because non-core modules will not have the option of adding to translation_entity_views_data_alter(). This might mean that some contrib modules fail to expose their data for translation_entity, but if so, that can be treated as a bug in those modules.

(That said I do think some of the module_exists() for translation entity are kind of awkward. E.g., user_admin_account().)

plach’s picture

why should I as an entity type provider have to care about translation_entity, when the handler is generic and could/should be handled elsewhere? It basically means that contrib modules won't bother.

I don't think it's correct that ET has to know anything about the modules integrating with it: for instance contrib modules wishing to leverage the translate link will have to do exactly what I suggested in #7. ET provides yet another entity-level feature, I think asking it to integrate with any possible entity-defining module out there makes no sense. It would be like asking the Entity module to provide the necessary entity info keys to integrate the various controllers for nodes, terms, comments, users and so on. This is reversing the dependency chain.

ET is coded in a way that it doesn't need to know anything about entity-defining modules. OTOH if a module wishes to integrate translation it needs to provide the related keys. Hence it (softly) depends on ET. It may sound blasphemy but, yes, Node depends on ET and not viceversa.

The only way I'd see for going the way you are proposing would be to define yet other entity info keys allowing ET to alter views data in a generic fashion. And also in this case it would be Node's (and friends') responsibility to provide them.

plach’s picture

That said I do think some of the module_exists() for translation entity are kind of awkward.

I totally agree, we have an issue that may help with this: #1446382: Need a reliable way to determine if a specific bundle for an entity type is translatable.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.72 KB

Per #8, I think this works out well, mostly because it demonstrates to contrib what to do.

tim.plunkett’s picture

FileSize
11.53 KB

entity_test apparently doesn't support multilingual, only entity_test_mul does? And there are seven or so entity_test_* entity types, only one of which has basic Views integration. So I used user instead.

Status: Needs review » Needs work

The last submitted patch, et-1896268-12.patch, failed testing.

dawehner’s picture

+++ b/core/modules/comment/comment.views.incundefined
@@ -376,6 +376,18 @@ function comment_views_data() {
+  if (drupal_container()->get('module_handler')->moduleExists('translation_entity')) {

Is there any reason why we don't iterate over all entity types in entity_translation.views.inc?

+++ b/core/modules/comment/comment.views.incundefined
@@ -376,6 +376,18 @@ function comment_views_data() {
+        'click sortable' => FALSE,

What about override click_sort() in the handler?

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Plugin/views/field/TranslationLink.phpundefined
@@ -0,0 +1,82 @@
+   * @param \Drupal\Core\Entity\EntityInterface $entity

Missing doc :)

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Plugin/views/field/TranslationLink.phpundefined
@@ -0,0 +1,82 @@
+    if (module_invoke('translation_entity', 'translate_access', $entity)) {

Is there a reason why we use module_invoke and not a simple function call, but in general I never understood the advantage.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/Views/TranslationLinkTest.phpundefined
@@ -0,0 +1,67 @@
+  function getTranslatorPermissions() {

should be probably protected.

plach’s picture

Is there any reason why we don't iterate over all entity types in entity_translation.views.inc?

Well, as I said in #9 if there's a way for ET to deal with that stuff in a generic way I have no problem to include those in entity_translation.views.inc.

tim.plunkett’s picture

There is no easy or generic way I know of, especially since it checks 'user' and then adds to the 'users' data, and 'taxonomy' => 'taxonomy_term_data'.

That module_invoke is certainly ugly, but it is moved code.
Yes, it 100% should be protected, but the parent method is not, and fixing that in 4 classes is out of scope :(

I'll fix the docblock and the click_sort in a bit.

plach’s picture

That module_invoke is certainly ugly, but it is moved code.

Would it make sense to move the code wrapped by the ifs in its own autoloaded class? Can we leverage plugins here?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.81 KB
11.59 KB

If by that you mean if (module_invoke('translation_entity', 'translate_access', $entity)) {, I have no idea. I don't know how plugins would help with that.

This addresses the feedback from #14.

Status: Needs review » Needs work

The last submitted patch, et-1896268-18.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, et-1896268-18.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
888 bytes
11.9 KB

EntityTranslationUITest contains really useful helper methods to set up the translation UI. However, despite it being an abstract class, it contains a test method. So there is no way to use those helper methods without having to override that test method. This is more follow-up fodder :)

tim.plunkett’s picture

#1807776: Support both simple and editorial workflows for translating entities does the refactoring we need, but it's buried inside a feature request. Once this is in I'll help reroll that one.

dawehner’s picture

The only thin I'm wondering about is whether we should test the case in which the user does not have access to the entity.

tim.plunkett’s picture

FileSize
819 bytes
12.04 KB

#24 is a good idea.

plach’s picture

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Plugin/views/field/TranslationLink.php
@@ -0,0 +1,90 @@
+    if (module_invoke('translation_entity', 'translate_access', $entity)) {

Not sure this is needed: given that the plugin is part of ET can we hust call translation_entity_translate_access()?

Otherwise looks RTBC to me.

tim.plunkett’s picture

FileSize
929 bytes
12.02 KB

Ah, good call.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Perfect if it is green!

damiankloip’s picture

Sorry, just a small nitpick, otherwise I agree this is good.

+++ b/core/modules/node/node.views.incundefined
@@ -226,6 +226,17 @@ function node_views_data() {
+      'help' => t('Provide a link to the translations overview for nodes.'),

Just wondering, should this not say '...overview for nodes' but '..overview for a node' ? Same with all of the others. That might mean users think it's going to some super translation page for ALL THE NODES?

+++ b/core/modules/user/user.views.incundefined
@@ -295,6 +295,17 @@ function user_views_data() {
+  if (drupal_container()->get('module_handler')->moduleExists('translation_entity')) {

Eck :/ I guess this can't be helped now..

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looking at the other fields' labels it seems like they're a little all over the place wrt singular vs. plural, so I think consistencizing them everywhere can be a follow-up. And yes, the things we are doing to perfectly lovely wrapper functions in D8 make me want to head-desk. :(

From the "Things that make you go 'Buh?!'" department...

+  public function render_link(EntityInterface $entity, \stdClass $values) {
+    if (translation_entity_translate_access($entity)) {
+      $text = !empty($this->options['text']) ? $this->options['text'] : t('view');

This is a link to node/1/translations and the like, not node/1/view. So it should not be labeled 'view.' :)

Otherwise, this looks good to me, so I fixed that up on commit (renamed to 'translate') and...

Committed and pushed to 8x. Thanks!

plach’s picture

Otherwise, this looks good to me, so I fixed that up on commit (renamed to 'translate') and...

Actually, now that I think about it, node and comment links use the "Translations" text. Maybe a small follow-up?

webchick’s picture

You mean relabel the link to "translations" instead? I thought about that but no, we want parity with what admin/content and the like listings are doing now. Those dropbuttons all use "action" keywords: 'view', 'edit', 'translate'.

plach’s picture

Then I guess comment and node links are wrong, because functionally those links are the same.

webchick’s picture

Oh. Right you are. :) Didn't notice that.

I dunno though. On nodes we also have "revisions" tab, which is a noun. But it's worth filing a follow-up so we can figure out how/if we want to reconcile that.

YesCT’s picture

Automatically closed -- issue fixed for 2 weeks with no activity.