Entity translation UI in core (part 2)

Quoting @sun:

Aloha :)

I wasn't able to review the entire code, and I had to apply it locally to make sense of it.

In overall terms, it is exceptional to see how much work went into this issue. Given this massive amount of contribution, that inherently puts some pressure on reviewers. And I am indeed inclined to get this in rather earlier than later, so as to make an end to this epic issue. However, a couple of aspects of the implementation do not really look very clean and well designed, and after (briefly) reviewing the code, I still did not understand the main architecture of how the UI works (technically).

These two sides leave me in a terrible state, because on one hand, I see @plach and a handful of people rocking this issue and trying hard and harder to achieve a corner-stone for Drupal's long-term success, which will have a serious impact on its adoption, especially in Europe and other places of the world. On the other hand, however, the current state of the code definitely leads to a fair amount of confidence that this implementation will get "in the way" for many other core contributors who are working on completely different things. I'd be more than happy to say to everyone, "well, you have to eat that cake, deal with it", but to be honest, I fear some major breakage down the line.

Facing this scenario, the first thing I wondered was whether it would be possible to scale down the size and impact of this initial patch? For example (not sure whether this makes sense), only introducing entity_translation.module on its own, without wide-scale integration for all other core entities, and only proving that it works within its own tests?

Second, I also wondered how many follow-up tasks (as in: features) will actually depend on this initial patch? As in: If there are none, or not many, then we could possibly try to polish this some more until feature freeze, so as to produce less headaches for other core contributors until then? (Tweaks and polishing of this newly introduced Entity Translation UI feature itself would of course be exempt from feature freeze; that's what the phase until code freeze is for.)

.............SNIP.............

@webchick just pushed the button. Heh.

.............SNIP.............

Erm. Regardless of that, here's my review:

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -140,6 +144,16 @@
+ * - menu_base_path: (optional) The base menu router path to which the entity
+ *   administration user interface responds. It can be used to generate UI
+ *   links and to attach additional router items to the entity UI in a generic
+ *   fashion.
+ * - menu_view_path: (optional) The menu router path to be used to view the
+ *   entity.
+ * - menu_edit_path: (optional) The menu router path to be used to edit the
+ *   entity.
+ * - menu_path_wildcard: (optional) A string identifying the menu loader in the
+ *   router path.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationControllerInterface.php
@@ -0,0 +1,190 @@
+ * To make Entity Translation automatically support an entity type some keys
+ * may need to be defined, but none of them is required unless the entity path
+ * is different from ENTITY_TYPE/%ENTITY_TYPE (e.g. taxonomy/term/1), in which
+ * case at least the 'menu_base_path' key must be defined. This is used to
+ * determine the view and edit paths if they follow the standard path patterns.
+ * Otherwise the 'menu_view_path' and 'menu_edit_path' keys must be defined. If
+ * an entity type is enabled for translation and no menu path key is defined,
+ * the following defaults will be assumed:
+ * - menu_base_path: ENTITY_TYPE/%ENTITY_TYPE
+ * - menu_view_path: ENTITY_TYPE/%ENTITY_TYPE
+ * - menu_edit_path: ENTITY_TYPE/%ENTITY_TYPE/edit
+ * The menu base path is also used to reliably alter menu router information to
+ * provide the translation overview page for any entity.
+ * If the entity uses a menu loader different from %ENTITY_TYPE also the 'menu
+ * path wildcard' info key needs to be defined.

The addition of these entity info properties is in direct conflict with #1332058: Need a way to distinguish "public/final" URIs for entities from admin and redirected ones and #1783964: Allow entity types to provide menu items.

TBH, I do not understand the properties being proposed here.

However, scanning through the patch, it actually doesn't look like they are used anywhere (except for test entities), the core entities seem to specify the translation_controller_class property only — so perhaps we're in luck and can remove them from this patch without too much hazzle?

FWIW, for now, EntityListController simply leverages $entity->uri() and assumes that to be the entity's primary "view URI", and the default list controller also assumes that /edit and /delete are registered child routes below $entity->uri().

So before introducing these entity info properties (which appear to be conceptually wrong to me), I'd rather go with similar assumptions in the default translation controller, and require entity-specific controllers to override the appropriate (+ separated) methods, if their URI pattern is different for any reason.

+++ b/core/modules/comment/comment.module
@@ -1009,6 +1009,16 @@ function comment_links(Comment $comment, Node $node) {
       $links['comment-forbidden']['html'] = TRUE;
...
+      'title' => t('translations'),
...
+      'html' => TRUE,

It appears from the patch context that the existing comment links are specifying 'html' TRUE already, but that's not really a reason to continue the bogus pattern ;) There's no HTML involved in this link title, so 'html' should be FALSE (or just simply omitted). :)

+++ b/core/modules/node/lib/Drupal/node/NodeTranslationController.php
@@ -0,0 +1,50 @@
+  protected function entityFormTitle(EntityInterface $entity) {
+    $type_name = node_get_type_label($entity);
+    return t('<em>Edit @type</em> @title', array('@type' => $type_name, '@title' => $entity->label()));
+  }

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationController.php
@@ -0,0 +1,433 @@
+  public function entityFormAlter(array &$form, array &$form_state, EntityInterface $entity) {
...
+      $title = $this->entityFormTitle($entity);
+      // When editing the original values display just the entity label.
+      if ($form_langcode != $entity->language()->langcode) {
+        $t_args = array('%language' => $languages[$form_langcode]->name, '%title' => $entity->label());
+        $title = empty($source_langcode) ? $title . ' [' . t('%language translation', $t_args) . ']' : t('Create %language translation of %title', $t_args);
+      }
+      drupal_set_title($title, PASS_THROUGH);

I do not understand why we need a entityFormTitle() method on EntityTranslationController, if that title gets overridden in 80% of all cases anyway, and can be easily auto-generated from $entity in the first place? I think we can remove that method.

Second, but minor: The auto-generated title composed of the returned $title is not a translatable string. ;)

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/EntityTranslationController.php
@@ -0,0 +1,433 @@
+  /**
+   * Implements EntityTranslationControllerInterface::getAccess().
+   */
+  public function getAccess(EntityInterface $entity, $op) {
+    return TRUE;
+  }

Normally, everything related to access should default to FALSE.

+++ b/core/modules/translation_entity/translation_entity.install
@@ -0,0 +1,71 @@
+function translation_entity_install() {

+++ b/core/modules/translation_entity/translation_entity.module
@@ -0,0 +1,702 @@
+function translation_entity_entity_info_alter(array &$entity_info) {
...
+function translation_entity_menu() {
...
+  foreach (entity_get_info() as $entity_type => $info) {
...
+function translation_entity_menu_alter(array &$items) {
...
+  foreach (entity_get_info() as $entity_type => $info) {

There are a lot of hidden dependencies going on here. At minimum, the hook_install() implementation should set a higher module weight via module_set_weight(), so that the entity_translation.module's hooks are executed after other modules. A reasonable weight might be 10.

(Btw, I have absolutely no idea how we're going to deal with these kind of race conditions in the new kernel/service/container world, so there's a lot of fun ahead of us...)

+++ b/core/modules/translation_entity/translation_entity.install
@@ -0,0 +1,71 @@
+function translation_entity_enable() {
...
+  drupal_set_message($message, 'warning');

1) This hook_enable() implementation outputs a message after enabling — but does not seem to prevent that from happening when installing Drupal (with the module).

2) The message is a first-time installation message, but is re-displayed whenever the module is (re-)enabled.

3) Why is this positive message a warning? Warnings are for... well, warnings. :)

4) The message reads a bit long. I still haven't actually read it, which is a good indicator that it is way too long. ;)

+++ b/core/modules/translation_entity/translation_entity.module
@@ -0,0 +1,702 @@
+function translation_entity_language_types_info_alter(array &$language_types) {
+  unset($language_types[LANGUAGE_TYPE_CONTENT]['fixed']);
+}

Putting on my innocent developer hat:
I do not understand what this alter hook is doing and why it is legitimate to unset language type info in the system like that. This definitely needs explanation in an inline comment.

+++ b/core/modules/translation_entity/translation_entity.module
@@ -0,0 +1,702 @@
+      $entity_position = count(explode('/', $path)) - 1;
+      $keys = array_flip(array('theme_callback', 'theme_arguments', 'access_callback', 'access_arguments', 'load_arguments'));
+      $menu_info = array_intersect_key($info['translation']['translation_entity'], $keys) + array('file' => 'translation_entity.pages.inc');
...
+      foreach ($menu_info as $key => $value) {
+        $item[str_replace('_', ' ', $key)] = $value;
+      }
...
+  $regex = '|%[^/]+|';
+  foreach ($items as $path => $item) {
+    $path = preg_replace($regex, '%', $path);
+    $paths[$path] = $path;
+  }
...
+      if (!isset($paths[preg_replace($regex, '%', $path)])) {
+        drupal_set_message(t('The entities of type %entity_type do not define a valid base path: it will not be possible to translate them.', array('%entity_type' => $info['label'])), 'warning');

This menu system futzing looks very concerning. Not only from a performance perspective, but also from an implementation perspective. I need to study the code some more to understand what it actually tries to achieve, and whether that couldn't be achieved in an easier way.

Second, we definitely do not want to output a message whenever the menu/router is rebuilt. This is rather hook_requirements()/status report material.

+++ b/core/modules/translation_entity/translation_entity.module
@@ -0,0 +1,702 @@
+/**
+ * Returns the key name used to store the configuration item.
+ *
+ * Based on the entity type and bundle, the variables used to store the
+ * configuration will have a common root name.
...
+ * @return string
+ *   The key name of the configuration item.
...
+function translation_entity_get_config_key($entity_type, $bundle, $setting) {
+  $entity_type = preg_replace('/[^0-9a-zA-Z_]/', "_", $entity_type);
+  $bundle = preg_replace('/[^0-9a-zA-Z_]/', "_", $bundle);
+  return $entity_type . '.' . $bundle . '.translation_entity.' . $setting;
+}

Wowza. This contains a ton of configuration system + config entity/bundle assumptions that are actually not enforced anywhere (and shouldn't be).

Is there any reason why this cannot use the entity info and check for the "config_prefix" key contained in there?

Hm. Perhaps I'm also mistaken and this doesn't actually make any assumptions about config object/file names, but only generates a key name that is internal to entity_translation.module...?

+++ b/core/modules/translation_entity/translation_entity.module
@@ -0,0 +1,702 @@
+function translation_entity_form_field_ui_field_edit_form_alter(array &$form, array &$form_state, $form_id) {
...
+      '#prefix' => '<div class="translatable"><label>' . $label . '</label>',
...
+      '#prefix' => '<label>' . $label . '</label>',
+      '#type' => 'checkbox',

I do not understand why this form alter uses completely custom HTML markup instead of the appropriate form element #types.

Both of them should be wrapped in a #type 'item' instead of the manual HTML + label futzing. :)

@catch:

This feels like something the entity translation module could provide a function for? Or is it very, very specific to node module?

Well, at the moment only nodes have a vertical tab for the transation settings: however I agree we could try to make ET expose it. Sounds as good follow-up for the guys sprinting in Zurich.

@sun:

However, a couple of aspects of the implementation do not really look very clean and well designed

I already said that there are several aspects in the current architecture that I'm not happy with, but I think committing the patch was the right choice because many of them rely on changes that are not in yet or do not have a proper solution either. I think we would could easily end up not having the Entity Translation UI in core even for D8 if we we were required to fix all of them before committing the initial patch.

and after (briefly) reviewing the code, I still did not understand the main architecture of how the UI works (technically).

Well, the basic idea is pretty simple: there is an entity translation controller that alters the form provided by the entity form controller and allows to specify which language translatable values should be saved in. Most of the translation capabilities are already baked into entity forms.

On the other hand, however, the current state of the code definitely leads to a fair amount of confidence that this implementation will get "in the way" for many other core contributors who are working on completely different things. I'd be more than happy to say to everyone, "well, you have to eat that cake, deal with it", but to be honest, I fear some major breakage down the line.

I'm totally willing to refactor things so that we leverage the right tools and adhere to the best core practices, the main problem is that in many cases they are not there yet. The menu_* entity info keys are a clear example of this: they are a one-off hackish solution that I'm sure will go away as soon as we come up with something that makes actual sense (I was discussing this with @tim.plunkett yesterday), but right now we have nothing yet. And ET cannot absolutely work without being able to attach itself to to router paths and to generate entity UI urls in an entity-agnostic fashion.

Second, I also wondered how many follow-up tasks (as in: features) will actually depend on this initial patch? As in: If there are none, or not many, then we could possibly try to polish this some more until feature freeze, so as to produce less headaches for other core contributors until then?

The plan is exactly to address the follow-ups bound to feature-freeze (not many I believe) in these last weeks and focus on the rest afterwards. See the issue summary for a full list.

The addition of these entity info properties is in direct conflict with #1332058: Need a way to distinguish "public/final" URIs for entities from admin and redirected ones and redirected ones and #1783964: Allow entity types to provide menu items menu items.

Yes, I totally wish to leverage whatever comes up from those.

TBH, I do not understand the properties being proposed here.

They serve a double purpose:

• The base path is used to alter menu router information and provide the translate overview page
• It is used as a way to generate the urls used for redirecting and for all the links of the translation overview. The problem right now is that we cannot assume an edit URL will always be 'entity/1/edit' hence we need something more reliable.

So before introducing these entity info properties (which appear to be conceptually wrong to me), I'd rather go with similar assumptions in the default translation controller, and require entity-specific controllers to override the appropriate (+ separated) methods, if their URI pattern is different for any reason.

I evaluated this possibility in the early days of the D7 stuff, but it has the drawback of requiring you to provide a new class instead of just defining a couple of info elements. That said, we need a better solution for this.

(I have to go now: I will complete this answer later and a provide a patch fixing everything that does not need more discussion)

Bojhan we are creating them right now :)

The main one (the wizard!) is #1810386: Create workflow to setup multilingual for entity types, bundles and fields (didn't update the issue summary yet).

Suppose first we need to remove entity_id in favor of entity_uuid! D8 entities are always has uuid assigned so I see no reason to have enetity_id as integer or long varchar

Opened #1833022: Only display interface language detection options to customize more granularity for usability improvements in language selection, related to this issue.

Guys, we have now a proper component element to filter the queue with. No point in cluttering this issue whenever a new one is created.

Let's stay focused on fixing @sun's and @catch's concerns (I should be almost there, I have some failing tests).

Another follow-up: #1833604: Invalid string index notice in EntityTranslationController with PHP 5.4 (not sure if there already is one)

The last submitted patch, et-ui2-1831530-17.patch, failed testing.

Aw, it's still there, didn't save, sorry :(

#20: et-ui2-1831530-20.patch queued for re-testing.

I'm going to work on cleaning up the follow-up list in the issue summary. I'll copy the text that is accompanying the issue links into those issues and remove the text from here.

oops. I didn't see your post. I dont mind at all. And you have so much more context of the overall picture.
Here I was really just cleaning. Revert here if you wish. :)
[edited]

A small follow we noticed while refactoring taxonomy display.
Code:

so $form_langcode is a string that is used as array afterwards. Sorry I did not investigate all the way. I am having difficulties at my own tests and don't want to disturb my head even more.
If you need more information, ping stalski or zuuperman on IRC

Ok sorry, didn't review all patches. So great job :)

I guess @catch will want to have a look to this.

I'm wondering why we keep having to check for the admin paths for entities that are translation enabled, rather than throwing an error somewhere if an entity is translation enabled without the path?

This was my initial approach, but @sun made the following objection:

Second, we definitely do not want to output a message whenever the menu/router is rebuilt. This is rather hook_requirements()/status report material.

which looked sensible to me. OTOH you have a point too in:

Who's this directed to? I'd expect the site administrator but they're not going to be able to fix this. If it's aimed at developers is there somewhere we could throw an exception instead - i.e. in translation_entity_enabled()?

Persoanlly I'd go either with an exception or better with a watchdog error. We don't want the page response to break just because an entity is not translatable when it should be.

Why isn't this enough?

If you are asking me why enabling an entity for translation is not enough for it to be translatable, the answer is that without at least a valid base path, the translation overview and all the links in it cannot be generated.

The part of this remaining to improve it a bit: #1870946: Improve target of entity needing to be translatable warning message by using watchdog

I think that means everything else is addressed.

Here is the reroll.

Put on sprint.

#38: et-ui2-1831530-38.patch queued for re-testing.

The last submitted patch, et-ui2-1831530-38.patch, failed testing.

here was the issue that caused the conflict: #1810386: Create workflow to setup multilingual for entity types, bundles and fields

#45: et-ui2-1831530-45.patch queued for re-testing.

The last submitted patch, et-ui2-1831530-45.patch, failed testing.

Previous fail was unrelated.

#1870946: Improve target of entity needing to be translatable warning message by using watchdog will address the watchdog concern.

That leaves only remaining task: the weights. ( Probably for @plach )

This should address @catch's concerns, plus a small improvement to the field settings page:

Here are some fixes:

@tstoeckler:

Thanks for the review!

If I'm not mistaken, the default module weight is 0, so this is actually a rather *high* weight. The comment is otherwise correct, because the high weight causes module's hooks to be called last.

Yes the alterations performed in hook_entity_info_alter() are essential to make ET work, hence we need to be reasonably sure that nothing comes after and screw them or simply miss them. Modules with such a low weight and integrating with ET are responsible for providing all the required entity info.

I might be mistaken, but I think this is missing a space between the two sentences. In general, I think this sort of string concatenation hurts translatability (no pun intended), but I couldn't think of a nicer way to do this, off the top of my head.

Actually they are two distinct messages so now we just output them as such.

So this code is to support setting a menu_base_path of foo/%bar in the entity info and then have a foo/% menu item in hook_menu()? What is the use-case for that, I don't get it.

We might have a mix of loaders and % placeholders in related paths. See for instance comment_menu().

In relation to the above, it seems if I have reached this code the developer that wrote the entity type is doing something wrong and I have no way to fix it other than to patch the module. As a developer, it is nice to get a handy watchdog, so that I know there is this bug to fix, but I think this falls into the realm of "babysitting broken code".

Well, a module might alter the menu paths so that ET does not work anymore, without the entity-defining module being responsible for it. I'd call this reporting failing conditions, not babysitting code, which incidentally is exactly what logs are about :)

Moreover @catch explicitly asked for this twice, hence I won't present him a third patch missing this part. Obviously he'll be free to consider your argument before committing anything.

I still hope we can remove all that regexing after #1332058: Need a way to distinguish "public/final" URIs for entities from admin and redirected ones and #1783964: Allow entity types to provide menu items but I can live with it for now.

Sure, all that part will probably be revisited once we have an Operation API.

Thanks! Committed/pushed to 8.x.

I'd noticed the html => TRUE change here but failed to put it in a comment. iirc we did indeed add that at some point for performance since check_plain() was running a ridiculous number of times on comment links defined entirely in code (i.e. no user input). Since that's removed from the patch no problem here now.

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