Entity translation UI in core (part 2)

Quoting @catch:

I don't have time to properly review this at the moment, but Gabor pinged me about it so I've taken a quick look through. Seems like webchick's already looked like this a fair bit.

A few things I noticed that looked a bit odd. The only one that's not minor is the hook_menu()/hook_menu_alter() stuff since I can't understand what it's doing at all. Leaving RTBC so Angie sees it, and we can open follow-ups for this stuff if she's happy committing (I'm not able to see if all her feedback was dealt with at the moment so won't commit this evening).

+++ b/core/modules/comment/comment.moduleundefined
@@ -1009,6 +1009,16 @@ function comment_links(Comment $comment, Node $node) {
+  // Add translations link for translation-enabled comment bundles.
+  if (module_exists('translation_entity') && translation_entity_translate_access($comment)) {
+    $links['comment-translations'] = array(
+      'title' => t('translations'),
+      'href' => 'comment/' . $comment->id() . '/translations',
+      'html' => TRUE,
+    );

I'd expect to see a separate check for whether comments (or the comment bundle most likely) are translation enabled. Why rely on the access callback?

+++ b/core/modules/node/node.jsundefined
@@ -42,6 +42,21 @@ Drupal.behaviors.nodeFieldsetSummaries = {
+
+    $context.find('fieldset.node-translation-options').drupalSetSummary(function (context) {
+      var translate;
+      var $checkbox = $context.find('.form-item-translation-translate input');
+
+      if ($checkbox.size()) {
+        translate = $checkbox.is(':checked') ? Drupal.t('Needs to be updated') : Drupal.t('Does not need to be updated');
+      }
+      else {
+        $checkbox = $context.find('.form-item-translation-retranslate input');
+        translate = $checkbox.is(':checked') ? Drupal.t('Flag other translations as outdated') : Drupal.t('Do not flag other translations as outdated');
+      }
+
+      return translate;
+    });
   }

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

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermTranslationController.phpundefined
@@ -0,0 +1,42 @@
+  function entityFormSave(array $form, array &$form_state) {
+    if ($this->getSourceLangcode($form_state)) {
+      $entity = translation_entity_form_controller($form_state)->getEntity($form_state);
+      // We need a redirect here, otherwise we would get an access denied page
+      // since the curret URL would be preserved and we would try to add a
+      // translation for a language that already has a translation.
+      $form_state['redirect'] = $this->getEditPath($entity);

s/curret/current

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -0,0 +1,702 @@
+/**
+ * Implements hook_menu_alter().
+ */
+function translation_entity_menu_alter(array &$items) {
+  // Some menu loaders in the item paths might have been altered: we need to
+  // replace any menu loader with a plain % to check if base paths are still
+  // compatible.
+  $paths = array();
+  $regex = '|%[^/]+|';
+  foreach ($items as $path => $item) {
+    $path = preg_replace($regex, '%', $path);
+    $paths[$path] = $path;

I don't understand this at all. Are we checking that the translation paths dynamically defined in hook_menu() are still valid after hook_menu_alter() has run? If that's the case, why not just do the dynamic paths in hook_menu_alter() in the first place. Additionally if a contrib module alters a router item, they're expected to fix the fallout if they break stuff, there's no need for core to worry about that.

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -0,0 +1,702 @@
+      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 should be trigger_error(). If an end-user triggers a menu_rebuild() they'll have no idea what this means.

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -0,0 +1,702 @@
+  $result = db_select('translation_entity', 'te')
+    ->fields('te', array())
+    ->condition('te.entity_type', $entity_type)
+    ->condition('te.entity_id', array_keys($entities))

This should be just db_query(), not db_select() - it's not a dynamic query.

Sorry this is so incomplete and spotty, I might be able to look a bit more tomorrow unless I'm beaten to it. Overall I'd really like to see this go in so we can continue towards the removal of Translation module.

Updated issue summary with link to the change dropbutton label issue 1831604

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. :)

And here is me answering one of @catch's points:

I don't understand this at all. Are we checking that the translation paths dynamically defined in hook_menu() are still valid after hook_menu_alter() has run? If that's the case, why not just do the dynamic paths in hook_menu_alter() in the first place. Additionally if a contrib module alters a router item, they're expected to fix the fallout if they break stuff, there's no need for core to worry about that.

Well, this is has been straightly ported from D7. I agree we could avoid worrying about alterations, but declaring items in the menu alter hook caused a lot of pain in D7 because of missing keys added by the menu system only to items returned by hook_menu() implementations. Moreover some module could see the items in place and some other couldn't depending on the hook execution order.

I'd be ok to remove the part messing with menu loaders and just leave the essentials in place, however I think this will be totally revamped by whatever we come up with in #1783964: Allow entity types to provide menu items.

@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)

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.

Well, I introduced it to keep consistency with the entity form title displayed when ET is not enabled: currently every entity has its own variant. A method call there avoids to rewrite the whole form alteration just to alter the title. Honestly I fail to see why the existence of this method is problematic and why we should remove it.
Maybe a UX test will tell us that we need more consistency among entity form titles, at the point the method will be useless.

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

Well, here I was not sure whether showing always the original title or the translated version: the translated value is already available in the form. However it should be just matter of passing the current form langcode to the label() method, although the current Entity Field API has a problem with this because if you ask for value in a language for which there is no translation, it will create an empty value and then EntityInterface::getTranslationLanguages() will say that you have a translation also for the missing translation. Mess :)

This should be fixed in #1810370: Entity Translation API improvements.

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.

Yeah, in D7 we are using module weight + hook_module_implements_alter(). Would that work for you?

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

Do you mean that we should detect through drupal_installation_attempted() whether we are installing a profile and avoid to output the message?

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

What about showing it everytime you enable ET but only if you have only one language enabled?

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

This is a warning IMHO: it's saying "Man, if you don't fix the current situation things won't work".

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.

That's a remaining of the D7 code: already agreed with @catch to remove it.

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?

Not sure I get what you mean, I'm no config expert :)

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...?

I just tried to stick close to what we did in #1739928: Generalize language configuration on content types to apply to terms and other entities. I also read #1783346: Determine how to identify and deploy related sets of configuration but it's not very helpful right now. Basically we need to store config for each entity and bundle and we are prefixing those settings with the entity_type and bundle names.

I'll fix the rest soon along with the stuff identified by @catch.

Updated issue summary.

Added follow-up list

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).

Updated issue summary.

Updated issue summary.

Updated issue summary.

@Bojhan:

Here is the full list.

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).

I'm not offended, simply I created this issue with the goal of addressing the remaining concerns. Keeping posting follow-ups here just makes it hard to stay focused and follow what's happening.

If you feel the need for a meta issue where tracking the improvements let's create one, but I don't think this is the right place. At least let's just update the OP and avoid comment noise.

That said, here is a patch trying to fix everything I was able to address without clarifications.

@sun:

Just a couple of remarks:

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

I was not able to make the checkbox an item since wrapping it breaks the #tree structure and translatability is not magically saved anymore. We would need to introduce a submit handler just to fix this, not sure it's worth. However I used theme_form_element_label() to avoid "futzing with HTML" :)

From #1831608-14: Show or hide the "Make field translatable" checkbox on the add field form depending on translatability (I think it's more relevant here):

It feels very odd that translation_entity module only alters this Field UI form (whereas Field UI is not necessary enabled to begin with), but does not massage field [type] info at all. In fact, this feels a bit like D6-era drupal_execute().

Not sure what you mean: we don't need any field info massaging since 'translatable' is already a property of the field data structure. If you mean there is no code actually updating it, it's just because Field UI takes care of that automatically. Are you suggesting to introduce an API function to switch translatability?

Bot hiccup, I guess. Uploading a new patch fixing a typo:

+++ b/core/modules/translation_entity/translation_entity.module
@@ -173,28 +190,19 @@ function translation_entity_menu() {
+      // for this entity type. Is some case the actual base path might not have

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

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

@catch:

Overall I'd really like to see this go in so we can continue towards the removal of Translation module.

Meanwhile I created #1834250: Hide Content Translation module until we are able to remove it.

@YesCT:

I'd like to create a proper meta issue to track improvements. Would you mind to wait for me to do it?

Add another follow-up

Here is a meta issue: #1836086: [meta] Entity Translation UI improvements.

Updated issue summary.

That should be fixed by #20.

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.

ping?

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

I don't think we need a follow-up for that one, especially after that @catch asked for this change twice :)
Will look into this asap.

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

4bc4fc9 Issue #1831530: Fixed translatable widget order.
051b22e Issue #1831530: Improved comments.
eb0da4b Issue #1831530: Switched from status report to watchdog warning.

Here are some fixes:

91ca116 Issue #1831530: Fixed comment links.
98a290f Issue #1831530: Improved warning on enable.
e07642b Issue #1831530: Improved inline comments.

@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.