In #1810370: Entity Translation API improvements we agreed that entity translation metadata should be considered regular translatable (non-configurable) fields and as such should be stored in the various data tables. The conversion will need to wait for all the legacy entities to be converted to the Entity Field API and refactored to use the data table for translatable properties.

Files: 
CommentFileSizeAuthor
#57 interdiff-1916790-54-57.txt3.29 KBherom
#57 ct-metadata_fields-1916790-57.patch26.65 KBherom
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,994 pass(es), 54 fail(s), and 19 exception(s).
[ View ]
#54 interdiff-50-54.txt696 bytesYesCT
#54 ct-metadata_fields-1916790-54.patch27 KBYesCT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,822 pass(es), 84 fail(s), and 37 exception(s).
[ View ]
#50 ct-metadata_fields-1916790-50.patch27 KBYesCT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,615 pass(es), 57 fail(s), and 31 exception(s).
[ View ]
#46 interdiff-reroll-1916790-37-46-do-not-test.diff3.91 KBdas-peter
#46 ct-metadata_fields-1916790-46.patch26.88 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 63,142 pass(es).
[ View ]
#37 ct-metadata_fields-1916790-37.patch26.52 KBplach
PASSED: [[SimpleTest]]: [MySQL] 59,287 pass(es).
[ View ]

Comments

Title:Convert translation metadata into regular entity propertiesConvert translation metadata into regular entity fields

Issue tags:+Post NG clean-up

Assigned:Unassigned» plach

Another one I'll try to kick off.

Issue tags:+D8MI, +sprint, +language-content

Issue tags:-sprint

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

It should still be possible, I guess. We have lots of stuff that are still to be declared as fields in the move to NG entities.

Assigned:plach» penyaskito
Status:Postponed» Active

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

Status:Active» Needs work
StatusFileSize
new2.27 KB
FAILED: [[SimpleTest]]: [MySQL] 58,462 pass(es), 14 fail(s), and 14 exception(s).
[ View ]

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.

StatusFileSize
new2.27 KB
FAILED: [[SimpleTest]]: [MySQL] 58,079 pass(es), 14 fail(s), and 14 exception(s).
[ View ]
new752 bytes

Typo, wrong type.

After the entity storge discussion we agreed to proceed with making these regular fields on the entity. For now we will probably keep our current custom storage, but we can proceed with the original plan and make ET metadata regular fields. Sorry for not reporting earlier :(

Assigned:penyaskito» plach
Status:Needs work» Needs review
Issue tags:+API change
StatusFileSize
new14.35 KB
FAILED: [[SimpleTest]]: [MySQL] 58,433 pass(es), 16 fail(s), and 357 exception(s).
[ View ]

Working a bit on this, as we need it in #597236: Add entity caching to core. No test fixed yet.

Status:Needs review» Needs work

The last submitted patch, ct-metadata_fields-1916790-11.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new19.28 KB
FAILED: [[SimpleTest]]: [MySQL] 58,473 pass(es), 16 fail(s), and 357 exception(s).
[ View ]
new4.93 KB

Some test fixes

tagging

Status:Needs review» Needs work

The last submitted patch, ct-metadata_fields-1916790-13.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new24.64 KB
FAILED: [[SimpleTest]]: [MySQL] 59,280 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
new10.72 KB

This one should be better.

Status:Needs review» Needs work

The last submitted patch, ct-metadata_fields-1916790-16.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new24.67 KB
PASSED: [[SimpleTest]]: [MySQL] 59,287 pass(es).
[ View ]
new1.25 KB

This should fix the last failures.

StatusFileSize
new24.7 KB
PASSED: [[SimpleTest]]: [MySQL] 59,412 pass(es).
[ View ]
new1.04 KB

Just a minor fix.

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.

Totally +1 on #20.

I think translation metadata should definitely be part of the entity: this would be consistent with revision metadata (revision_uid, revision_created, ...).

Related discussion: #1807800: Add status and authoring information as generic entity translation metadata.

I was also wondering whether it would make sense to namespace field names.

Issue summary:View changes
Related issues:+#597236: Add entity caching to core

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?

This was the conclusion of #1807800-57: Add status and authoring information as generic entity translation metadata:

There was an IRC meeting today where we decided how to proceed with #1810370: Entity Translation API improvements. With the current proposal entity translation metadata would be regular multilingual entity fields living on the canonical data table.

In this scenario entities (node!) could alter their field definitions to skip any duplicate translation metadata. This way we'd avoid the confusion this issue has been generating so far.

Letting alone the storage, which may still change in the future, what we would be missing is the node alteration- Are we still ok with this plan?

StatusFileSize
new1.04 KB
new24.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ct-metadata_fields-1916790-19.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled

Status:Needs review» Needs work

The last submitted patch, 27: ct-metadata_fields-1916790-19.patch, failed testing.

StatusFileSize
new23.64 KB
PASSED: [[SimpleTest]]: [MySQL] 60,320 pass(es).
[ View ]

Sorry, wrong patch

Status:Needs work» Needs review

Status:Needs review» Needs work

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

<?php
@@ -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

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

<?php
@@ -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?

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

<?php
@@ -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()

Status:Needs work» Needs review
StatusFileSize
new4.84 KB
new26.35 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-metadata_fields-1916790-32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks, here is an updated patch.

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

Yep, totally, a novice issue would be great :)

Why the uid is a special case?

