Problem/Motivation

  1. We need a reliable way to tell whether an entity type can be multilingual: a non-multilingual entity will never have multilingual support. For instance its SQL storage controller won't create a data table. This will be useful for ET to determine which bundles can be enabled for translation (see the next item). We could even provide simplified controllers based on this.
  2. We need a reliable way to tell whether a bundle is translatable or not.
  3. We should not require ET to be enabled to retrieve the information above.
  4. We should avoid calling entity_get_bundles() from translation_entity_entity_info_alter().

Proposed resolution

  • Add a 'translatable' boolean entity key.
  • Add a 'translatable' entity key to the bundle info and add also a helper method EntityInterface::isTranslatable() so that the key does not need to be actually populated.
  • Stop calling entity_get_bundles() while altering the entity info: ET should create the translation overview route for each translatable entity type, no matter if it has any bundle enabled for translation yet. The tab visibility should be determined by access control as usual.

Remaining tasks

  • Discuss whether entities should default to translatable or not
  • RTBC
  • Determine a strategy to backport this
  • Write a patch
  • Review, test, RTBC

User interface changes

None

API changes

  • The field translation handler concept has been removed in favor of a 'translatable' flag in the entity info.
  • A new 'translatable' flag has been added to the bundle info, determining whether the bundle is enabled for translation.

Follow-ups

#1945348: Call decorated storage methods in ViewsUI explicitly

Original report by Dave Reid

For example, if I enable content translation (either via translation.module or a contrib module) for article node types but not page nodes, those modules should be altering hook_entity_info() for $info['node']['bundles']['article'] to indicate that translating is enabled.

Something like:

/**
 * Implements hook_entity_info_alter().
 */
function translation_entity_info_alter(&$info) {
  foreach ($info['node']['bundles'] as $type => &$bundle) {
    if (translation_supported_type($type)) {
      // Mark that translation.module is providing localization for this content type.
      $bundle['translation']['translation'] = TRUE;
    }
  }
}

Would also be nice to have a isTranslatable() method in the default Entity class that we can call as well (and an entity_is_translatable($entity_type, $entity) API addition for D7).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Issue tags: +pathauto

This would assist the Pathauto contrib project help provide URL aliasing in an entity-generic fashion.

YesCT’s picture

We might have an api like that now...

Maybe it was added on the way to #1810386: Create workflow to setup multilingual for entity types, bundles and fields, or that might show it in use as an example that would be useful.

plach’s picture

Yes, we totally need something like this, also to remove lots of crufty module_exists() spread around core atm. Will try to work on this as soon as I have a minute.

plach’s picture

Coming from #111715-90: Convert node/content types into configuration.

@Berdir:

E.g, should the whole key be moved to a separate API/Hook or should we just move the translation_entity specific stuff into a separate, specific hook (which would have the advantage of being able to properly document it within translation_entity.api.php)

Yes, this sounds as a good pro. Do you think we should follow the entity_get_info() pattern and cache the result of a hook_translation_entity_info() + alter invocation?

I don't even really understand why we need to support multiple things there, given that translation_entity is now in core and is *the* way to handle entity/field translations? Why would you not use it?

This was introduced as a of way to lock out field translatablity when we weren't sure what ET would look like in D7 and whether it would actually work. In D8 I see no real point in providing a way to declare multiple field translation handlers. AAMOF I already planned to remove all that cruft. OTOH that was a cool way for modules to avoid explicitly checking for the presence of ET. I guess we need to retain some level of information at entity system level, that is something that can be reliably checked even when ET is disabled.

sun’s picture

plach’s picture

Issue tags: +API clean-up

I was thinking about a plan to address this.

Goals

  1. We need a reliable way to tell whether an entity type can be multilingual. With this I mean that a non-multilingual entity will never have multilingual support. For instance its SQL storage controller won't create a data table. This will be useful for ET to determine which bundles can be enabled for translation (see the next item). We could even provide simplified controllers based on this.
  2. We need a reliable way to tell whether a bundle is translatable or not.
  3. We should not require ET to be enabled to retrieve the information above.
  4. We should avoid calling entity_get_info() from translation_entity_entity_info_alter().

Proposed solution

  • Add a 'multilingual support' boolean entity key.
  • Add a 'translatable' entity key holding an array of boolean values keyed by bundle machine name. We can add also a helper method EntityInterface::isTranslatable() so that the key does not need to be actually populated.
  • Move all the other entity info needed by ET to a separate info hook, e.g. hook_entity_translation_info() + alter.

If we go this way we can remove all the field translation handler cruft from the entity info, that is the 'translation' key. As a first step, while we are not done with the Entity Field API conversion yet, we can just make field_is_translatable() depend on the 'translatable' entity info key. Then we can just drop that part and simply check the 'translatable' field info key.

