Follow-up to: #1188388: Entity translation UI in core. Work is ongoing in the D8MI - Entity Translation sandbox.

Problem

The current Entity Translation API is:

  • Inefficient: To determine whether a translation exists, all fields are scanned (each time) to collect all language-specific values.
  • Fragile: When invoking EntityNG::getTranslation($langcode) a copy of each field value in the given language is attached to the returned EntityTranslation object. If there is no value for the given language, fields are initialized with an empty vaule. This way the next time EntityNG::getTranslationLanguages() is called, $langcode erroneously appears as an existing translation.
  • There is no API to create or delete a translation, and no hooks are fired to allow to react to these events.
  • There is no concept of "active language"; i.e., the language of the translation being manipulated. Therefore, the active language has to be passed forward in a way that is bad for DX and testability (see the API Changes section below).
    • If the active language is not passed forward, all code falls back to the default language.
    • If the active language is passed forward, every line of code dealing with entities needs to retrieve it and explicitly take it into account.

    Currently we would need to write something like this:

    <?php
    // Retrieve the active language in some way and put it into $langcode.
    if ($entity->language()->langcode == $langcode) {
      // Do something on $entity.
      $entity->foo->value = 'bar';
    }
    else {
      $translation = $entity->getTranslation($langcode);
      // Do something probably identical on $translation.
      $translation->foo->value = 'baz';
    }
    $entity->save();
    ?>
    

    Or simply always do something like:

    <?php
    // Retrieve the active language in some way and put it into $langcode.
    // Do something on $entity.
    $entity->getTranslation($langcode)->foo->value = 'baz';
    $entity->save();
    ?>
    
  • There is no indication of which translations exist for an entity at the storage level, thus we have no table to reliably join on when needing to filter queries per language.

Goal

  • Provide a clean way to deal with translations in the Entity Translation API.
  • Improve Views integration.

Details

  • #1260640: Improve field language API DX already concluded that it is OK to for all code to act on a single language, but the code needs to know which one.
  • Code that needs to act on all languages (rare) can use Entity::getTranslationLanguages() to retrieve all languages.
  • The current EntityNG::getTranslation() method returns an instance of the EntityTranslation class which represents a "facet" of the entity in a particular language.

