Problem/Motivation

The Content Translation module implements most of its business logic based on translation metadata such as the source language, the translation publication status, the translation authoring information and so on. Currently these metadata items are stored in a table holding values for any entity type and they are not exposed through regular field definitions.

This has several drawbacks:

  • Metadata storage is not swappable, as SQL storage is hard-coded.
  • Metadata items do not have access to all the benefits of being fields, such as automatic REST or Rules integration.
  • Some information is duplicated because there's currently no way to have entity-type-specific customizations.
  • The current implementation is deprecated and is an example of how things should NOT be done in Drupal 8.

Proposed resolution

Introduce a wrapper class dealing with metadata and use the Content Translation handler to expose field definitions for each metadata item. This entity translation wrapper provides methods to access the various metadata values. This integrates pretty well with our regular entity-type-specific interfaces and could even serve as a pattern for modules adding new base fields. This approach allows to provide an alternative set of field definitions for each entity type and retrieve field values accordingly or even implement more complex logic on top of those. For instance node translation metadata is mapped this way:

  • Translation source -> New node base (translatable) field
  • Translation outdated status -> New node base (translatable) field
  • Translation published status -> Node published status (translatable field)
  • Translation author -> Node author (translatable field)
  • Translation creation time -> Node creation time (translatable field)
  • Translation modification time -> Node modification time (translatable field)

The current custom storage is replaced by our regular entity storage.

Remaining tasks

Reviews

User interface changes

Like we already do for nodes, widgets for the translation name, status and created on the comment form were hidden, as the native ones are used.

API changes

Data access for metadata items has changed from a custom entity property to regular field access. Actual access is performed by the translation metadata wrapper.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because the functionality already exists and is not broken, although implemented through D7-style code.
Issue priority Major because this provides the first actual usage in core of new the Entity Storage Update API. Not critical because we could release Drupal 8 with the current code without introducing functional problems.
Prioritized changes The main goal of this issue is reducing fragility by providing the first core implementation of a new core API and serving as an example for contrib module authors. This was specifically approved by a branch maintainer in #98.
Disruption Disruptive for contributed and custom modules doing very specific things with content translation metadata, which should be a fairly small number.
Files: 
CommentFileSizeAuthor
#152 interdiff.txt3.45 KBGábor Hojtsy
#152 ct-metadata_fields-1916790-152.patch77.74 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,526 pass(es).
[ View ]
#149 ct-metadata_fields-1916790-149.patch76.54 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,461 pass(es).
[ View ]
#146 ct-metadata_fields-1916790-146.patch76.3 KBplach
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch ct-metadata_fields-1916790-146.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

plach’s picture

Title:Convert translation metadata into regular entity properties» Convert translation metadata into regular entity fields
plach’s picture

Issue tags:+Post NG clean-up
plach’s picture

Assigned:Unassigned» plach

Another one I'll try to kick off.

plach’s picture

Issue tags:+D8MI, +sprint, +language-content
Gábor Hojtsy’s picture

Issue tags:-sprint

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

plach’s picture

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.

penyaskito’s picture

Assigned:plach» penyaskito
Status:Postponed» Active

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

penyaskito’s picture

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.

penyaskito’s picture

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

Typo, wrong type.