Thoughts?

tstoeckler’s picture

Instead of a translatable key on the entity info that is an array keyed by bundle names, I think we should be using the bundle info directly. Specifically, I think we should add a "translatable" key to hook_entity_bundle_info(). We could make the EntityManager be smart and make the default of that key depend on the "multilingual support" key of the entity (altough A. dependence of the bundle info on the entity info is exactly what hook_entity_bundle_info() tries to avoid, so that might actually do more harm than good and B. shouldn't that key be called "translation Support" for consistency (or the bundle key be called "multilingual")?).

plach’s picture

Instead of a translatable key on the entity info that is an array keyed by bundle names, I think we should be using the bundle info directly.

This solution sounds sensible to me but how would we deal with bundless (tm) entities, such as User?

We could make the EntityManager be smart and make the default of that key depend on the "multilingual support" key of the entity.

Not sure about this: it would mean the by default nodes would be translatable on any D8 site, but since ET is not enabled in the standard profile, this would be quite inconsistent. Actually in my mind the key would be populated by ET by implementing hook_entity_info_alter() and checking its config values.

shouldn't that key be called "translation Support" for consistency (or the bundle key be called "multilingual")?).

Yep, "multilingual" sounds better to my ears, but all the Entity Translation API right now is using the "Translation" term hence "Translation support" is probably better, although it may be unclear which is the difference between the two keys.

tstoeckler’s picture

This solution sounds sensible to me but how would we deal with bundless (tm) entities, such as User?

Well "bundless" :-) entities get a default bundle that is named after the entity type. So I think if the entity type has declared multilingual => TRUE (as the top-level entity type property) it would make sense to make the bundle translatable. If multitlingual => FALSE we don't make it translatable. Thoughts?

plach’s picture

Well "bundless" :-) entities get a default bundle that is named after the entity type.

Yes, this just works :)

So I think if the entity type has declared multilingual => TRUE (as the top-level entity type property) it would make sense to make the bundle translatable. If multitlingual => FALSE we don't make it translatable. Thoughts?

As I was saying above I think this may cause an inconsistent behavior: unless I'm missing something, if we go this way, as soon as you install the standard profile and call $node->isTranslatable() you will get TRUE. But this would clash with the fact that ET is not enabled. Instead if we default to FALSE and alter the bundle key based on the ET config settings, we will have that the bundle is marked as translatable exactly when ET will allow to translate it.

plach’s picture

Issue tags: +sprint
plach’s picture

Assigned: Unassigned » plach
Issue tags: +language-content

Going to work on this tonight.

plach’s picture

Assigned: plach » Unassigned
Status: Active » Needs review
FileSize
19.33 KB

Here is patch implementing #6. The only thing I left out is migrating the entity translation info to a dedicated info hook, since it is likely to disappear altogether when #1839516: Introduce entity operation providers is done. However I removed the entity_get_bundles() call from the translation_entity_entity_info_alter(), which should unblock #111715: Convert node/content types into configuration.

Status: Needs review » Needs work

The last submitted patch, et-translatable_bundle-1446382-13.patch, failed testing.

plach’s picture

This one should fix most failures.

Status: Needs review » Needs work

The last submitted patch, et-translatable_bundle-1446382-15.patch, failed testing.

plach’s picture

Now tests should pass.

Berdir’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.phpundefined
@@ -30,6 +30,7 @@
  *   fieldable = TRUE,
+ *   translatable = TRUE,

It's a bit unfortunate that this needs to be set explicitly everywhere now. Can we maybe default to TRUE if it is fieldable or something like that...

Otherwise, this looks great to me.

plach’s picture

Issue summary: View changes

Added issue summary

plach’s picture

@Berdir:

Can we maybe default to TRUE if it is fieldable or something like that...

Not sure about this, the translatable flag might have heavy implications, for instance it might trigger the creation of the data table. Will ask @Gabor to chime in.

Edit: Added an issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

@Berdir: it does not need to be set everywhere now. For example, this patch makes contact content entities non-translatable as well I believe, which is a great side-fix for the problem that they show up for multilingual configuration, since you cannot currently tell if an entity can be configured for translation. I agree with @plach that it is best to leave the decision possibility to entity implementations, sites using their own entity types should get the possibility to opt out of translation support for their entities even if they use fields.

plach’s picture

this patch makes contact content entities non-translatable as well I believe

Of course :)

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

I did a standards and comment review and the code looks clean.

In the issue summary, there is a remaining task: Discuss whether entities should default to translatable or not

This patch sets
translatable = TRUE
in 7 plugins