Proposed solution

  1. Add the following methods:
    • TranslatableInterface::getOriginal();
    • TranslatableInterface::hasTranslation($langcode)
    • TranslatableInterface::addTranslation($langcode, $values = array())
    • TranslatableInterface::removeTranslation($langcode)
    • TranslatableInterface::getTranslationStatus() (created/existing/removed)
  2. After adding or removing translations, when storing the updated entity, fire these hooks:
    • hook_entity_translation_create()
    • hook_entity_translation_delete()
  3. Remove the EntityTranslation class and refactor the EntityNG object ato make it return a shallow clone of itself when a translation object is retrieved through EntityNG::getTranslation() (see #30 and #38 for details). The only difference for API consumers is that the value returned by EntityNG::language() will be different based on the language of the translation object (the active language). When retrieving a translation corresponding to the entity language the entity object itself is returned. To get the entity language we just need to retrieve the original entity object: $translation->getOriginal()->language().
  4. Rely on the entity storage to track the existence of translations through a separate data table for each translatable entity type. It must contain a row for each available translation; e.g.:
    | entity_id* | langcode* | default_langcode |
    

    This table could also hold translation metadata information, see #1807800: Add status and authoring information as generic entity translation metadata.

Implementation notes

One tricky aspect to address in the current proposal is dealing with translation removal. When removing the translation corresponding to the active language, we need to invalidate the translation object, by setting the translation status to removed. Any invocation of the (magic) accessor methods following a translation removal implies throwing an exception: basically the translation object becomes unusable and a new one needs to be instantiated. Since the need to reuse a translation after removing it should be pretty rare, being strict and not allowing it directly will avoid unexpected behaviors.

As a sidenote there is consensus that language fallback logic does not belong to the entity/transation objects. See #2019055: Switch from field-level language fallback to entity-level language fallback for details.

API changes

Before

See the realted issue: #1807692-43: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget.

function translation_entity_field_attach_presave(EntityInterface $entity) {
  if (translation_entity_enabled($entity->entityType(), $entity->bundle())) {
    $attributes = drupal_container()->get('request')->attributes;
    translation_entity_sync($entity, $attributes->get('working_langcode'), $attributes->get('source_langcode'));
  }
}

After

function translation_entity_field_attach_presave(EntityInterface $entity) {
  if (translation_entity_enabled($entity->entityType(), $entity->bundle())) {
    translation_entity_sync($entity, $entity->language()->langcode, $entity->source->value);
  }
}

The EntityTranslation object wraps the entity. This greatly simplifies the Entity API DX, because we introduce the concept of "active language" without introducing a state in the entity object.

Blocking this issue

We are postponed (in #20) on "various NG conversions".

Related issues

CommentFileSizeAuthor
#132 et-api-1810370-131.interdiff.do_not_test.patch3.14 KBplach
#132 et-api-1810370-131.patch100.92 KBplach
#128 et-api-1810370-128.interdiff.do_not_test.patch2.95 KBplach
#128 et-api-1810370-128.patch100.74 KBplach
#126 et-api-1810370-126.interdiff.do_not_test.patch4.4 KBplach
#126 et-api-1810370-126.patch100.73 KBplach
#124 et-api-1810370-124.patch100.77 KBplach
#123 et-api-1810370-123.do_not_test.patch100.77 KBplach
#121 et-api-1810370-121.patch100.77 KBkfritsche
#121 interdiff-119-121.txt8.07 KBkfritsche
#119 et-api-1810370-119.patch101.36 KBplach
#118 et-api-1810370-118.interdiff.do_not_test.patch5.04 KBplach
#118 et-api-1810370-118.patch101.7 KBplach
#117 et-api-1810370-117.interdiff.do_not_test.patch1.29 KBplach
#117 et-api-1810370-117.patch101.15 KBplach
#115 et-api-1810370-115.interdiff.do_not_test.patch2.09 KBplach
#115 et-api-1810370-115.patch101.15 KBplach
#114 et-api-1810370-114.patch99.96 KBplach
#110 et-api-1810370-110.interdiff.do_not_test.patch934 bytesplach
#110 et-api-1810370-110.patch98.59 KBplach
#108 et-api-1810370-108.interdiff.do_not_test.patch3.27 KBplach
#108 et-api-1810370-108.patch98.87 KBplach
#106 et-api-1810370-106.patch97.52 KBplach
#100 et-api-1810370-100.interdiff.do_not_test.patch33.44 KBplach
#100 et-api-1810370-100.patch97.46 KBplach
#86 et-api-1810370-86.interdiff.do_not_test.patch1.95 KBplach
#86 et-api-1810370-86.patch95.08 KBplach
#81 et-api-1810370-81.interdiff.do_not_test.patch717 bytesplach
#81 et-api-1810370-81.patch95.43 KBplach
#79 et-api-1810370-79.patch94.73 KBplach
#75 et-api-1810370-75.interdiff.do_not_test.patch6.45 KBplach
#75 et-api-1810370-75.patch82.27 KBplach
#73 et-api-1810370-73.interdiff.do_not_test.patch18.25 KBplach
#73 et-api-1810370-73.patch75.56 KBplach
#68 et-api-1810370-68.patch63.69 KBplach
#66 et-api-1810370-66.patch62.61 KBplach
#64 et-api-1810370-64.interdiff.do_not_test.patch1.19 KBplach
#64 et-api-1810370-64.patch62.01 KBplach
#57 et-api-1810370-57.interdiff.do_not_test.patch11.49 KBplach
#57 et-api-1810370-57.patch61.69 KBplach
#48 et-api-1810370-48.patch57.05 KBplach
#46 et-api-1810370-46.patch53.66 KBplach
#19 et-api-2013_02_01.log_.txt19.8 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

Background

Originally in #1188388-19: Entity translation UI in core
plach

We need to find clean way to deal with translation metadata and the bare existence of translations: currently to determine whether a translation exists we just scan (each time) all the fields and properties to collect any language we can find. That collection is returned as the existing translations. This approach is IMHO inefficient and fragile, as all the changes I had to do the EntityTest and the related storage controller demonstrate. @fago and I discussed about a related issue in Munich: how to deal with language when accessing properties trough the upcoming Property API. We agreed that a variant of the previously suggested active language would work: basically we would have an EntityInterface::getTranslation($langcode) method that would return a class implementing the EntityInterface having as default the passed language. This would be used to access the properties in the usual way. We agreed that this could be a viable solution since it does not introduce the concept of state to the entity itself, we just retrieve a facet to act on (note that the class would not be a full entity class, just a wrapper or something like that). Building upon this I think we could have a full-fledged EntityTranslation class with its own storage controller and its properties, which would hold metadata like the status or the source language for that particular translation and an entity reference to the parent entity. As an alternative we could go the current way and declare metadata as multilingual properties of the parent entity, but the former is a solution worth to be explored as we would get the CRUD hooks or per-language access for free, just to make a couple of examples. If there are perfomance concerns for monolingual sites we could have a null implementation of the storage controller that performs no query in this case.

And responded in #1188388-81: Entity translation UI in core
Gabor

I don't think people would have null implementations for mono-lingual sites per say, unless we provide that explicitly. Also, this sounds like a rather big body of development to do to optimise translation access and handling. Sounds like if we want to do this, then it would need to be done before December 1st, regardless if we want to make it happen as part of the initial patch or not. My understanding is that the current discovery of translations would need to be kept to be a source mechanism to access those EntityTranslations, so this is a first step anyway. We can open a major followup unless you need this implemented for other reasons in the first pass.

And responded in #1188388-82: Entity translation UI in core
plach

This probably won't be needed as EntityNG::getTranslation() returns the entity itself when the passed langcode is the default language.
plach’s picture

Title: find clean way to deal with translation metadata and bare existence of translations [Follow-up to Entity Translation UI in core] » Find clean way to deal with translation metadata and the existence of translations
Component: language system » translation_entity.module
Issue tags: +API clean-up
plach’s picture

Status: Postponed » Active
plach’s picture

Title: Find clean way to deal with translation metadata and the existence of translations » Find a clean way to deal with translation metadata and the existence of translations
Assigned: Unassigned » plach
fago’s picture

+1 for introducing translation "CRUD hooks. This is what the UI and the concepts do already, so having hooks to respond to it would be reasonable. E.g., you could easily log "translation created".

hook_entity_translation_create()
hook_entity_translation_delete()

I don't think we need hook_entity_translation_insert/update() though, as we can always have $entity being updated there. I'd see use introducing an easy way to determine which fields changed there - by language code? That should make it easier to write code working with translations as you don't have to specifically implement the entity and the translation update hook but handle all cases in one hook implementation working with all translations?

I don't think we need a loading hook either (instead I'd love to consider removing hook_entity_load()).

plach’s picture

Issue summary: View changes

Updated issue summary

plach’s picture

Title: Find a clean way to deal with translation metadata and the existence of translations » Entity Translation API improvements
Priority: Normal » Major

After thinking about this for quite some time and actually trying some experimental code, I felt that the previously proposed approach was not going to work. Hence I updated the issue summary with what I think should be the proper way to address this. I'm also slightly broadening the scope of this issue, since a couple of related problem were previously left out.

Promoting to major since solving this is of fundmental importance for lots of different issues concerning ET.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

For what storage is concerned we could simply enforce the existence of the data table for translatable entity types, and ensure it has a row for each available translation by forcing the presence of at least some basic data in it. For instance:

We cannot enforce that, we do not even enforce SQL.

In this scenario the entity translation object could be passed around and used as the wrapped entity. This would greatly simplify the Entity API DX, because we would be introducing the concept of "active language" without introducing a state in the entity object.

Well, you can already pass around the EntityTranslation object, not? So is the only thing concerning to you that code has to differ between $entity and $translation?

I'm not sure about having $translation implementing the EntityInterface. What does $translation->delete() and $translation->save() do? Working on entity-level might be confusing, breaking operations down to translations might have unexpected consequences. Which one have you been thinking of?

Related, we need to start thinking about language fallbacks - mostly for display, but it should be at optionally respected by e.g. entity serialization also. Then the language-fallback rules should be available declarative, such that entity query could be improved to pick it up (e.g. in contrib).

I guess it would make sense to have a Decorator object that applies the language fallback rules, or should it even be in EntityTranslation?

plach’s picture

We cannot enforce that, we do not even enforce SQL.

We can enforce it for the SQL database storage, that's all I'm concerned about. Other storage backends will have to find their way to deal with the existence of translations, but that's how a storage-agnostic solution is expected to work, I guess.
Think of an entity having only configurable-fields: strictly-speaking it wouldn't need the data table, but this way we'd have no "official" table to join on to determine the existence of a translation.

Well, you can already pass around the EntityTranslation object, not? So is the only thing concerning to you that code has to differ between $entity and $translation?

Yes, exactly, I don't think this is a small concern. I'd wish that the only difference between an EntityTranslation and an Entity object were the value returned by ::language(). Any other method invocation, including delete() and save() would be proxied to the wrapped entity. Entity-handling code should not need to worry about the concept of translation in any way, in fact in this scenario we'd always use an $entity variable. I don't think there is an alternative approach if we want to have a sane DX this time.

Working on entity-level might be confusing, breaking operations down to translations might have unexpected consequences. Which one have you been thinking of?

I'm seeing the EntityTranslation wrapper as an entity facet whose role is just providing the default language to act on, for anything else it would behave as the regular Entity. To remove a translation I would just do:

<?php
// $entity is an instance of EntityTranslation
$entity->removeTranslation($langcode);
$entity->save();
?>
Related, we need to start thinking about language fallbacks - mostly for display, but it should be at optionally respected by e.g. entity serialization also. Then the language-fallback rules should be available declarative, such that entity query could be improved to pick it up (e.g. in contrib).
I guess it would make sense to have a Decorator object that applies the language fallback rules, or should it even be in EntityTranslation?

Yep, this totally makes sense: in D7 language fallback is applied only in the render phase and there is a reusable (to some extent) logic behind it. The one you are outlining above seems the natural evolution of this approach. My only doubt is about the declarative aspect, not sure whether it'll be that easy.

plach’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

I've taken the liberty to completely rewrite the issue summary, in order to understand the complete proposal.

There's only one @todo in the Problem section about the claim of "fragile", which should be filled out.

Aside from that, this proposal makes a lot of sense to me. I almost guess it would have the potential of finally getting rid of the langcode keys in the field properties, so $entity->fieldname contains the field items directly. If that is part of the vision, then we should add it as last point of the Proposed solution section.

yched’s picture

I like the overall proposal.

(plach #8) I'd wish that the only difference between an EntityTranslation and an Entity object were the value returned by ::language(). Any other method invocation, including delete() and save() would be proxied to the wrapped entity

Off hand, I'd echo @fago's worries about EntityTranslation implementing EntityInterface, and both Entity & EntityTranslation objects being passed through code as $entity.
Feels like there's potential for lots of confusion :
- When exactly can you do $entity->someMethodOfEntityTranslationInterface() if you don't know for sure what you receive ? test instance_of ?
- $entity(_translation)->delete() deletes the whole entity ?

yched’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

@sun:

Thanks for streamlining the summary, I completed the todo and fixed the storage bullet which was inexact. The rest looks great to me :)

Aside from that, this proposal makes a lot of sense to me. I almost guess it would have the potential of finally getting rid of the langcode keys in the field properties, so $entity->fieldname contains the field items directly. If that is part of the vision, then we should add it as last point of the Proposed solution section.

I assume you are referring to the internal values since the developer-facing API already got rid of the langcode keys, at least for NG entities. I think retaining the langcode level has a value because it makes easier to deal with non-translatable (shared) fields, whereas having langcode-keyed arrays of field objects would force us to copy a reference of every shared field object for each available language.

IMO the real (huge) pro of this proposal is that we will avoid tons of LoC like the following whenever a piece of code might need to deal with a multilingual entity (potentially everywhere we deal with an entity):

<?php
// Retrieve the active language in some way and put it into $langcode.
if ($entity->language()->langcode == $langcode) {
  // Do something on $entity.
  $entity->foo->value = 'bar';
}
else {
  $translation = $entity->getTranslation($langcode);
  // Do something probably identical on $translation.
  $translation->foo->value = 'baz';
}
$entity->save();
?>

Or simply always do something like:

<?php
// Retrieve the active language in some way and put it into $langcode.
// Do something on $entity.
$entity->getTranslation($langcode)->foo->value = 'baz';
$entity->save();
?>

which woiuld be nothing more than a more verbose and less readable version of the current proposal. Let alone being able to determine the active language, which is something that might be not so easy to do in a clean way as stated in the OP.

@yched:

Off hand, I'd echo @fago's worries about EntityTranslation implementing EntityInterface, and both Entity & EntityTranslation objects being passed through code as $entity.
Feels like there's potential for lots of confusion :
- When exactly can you do $entity->someMethodOfEntityTranslationInterface() if you don't know for sure what you receive ? test instance_of ?
- $entity(_translation)->delete() deletes the whole entity ?

Honestly I don't see the potential for confusion. What behavior would you expect from the following line?

<?php
$entity->delete();
?>

I expect it to delete an entity and all of its translations with it. I don't need to know nor care whether the active language is 'it' or 'fr'. The new proposed methods would just act at data-structure level. Instead the new hooks would be fired by the storage controller when storing the updated entity. We can discuss whether when deleting an entity we should invoke hook_entity_translation_delete() (personally I wouldn't), but this is the biggest kind of doubt this approach inspires me. Which is a very little concern if compared to the DX wtf sketched above.