plach’s picture

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 :(

plach’s picture

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.

plach’s picture

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

plach’s picture

tagging

Status:Needs review» Needs work

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

plach’s picture

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.

plach’s picture

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.

plach’s picture

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

Just a minor fix.

fago’s picture

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.

plach’s picture

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.

plach’s picture

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

plach’s picture

Issue summary:View changes
Related issues:+#597236: Add entity caching to core
plach’s picture

Gábor Hojtsy’s picture

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?

plach’s picture

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?

plach’s picture

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.

plach’s picture

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

Sorry, wrong patch

plach’s picture

Status:Needs work» Needs review
penyaskito’s picture

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

plach’s picture

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.

aspilicious’s picture

Status:Needs review» Needs work

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

plach’s picture

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

amateescu’s picture

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

plach’s picture

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.

amateescu’s picture

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

Berdir’s picture

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

plach’s picture

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
plach’s picture

Assigned:plach» fago
fago’s picture

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.

fago’s picture

Assigned:fago» Unassigned
das-peter’s picture

Assigned:Unassigned» das-peter

re-rolling

das-peter’s picture

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

Re-roll done. Start reviewing.

das-peter’s picture

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.

YesCT’s picture

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

What should happen next here?

jibran’s picture

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

YesCT’s picture

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.

YesCT’s picture

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.

Berdir’s picture

That is getFieldDefinitions now

YesCT’s picture

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.

YesCT’s picture

Assigned:YesCT» Unassigned

I wont get to this till later.

herom’s picture

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.

plach’s picture

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.

Berdir’s picture

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.

andypost’s picture

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

andypost’s picture

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

plach’s picture

Issue tags:+beta target

Just spoke with @xjm about this and she suggested me to mark this as beta target.

Wim Leers’s picture

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

martin107’s picture

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.

plach’s picture

I spoke with @fago and @berdir some time ago and if I am not mistaken, their performance concerns are now obsoleted by the latest HEAD code. Thus we should be able to go on with the approach currently implemented in the latest patches. We should probably wait for #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions, so we can get rid of the {content_translation} table and just exploit entity (field) storage.

andypost’s picture

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

plach’s picture

I'd really prefer not to have a translation field type, I see no reason for it to exist. We have revision metadata which is just a set of regular "primitive" fields. We can do exactly the same with translation metadata. We should not feel constrained by the current implementation, which is derived from D7 where we implemented a completely different solution. If we don't want to wait for #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions that's fine, then the current patch is mostly ready: we just need to mark fields as custom storage and retain the {content_translation} table until we are able store translation metadata in the entity schema.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new25.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,154 pass(es), 50 fail(s), and 17 exception(s).
[ View ]
new4.12 KB

Here is a manual reroll (I skipped some unrelated changes). Tomorrow I will address

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

My plan is declaring translation field overlapping with node fields as computed (just for nodes) and keeping them in sync. This way we'd avoid the double storage and actually we'd have an identity between $node->uid and $node->translation_uid which could just be ignored, except where explicitly needed (only in CT I'd guess).

Status:Needs review» Needs work

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

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new29.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,579 pass(es).
[ View ]
new10.56 KB

Just a work in progress.

andypost’s picture

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

Berdir’s picture

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

plach’s picture

Component:entity system» content_translation.module
Issue tags:-beta target+beta deadline

This implies non critical storage schema changes, so it's definitely beta deadline. Also, this belongs to the CT queue.

plach’s picture

Assigned:Unassigned» plach

Working on this...

plach’s picture

StatusFileSize
new58.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,594 pass(es), 100 fail(s), and 24 exception(s).
[ View ]

I was finally able to complete my work here. I followed the approach started in the previous patch but I made it more flexible/reliable/consistent by introducing a new (optional) entity handler dealing with translation metadata. Basically it's a wrapper for the translation object that provides methods to access the various metadata values. I think it integrates pretty well with our regular entity-type-specific interfaces and could even serve as a pattern for modules adding new base fields. This allow subclasses to provide an alternative set of field definitions and retrieve field values accordingly or even implement more complex logic on top of those. For instance node translation metadata is currently mapped this way:

  • Translation source -> New node base (translatable) field
  • Translation outdated status -> New node base (translatable) field
  • Translation published status -> Node published status (translatable field)
  • Translation author -> Node author (translatable field)
  • Translation creation time -> Node creation time (translatable field)
  • Translation modification time -> Node modification time (translatable field)

This patch also removes CT's custom field storage and exploits our brand new storage schema handler. Things are working reasonably well, but there are still a few issues that I could use feedback on, or that could be addressed in separate, non beta-bound, issues:

  1. Base vs bundle fields: initially I was in doubt whether base or bundle fields would be more correct to re-implement our current business logic properly. At first sight bundle fields look like the right choice, since translation is enabled per-bundle. OTOH we can exploit base fields and defaults to avoid the need to process all values when enabling translation for a bundle with existing data. In fact translation metadata items are the same for every bundle. As a plus base fields provide more efficient querying, which does not hurt. This seems to suggest base fields are the correct choice, but other opinions are welcome :)
  2. Initial values: if we go with base fields, when attaching fields to entity types for the first time we need to provide some initial values. However we should probably handle this separately once we have a generic solution for assigning initial values to new fields (this is a known issue, although I could not find the node).
  3. Metadata translatability: all the current code assume that metadata values are different for each translation, but this is not always true for cases like Node where the translation published status metadata item is tied to the value of node translation published status (which might not be enabled for translation). This is the main reason why I did not follow (yet) Andy's nice suggestion of checking for EntityOwnerInterface and EntityChangedInterface in the base content translation metadata handler implementation. 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?
  4. 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.

Last but not least I am experiencing a weird exception when trying to attach some base fields to the {user_field_data} table. These are the problematic ones:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[01000]: Warning: 1265 Data truncated for column 'translation_outdated' at row 1: ALTER TABLE {users_field_data} CHANGE `translation_outdated` `translation_outdated` TINYINT NOT NULL COMMENT 'A boolean indicating whether this translation needs to be updated.'; Array ( ) in Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema->createSharedTableSchema() (line 1104 of /www/test.dd/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php).
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[01000]: Warning: 1265 Data truncated for column 'translation_status' at row 1: ALTER TABLE {users_field_data} CHANGE `translation_status` `translation_status` TINYINT NOT NULL COMMENT 'A boolean indicating whether the translation is visible to non-translators.'; Array ( ) in Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema->createSharedTableSchema() (line 1104 of /www/test.dd/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php).
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[01000]: Warning: 1265 Data truncated for column 'translation_uid' at row 1: ALTER TABLE {users_field_data} CHANGE `translation_uid` `translation_uid` INT unsigned NOT NULL COMMENT 'The author of this translation.'; Array ( ) in Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema->createSharedTableSchema() (line 1104 of /www/test.dd/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php).

Despite this things seem to work properly so, unless someone has some brilliant suggestion to fix this, I am wondering whether it would be ok to add a @todo to investigate these errors separately and avoid holding up the issue on them.

I am not attaching an interdiff as I touched almost every line of the previous patch, sorry.

Status:Needs review» Needs work

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

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new3.76 KB
new60.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,571 pass(es).
[ View ]

This should be green.

Gábor Hojtsy’s picture

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?

plach’s picture

Status:Needs review» Needs work

Discussed this with Gabor in IRC:

16:45 GaborHojtsy *plach *: what is the specific thing you are looking for
UX feedback on this?
16:46 plach GaborHojtsy : mainly if you think it's ok to introduce the
temporary UX regression I am referring to and if you ok with the
proposed fix and
16:47 plach above all the metadata translatability bullet
16:48 plach GaborHojtsy : by default we enable translation for fields when
enabling the bundle, so things should behave correclty
16:48 plach but if someone disable translation for, say, the node author,
with the current approach it's no longer possible to keep track of the
translation author
16:49 plach GaborHojtsy : I am not sure whether that's ok, or it's
something that might confuse users
16:49 plach the alternative solution is biting the bullet and possibly
duplicate the information
16:51 GaborHojtsy  has left IRC (Quit: GaborHojtsy )
16:51 GaborHojtsy  has joined
(anonymous@conference/drupalcon/x-ygmcoexxensgmzdu)
16:52 GaborHojtsy *plach *: I think its fine to not be able to track
authors of translators if people asked not to make it per language
information aka translatable :)
16:53 plach GaborHojtsy : yes, my main doubt is are we completely sure
that's the same information?
16:53 plach I mean the author of a translation is more like the author of
a revision, rather than the owner of the node in a particular language
16:54 plach GaborHojtsy : ^
16:54 plach they probably overlap in many cases, and that could probably
justify the current approach
16:54 plach but I can imagine of situations when they would be distinct roles
16:54 GaborHojtsy *plach *: I think it justifies
16:54 GaborHojtsy *plach *: we don’t need to solve all the problems :D
16:55 plach GaborHojtsy : ok, and the current solution is swappable
16:55 plach a different metadata handler would allow to implement a
different logic
16:55 plach GaborHojtsy : ^
16:55 GaborHojtsy *plach *: people can use tmgmt as well to keep track of
meta metadata :D
16:56 plach yep, right
16:56 plach TMGMT could also integrate and replace the core handler
16:56 plach sky is the limit ;)
16:56 plach a lesser concern is that if we rely on EntityOwnerInterface
and EntityChangedInterface
16:57 plach we cannot be sure the values are actually translatable as we
are not guaranteed fields are involved
16:57 plach but I think that's acceptable too, given what's actually
implemented in core
16:57 plach GaborHojtsy : ^
17:00 GaborHojtsy *plach *: yeah I don’t have enough context on that to
say anything meaningful :)
17:01 plach GaborHojtsy : ok, sorry :)
17:01 fago has left IRC (Ping timeout: 245 seconds)
17:01 plach and what about the UX regression while configuring
transltability?
17:01 plach GaborHojtsy : ^
17:01 plach (bullet 4)
17:02 GaborHojtsy *plach *: I commented on the issue on that
17:02 GaborHojtsy *plach *: how is it a regression
17:02 plach awesome, thanks :)
17:02 GaborHojtsy *plach *: ?
17:02 plach GaborHojtsy : well, you need to visit the status report and
apply updates before things actually work
17:04 GaborHojtsy *plach *: WOOOOOOOAH
17:04 plach yes
17:04 plach GaborHojtsy : that's just for the first bundle
17:04 plach but it's annoying
17:05 plach GaborHojtsy : "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."
17:05 plach what do you think?=
17:06 plach GaborHojtsy : I can try to discuss a viable solution with
berdir, fago and eff ASAP, but I think they are pretty busy
17:06 GaborHojtsy *plach *: I think needing run update.php is TOTALLY not
viable
17:07 plach I suspected that :)
17:07 GaborHojtsy *plach *: I don’t see how a core comtiter would commit
it and not block beta on your followup fix
17:07 GaborHojtsy *plach *: I can rather see they let this in after beta
than beta being out where you need to run update.php
17:07 plach GaborHojtsy : ok, then I will implement a workaround and add a
@todo to clean it up
17:07 plach after we have a proper fix, sounds good? :)
17:07 GaborHojtsy *plach *: much better :)
plach’s picture

