Convert translation metadata into regular entity fields

I'm not sure this will even be possible at this point? And menu items are not yet converted.

I'll try this one, I have @plach close for advice.

There is a discussion going on about if we should add the translation fields to the entity or if we should create a "translation_entity" or alike, so we are postponing this one on that discussion.

For now what I have is the fields implementation that are attached and I hope we can reuse that.

Typo, wrong type.

hm, so we need to answer one question first: Should be translation metadata part of the entity?

If yes, then this is a good step into the right direction. But moreover, I think this should move to use extensible entity storage once it is re-usable by modules without having configurable fields. That way it's ensured the entity can be stored in non-sql storage engines (as plach points out above also).

However - if the answer to above question is no, it shouldn't be declared as entity fields, but as separate entities - i.e. translation info entities or so. If you consider the content entity living in mongo and this data in sql, it shows that this really is a separate entity that just happens to hold additional information about another one.

I don't really know the right answer to #20, I'm inclined to agree that the data is related to the entity the same way revisions are...

As for the actual patch, I think it may be confusing to have a content translation provided translation author/created/status, etc. field while an entity may very well have those as translatable properties. So where is the real data then?! I'd assume the status of the translation would be the "translated" status property value. No?

I don't know if its dreditor or my chrome installation, but I hate when dreditor dont paste :___(