If we want to be sure we are dealing with the original values, we juest need to do something like this:

<?php
$entity->getOriginal()->language()->langcode;
?>

but this is a way less common use case than the ones above. We should be optimizing DX for the most common cases, not the remaining 20%.

sun’s picture

Thanks! That makes this issue (summary) and proposal ready for syndication.

plach’s picture

Tagging for sprint to get more eyes on this.

fago’s picture

$entity->delete();

Indeed, your proposal should work out as long as we pass $translation around as $entity, i.e. never have $translation. That means EntityTranslation should probably be not a translation of the entity, but a just a different facet defaulting to a different language? Not sure, whether we could better clarify that with another name?

However, what happens if you call
$entity->removeTranslation('de')
on the translation of 'de' ? You are working with invalid values then?

fago’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

That means EntityTranslation should probably be not a translation of the entity, but a just a different facet defaulting to a different language?

Yes, this is exactly how I think about it when I try to imagine how code dealing with the future API should interpret it.

Not sure, whether we could better clarify that with another name?

I was thinking about this too. The translation term is misleading in first place, since you cannot translate a map point, but you can have different points for different languages. I'd be happier if we could use the multilingual term, but this is not a hard requirement :)

The initial proposal refers to the EntityTranslation class because I didn't want to introduce too much changes in the current API, but probably we should rename it to convey the proper meaning.

However, what happens if you call $entity->removeTranslation('de') on the translation of 'de' ? You are working with invalid values then?

When I was playing with this idea, I thought about two possible solutions:

  • we could just switch the internal active language to the default one, but this might have tricky consequences when going on with the code flow;
  • we could just throw an exception: if you wish to delete the current translation be sure to do something like this:
    <?php
    // The active language is 'de', the entity language is 'it'.
    $entity->getOriginal()->removeTranslation('de')
    ?>
    

    I think it's fair to require code explicitly dealing with translations to be aware of the concept of active language.

yched’s picture

(fago) Indeed, your proposal should work out as long as we pass $translation around as $entity, i.e. never have $translation. That means EntityTranslation should probably be not a translation of the entity, but a just a different facet defaulting to a different language?
(plach) Yes, this is exactly how I think about it when I try to imagine how code dealing with the future API should interpret it.

Okay - but then I'm wondering how really different or better it is from the approach where we'd add a "current working language" state on the (unique, original) $entity object ?

plach’s picture

Well, this way the wrapped object could be serialized/cached more reliably, I guess, whereas a stateful one would be more tricky to.

plach’s picture

Component: translation_entity.module » entity system

Moving to the right queue.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

FileSize
19.8 KB

An IRC meeting has been held today in #drupal-entity. Partecipants were das-peter, fago, plach, sun and tim.plunkett. Basically we agreed to proceed in the direction outlined in the OP. Some details were fleshed out. You can read the full discussion in the attached IRC log. The issue summary has been updated accordingly.

The decorator class name, currently EntityLanguageDecorator, is still open for debate so any suggestion is welcome.

plach’s picture

Status: Active » Postponed

Before working on this we probably need to complete the various NG conversions.

plach’s picture

Issue summary: View changes

Updated the issue summary after the IRC meeting

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

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

Issue summary: View changes

Updated issue summary.

msonnabaum’s picture

The proposed solution will enforce us to use only EntityInterface in type hints, even where we expect a specific entity type. This should be an acceptable limitation since entities are data objects and as such do not define additional methods/business logic wrt the implemented interface.

Is that really the consensus of the community? I completely disagree with it.

I'd like to see some of our entity classes turn into actual domain models, and this issue would prevent that. Forcing code to type hint EntityInterface almost defeats the purpose of type hinting it in the first place. I would consider this limitation a total non-starter.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Is that really the consensus of the community? I completely disagree with it.

This is the consensus reached by the few people that participated to the IRC meeting. I tried to the spread the voice about it but very few showed up unfortunately.

That said, it seems both @tim.plunkett and @fago no longer agree with the conclusion above now.

tim.plunkett’s picture

Compared to the other suggestions about how to solve this problem, I (at the time) thought a decorator was a reasonable solution. I didn't realize it would cause this interface problem, since we were just talking in the abstract, not with any code.

TL;DR I'm -1 on this now.

plach’s picture

Well, we can still implement this solution and allow entity-type-specific decorators or just skip them if the entity definition does not specify neither the generic ELD nor an entity-type-specific version. This approach would allow us to use whatver interface we want in type hints.

tim.plunkett’s picture

Can't we just have a bunch of empty classes like:

NodeLanguageDecorator extends EntityLanguageDecorator implements NodeInterface

If not, the entity_info approach in #25 sounds great. Either seems like it would unblock #1391694: Use type-hinting for entity-parameters.

plach’s picture

We can surely go both ways: the former would mean implementing C, the latter would mean B or C depending on which interface the entity class implements. I think ths would be a reasonable compromise, but I could live also with strict C and empty classes/interfaces, as already stated in #1391694: Use type-hinting for entity-parameters. Do you think I didn't explain myself well over there? I didn't realize I was actually blocking it.

Gábor Hojtsy’s picture

How would the different proposed approaches change the DX as proposed in the OP? I'm afraid I don't have the overview that others might have or that they don't have it either and we are just reflecting on certain particularities, not on what such decisions would entail for the overall DX.

plach’s picture

@Gabor:

As long as we can use a decorator the API change proposed in the OP, with its evident DX gains, is feasible. The moment we agree (and I never will :) in #1391694: Use type-hinting for entity-parameters that we should use class names instead of interfaces in type-hints we lose all of this.

plach’s picture

Issue summary: View changes

Improved issue summary.

Crell’s picture

effulgentsia explained this thread to me on the REST team call this week, so I *think* I finally understand what is going on here and how it impacts #1391694: Use type-hinting for entity-parameters. :-) From my POV, plach's original proposal (if I understand it) is much better than forcing a wrapper, and not just for the type hinting potential.

In part, it has the potential to be considerably more memory efficient. Consider, an EntityNG is, largely, a wrapper around a bunch of field objects. (If I have my APIs straight.) The difference for different languages is which language we're passing to those fields to retrieve the correct language's data. That means getTranslation() can be implemented as... a shallow clone. Consider the following over-simplified example:

class Entity {
  // This is not an array, but an object collection; probably ArrayObject.
  protected $fields;

  // This is a primitive, likely string.
  protected $language;
  
  public function getFieldValue($field) {
    return $this->fields[$field]->getValue($this->language);
  }

  public function __clone() {
    // Do nothing here, because we want $fields to NOT be duplicated.
  }

  public function getTranslation($lang) {
    $translation = clone($this);
    // Since it's the same class, we can access its protected properties even though it's a different
    // object.  PHP is weird like that, but it works.
    $translation->language = $lang;
    return $translation;
  }
}