Parent issue:» #1836086: [meta] Entity Translation UI improvements
Related issues:+#2282119: Make the Entity Field API handle field purging, +#2346019: Handle initial values when creating a new field storage definition
StatusFileSize
new5.22 KB
new61.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch ct-metadata_fields-1916790-81.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This fixes #76.4 by automatically creating field columns when settings are saved. Field definitions are kept also when translation is disabled for all bundles so we don't have to cope with field purging, which is not supported yet (see #2282119: Make the Entity Field API handle field purging). This might be a desirable behavior so data is not lost if translation is enabled again, but we can discuss that in a separate issue.

plach’s picture

Status:Needs work» Needs review

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

plach’s picture

StatusFileSize
new66.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,666 pass(es), 10 fail(s), and 1 exception(s).
[ View ]

Quick reroll + some new stuff. I will provide an interdiff later, I have to board a plane now.

Status:Needs review» Needs work

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

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new68.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,711 pass(es).
[ View ]

This should be green again. I did a few merges meanwhile, I will post an interdiff soon with an explanation of the lastest changes.

plach’s picture

StatusFileSize
new21.08 KB

This is the interdiff since #81 excluding rerolls/merges. Basically I added some logic in the base metadata handler so that all core content entity types are correctly handled, avoiding field data duplication. This required also to fix the comment translation UI to remove the translation name, status and created widgets as the native ones are used.