Because it's an entity reference and it needs to be set using the target_id property. I removed the special casing on get, though.

Status:Needs review» Needs work

The last submitted patch, 32: et-metadata_fields-1916790-32.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.41 KB
FAILED: [[SimpleTest]]: [MySQL] 58,096 pass(es), 43 fail(s), and 20 exception(s).
[ View ]

Rerolled

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

StatusFileSize
new4.83 KB
new26.52 KB
PASSED: [[SimpleTest]]: [MySQL] 59,287 pass(es).
[ View ]

Fixed #36 and updated field definitions to the new object format.

The last submitted patch, 35: ct-metadata_fields-1916790-35.patch, failed testing.

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 had a discussion about this with @Berdir in IRC, where in the end we both agreed we need @fago's feedback on #26 and #40.

Here is the full log:

[23:35] plach berdir: hey, I just saw your comment on https://drupal.org/node/1916790#comment-8271235
[23:36] Druplicon https://drupal.org/node/1916790 => Convert translation metadata into regular entity fields [#1916790] => 40 comments, 6 IRC mentions
[23:36] plach I have a couple of questions
[23:36] plach 1) would you recommend the same strategy for revision metadat?
[23:37] plach berdie: because I used revision metadata as a model during the conversion and I think it would be important to get a consistent solution
[23:37] plach berdir: ^
[23:38] plach 2) If we make translation_uid a property would it sill behave as an entity reference? I guess not
[23:38] berdir good question, we support fields now with multiple properties, so we'd be able to rename revision_uid to revision__uid and it would get mapped to revision->uid
[23:38] berdir sure, no problem
[23:39] berdir ok, let's make that it's possible
[23:39] plach is that answering #2?
[23:39] berdir but entity_reference field type is nothing more than target_id + entity
[23:39] berdir you can have that too in your field type
[23:40] plach I think this is overcomplicating things
[23:40] plach those maps to fields in my mind, not as single megafield
[23:40] berdir fields are costly
[23:41] berdir if we move this to extensible storage, it means 10 tables
[23:41] plach berdir: is performance your only concern?
[23:41] berdir a single field is two
[23:41] berdir main concern
[23:41] berdir a single field seems closer to what we have right now?
[23:41] plach berdir: my plan included having the possibility to dynamically add/remove columns to the base table(s)
[23:42] plach we briefly discussed that in Prague
[23:42] plach not sure whether it's in the notes
[23:42] berdir amateescu has similar plans for menu link
[23:42] berdir he wants to combine p0-9 into a single field for example
[23:42] berdir and make 5 fields into a single link field
[23:43] plach berdir: that makes sense
[23:43] plach but let's face it
[23:43] berdir would be great to have fago's opinion
[23:43] plach if they were base fields they would be single fields not a single megafield
[23:43] berdir plach: the think is, you can not move your entity hooks to class methods if you have multiple fields
[23:44] berdir yes but the most important reason for that would be that we didn't support anything else until a few days ago
[23:45] plach berdir: another reason is that we might want them to have separate behaviors once we have widget and formatters for all base fields
[23:45] plach sorry, I mean non-configurable fields
[23:46] berdir plach: if you're strictly against it, so be it. But I'd like to hear what fago thinks
[23:48] plach berdir: I am not *strictly* against it, but I think it won't be easy to convince people to implement their own field definitions when their business logic maps closely to single fields
[23:48] plach it would be better to come up with a solution that allows for that
[23:48] =-= timplunkett is now known as timplunkettAFK
[23:49] plach I realize that might not be easy
[23:49] plach but if it weren't for perfomance I'd never think to a single field for those data
[23:50] plach It feels very unnatural
[23:50] berdir not sure, it didn't to me
[23:50] plach berdir: anyway, we definitely need fago's feedback also to answer https://drupal.org/comment/8151941#comment-8151941
[23:51] plach so (with your permission) I will post this on the issue and assign it to him

Assigned:plach» fago

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.

Assigned:fago» Unassigned

Assigned:Unassigned» das-peter

re-rolling

StatusFileSize
new26.88 KB
PASSED: [[SimpleTest]]: [MySQL] 63,142 pass(es).
[ View ]
new3.91 KB

Re-roll done. Start reviewing.

Assigned:das-peter» Unassigned

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.

StatusFileSize
new27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,615 pass(es), 57 fail(s), and 31 exception(s).
[ View ]

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(

Status:Needs review» Needs work

The last submitted patch, 50: ct-metadata_fields-1916790-50.patch, failed testing.

Assigned:Unassigned» YesCT

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

Status:Needs work» Needs review
StatusFileSize
new27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,822 pass(es), 84 fail(s), and 37 exception(s).
[ View ]
new696 bytes

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.

Status:Needs review» Needs work

The last submitted patch, 54: ct-metadata_fields-1916790-54.patch, failed testing.

Assigned:YesCT» Unassigned

I wont get to this till later.

Status:Needs work» Needs review
StatusFileSize
new26.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,994 pass(es), 54 fail(s), and 19 exception(s).
[ View ]
new3.29 KB

fixing a bit.

Status:Needs review» Needs work

The last submitted patch, 57: ct-metadata_fields-1916790-57.patch, failed testing.

Please be aware that the patch currently an approach that is not what berdir and fago were suggesting. I am not sure I agree with their proposed solution, but probably it would be better to make sure we are all on the same page before working more on this.

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