That means if you call $trans = $node->getTranslation('de'), $node and $trans now refer to the same field objects, but different default languages. That means you can use both variables to get/set field values in that language only, but there's still only one data structure in memory. And, moreover, if you call ->save() on one of them then you save... the one copy of the data. So calling save() on ANY of them will work.

The net overhead here is the cost of an object, plus the language string. That's on the order of 50 bytes or less. All of the big data is single-instanced. IMO, that's huge for memory savings *and* for DX, and we get type hinting back because we don't need to have a generic translation wrapping object. The only caveat is that we need to have the $fields variable be an object rather than a basic array, but that should be simple enough.

And yes, I tested that the above does actually work in PHP. :-)

Is my understanding here correct? Does this all make sense?

plach’s picture

@Crell:

Hey, good to see you here :)

From my POV, plach's original proposal (if I understand it) is much better than forcing a wrapper, and not just for the type hinting potential.

Well, my original proposal is mostly what the OP is talking about. The solution currently implemented, which is closer to yours, was designed by @fago and me when introducing the Entity Field API. I spent much time thinking about the current proposal because also a wrapper has the potential to heavily reduce memory usage, but it does not imply introducing a "state" in the Entity object. AAMOF if I'm not mistaken your solution could be simplified even more if we just added an activeLanguage field to the Entity object itself, as I think we don't need different translation objects being instantiated at the same time in 99% of our use cases. This would also avoid losing entity deep-cloning, which we currently have (not sure whether it's needed only by the BC layer). When originally talking about this problem space, @fago strongly felt we should not introduce a state into the entity object, as this might be tricky when, for instance, serializing it. A possible solution for this particular issue is always reverting the active language to the entity language during serialization, but I guess this has the potential to introduce some tricky edge cases.

Is my understanding here correct? Does this all make sense?

I think you are totally grasping the issue, not sure whether your proposal actually respects the design constraint of "not introducing a state in the Entity object".

Crell’s picture

I believe the serializer only serializes fields. Unless you mean PHP serialize(), in which case we should implement the Serializable interface and state explicitly what to serialize. So a non-field state property would be fine from that POV, as long as it can be adequately restored.

Actually, adding an activeLanguage Field to the Entity would be a terrible idea. :-) For one, it's not part of the stored state of the entity (the entity as it exists in the database has no "language that should be used", AFAIK, as that's a runtime value). For another, that would make it shared between instances and thus defeat the entire purpose of using a shallow copy; we'd need to do a deep copy, and then save() breaks, memory balloons, etc.

Since entities are by definition state, I'm not sure what is meant by "not introducing state". ;-)

plach’s picture

Unless you mean PHP serialize(), in which case we should implement the Serializable interface and state explicitly what to serialize. So a non-field state property would be fine from that POV, as long as it can be adequately restored.

Yep, I meant this. And the last sentence outlines where tricky stuff might be: if we store contextual information in the entity data structure, when we restore it context might have changed. Anyway this was just an example. I think we could find a way to properly deal with this in the 80% of our use cases.

Actually, adding an activeLanguage Field to the Entity would be a terrible idea. :-) [...]

I think I didn't explain myself well, as usual: currently in the D7 Entity API we have a method to explicitly switch the active language, i.e. the language field accessors default to. There would be no clone in this scenario, although the ugliness is exactly in having runtime stuff on the entity object that is not persisted. My only doubt is that you proposal might end-up being close to this scenario as we would have a runtime language value that would not match the stored one.

Since entities are by definition state, I'm not sure what is meant by "not introducing state". ;-)

The runtime stuff you were talking about above :)

Anyway, I think your proposal is way more attractive than the D7-inspired one I was referring to and has certainly evident advantages over the decorator-based one. I guess we could even support both shallow and deep cloning based on a flag, so this would be a non-issue. Overall if @fago is happy with it I think we have a deal :)

Crell’s picture

I don't know when we'd be doing PHP serialize() on an entity object and it not being a bad idea, frankly. :-)

Berdir’s picture

$form_state['node']? tempstore for views?

Crell’s picture

Oh. That. FAPI--

In that case, we *would* want to persist "transient state", wouldn't we?

fago’s picture

Consider, an EntityNG is, largely, a wrapper around a bunch of field objects. (If I have my APIs straight.) The difference for different languages is which language we're passing to those fields to retrieve the correct language's data.

Correct, but we have field objects which hold their own data in a single language right now, i.e. we have field objects per language. We do not have a single field object holding all language values.

Actually, adding an activeLanguage Field to the Entity would be a terrible idea. :-) For one, it's not part of the stored state of the entity (the entity as it exists in the database has no "language that should be used", AFAIK, as that's a runtime value). For another, that would make it shared between instances and thus defeat the entire purpose of using a shallow copy; we'd need to do a deep copy, and then save() breaks, memory balloons, etc.

Exactly - with introducing "state" we were talking about introducing "runtime" information, which would be shared among instances and thus a problem.

$translation = clone($this);
// Since it's the same class, we can access its protected properties even though it's a different
// object. PHP is weird like that, but it works.
$translation->language = $lang;

So what we've right now is a different Entity and Translation class. If I get this right, you are proposing to use the same class for the translations?

Right now, translation objects do not necessarily have all fields as not all fields are translatable. So we have two modes for dealing with that (strict mode or not). Without strict mode we fallback on default language, what makes all fields accessable. But by default we have strict mode enabled and do not make untranslatable fields available from the translation, such that you can explicitly deal with all translated fields, e.g.

foreach ($translation as $field) {
  // some something
}

But furthermore I think we should add handling language fallback as we have it during rendering (thanks to the field API) right now. I'd add it as part of the regular API, so that you can do $translation->field->value in order to get the value with fallback logic applied. That way it's easy to programmatically access the same values as they are rendered, what I think is rather important to have.

So we could simply do that with a decorator object that implements the language fallbacks as necessary - given we have interfaces and can do decorator objects. Similar like we have a separate Translation object/class right now. But without decorator objects we'd have to bake everything into the Entity object, what would result in more complexity there and a bad SoC.

-> Let's better keep that logic separated.

Crell’s picture

I'm trying to remove the decorator entirely, as it breaks things. (Like, any custom methods you add to an entity type, interfaces for entity types, etc.) Decorator-- here.

What I'm proposing is that a "translation" is simply another entity object that has the same persisted state, but different transient state. That is, a given Node object has a property that says what its default language is, whether it should fallback to default language or not, etc. That's all transient state and is never saved.

The persisted state is... fields.

What makes this work is the way clone() works in PHP. When you clone an object, its primitive and array data members clone with it, while its object data members do not, unless you tell them to. Thus:

class Foo {
  public $a;
  public $b;

  public function __construct() {
    $this->a = 1;
    $this->b = new stdClass();
  }
}

$foo = new Foo();
$bar = clone($foo);

$foo->a === $bar->a; // FALSE, because $a was cloned.
$foo->b === $bar->b, // TRUE, because only the object reference was copied, not the object.

So "language to return data for", "language fallback logic", etc. we ensure are stored in primitives (like $a). Persisted state (fields) we ensure are stored in objects (like $b). Now:

class Entity {
  public function getTranslation($lang, $default_handling = 'fallback') {
    $translation = clone($this);
    // Since it's the same class, we can access its protected properties even though it's a different
    // object.  PHP is weird like that, but it works.
    $translation->language = $lang;
    $translation->defaultHandling = $default_handling;
    return $translation;
  }
}

And now language and defaultHandling are unique between the two objects, but the data is not. That the fields are one object per language is irrelevant. The class just needs to know how to get to the right one via $this->language and $this->defaultHandling... which is needs to know anyway. That gets the translation fallback logic fago mentions in #37. because $node_fr and $node_en have exactly the same API and same data... just different transient state.

plach’s picture

I am not sure the fallback decorator would need to be passed around as we would do with a translation object (however we implement it), instead I think its use would be confined to very specific tasks, such as entity rendering, that would not need to deal with entity-type-specific stuff. AAMOF by definition the fallback decorator would just deal with field values, thus a generic implementation should work with any entity type. We could still provide entity-type-specific ones through entity info, but this would not be a hard requirement.

If these assumptions are correct, I think translation objects as described by @Crell (cloned entities with different language default) could live together with the fallback decorator envisioned by @fago.