The metadata handler for now relies on the existence of field definitions with well-known names, we should probably add interfaces for the publishing status and created metadata, so we can fix that. Follow-up work :)

This should be ready to go, hopefully.

plach’s picture

StatusFileSize
new68.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,705 pass(es).
[ View ]
new1.22 KB
plach’s picture

Issue summary:View changes
Related issues:+#2347301: "Data truncated for column" exceptions thrown when adding fields to the user entity type

Updated issue summary.

plach’s picture

Issue summary:View changes
andypost’s picture

Issue summary:View changes
Related issues:-#2347301: "Data truncated for column" exceptions thrown when adding fields to the user entity type
  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?

andypost’s picture

  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

plach’s picture

Issue summary:View changes

Restored issue summary, and addressed #92.2.

1. We cannot use field access there, since there are no corresponding fields defined
4. Just to have symmetric accessors, no technical reason

Quoting @andypost from IRC:

plach, thi is nitics +1 to rtbc

plach’s picture

StatusFileSize
new1.81 KB
new68.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,837 pass(es).
[ View ]

Oops, the patch

larowlan’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new40.92 KB

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

andypost’s picture

The polish in #93 is just 2am

catch’s picture

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.

fago’s picture

Status:Reviewed & tested by the community» Needs work

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

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new68.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,462 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Just a reroll for now.