@@ -328,7 +391,8 @@ function content_translation_view_access(EntityInterface $entity, $langcode, Acc
   if (!empty($info['permission_granularity']) && $info['permission_granularity'] == 'bundle') {
     $permission = "translate {$entity->bundle()} $entity_type";
   }
-  return !empty($entity->translation[$langcode]['status']) || user_access('translate any entity', $account) || user_access($permission, $account);
+  $status = $entity instanceof ContentEntityInterface ? !empty($entity->getTranslation($langcode)->translation_status->value) : FALSE;
+  return $status || user_access('translate any entity', $account) || user_access($permission, $account);

We should use $account->hasPermission() here

@@ -702,16 +766,26 @@ function content_translation_load_translation_metadata(array $entities, $entity_
+        if ($key != 'uid') {
+          $item->value = $value;
+        }
+        else {
+          $item->target_id = $value;
+        }

@@ -726,14 +800,27 @@ function content_translation_entity_insert(EntityInterface $entity) {
+        $value = $key != 'uid' ? $item->value : $item->target_id;

Why the uid is a special case?

@@ -90,10 +91,9 @@ function content_translation_overview(EntityInterface $entity) {
         // @todo Add a theming function here.
-        $status = '<span class="status">' . $status . '</span>' . (!empty($translation['outdated']) ? ' <span class="marker">' . t('outdated') . '</span>' : '');
+        $status = '<span class="status">' . $status . '</span>' . (!empty($translation->translation_outdated->value) ? ' <span class="marker">' . t('outdated') . '</span>' : '');

This todo should be a issue somewhere, but unrelated to this patch AFAIK.

@@ -255,7 +255,12 @@ public function entityFormAlter(array &$form, array &$form_state, EntityInterfac
         );
       }
 
-      $name = $new_translation ? $GLOBALS['user']->getUsername() : user_load($entity->translation[$form_langcode]['uid'])->getUsername();
+      if ($new_translation) {
+        $name =  $GLOBALS['user']->getUsername();
+      }
+      else {
+        $name = $entity->translation_uid->value ? $entity->translation_uid->entity->getUsername() : '';
+      }

\Drupal::currentUser()

32: et-metadata_fields-1916790-32.patch queued for re-testing.

@plach, you can now use FieldItemInterface::getMainPropertyName() which will return 'target_id' for entity reference fields :)

I can't speak for the rest of the patch as I'm not too familiar with this area but the interdiff in #37 looks good :)

+++ b/core/modules/content_translation/content_translation.module
@@ -139,6 +140,56 @@ function content_translation_entity_field_info_alter(&$info, $entity_type) {
 /**
+ * Implements hook_entity_field_info().
+ */
+function content_translation_entity_field_info($entity_type) {
+  if (content_translation_enabled($entity_type)) {
+    $info = array();
+
+    $info['definitions']['translation_source'] = FieldDefinition::create('language')
+      ->setLabel(t('Translation source'))
+      ->setDescription(t('The source language from which this translation was created.'))
+      ->setTranslatable(TRUE);
+
+
+    $info['definitions']['translation_outdated'] = FieldDefinition::create('boolean')
+      ->setLabel(t('Translation outdated'))
+      ->setDescription(t('A boolean indicating whether this translation needs to be updated.'))
+      ->setTranslatable(TRUE);
+
+    $info['definitions']['translation_uid'] = FieldDefinition::create('entity_reference')
+      ->setLabel(t('Translation author id'))
+      ->setDescription(t('The author of this translation.'))
+      ->setSettings(array(
+        'target_type' => 'user',
+        'default_value' => 0,
+      ))
+      ->setTranslatable(TRUE);
+
+    $info['definitions']['translation_status'] = FieldDefinition::create('boolean')
+      ->setLabel(t('Translation status'))
+      ->setDescription(t('A boolean indicating whether the translation is visible to non-translators.'))
+      ->setSettings(array(
+        'default_value' => 1,
+      ))
+      ->setTranslatable(TRUE);
+
+    $info['definitions']['translation_created'] = FieldDefinition::create('integer')
...
+      ->setDescription(t('The Unix timestamp when the translation was created.'))
+      ->setTranslatable(TRUE);
+
+    $info['definitions']['translation_changed'] = FieldDefinition::create('integer')
+      ->setLabel(t('Translation changed time'))
+      ->setDescription(t('The Unix timestamp when the translation was most recently saved.'))
+      ->setPropertyConstraints('value', array('EntityChanged' => array()))
+      ->setTranslatable(TRUE);
+
+    return $info;
+  }
+}

I think it would be better to make this a single field of a new field type, than then has status, created, changed, source and so on as field item properties.

That's quite a number of classes less that have to be created. And it should make it easy to move the hook_entity_*() implementations to methods of that field type/item class. (except load)

That should also make some of that code quite a bit easier, you should be able to replace those foreach statements with $item/$this->getValue() and setValue().

It should also bring you closer to being able to store this like a configurable field without being one.

Until then, there's still a problem with loading the values, in combination with #597236: Add entity caching to core. Right now, entities coming from the cache need to go through hook_entity_load(), as there's a lot of stuff that's not a field yet. Maybe we can avoid that, I don't know yet. The problem is that there's not really a way to load those values in an efficient way from within the field item class.

I must say that the "megafield" approach seems quite natural too me and does away with all that translation_* prefixed fields, so DX wise I'd see it as a plus.
I like the performance advantages of the megafield as well, but I share the concern of plach regarding formatter and widgets.

This eases doing a common formatter, which formats all of the fields in a good readable summary, it doesn't necessarily make things easier at the widget side. We could still try re-using the separate widgets by implementing a "composite" widget that just comes up with field definitions and forwards them to the individual widgets.

Thus, given the performance implications I'd agree with berdir that a megafield makes sense here. This together with the dx benefits, outweighs the formatting/widget difficulties imho.

Thinking ahead, something like a field-collection-ng would make most sense here imo, i.e. field-collection leveraging regular host-entity field storage, but doing a collection entity which is used for rendering/formatting.
Apart from that this shows us the disadvantages from having Widgets/Formatters bound to the item level when they really are about individual values.

re-rolling

Re-roll done. Start reviewing.

While reviewing the patch I was wondering why we've the table content_translation.
As far as I can tell all the data that are held in this table could reside in the data_table of an entity type too.
The missing fields would be source_language, translation_outdated and maybe translation_status to provide a translation specific status besides the entity status.
Those fields would be mandatory for translation support, but the others e.g. uid could be optional and thus handled similar to the title field right now.

With this the discussion about fields / "megafields" would be obsolete too and as far as I know we could have a widget for all of those fields.
Please tell me if I'm on the total wrong track or if the I could work on changing the approach.

@plach Did @fago's comment in #43 give you want you needed?

What should happen next here?

I reviewed the patch but haven't find any doc related issue.

just rerolling.

nothing really to note (unless it fails).

just for the record:
1.
conflict in NodeTranslationUITest:

<<<<<<< HEAD
      $this->assertEqual($entity->translation[$langcode]['uid'], $values[$langcode]['uid'], 'Translation author correctly stored.');
      $this->assertEqual($entity->translation[$langcode]['created'], $values[$langcode]['created'], 'Translation date correctly stored.');
=======
      $translation = $entity->getTranslation($langcode);
      $this->assertEqual($translation->translation_uid->target_id == $values[$langcode]['uid'], 'Translation author correctly stored.');
      $this->assertEqual($translation->translation_created->value == $values[$langcode]['created'], 'Translation date correctly stored.');
>>>>>>> 46

assertEqual should have separate arguments to compare (not == in the first arg).

so resolved like:

      $translation = $entity->getTranslation($langcode);
      $this->assertEqual($translation->translation_uid->target_id, $values[$langcode]['uid'], 'Translation author correctly stored.');
      $this->assertEqual($translation->translation_created->value, $values[$langcode]['created'], 'Translation date correctly stored.');

2.
conflict in ContentTranslationUITest

<<< HEAD
        $entity = entity_load($this->entityTypeId, $this->entityId, TRUE);
        $this->assertFalse($entity->translation[$enabled_langcode]['outdated'], 'The "outdated" status has been correctly stored.');
=======
        $entity = entity_load($this->entityType, $this->entityId, TRUE);
        $this->assertFalse($entity->getTranslation($enabled_langcode)->translation_outdated->value, 'The "outdated" status has been correctly stored.');
>>>>>>> 46

context like change ->entityTypeId

resolved like:

        $entity = entity_load($this->entityTypeId, $this->entityId, TRUE);
        $this->assertFalse($entity->getTranslation($enabled_langcode)->translation_outdated->value, 'The "outdated" status has been correctly stored.');

3.
then similar:

<<<<<<< HEAD
        $entity = entity_load($this->entityTypeId, $this->entityId, TRUE);
        $this->assertFalse($entity->translation[$langcode]['status'], 'The translation has been correctly unpublished.');
=======
        $entity = entity_load($this->entityType, $this->entityId, TRUE);
        $this->assertFalse($entity->getTranslation($langcode)->translation_status->value, 'The translation has been correctly unpublished.');
>>>>>>> 46

resolved like:

        $entity = entity_load($this->entityTypeId, $this->entityId, TRUE);
        $this->assertFalse($entity->getTranslation($langcode)->translation_status->value, 'The translation has been correctly unpublished.');

4.
in ContentTranslationController

<<<<<<< HEAD
  public function __construct(EntityTypeInterface $entity_type) {
    $this->entityTypeId = $entity_type->id();
    $this->entityType = $entity_type;
=======
  public function __construct($entity_info, AccountInterface $account = NULL) {
    $this->entityType = $entity_info->id();
    $this->entityInfo = $entity_info;
    $this->currentUser = (empty($account)) ? \Drupal::currentUser() : $account;
>>>>>>> 46

resolved like:

    $this->entityTypeId = $entity_type->id();
    $this->entityType = $entity_type;
    $this->currentUser = (empty($account)) ? \Drupal::currentUser() : $account;
 

5.
conflict in content_translation.module

<<<<<<< HEAD
function content_translation_controller($entity_type_id) {
  $entity_type = \Drupal::entityManager()->getDefinition($entity_type_id);
  // @todo Throw an exception if the key is missing.
  $class = $entity_type->getControllerClass('translation');
  return new $class($entity_type);
=======
function content_translation_controller($entity_type, AccountInterface $account = NULL) {
  $entity_info = \Drupal::entityManager()->getDefinition($entity_type);
  // @todo Throw an exception if the key is missing.
  $class = $entity_info->getControllerClass('translation');
  return new $class($entity_info, $account);
>>>>>>> 46

resolved like

function content_translation_controller($entity_type_id, AccountInterface $account = NULL) {
  $entity_type = \Drupal::entityManager()->getDefinition($entity_type_id);
  // @todo Throw an exception if the key is missing.
  $class = $entity_type->getControllerClass('translation');
  return new $class($entity_type, $account);

6.
conflict

<<<<<<< HEAD
    $translation['entity_type'] = $entity->getEntityTypeId();
    $translation['entity_id'] = $entity->id();
    $translation['langcode'] = $langcode;
=======
    $record['entity_type'] = $entity->entityType();
    $record['entity_id'] = $entity->id();
    $record['langcode'] = $langcode;
>>>>>>> 46

resolved like

    $record['entity_type'] = $entity->getEntityTypeId();
    $record['entity_id'] = $entity->id();
    $record['langcode'] = $langcode;

7.
conflict in content_translation.admin.inc

<<<<<<< HEAD
      if ($entity_type->isFieldable()) {
        $fields = $entity_manager->getFieldDefinitions($entity_type_id, $bundle);
=======
      if ($entity_info->isFieldable()) {
        $fields = $entity_manager->getFieldDefinitions($entity_type, $bundle);
        $translation_metadata = content_translation_entity_field_info($entity_type);

>>>>>>> 46

was just adding:

+        $translation_metadata = content_translation_entity_field_info($entity_type);
+

so resolved like:

      if ($entity_type->isFieldable()) {
        $fields = $entity_manager->getFieldDefinitions($entity_type_id, $bundle);
        $translation_metadata = content_translation_entity_field_info($entity_type);

8.
also

<<<<<<< HEAD
            // that support translation can be enabled or disabled.
            elseif (isset($field_settings[$field_name]) || $definition->isTranslatable()) {
              $form['settings'][$entity_type_id][$bundle]['fields'][$field_name] = array(
=======
            // that support translation can be enabled or disabled. We skip
            // our own fields as they are always translatable.
            elseif (isset($field_settings[$field_name]) || (!isset($translation_metadata['definitions'][$field_name]) && $definition->isTranslatable())) {
              $form['settings'][$entity_type][$bundle]['fields'][$field_name] = array(
>>>>>>> 46

resolved like:

            // that support translation can be enabled or disabled. We skip
            // our own fields as they are always translatable.
            elseif (isset($field_settings[$field_name]) || (!isset($translation_metadata['definitions'][$field_name]) && $definition->isTranslatable())) {
              $form['settings'][$entity_type_id][$bundle]['fields'][$field_name] = array(

Fatal error: Call to undefined method Drupal\comment\Entity\Comment::getPropertyDefinitions() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/content_translation/content_translation.module on line 716

Fatal error: Call to undefined method Drupal\entity_test\Entity\EntityTestMul::getPropertyDefinitions() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/content_translation/content_translation.module on line 716

...

I'l try and fix those.

That is getFieldDefinitions now

got distracted with irc. back to this.

ah, #2002134: Move TypedData metadata introspection from data objects to definition objects

huh, only one in the patch. easier than I thought. let's see what the testbot says.

I think there are still some type things.

I wont get to this till later.

fixing a bit.

Yeah...

There are two open questions..

First is if this should be a collection of fields or a single one. Both approaches have their advantages and disadvantages, a single field would require less code to get/set the values but it's unclear how to do deal with widgets then.

The other one is how and what exactly even makes sense. Which is quite a tricky question. See #47. If you look at node, then status/created/changed seems duplicated, and it's very unclear what the difference between the node fields and the translation metadata is, if you do a query, which one should you filter on if you want to see published translations? Same for created/changed...

On the other side, assuming you would translate user fields, for whatever reason, status will not be a translatable field (that would be too crazy ;)), so there a per translation status might be useful again? I'm a bit lost... :)

Also, note that #597236: Add entity caching to core is now capable of dealing with this, fields or not, so from a performance perspective, this doesn't matter much anymore. It still is relevant for #1977266: Fix ContentEntityBase::__get() to not return by reference, though.

Another big question here about language for the fields, that one raised in #2201051: Convert path.module form alters to a field widget
Entity could change its language during submit/save so all fields need a way to update their language somehow

Maybe making a Translation entity with base fields could help with widgets?

I see no big difference between "mega-field-collection" and entity like MenuLink

#597236: Add entity caching to core is RTBC and is about to be committed. It replaces hook_entity_load() with hook_entity_storage_load(), to deal with uncacheable data. Comment statistics is the first use case, this is the second. If #2304939: Stop loading comment statistics into entity object and this issue land, we might be able to remove hook_entity_storage_load(), which would make the API simpler. Unless we want to keep it because there are valid contrib use cases.
In any case, something worth keeping into account when pushing this forward :)

Patch no longer applies and is quite badly out of date... given the open nature of the questions being asked ... I am not sure a reroll is best maybe #57 should be regarded as an interesting place to harvest large code snippets.

The problem with {content_translation} table that it allows to store only integer entity IDs.

I think there's no reason to wait for #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions
Supposed route:
1) Implement special field type "translation" with no_ui setting, configurable field is translatable by default so probably the setting for the field should inherit bundle translatable settings
2) Attach the field dynamically according to translatable entity's status
3) Try to use base field override to proxy the translation field to base field and clean-up after #1498720 is in

Then just ideas:
4) widget could replace language element...
5) field default value and purge needs some love and tests

It's a great step forward, computed/proxy fields are nice .. except status because some entities does not have published status or have a different state model.

otoh having a one field should help with loading the data by one query and caching - the field also could define its schema depending on base entity interfaces

+++ b/core/modules/node/src/NodeTranslationHandler.php
@@ -19,6 +19,35 @@ class NodeTranslationHandler extends ContentTranslationHandler {
+    // TODO
+    $computed_keys = array('uid', 'status', 'created', 'changed');
...
+    $computed_fields = array('uid', 'status', 'created', 'changed');

For uid there's EntityOwnerInterface
for changed - EntityChangedInterface

+++ b/core/modules/content_translation/content_translation.module
@@ -636,14 +661,35 @@ function content_translation_form_field_ui_field_instance_edit_form_alter(array
+ * Implements hook_entity_load().
+ */
+function content_translation_entity_load(array $entities, $entity_type_id) {

You should use hook_entity_storage_load(), so that it is then part of the cached entity.

I am wondering whether it's reasonable to ask a user to manually enable metadata fields for translation, before things start working in the (most commonly) intended way. Might this be an indicator that the translation status and node per-language status should be handled separately? And if not what could be a reasonable UX to make things work smoothly?

But we built the integrated UI to enable all the fields for translatability at once? I don't get how this situation arises?

1. +++ b/core/modules/comment/src/CommentTranslationHandler.php
   @@ -18,8 +19,39 @@ class CommentTranslationHandler extends ContentTranslationHandler {
   +    if (isset($form['content_translation'])) {
   +      // We do not need to show these values on comment forms: they inherit the
   +      // basic node property values.
   +      $form['content_translation']['status']['#access'] = FALSE;
   +      $form['content_translation']['name']['#access'] = FALSE;
   +      $form['content_translation']['created']['#access'] = FALSE;
   

   we can use here field access

2. +++ b/core/modules/comment/src/CommentTranslationHandler.php
   @@ -18,8 +19,39 @@ class CommentTranslationHandler extends ContentTranslationHandler {
   +      $translation['name'] = $entity->getAuthorName();
   +      if ($translation['name'] == \Drupal::config('user.settings')->get('anonymous')) {
   +        $translation['name'] = '';
   

   suppose name is fragile but can;t remember exactly is this set differently for anonymous and authorized. maybe check here is needed... maybe could be moved to boilerplate code for EntityAuthoredInterface

3. +++ b/core/modules/content_translation/content_translation.module
   @@ -438,7 +438,8 @@ function content_translation_language_fallback_candidates_entity_view_alter(&$ca
   -        if ($manager->getTranslationMetadata($entity->getTranslation($langcode))->isPublished()) {
   ...
   +        if (!$metadata->isPublished()) {
   

   good catch

4. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -418,7 +419,7 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
   -    $metadata->setAuthorId(!empty($values['name']) && ($account = user_load_by_name($values['name'])) ? $account->id() : 0);
   +    $metadata->setAuthor(!empty($values['name']) && ($account = user_load_by_name($values['name'])) ? $account : User::load(0));
   

   any reason for the full object?

1. +++ b/core/modules/block_content/src/Tests/BlockContentTranslationUITest.php
   @@ -143,9 +143,8 @@ public function testDisabledBundle() {
   -    $rows = db_query('SELECT * FROM {content_translation}')->fetchAll();
   +    $rows = db_query('SELECT * FROM {block_content_field_data} WHERE id = :id', array(':id' => $enabled_block_content->id()))->fetchAll();
        $this->assertEqual(1, count($rows));
   -    $this->assertEqual($enabled_block_content->id(), reset($rows)->entity_id);
   

   is not equal

2. +++ b/core/modules/content_translation/content_translation.admin.inc
   @@ -336,8 +337,25 @@ function content_translation_form_language_content_settings_submit(array $form,
   +  // @todo Generalize this code in https://www.drupal.org/node/2346013.
   +  // @todo Handle initial values in https://www.drupal.org/node/2346019.
   

   2 todo

3. +++ b/core/modules/content_translation/content_translation.install
   @@ -5,82 +5,10 @@
   -function content_translation_schema() {
   

   yay

4. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
   @@ -19,6 +21,30 @@
   +  protected $manager;
   

   contentTranslationManager better

5. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
   @@ -46,9 +72,11 @@ public function prepareTranslation(ContentEntityInterface $entity, LanguageInter
   +    $manager = $this->manager;
   

   the same

6. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
   @@ -145,13 +176,12 @@ public function overview(Request $request, $entity_type_id = NULL) {
   +              'status' => $metadata->isPublished(),
   

   is there a actual status? this could be used for "draft"

7. +++ b/core/modules/content_translation/src/Tests/ContentTranslationTestBase.php
   @@ -79,6 +79,11 @@
   +  protected $manager;
   

   yep

8. +++ b/core/modules/taxonomy/src/Tests/TermTranslationUITest.php
   @@ -95,9 +95,9 @@ public function testTranslationUI() {
   +    $rows = db_query('SELECT tid, count(tid) AS count FROM {taxonomy_term_field_data} WHERE vid <> :vid GROUP BY tid', array(':vid' => $this->bundle))->fetchAll();
   

   where ... +and CTfield column = some data

rtbc by proxy, 3am need for sleep took it's toll

The polish in #93 is just 2am

Discussed this with Plach and Alex Pott today. There's no contrib module-facing API change here (unless you do very specific things with translation metadata form elements similar to the comment hunks in the patch, which isn't going to affect most modules).

So making this beta target/D8 upgrade path. I don't think I'd want to do it once we're supporting the upgrade path though so it'd have to go in before that.

The @todo with the try/catch looks like it'd be fixed by #2232477: Fatal when adding new fields with NOT NULL constraints in a base table that contains existing entities. That bug isn't introduced here but would be happier committing here with that issue fixed and no @todo if we can.

Did a first review since a while - remarks/questions below:

1. +++ b/core/modules/content_translation/content_translation.module
   @@ -141,6 +155,25 @@ function content_translation_entity_bundle_info_alter(&$bundles) {
   +    $definitions = $manager->getEntityTypeTranslationMetadata($entity_type)->getFieldDefinitions();
   +    $installed_storage_definitions = \Drupal::entityManager()->getLastInstalledFieldStorageDefinitions($entity_type_id);
   

   That's strange, why does it only return already installed definitions. How can those be ever added then?

2. +++ b/core/modules/content_translation/src/ContentTranslationManager.php
   @@ -34,6 +36,23 @@ public function __construct(EntityManagerInterface $manager) {
   +    $metadata = clone $this->getEntityTypeTranslationMetadata($translation->getEntityType());
   +    $metadata->setTranslation($translation);
   

   I do not fully get this. So this a handler which has an instance per translation?

3. +++ b/core/modules/content_translation/src/ContentTranslationManagerInterface.php
   @@ -31,4 +34,26 @@ public function getSupportedEntityTypes();
   +   * Content translation metadata factory.
   ...
   +  public function getEntityTypeTranslationMetadata(EntityTypeInterface $entity_type);
   

   If this is a factory, shouldn't it be called create something ?

4. +++ b/core/modules/content_translation/src/ContentTranslationManagerInterface.php
   @@ -31,4 +34,26 @@ public function getSupportedEntityTypes();
   +   * Content translation metadata factory.
   

   This method has the same summary as the one before - but it seems to be different?

5. +++ b/core/modules/content_translation/src/ContentTranslationMetadata.php
   @@ -0,0 +1,251 @@
   +    if (!$this->supportsNativePublishedStatus()) {
   

   What is a native published status vs non native? Both sits on the entity objects, so both are "native" imo. Why not just call all those methods "hasWHATEVER" ?

6. +++ b/core/modules/content_translation/src/ContentTranslationMetadataInterface.php
   @@ -0,0 +1,141 @@
   + * Common interface for content translation metadata handlers.
   ...
   +interface ContentTranslationMetadataInterface extends EntityChangedInterface {
   

   This is talking about handlers but then the interface is nothing about handlers ? Also, if it's just adding some additional logic that is not customized by entity type it probably shouldn't be a handler, but a service.

7. +++ b/core/modules/user/src/Entity/User.php
   @@ -35,7 +35,8 @@
   + *     "content_translation_metadata" = "Drupal\user\ProfileTranslationMetadata"
   

   ?!? Why does it talk about profiles if it is about users? We already have "account" and "user", please do not start using another noun for it ;)

1: Installed definitions are taken into account only when no bundle is enabled for translation: this allows to keep field data around if a bundle is disabled and then enabled again. See the updated comment in the interdiff.

ah, I see. I had to look at the code though to understand the flow - it's actually not returning last installed definitions at all - but just determines whether to add/return the metadata fields. So maybe the comment could say something like

We return metadata storage fields whenever content translation is enabled or it was enabled before, such that we keep translation metadata around when translation is disabled.

Still not sure about the handler as wrapper thing. Let's discuss it with berdir in person? :)

Thanks! I took a look at the changes. Here some remarks:

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1093,7 +1093,15 @@ protected function createSharedTableSchema(FieldStorageDefinitionInterface $stor
   +                //   See https://www.drupal.org/node/2347301.
   +                watchdog_exception('php', $e);
   

   Should link to https://www.drupal.org/node/2232477. Also, we do postpone committing this one on the mentioned issue, right?

2. +++ b/core/modules/content_translation/content_translation.module
   @@ -117,8 +117,8 @@ function content_translation_entity_type_alter(array &$entity_types) {
   +      if (!$entity_type->get('content_translation_metadata')) {
   +        $entity_type->set('content_translation_metadata', 'Drupal\content_translation\ContentTranslationMetadata');
   

   As discussed, setting the default can/should happen in hook_entity_type_build()

3. +++ b/core/modules/content_translation/src/ContentTranslationManagerInterface.php
   @@ -35,15 +35,15 @@ public function getSupportedEntityTypes();
   +   * Content translation controller factory.
   

   handler

4. +++ b/core/modules/content_translation/src/ContentTranslationManagerInterface.php
   @@ -35,15 +35,15 @@ public function getSupportedEntityTypes();
   +   *   An instance of the content translation controller interface.
   

   handler

5. +++ b/core/modules/content_translation/src/ContentTranslationMetadata.php
   @@ -21,150 +16,53 @@
    class ContentTranslationMetadata implements ContentTranslationMetadataInterface {
   

   Should be ContentTranslationMetadataWrapper (and same for the interface) such that the name already reflects it's wrapping it?

6. +++ b/core/modules/content_translation/src/ContentTranslationMetadata.php
   @@ -21,150 +16,53 @@
   +   * The field definition names for the wrapped entity translation.
   

   Should clarify it's about the metadata fields only.

7. +++ b/core/modules/content_translation/src/ContentTranslationMetadata.php
   @@ -187,18 +85,18 @@ public function setOutdated($outdated) {
   +    return isset($this->fieldDefinitionNames['content_translation_uid']) ? $this->translation->get('content_translation_uid')->entity : $this->translation->getOwner();
   

   Should use $translation->hasField() as it's better readable. We can do away with $this->fieldDefinitionNames then I think.

8. +++ b/core/modules/user/src/Entity/User.php
   @@ -35,8 +35,7 @@
   + *     "translation" = "Drupal\user\ProfileTranslationHandler"
   

   Should be user translation handler.

9. +++ b/core/modules/user/src/ProfileTranslationHandler.php
   @@ -19,6 +19,22 @@ class ProfileTranslationHandler extends ContentTranslationHandler {
   +    // User status has nothing to do with translations visibility.
   +    return FALSE;
   

   Not sure about that. Blocked users are not accessible aka published either?

2: I don't agree with that, sorry: hook_entity_type_build() should be implemented by methods implementing the CT API, this is the only way to ensure CT enters a value only when an explicit one is not defined.

So the way I understood our discussion was that CT provides the default via the build, and modules can alter the default. But yeah, it seems to be better to have the modules implement the build and leave the alter to other modules that need to alter.

Thinking about it, IMO there's no need to expose that implementation detail in the interface name

Imo, this is no implementation detail, but a fact that every dev making use of the interface has to understand - so have it in the name helps by clearly communicating it.

8: That would be an unnecessary API change, that class has not been introduced here.

I see, missed that.

That would mean tying the visibility of a biography field with the ability of a user to log in, that would not make much sense to me.

That's already the case if you look at UserAccessControlHandler:

      case 'view':
        // Only allow view access if the account is active.
        if ($account->hasPermission('access user profiles') && $entity->isActive()) {
          return AccessResult::allowed()->cachePerRole()->cacheUntilEntityChanges($entity);
        }

I see, makes sense.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1092,7 +1093,15 @@ protected function createSharedTableSchema(FieldStorageDefinitionInterface $stor
+              catch (DatabaseExceptionWrapper $e) {
+                // @todo In some cases a "Data truncated for column" exception
+                //   is thrown, but thing seem to work properly nonetheless.
+                //   See https://www.drupal.org/node/2347301.
+                watchdog_exception('php', $e);

Looked at the interdiff and patch once again - looks both solid to me. Given we can remove this with #2232477 being fixed, this should be ready as well. (Not setting RTBC for now because of that only).

A very minor issue.

+++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
@@ -19,6 +21,30 @@
+  protected $manager;

+++ b/core/modules/content_translation/src/Tests/ContentTranslationTestBase.php
@@ -79,6 +79,11 @@
+  protected $manager;

Can we rename this to contentTranslationManager?

Great to see it working without the hack, RTBC then.

Thanks for the reroll. Back to RTBC.

Configuration UX: the current patch makes the UX of enabling translation on an entity type for the first time slightly worse as updates need to be run before things actually work. I think we should add a method on the defintion update manager to apply changes only pertaining a single provider (module). This would allow CT to update the schema automatically. I think this particular case does not need a manual confirmation by the user, unless a huge number of entity is already existing, which could make adding columns a very long operation. Since I feel this could deserve some discussion to figure out the best solution + API, I'd open a separate (non beta-bound) issue and restore CT's original UX once we fix that.

So we need a test which makes an entity type translatable through the config importer. At the very least if that occurs we need to inform the user that db updates needs to be performed. Maybe even better would be to actually run the updates.