@fago:

So what we've right now is a different Entity and Translation class. If I get this right, you are proposing to use the same class for the translations?

This is one of my main goals even if we go for ELDs: I think having different objects (interfaces!) to deal with is a huge wtf in terms of DX, for an example you can see the interdiff in #1498674-216: Refactor node properties to multilingual. I don't think the current approach of having strict mode and objects with just translatable fields on is revealing particularly useful, in turn it brings a very bad DX. I strongly feel we should just provide an entity object with a language default matching the translation language, however we implement it. I think the risk of unintentionally overwriting the value of a shared field is a small concern compared to the additional complexity and DX burden this solution is bringing. In my experience writing D8 code dealing with ML entities is really painful at the moment :(

Crell’s picture

I don't know what "Fallback decorator" you mean. My main concern is type hinting NodeInterface in a method, and getting an actual honest to goodness node object, not something that may or may not be wrapped in some other class that doesn't know that NodeInterface has extra methods. If this decorator is internal to that object, meh. :-) But all of these extra wrappers around the entity object need to go.

I actually think it's better DX that you *do* overrwrite a shared field, because different translations are the same node. The API should reflect that. It shouldn't make it seem like they're different nodes when they really aren't.

fago’s picture

I can get behind the idea of re-using the entity object for translation instances I think. crell is right in that we could implement and separate the fallback in an internal helper object also - if we want to. I'm still not convinced that the outlined approach leads to a reasonable simple way to implement it though, but let's think it through. I agree that just having one class makes things simpler and so would be a win.

However - we would have to fix terminology. You have been using the term "default language" as "active translation language", but "default language" is already the term we use for the original language of the entity. We need to come up with a consistent terminology here - like "default language" and "active language" maybe?

And now language and defaultHandling are unique between the two objects, but the data is not. That the fields are one object per language is irrelevant.

Well, I'm not sure about this. If a field is just one object it needs to hold all values in all languages (what it doesn't right now). In order to know with which language to work it needs to have the transient state of the active language also. For that transient state to be different between various translations we need different field object instances. Consequently, we'd have to do a deep clone for translations.

I actually think it's better DX that you *do* overrwrite a shared field, because different translations are the same node. The API should reflect that. It shouldn't make it seem like they're different nodes when they really aren't.

Sounds reasonable. But I guess we'd still need to have strict mode then as well? I guess that could be just be a special fallback logic case.

So here is a question: When a a node has default language of EN and I'd have an entity object with active language DE am I supposed to pass it around as regular entity object? I think that would be the goal and makes sense as it's an instance of the same class. But what if we have language fallback rules active that makes required stuff like $node->status inaccessible because it's not available in a certain language? I guess we'd have to assume that any available translation passes validation, i.e. has all required fields?

YesCT’s picture

Given the recent discussion, can we update anything in the issue summary?

YesCT’s picture