Status:Needs review» Needs work

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

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new15.58 KB
new69.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,913 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

@fago:

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.
3: Not necessarily: the entity manager handler factory method is called ::getHandler(). I think ::getTranslationMetadata() makes way more sense DX-wise.
5: Fixed.
7: Fixed. We have ProfileForm and ProfileTranslationHandler (which is tied to that form), but translation metadata is not tied to forms, so I guess UserTranslationMetadata makes more sense.

2/4/6:

Quoting from the ContentTranslationMetadataInterface PHP docs:

The metadata handler can act as a wrapper for an entity translation object, encapsulating the logic needed to retrieve translation metadata. The handler does not require a translation object to be instantiated as it can be used just to retrieve the field definitions used to store translation metadata.

The main point of the current approach is that every entity type may need to implement translation metadata in a different way, relying on a different set of field definitions or even on completely custom logic, thus a service is really not viable here. In most places the metadata handler acts as a content translation metadata wrapper (this concept may sound familiar to you ;), providing an interface to access translation metadata in the same way entity-type-specific interfaces do for "native" base fields. I think this approach works well and could even become a pattern for contrib modules adding base fields to entity types. We could even add the concept of wrapper as a section in the Entity plugin definition and a helper factory method to the entity manager, if we decided this is a good pattern.

There are also a few cases where just the field definitions are needed, so the metadata handler does not require to act as a wrapper. This is why I added two factory methods: in Java I would exploit overloading to give them the same name but different signatures. We could also conflate them into a single method and remove type hinting but I am not sure the result would be cleaner.

plach’s picture

Issue summary:View changes
plach’s picture

@fago:

Is #102 fine? Can we go back to RTBC?

plach’s picture

Status:Needs review» Reviewed & tested by the community

The last paragraph in #102 is what might need some additional thought, but IMO #102 addresses @fago's review so back to RTBC...

plach’s picture

Assigned:plach» catch

Assigning to @catch per #98.

plach’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new1.3 KB
new68.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,912 pass(es), 2 fail(s), and 4 exception(s).
[ View ]

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

plach’s picture

Assigned:catch» Unassigned

Working on the test failures, sorry for the noise.

Status:Needs review» Needs work

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

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new611 bytes
new68.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,943 pass(es), 2 fail(s), and 3 exception(s).
[ View ]

This should fix the new failure reported in #102. The other three test failures will be fixed by #2232477: Fatal when adding new fields with NOT NULL constraints in a base table that contains existing entities (checked locally).

Status:Needs review» Needs work

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

plach’s picture

Status:Needs work» Postponed
fago’s picture

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

plach’s picture

Discussed this with @fago and @berdir, I'm going to:

  • Move field definitions to the Content Translation handler
  • Turn the Content Translation Metadata wrapper into a true wrapper that can be instantiated only with an entity object
  • Add an entity type key to define the CTMD wrapper class
  • Prefix metadata field names with content_translation_
plach’s picture

Status:Postponed» Needs review
StatusFileSize
new27.28 KB
new70.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,038 pass(es).
[ View ]

This should address #115 and#116

plach’s picture

Issue tags:+Ghent DA sprint
fago’s picture

Status:Needs review» Needs work

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?

plach’s picture