and translatable = FALSE
in none.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Core/Entity/CustomBlock.phpundefined
@@ -31,6 +31,7 @@
+ *   translatable = TRUE,

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.phpundefined
@@ -29,6 +29,7 @@
+ *   translatable = TRUE,

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/Node.phpundefined
@@ -30,6 +30,7 @@
+ *   translatable = TRUE,

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Plugin/Core/Entity/EntityTestMul.phpundefined
@@ -27,6 +27,7 @@
+ *   translatable = TRUE,

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Plugin/Core/Entity/EntityTestMulRev.phpundefined
@@ -28,6 +28,7 @@
+ *   translatable = TRUE,

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.phpundefined
@@ -30,6 +30,7 @@
+ *   translatable = TRUE,

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.phpundefined
@@ -31,6 +31,7 @@
+ *   translatable = TRUE,

CustomBlock
Comment
Node
(Test Entity)
(Test Entity Revisions)
Term
User

Next steps

What questions do we still need to ask and answer to finish off this issue? (re: Discuss whether entities should default to translatable or not)
It sounds like this patch is proposing that by default entities *should* default to explicitly stating either translatable or not.
Do we need to ask which entities or which bundles should default to translatable?

After writing this comment, and re-reading the question, the issue summary and the patch, I change my mind. I think this patch makes translatable false for everything by default, and sets true for some (see the plugin list). And I think the discussion has taken place and the decision is that entities should default to not translatable unless explicitly set to true because we cannot make an algorithmic guess that things should be translatable. See #19.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/ConfigTestTranslationUITest.phpundefined
@@ -35,6 +35,14 @@ function setUp() {
+  protected function enableTranslation() {
+    $this->container->get('state')->set('config_test.translatable', TRUE);
+    parent::enableTranslation();
+  }

here is a way to enableTranslation (in a test)

--

the point?

this function is for the content translation module to determine if a bundle is translatable? And also this patch makes a way for config to store that?

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -445,26 +442,26 @@ function translation_entity_set_config($entity_type, $bundle, $setting, $value)
+function translation_entity_enabled($entity_type, $bundle = NULL) {

returns TRUE if an bundle is translatable, or if any bundle in an entity is translatable.

But I dont see where this patch is setting defaults, for example #21

+++ b/core/includes/entity.api.phpundefined
@@ -107,6 +107,8 @@ function hook_entity_view_mode_info_alter(&$view_modes) {
+ *   - translatable: (optional) A boolean value specifying whether this bundle
+ *     has translation support enabled. Defaults to FALSE.

why a special case for entity view mode?

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -440,4 +440,14 @@ public function setContext($name = NULL, ContextAwareInterface $parent = NULL) {
+  public function isTranslatable() {

this method is the point? and this works without content translation module being enabled?

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -75,11 +75,8 @@
+ * - translatable: (optional) Boolean indicating whether entities of this type
+ *   have mutlilingual support. Defaults to FALSE.

Ah, ha. So everything else is false, other than the stuff in here where the plugin is set to true?
This seems to be describing that, but I dont see in the code where it is done.

Summary

AFAIK, the remaining tasks are done. And my questions are non-blocking.
And a change notice with examples will help clarify things.

Berdir’s picture

Agreed, this looks good to me as well, let's get it in so that we can unblock the node type conversion.

xjm’s picture

Priority: Normal » Critical

If this blocks the node type conversion it should be the same priority we just bumped that one to. (Haven't reviewed the patch, just bumping it.)

YesCT’s picture

Priority: Critical » Normal

I think #111715: Convert node/content types into configuration is what @Berdir mentioned is waiting on this issue

YesCT’s picture

Priority: Normal » Critical
xjm’s picture

+++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewUI.phpundefined
@@ -985,6 +985,13 @@ public function getOriginalEntity() {
+    return $this->__call(__FUNCTION__, func_get_args());

Uh. What? Magic! Comment on this line might be good.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Let's get one more round of updates here for the most recent comments.

Also, I'm not sure this is backportable to D7..?

Berdir’s picture

@xjm: That's the same pattern as dozens of existing methods in that class, I don't think it needs a special comment.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

As per #29 and above :)

xjm’s picture

@xjm: That's the same pattern as dozens of existing methods in that class, I don't think it needs a special comment.

It needs something. :P We just spent 15 minutes in IRC debating WTF it does. Maybe a followup for the whole class?

There's a reason I am NOT in MAINTAINERS.txt for Views UI. ;)

Berdir’s picture

Follow-up sounds good to me. This patch just applies an existing pattern in that class.

What it does it pass the call through to __call() which forwards any method call to $this->storage.

What I would suggest is to call $this->storage directly instead of doing that indirection which just slows things down. The same way that EntityBCDecorator does it, which means simply $this->storage->isTranslatable() in this case or $this->storage->setNewRevision($value) for an example with arguments. The additional human-overhead of doing this once is not big but it helps both with performance and it's better to understand (which is not often the case :)). You had no trouble to understand the same method in EntityBCDecorator I assume?

tim.plunkett’s picture

That approach is from http://stackoverflow.com/a/4966752/614351, it ties in with the standard __call() pattern used in all decorators in core:

  /**  
   * Passes through all unknown calls onto the storage object.
   */
  public function __call($method, $args) {
    return call_user_func_array(array($this->storage, $method), $args);
  }

Where $this->storage is the decorated object.

YesCT’s picture

in #22 I asked/wondered

But I dont see where this patch is setting defaults

and

This seems to be describing that, but I dont see in the code where it is done.

So here I answer my own question:

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -87,39 +88,37 @@ function translation_entity_entity_info_alter(array &$entity_info) {
+/**
+ * Implements hook_entity_bundle_info_alter().
+ */
+function translation_entity_entity_bundle_info_alter(&$bundles) {
+  foreach ($bundles as $entity_type => &$info) {
+    foreach ($info as $bundle => &$bundle_info) {
+      $enabled = translation_entity_get_config($entity_type, $bundle, 'enabled');
+      $bundle_info['translatable'] = !empty($enabled);
     }
   }
 }

so this reads it out of config, and if it's missing or false, then translatable is false. If it's true in config, then it's true, and then the array described in

*   - translatable: (optional) A boolean value specifying whether this bundle
 *     has translation support enabled. Defaults to FALSE.
 *
 * @see entity_get_bundles()
 * @see hook_entity_bundle_info_alter()
 */
function hook_entity_bundle_info() {

gets it's translatable value. (which is false unless the plugin set it to be true)
the documentation that there is a translatable element in the array is in the hook.

but would that happen only when the content translation module (translation_entity) module is enabled?
is $enabled if the bundle is enabled, or if translation is enabled on the bundle?

YesCT’s picture

YesCT’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
24.75 KB
2.09 KB

@webchick:

Also, I'm not sure this is backportable to D7..?

The current solution is certainly not backportable. Not sure whether the original one proposed by @Dave Reid is.

@YesCT:

[...] gets it's translatable value. (which is false unless the plugin set it to be true)
the documentation that there is a translatable element in the array is in the hook.
but would that happen only when the content translation module (translation_entity) module is enabled?
is $enabled if the bundle is enabled, or if translation is enabled on the bundle?

Not sure I understand the question, but I have the feeling there is a bit of confusion about the implemented solution: we have two keys:

  • An entity-level 'translatable' key in the entity plugin info, which states whether the entity type supports translation or is inherently language-unaware (the latter is the default).
  • A bundle-level 'translatable' key in the bundle info, which states whether that specific bundle has translation support enabled (by default it doesn't). As you pointed out above, ET alters the bundle info based on its configuration. If it is disabled other modules providing translation support could kick in and say whether a bundle is translatable or not.

I'm attaching a new patch, since I realized some field documentation was not updated. As a bonus I added a small clarification on the config_test changes.

xjm’s picture

Yep, still RTBC assuming it goes green.

webchick’s picture

Title: Need a reliable way to determine if a specific bundle for an entity type is translatable » Change notice: Need a reliable way to determine if a specific bundle for an entity type is translatable
Status: Needs review » Active
Issue tags: +Needs change record

Ok, looks good. About to board a plane, so hopefully this will work without any dramas. ;)

Committed and pushed to 8.x. Thanks!

This needs a change notice methinks.

plach’s picture

Title: Change notice: Need a reliable way to determine if a specific bundle for an entity type is translatable » Need a reliable way to determine if a specific bundle for an entity type is translatable
Version: 8.x-dev » 7.x-dev
Priority: Critical » Normal
Issue tags: -API clean-up, -Needs change record, -sprint +API addition

Here it is: http://drupal.org/node/1945906.

Moving to the D7 queue and demoting back to normal. Leaving the status to active since we don't have a backportable patch.

plach’s picture

Issue summary: View changes

added follow-up

Gábor Hojtsy’s picture

(Change notice looks good to me.)

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

  • webchick committed 1bec763 on 8.3.x
    Issue #1446382 by plach: Need a reliable way to determine if a specific...

  • webchick committed 1bec763 on 8.3.x
    Issue #1446382 by plach: Need a reliable way to determine if a specific...

  • webchick committed 1bec763 on 8.4.x
    Issue #1446382 by plach: Need a reliable way to determine if a specific...

  • webchick committed 1bec763 on 8.4.x
    Issue #1446382 by plach: Need a reliable way to determine if a specific...