We are postponed (in #20) on "various NG conversions".
Maybe we should list some.

YesCT’s picture

Issue summary: View changes

Updated issue summary

Crell’s picture

A required property being completely inaccessible in a given language sounds like a configuration bug that should be prevented, IMO.

plach’s picture

Status: Postponed » Active
Issue tags: +Entity Field API

Tagging and unpostponing. I am actually working on this in http://drupalcode.org/sandbox/plach/1719670.git/shortlog/refs/heads/8.x-....

plach’s picture

Issue summary: View changes

adding why this is postponed.

plach’s picture

Status: Active » Needs review
FileSize
53.66 KB

Here is a first draft, let's see how many failures we have.

Status: Needs review » Needs work

The last submitted patch, et-api-1810370-46.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
57.05 KB

Rerolled and fixed some test failures. I successfully completed a standard installation with the patch applied, hence hopefully the failure in #46 was just a bot hiccup.

Status: Needs review » Needs work

The last submitted patch, et-api-1810370-48.patch, failed testing.

plach’s picture

No idea of what's happening :(

Gábor Hojtsy’s picture

Is the issue summary up to date in terms of the changes proposed?

+++ b/core/includes/entity.api.phpundefined
@@ -241,6 +241,37 @@ function hook_entity_update(Drupal\Core\Entity\EntityInterface $entity) {
+ */
+function hook_entity_translation_insert(\Drupal\Core\Entity\EntityInterface $translation) {
...
+ */
+function hook_entity_translation_delete(\Drupal\Core\Entity\EntityInterface $entity, $langcode) {

If the module is named translation_entity, why name these hooks the other way?

plach’s picture

Is the issue summary up to date in terms of the changes proposed?

Not exactly, I am more or less implementing @Crell's suggestion from #30.

If the module is named translation_entity, why name these hooks the other way?

Because this is the entity system Entity Translation API, which the translation_entity module just builds on.

plach’s picture

I'll try to get in touch with @jthorston or @rfay to track down the testbot failure, meanwhile early reviews would be welcome :)

Gábor Hojtsy’s picture

Status: Needs work » Needs review

#48: et-api-1810370-48.patch queued for re-testing.

Gábor Hojtsy’s picture

I tried installing manually as well locally with this patch and noticed no issues.

Status: Needs review » Needs work

The last submitted patch, et-api-1810370-48.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
61.69 KB
11.49 KB

This should fix many failures.

Status: Needs review » Needs work

The last submitted patch, et-api-1810370-57.patch, failed testing.

YesCT’s picture

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.phpundefined
@@ -1046,41 +1047,48 @@ public function isTranslatable() {
-   * Implements \Drupal\Core\Entity\EntityInterface::getOriginal().
+   * Implements \Drupal\Core\TypedData\TranslatableInterface::getOriginal().

yep these implements changes agree with the use that is in ViewUI.php:
use Drupal\Core\TypedData\TypedDataInterface;

but.. they can also all just be {@inheritdoc}
https://drupal.org/node/1354#inheritdoc

andypost’s picture

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -496,6 +501,28 @@ protected function savePropertyData(EntityInterface $entity) {
+      switch ($translation->getTranslationStatus()) {
+        case TranslatableInterface::TRANSLATION_CREATED:
+          $this->invokeHook('translation_insert', $translation);
+          break;
+
+        case TranslatableInterface::TRANSLATION_REMOVED:
+          $this->invokeHook('translation_delete', $entity, $langcode);
+          break;
+      }

so no update?

plach’s picture

Status: Needs work » Needs review

so no update?

We already have hook_entity_update() for that, we don't track update per-language at the moment. Do you think there are uses cases for this? In D7 I never needed it so far.

fago’s picture

I think we missed an issue for language fallback right? So I created one: #2019055: Switch from field-level language fallback to entity-level language fallback

plach’s picture

Well, no, I was planning to add that in the next iteration. I'd be tempted to mark #2019055: Switch from field-level language fallback to entity-level language fallback as a duplicate of this one.

Let's go on with two issues :)

plach’s picture

Status: Needs review » Needs work

The last submitted patch, et-api-1810370-64.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
62.61 KB

Straight reroll. Not that many failures here, let's try a new testing round.

Status: Needs review » Needs work

The last submitted patch, et-api-1810370-66.patch, failed testing.

plach’s picture

FileSize
63.69 KB

This is more like it :)

Here are some more fixes.

plach’s picture

Status: Needs work » Needs review

The last submitted patch, et-api-1810370-68.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

Broken bot, I guess.

The last submitted patch, et-api-1810370-68.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
75.56 KB
18.25 KB

Unless I just broke something else, we should be almost done.

Status: Needs review » Needs work

The last submitted patch, et-api-1810370-73.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
82.27 KB
6.45 KB

This should hopefully fix the last failures.

The last submitted patch, et-api-1810370-75.patch, failed testing.

Berdir’s picture

#75: et-api-1810370-75.patch queued for re-testing.

Berdir’s picture

One time login link no longer valid assertion failure, sounds like as random as it gets to me.

plach’s picture

FileSize
94.73 KB

Added a brand new unit test to provide test coverage for the new stuff. I'm pretty happy with the current patch. Reviews welcome.

Status: Needs review » Needs work

The last submitted patch, et-api-1810370-79.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
95.43 KB
717 bytes

Fixed the node translation failure. The other looks unrelated and here the test is green.

Kristen Pol’s picture

I read the issue summary but I'm unclear as to how to test this patch manually. Write a custom module that calls these new functions? Or, is it only in need of code review? If the latter, than it's beyond me ;)

plach’s picture

@Kristen Pol:

Thanks for your efforts but, yes, we just need code reviews now :)

Writing a module using the new API just to test it would be pointless IMHO, because we already have dedicated unit tests.

Kristen Pol’s picture

@plach - Gotcha! That's what I figured :) Hope the code review goes well!!

andypost’s picture

+++ b/core/includes/entity.api.phpundefined
@@ -241,6 +241,37 @@ function hook_entity_update(Drupal\Core\Entity\EntityInterface $entity) {
+ * @param \Drupal\Core\Entity\EntityInterface $translation
...
+function hook_entity_translation_insert(\Drupal\Core\Entity\EntityInterface $translation) {
...
+ * @param \Drupal\Core\Entity\EntityInterface $entity
...
+function hook_entity_translation_delete(\Drupal\Core\Entity\EntityInterface $entity, $langcode) {

I think full-namespace is enough in doc-blcok, so hook_entity_translation_insert(EntityInterface $translation)

Also why $translation with entityinterface? Maybe better TranslationEntityInterface?

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -551,7 +578,7 @@ protected function mapToStorageRecord(EntityInterface $entity) {
-  protected function mapToRevisionStorageRecord(ComplexDataInterface $entity) {
+  protected function mapToRevisionStorageRecord(EntityInterface $entity) {

@@ -574,10 +601,10 @@ protected function mapToRevisionStorageRecord(ComplexDataInterface $entity) {
-    $default_langcode = $entity->language()->langcode;
+    $default_langcode = $entity->getOriginal()->language()->langcode;
...
-    $translation = $entity->getTranslation($langcode, FALSE);
+    $translation = $entity->getTranslation($langcode);

+1

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -294,10 +295,13 @@ public function language() {
-  public function getTranslation($langcode, $strict = TRUE) {
+  public function getTranslation($langcode) {
     // @todo: Replace by EntityNG implementation once all entity types have been

@@ -314,7 +318,7 @@ public function translations() {
-  public function getTranslationLanguages($include_default = TRUE) {
+  public function getTranslationLanguages($include_default = TRUE, $include_removed = FALSE) {
     // @todo: Replace by EntityNG implementation once all entity types have been

Maybe @deprecated? but needs href to issue

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -558,13 +738,19 @@ public function createDuplicate() {
   public function __clone() {
-    $this->bcEntity = NULL;
...
+    // Avoid deep-cloning when we are initializing a translation object, since
+    // it will represent the same entity, only with a different active language.
+    if (!$this->translationInit) {
+      $this->bcEntity = NULL;

Suppose more readable
if ($this->translationInit) {
//Avoid..
return;
}

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.phpundefined
@@ -1047,6 +1048,48 @@ public function isTranslatable() {
+  public function getOriginal() {
+    return $this->storage->getOriginal();
...
+  public function hasTranslation($langcode) {
+    return $this->storage->hasTranslation($langcode);
...
+  public function addTranslation($langcode, array $values = array()) {
+    return $this->storage->addTranslation($langcode, $values);
...
+  public function removeTranslation($langcode) {
+    $this->storage->removeTranslation($langcode);
...
+  public function getTranslationStatus() {
+    return $this->storage->getTranslationStatus();

is this mapping really needed? Maybe better to implement Base class?

plach’s picture

Thanks for reviewing!

I think full-namespace is enough in doc-blcok, so hook_entity_translation_insert(EntityInterface $translation)

Well, I couldn't find any directive in the coding standards but the rest of entity.api.php is using full namespaces, hence for consistency I kept those. I think the reason is that that code would work as-is whereas without the full namespace we'd need a use statement.

Also why $translation with entityinterface? Maybe better TranslationEntityInterface?

The whole point of this issue is unfying the handling of original and translated values and being able to pass a translation object around as if it were an entity object (actually it is). Thus a different interface is the last thing we want :)

Maybe @deprecated? but needs href to issue

Not sure what you are referring to. Can you elaborate?

is this mapping really needed? Maybe better to implement Base class?

Yes, it is. ViewUI implements EntityInterface but shares no ancestry with any other entity object, it's just a decorator around the View object.

The last submitted patch, et-api-1810370-86.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

#86: et-api-1810370-86.patch queued for re-testing.

plach’s picture

The issue summary is now up to date. Again, reviews welcome :)

plach’s picture

Issue summary: View changes

Updated issue summary

The last submitted patch, et-api-1810370-86.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

#86: et-api-1810370-86.patch queued for re-testing.

Bot, be good :(

Status: Needs review » Needs work

The last submitted patch, et-api-1810370-86.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

Every time a different failure, this is crazy :(

Status: Needs review » Needs work

The last submitted patch, et-api-1810370-86.patch, failed testing.

plach’s picture

#86: et-api-1810370-86.patch queued for re-testing.

Crell’s picture

Status: Needs review » Needs work

I haven't reviewed the entire patch, but the getTranslation() /__clone() logic from #86 looks good to me. My only comment would be to restructure the code to follow the usual if (empty($list[$key]) { $list[$key] = $whatever; } return $list[$key]; idiom. That way there's only one return statement instead of 2.

Otherwise, nice work, plach!

plach’s picture

Status: Needs work » Needs review
Berdir’s picture

Looks good to me in general, certainly better than the current special EntityTranslation class. Various coding style related feedback below, a bit about method names.

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -504,6 +509,28 @@ protected function savePropertyData(EntityInterface $entity) {
+   * @param \Drupal\Core\Entity\EntityInterface $entity

Missing description.

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -294,10 +295,13 @@ public function language() {
    * Implements \Drupal\Core\TypedData\TranslatableInterface::getTranslation().
+   *
+   * @return \Drupal\Core\Entity\EntityInterface

The @return was probably added by an IDE, not necessary.

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -588,4 +592,41 @@ public static function postLoad(EntityStorageControllerInterface $storage_contro
+   * Implements \Drupal\Core\TypedData\TranslatableInterface::getOriginal().

Should be {@inheritdocs} for this and those below.

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -588,4 +592,41 @@ public static function postLoad(EntityStorageControllerInterface $storage_contro
+  public function getOriginal() {
+    return $this->getTranslation(Language::LANGCODE_DEFAULT);

Not sure about the name. We already have $entity->original, and it's something completely different. Maybe getDefaultLangage() or getOriginalLanguage()? A bit confusing as it returns the entity, not the language...

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -7,7 +7,10 @@
+use Drupal\Component\Uuid\Uuid;

That seems like an accidental merge thing.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -76,12 +79,43 @@ class EntityNG extends Entity {
+   * @var boolean

bool

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -76,12 +79,43 @@ class EntityNG extends Entity {
+  protected $translationInit = FALSE;

I think we usually don't shorten variable names, so it should probably be translationInitialized?

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -111,10 +159,20 @@ protected function init() {
+   * Clear entity translation object cache to ensure we do not have stale references.
+   */
+  protected function clearTranslationCache() {
+    foreach ($this->translations as &$translation) {
+      unset($translation['entity']);
+    }
...
+    $this->clearTranslationCache();

Should we do this on __sleep() instead, seems unecessary to serialize it and then throw it away on unserialize?

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -323,12 +380,38 @@ public function isEmpty() {
+      ->access($this, $operation, $this->activeLangcode, $account);

Can you check how this relates to #1953892: Move language node access logic into NodeAccessController, in terms of language negotiation.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -340,75 +423,169 @@ public function language() {
+    // Populate entity translation object cache so it will be available for all
+    // translation objects.
+    if ($langcode == $this->activeLangcode) {
+      $this->translations[$langcode]['entity'] = $this;
+    }
+
+    // If we already have a translation object for the specified language we can
+    // just return it.
+    if (isset($this->translations[$langcode]['entity'])) {
+      return $this->translations[$langcode]['entity'];

We always execute the first condition, so we re-set it again? Couldn't we just as well simply do a return $this here?

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -340,75 +423,169 @@ public function language() {
+    // TODO Do we want to return $this instead?
+    throw new InvalidArgumentException("Invalid '$langcode' specified.");

Not sure :)

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -340,75 +423,169 @@ public function language() {
+   * @return boolean

bool

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -340,75 +423,169 @@ public function language() {
+   * Implements \Drupal\Core\TypedData\TranslatableInterface::getTranslationLanguages().

@inheritdoc

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -566,6 +750,9 @@ public function __clone() {
+    $this->bcEntity = NULL;

We're currently doing this manually in getTranslation(), right? could we move it above the check, so that we do it always?

+++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.phpundefined
@@ -42,15 +61,66 @@ public function getTranslationLanguages($include_default = TRUE);
+   * @return bool

I'm not sure what exactly the coding standard says for simple returns like this, but I fear you need to repeat it..

+++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.phpundefined
@@ -42,15 +61,66 @@ public function getTranslationLanguages($include_default = TRUE);
+   * @param string $langcode
+   */
+  public function removeTranslation($langcode);
...
+   * @param string $langcode
+   */
+  public function initTranslation($langcode);

Same.

+++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.phpundefined
@@ -42,15 +61,66 @@ public function getTranslationLanguages($include_default = TRUE);
+   * @return integer

int.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.phpundefined
@@ -7,9 +7,10 @@
 use Drupal\Core\Language\Language;
+use Drupal\Core\TypedData\TranslatableInterface;
+use Exception;
+use InvalidArgumentException;

Don't use Exception/InvalidArgumentException (the second is already here, not sure if you're touching the code that uses it), inline with \Exception.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUnitTestBase.phpundefined
@@ -21,8 +22,23 @@
+   * The state service.
+   *
+   * @var \Drupal\Core\KeyValueStore\KeyValueStoreInterface
+   */
+  protected $state;

Given that you document it here, should we also load it be default here?

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUnitTestBase.phpundefined
@@ -63,4 +79,34 @@ protected function createUser($values = array(), $permissions = array()) {
+   * Returns the entity_test hook invocation info recorded through the state service.

> 80 char. Maybe just leave the state service part out, that seems like an unecessary implementation detail?

+++ b/core/modules/system/tests/modules/entity_test/entity_test.moduleundefined
@@ -432,3 +432,48 @@ function entity_test_entity_operation_alter(array &$operations, EntityInterface
+ * @param string $hook
+ *   The hook name.
+ *
+ * @param mixed $data
+ *   Arbitrary data associated to the hook invocation.

No space between the two.

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.phpundefined
@@ -1047,6 +1048,48 @@ public function isTranslatable() {
+   * Implements \Drupal\Core\TypedData\TranslatableInterface::getOriginal().

More that should be @inheritdoc.

fago’s picture

Great work. The overall approach looks good to me.

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -294,10 +295,13 @@ public function language() {
-  public function getTranslation($langcode, $strict = TRUE) {

So strict mode goes away - is that problem somewhere?

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -111,10 +159,20 @@ protected function init() {
+  protected function clearTranslationCache() {
+    foreach ($this->translations as &$translation) {
+      unset($translation['entity']);
+    }
+  }
+
+  /**
    * Magic __wakeup() implementation.
    */
   public function __wakeup() {
     $this->init();

This looks like it should be done on __sleep() to prevent that data being serialized.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -323,12 +380,38 @@ public function isEmpty() {
   public function language() {
+    if ($this->activeLangcode != Language::LANGCODE_DEFAULT) {
+      $languages = language_list(Language::STATE_ALL);
+      if (isset($languages[$this->activeLangcode])) {
+        return $languages[$this->activeLangcode];
+      }
+    }

If this returns the active language, how do I get the original language then? I guess we should have getDefaultLanguage() or so.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -340,75 +423,169 @@ public function language() {
+    $entity = entity_create($this->entityType(), $default_values);
+    foreach ($entity as $name => $field) {
+      if (!isset($values[$name]) && !$field->isEmpty()) {

I think that can be done just by $field->applyDefaultValue() calls on the objects. If we need the hook, I guess it would make sense to have a separate one or use the same one with a langcode parameter?

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -458,17 +635,18 @@ public function updateOriginalValues() {
   public function &__get($name) {
+    $this->checkTranslationStatus();

I fear that's going to be expensive - we'll need to performance test it. Cannot the field values be removed when the translation is removed and the check be done on object creation only?

+++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.php
@@ -24,12 +39,16 @@ public function language();
+   * @param bool $include_removed
+   *   Whether languages referring to removed translations should be included.

Why would I bother with removed translations? To get the changes? Cannot that be derived via $entity->original? Having a $translation that says it's removed is a bit confusing to me.

+++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.php
@@ -42,15 +61,66 @@ public function getTranslationLanguages($include_default = TRUE);
+   * @todo Remove this as soon as translation metadata have been converted to
+   *    regular fields.
+   *
+   * @param string $langcode
+   */
+  public function initTranslation($langcode);

Having that in addition to addTranslation() seems weird to me, but good to know it will go away.

plach’s picture

Thanks for reviewing! The attached patch should fix almost everything noted above, there might be some failures due to the last merges. Some answers below.

@Berdir:

The @return was probably added by an IDE, not necessary.

No, actually it is needed to cast the returned value from TranslatableInterface to EntityInterface.

Should we do this on __sleep() instead, seems unecessary to serialize it and then throw it away on unserialize?

We'd need to provide the list of fields to be serialized. Added a @todo about this, see #2027795: Optimize content entity serialization.

We always execute the first condition, so we re-set it again? Couldn't we just as well simply do a return $this here?

Nope, because by populating the cache we make the reference available to all the translation objects, which wouldn't happen if we just returned.

Not sure about the name. We already have $entity->original, and it's something completely different. Maybe getDefaultLangage() or getOriginalLanguage()? A bit confusing as it returns the entity, not the language...

I agree having both is confusing but I think that $entity->original is the culprit actully: it should be called $entity->unchanged or better $entity->getUnchanged(). Checked with @Gabor and he agrees getOriginal() makes sense as a name.

@fago:

So strict mode goes away - is that problem somewhere?

Apparently not :)

I think that can be done just by $field->applyDefaultValue() calls on the objects. If we need the hook, I guess it would make sense to have a separate one or use the same one with a langcode parameter?

Yes, we want the hook to be applied to multiple languages. A single hook is better fo DX as people does not need to care about multilingual, and we don't need a language parameter because we have the translation object language :)

Why would I bother with removed translations? To get the changes? Cannot that be derived via $entity->original?

Great idea. I was really bugged by the need of exposing the status concept in the public API. I completely got rid of it and used the unchanged entity instead.

PS: The interdiff does not contain the latest merges but there shouldn't be relevant changes.

plach’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -461,43 +485,46 @@ public function getTranslation($langcode) {
+      throw new \InvalidArgumentException("Invalid '$langcode' specified.");

We need to improve this message :)

Status: Needs review » Needs work

The last submitted patch, et-api-1810370-100.patch, failed testing.

plach’s picture

Status: Needs work » Needs review

Failing tests are likely to be fixed (at least in part) by #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest. Reviews are welcome meanwhile.

plach’s picture

The last submitted patch, et-api-1810370-100.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
97.52 KB

Reroll

Status: Needs review » Needs work

The last submitted patch, et-api-1810370-106.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
98.87 KB
3.27 KB

This should (ought to) be green.

Status: Needs review » Needs work

The last submitted patch, et-api-1810370-108.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
98.59 KB
934 bytes

This should fix the last failure

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -369,77 +467,169 @@ public function onChange($property_name) {
+      // If the requested translation is valid, we instantiate a new translation
+      // object being a clone of the current one but with the specified language
+      // as active language. Before cloning we specify we are initializing a
+      // translation object to perform a shallow clone, in fact all the field
+      // data structures need to be shared among the translation objects to
+      // ensure all of them deal with fresh data.
+      if (isset($this->translations[$langcode])) {
+        $this->translationInitialize = TRUE;
+        $translation = clone $this;
+        $translation->activeLangcode = $langcode;
+        // Ensure that changes to fields, values and translations are propagated
+        // to all the translation objects.
+        // @todo Consider converting these to ArrayObject.
+        $translation->values = &$this->values;
+        $translation->fields = &$this->fields;
+        $translation->translations = &$this->translations;
+        $translation->translationInitialize = FALSE;
+        $this->translations[$langcode]['entity'] = $translation;
+        $this->translationInitialize = FALSE;
+      }
+      else {
+        // If we were given a valid language and there is no translation for it,
+        // we return a new one.
+        $languages = language_list(Language::STATE_ALL);
+        if (isset($languages[$langcode])) {
+          // If the entity or the requested language  is not a configured
+          // language, we fall back to the entity itself, since in this case it
+          // cannot have translations.
+          $translation = empty($this->getDefaultLanguage()->locked) && empty($languages[$langcode]->locked) ? $this->addTranslation($langcode) : $this;
+        }
       }

for @xjm to review.

xjm’s picture

Well, I still don't entirely quite understand why we don't create our own method rather than using clone, but @berdir and @plach both indicated it won't work. So, what I'm picturing is something like:

if (isset($this->translations[$langcode])) {
  $translation = $this->initializeTranslation($langcode);
}

and then


/**
 * Instantiates a translation object for an existing translation.
 *
 * The translated entity will be a clone of the current entity with the 
 * specified $langcode. All translations share the same field data
 * structures to ensure that all of them deal with fresh data.
 *
 * @param string $langcode
 *   The language code for the requested translation.
 *
 * @return WhateverInterface
 *   The translation object. The content properties of the translation object
 *   are stored as references to the main entity.
 *   
 */
protected function initializeTranslation($langcode) {
  // If the requested translation is valid, clone it with the current language 
  // as the active language. The $translationInitialize flag triggers a shallow
  // (non-recursive) clone.
  $this->translationInitialize = TRUE;
  $translation = clone $this;
  $translation->activeLangcode = $langcode;

  // Ensure that changes to fields, values and translations are propagated
  // to all the translation objects.
  // @todo Consider converting these to ArrayObject.
  $translation->values = &$this->values;
  $translation->fields = &$this->fields;
  $translation->translations = &$this->translations;
  $translation->translationInitialize = FALSE;
  $this->translations[$langcode]['entity'] = $translation;
  $this->translationInitialize = FALSE;
}

I think have a separate protected method would make the code easier to understand. @plach suggested a reason this might not be a good idea but I didn't quite follow, so putting the example here for people to read at least. :)

(NB, the wrapping is not correct in the comments. I typed this in the issue.)

plach’s picture

FileSize
99.96 KB

I had a final review with @Berdir and @Xjm, the interdiff would be pretty big as we are renaming a method so skipping it. Hopefullly this will still be green.

plach’s picture

Status: Needs review » Needs work

The last submitted patch, et-api-1810370-115.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
101.15 KB
1.29 KB

Obviously a BC decorator issue...

plach’s picture

I did some profiling after speaking with @Berdir.

plach’s picture

Status: Needs review » Needs work

The last submitted patch, et-api-1810370-119.patch, failed testing.

kfritsche’s picture

Status: Needs work » Needs review
FileSize
8.07 KB
100.77 KB

Rerolled 119 worked but it fails because the patch itself inserts langcode's. I changed the new langcode's now to id (see #1754246: Languages should be configuration entities).

When I didn't missed anything this should go green again.

Status: Needs review » Needs work

The last submitted patch, et-api-1810370-121.patch, failed testing.

plach’s picture

Thanks! Another reroll.

plach’s picture

Status: Needs work » Needs review
FileSize
100.77 KB

Actually it was just a random fail, uploading a fresh reroll as #121 does not apply anymore.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityInterface.phpundefined
@@ -323,4 +323,15 @@ public function getNGEntity();
+   * @todo Remove this as soon as translation metadata have been converted to

has been.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -26,6 +27,21 @@
   /**
+   * Status code indentifying a removed translation.
+   */
+  protected static $TRANSLATION_REMOVED = 0;

As discussed, let's change this to a constant.

I'e done some performance tests, and there's no measurable difference. 2.21 [#/sec] (8.x), 2.17 [#/sec] with the patch, with 500 requests on the frontpage with 50 nodes. We have 3000 calls to getUntranslated(), this is the biggest slowdown according to xhprof, 2998 of those are due to the BC decorator and should go away if we remove that. So we are good there.

plach’s picture

Status: Needs work » Needs review
FileSize
100.73 KB
4.4 KB

Actually since metadata technically is plural I changed another @todo to be consistent :)

Restored public constants, although I don't think this is actually an improvement as we are exposing an implementation detail, which has no place in the public API: if we change the internals of EntityNG, we might be forced to leave those constants there, even if we could remove them, to avoid API changes.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -153,7 +153,7 @@ public function __construct(array $values, $entity_type, $bundle = FALSE, $trans
-    $data = array('status' => self::$TRANSLATION_EXISTING);
+    $data = array('status' => self::TRANSLATION_EXISTING);

Sooo... @alexpott says this should be static::TRANSLATION_EXISTING.

plach’s picture

Berdir’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs architectural review

Ok. We went through this together in detail, did performance checks, detailed reviews of comments and so on. I think this is ready if testbot agrees.

Berdir’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/entity.api.phpundefined
@@ -241,6 +241,35 @@ function hook_entity_update(Drupal\Core\Entity\EntityInterface $entity) {
+function hook_entity_translation_insert(\Drupal\Core\Entity\EntityInterface $translation) {

+++ b/core/includes/entity.api.phpundefined
@@ -241,6 +241,35 @@ function hook_entity_update(Drupal\Core\Entity\EntityInterface $entity) {
+function hook_entity_translation_delete(\Drupal\Core\Entity\EntityInterface $translation) {

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -367,6 +368,9 @@ public function save(EntityInterface $entity) {
+        if ($this->dataTable) {
+          $this->notifyTranslationChanges($entity);

@@ -486,6 +490,28 @@ protected function savePropertyData(EntityInterface $entity) {
+  function notifyTranslationChanges(EntityInterface $entity) {

I'm not sure about these hook's being fired from the DatabaseStorageController. That couples them with our storage engine and this seems an odd decision given prior refactoring in #1893772: Move entity-type specific storage logic into entity classes...

OTOH (on the other hand) if this should be here...

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.phpundefined
@@ -486,6 +490,28 @@ protected function savePropertyData(EntityInterface $entity) {
+   * Checks translation statuses and invoke the related hooks if needed.
+   *
+   * @param \Drupal\Core\Entity\EntityInterface $entity
+   *   The entity being saved.
+   */
+  function notifyTranslationChanges(EntityInterface $entity) {

Missing scope - I think it should be on the interface so all storage controllers support translations...

Berdir’s picture

Discussed with @alexpott.

- The notify should be at the same place as the other update hook invocations.
- Move the implementation into the base class, so that a different implementation can easily re-use it, make it protected.
- Check with @chx of that's ok with him and in line with his changes.

plach’s picture

Thanks @alexpott and @berdir for taking some more time to discuss this. Here is a new one moving the translation hooks method in the base class and renaming it for consistency with the other available invoke* hooks.

chx is not available at the moment, but I checked his sandbox and the DatabaseStorageControllerNG::save() method appears identical to me, so this change should be fine. Uploading the patch meanwhile, so the testbot can have a look to it.

plach’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Ok, talked with @chx, and he is fine with having this here.

What might happen is that we internally refactor the save method and move more into the base class, but that is something that can be done internally and later on. But it will be called from the storage controller and will stay together with the other entity update hooks.

Therefore, back to RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

plach’s picture

Title: Entity Translation API improvements » Change notice: Entity Translation API improvements
Status: Fixed » Active
Issue tags: +Needs change record

Awesome, thanks!

YesCT’s picture

cool. I was about to post this. It's a follow-up instead. :)
#2031917: Docs follow-up Entity Translation API improvements

plach’s picture

Status: Active » Needs review
Berdir’s picture

Title: Change notice: Entity Translation API improvements » Entity Translation API improvements
Status: Needs review » Fixed
Issue tags: -Needs change record

Looks great! This is almost too nice for a mere change notice. Want to extract the part that explains how it works now and create a doc page below https://drupal.org/documentation/modules/entity ? (That needs a lot of work and probably a completely separate page for D8 but it can easily be moved around later.

plach’s picture

I copied the D8 pieces in https://drupal.org/node/2040721.

Berdir’s picture

Issue tags: -sprint

Untagging.

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

Anonymous’s picture

Issue summary: View changes

Spelling mistake