1: That was just needed to ensure tests are still passing, I will remove it again now.
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.
5: Thinking about it, IMO there's no need to expose that implementation detail in the interface name. This way it's more concise and meaningful, because we stress on metadata not on wrapper.
8: That would be an unnecessary API change, that class has not been introduced here.
9: 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. The user status is not translatable obviously, if we went this way it would not be possible to have per-language translation visibility for users and unpublishing a translation would block the user :)

fago’s picture

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);
        }
plach’s picture

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

I agree with the view part, but consider the other way around: unpublishing a translation would block the user, while I might just want to make the translation not accessible while it's being completed.

fago’s picture

I see, makes sense.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new10.16 KB
new69.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,354 pass(es).
[ View ]

This should address #119.

plach’s picture

StatusFileSize
new3.94 KB
new69.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,357 pass(es).
[ View ]

Reviewing my own patch:

  1. +++ b/core/modules/block_content/src/Tests/BlockContentTranslationUITest.php
    @@ -156,9 +156,8 @@ public function testDisabledBundle() {
         // Make sure that only a single row was inserted into the
         // {content_translation} table.

    We need to update this comment.

  2. +++ b/core/modules/comment/src/CommentTranslationHandler.php
    @@ -18,8 +19,36 @@ class CommentTranslationHandler extends ContentTranslationHandler {
    +      // basic node property values.

    node > comment

  3. +++ b/core/modules/content_translation/content_translation.module
    @@ -85,13 +86,22 @@ function content_translation_language_types_info_alter(array &$language_types) {
    + * To implement its business logic the content translation UI relies on various
    + * metadata items describing the translation state. The default implementation
    + * is provided by \Drupal\content_translation\ContentTranslationMetadata, which
    + * defines one field for each metadata item. Entity types needing to customize
    + * this behavior can specify an alternative class through the
    + * 'content_translation_metadata' key in the entity type definition. Every content
    + * translation metadata wrapper needs to implement
    + * \Drupal\content_translation\ContentTranslationMetadataWrapperInterface.
    + *
      * If the entity paths match the default pattern above and there is no need for

    This documentation is outdated.

  4. +++ b/core/modules/content_translation/src/ContentTranslationManager.php
    @@ -35,6 +37,24 @@ public function __construct(EntityManagerInterface $manager) {
    +    $metadata = new $class($translation, $this->getTranslationHandler($entity_type->id()));
    +    return $metadata;
    +  }

    These can be one single line.

fago’s picture

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

plach’s picture

StatusFileSize
new69.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,283 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Status:Needs review» Needs work

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

plach’s picture

plach’s picture

StatusFileSize
new69.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,282 pass(es).
[ View ]
new861 bytes
plach’s picture

StatusFileSize
new3.67 KB
new69.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,334 pass(es).
[ View ]

This fixes #126 and addresses the case where values coming from the DB are actually NULL. If green should be ready to go as per #126.

plach’s picture

StatusFileSize
new568 bytes
new69.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch ct-metadata_fields-1916790-132.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
+++ b/core/modules/content_translation/src/ContentTranslationHandler.php
@@ -7,6 +7,7 @@
+use Drupal\Component\Utility\String;

Unused

plach’s picture

Green, anyone willing to move this back to RTBC? :)

jibran’s picture

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?

plach’s picture

Not sure it's worth: it's a bit long and we are using a $manager variable everywhere else.

fago’s picture

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs work

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

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new70.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,519 pass(es).
[ View ]

Rerolled

plach’s picture

StatusFileSize
new1.1 KB
new69.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,534 pass(es).
[ View ]

Removed merge leftover.

plach’s picture

Issue summary:View changes

Added beta evaluation and updated the issue summary.

Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community

Thanks for the reroll. Back to RTBC.

plach’s picture

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

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.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new10.64 KB
new77.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,560 pass(es).
[ View ]

Good point, the attached patch applies the updates after config import has been finished and provides test coverage for that.

Just a note: the quote above is outdated, as updates are applied when submitting the configuration form now.

plach’s picture

StatusFileSize
new76.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,964 pass(es).
[ View ]

Just a reroll. Any chance to get a final review?

plach’s picture

StatusFileSize
new878 bytes
new76.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch ct-metadata_fields-1916790-146.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed DI in the new service.

A note for reviewers: I introduced a new service dealing with updates to avoid changing existing interfaces or introducing new ones, as this area could still change a bit after #2346013: Improve DX of manually adding/removing field storage definitions. Since we have no official policy about what constitutes a public API, not providing an interface is a way to say "hey, this is not for public use, do not rely on it".

plach’s picture

Status:Needs review» Reviewed & tested by the community

Hopefully this is good enough to go back to Alex directly...

Status:Reviewed & tested by the community» Needs work

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

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new76.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,461 pass(es).
[ View ]

Rerolled

plach’s picture

Status:Needs review» Reviewed & tested by the community

Green, back to RTBC

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

Some minor nits.

  1. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -338,4 +339,8 @@ function content_translation_form_language_content_settings_submit(array $form,
    +
    +  drupal_set_message(t('Settings successfully updated.'));

    This is unneeded - ContentLanguageSettingsForm::submitForm() adds exactly the same message.

  2. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -338,4 +339,8 @@ function content_translation_form_language_content_settings_submit(array $form,
    +

    Unnecessary blank line

  3. +++ b/core/modules/content_translation/content_translation.module
    @@ -83,13 +84,23 @@ function content_translation_language_types_info_alter(array &$language_types) {
    + * Every entity type needs a translation handler to be translated. This can
      * be specified through the 'translation' key in the 'handlers' entity

    Comment line wrapping needs redoing.

Gábor Hojtsy’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new77.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,526 pass(es).
[ View ]
new3.45 KB

Indeed. Removing that message, extra line and rewrapping.

plach’s picture

RTBC +1 :)

plach’s picture

Issue summary:View changes
alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 920382d and pushed to 8.0.x. Thanks!

Thank you for adding the beta evaluation to the issue summary.

  • alexpott committed 920382d on 8.0.x
    Issue #1916790 by plach, YesCT, penyaskito, Gábor Hojtsy, das-peter,...

  • alexpott committed 17df2c5 on 8.0.x
    Issue #1916790 by plach, YesCT, penyaskito, Gábor Hojtsy, das-peter,...
  • alexpott committed c15cf7d on 8.0.x
    Revert "Issue #1916790 by plach, YesCT, penyaskito, Gábor Hojtsy, das-...
plach’s picture

/me weeps softly ;)

miro_dietiker’s picture

I was confused about the messages above / commit / revert sequence. They are in wrong order.
17df2c5 is the final commit. c15cf7d reverted the one from before.

$ git lg -20 | grep "1916790"
* 17df2c5 - Issue #1916790 by plach, YesCT, penyaskito, Gábor Hojtsy, das-peter, herom, larowlan: Convert translation metadata into regular entity fields (8 hours ago)
* c15cf7d - Revert "Issue #1916790 by plach, YesCT, penyaskito, Gábor Hojtsy, das-peter, herom, larowlan: Convert translation metadata into regular entity fields" (8 hours ago)
* 920382d - Issue #1916790 by plach, YesCT, penyaskito, Gábor Hojtsy, das-peter, herom, larowlan: Convert translation metadata into regular entity fields (9 hours ago)
(8.0.x) /var/www/d8.dev/www$ git lg | grep "1916790"
* 17df2c5 - Issue #1916790 by plach, YesCT, penyaskito, Gábor Hojtsy, das-peter, herom, larowlan: Convert translation metadata into regular entity fields (8 hours ago)
* c15cf7d - Revert "Issue #1916790 by plach, YesCT, penyaskito, Gábor Hojtsy, das-peter, herom, larowlan: Convert translation metadata into regular entity fields" (8 hours ago)
* 920382d - Issue #1916790 by plach, YesCT, penyaskito, Gábor Hojtsy, das-peter, herom, larowlan: Convert translation metadata into regular entity fields (9 hours ago)
Gábor Hojtsy’s picture

Issue tags:-sprint

/me hands stack of napkins to @plach

Thanks for your persistence!

plach’s picture

:)

Status:Fixed» Closed (fixed)

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

penyaskito’s picture

Thanks!!