Download & Extend
Issues

Entity Translation API improvements

Posted by YesCT on October 11, 2012 at 7:55pm
Project:Drupal core
Version:8.x-dev
Component:entity system
Category:task
Priority:major
Assigned:plach
Status:active
Issue tags:API clean-up, D8MI, Entity Field API, language-content, Needs architectural review, Post NG clean-up, sprint, translatable fields

Issue Summary

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

Problem

  • 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:
    • EntityInterface::hasTranslation($langcode)
    • EntityInterface::addTranslation($langcode, $values = array())
    • EntityInterface::removeTranslation($langcode)
  2. After adding or removing translations, when storing the updated entity, fire these hooks:
    • hook_entity_translation_create()
    • hook_entity_translation_delete()
  3. Rename the EntityTranslation class to EntityLanguageDecorator (ELD) and refactor it to implement EntityInterface and be just a simple wrapper around the entity object. The only difference for API consumers is that the value returned by EntityLanguageDecorator::language() will differ from the one returned by EntityInterface::language(). When retrieving a translation corresponding to the entity language the entity object itself is returned. This way in monolingual contexts (or when dealing with the original values) we avoid additional method invocations.
  4. Change the entity storage to enforce the existence of 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

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. Obviously entity-type specific portions of code will expect certain fields to be defined, but this requirement can be fully satisfied by a decorator object proxying every method call to the actual entity object. It's just matter of wrapping the expected entity type.

One tricky aspect to address in the current proposal is dealing with translation removal. When trying to remove the translation corresponding to the active ELD, we need to invalidate the decorator, probably by setting an internal flag. In this scenario any invocation of the (magic) accessor methods following a translation removal implies throwing an exception: basically the decorator 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 objects or ELDs. Another dedicated decorator will be needed so that we can reuse it in different contexts, such as rendering or (possibly?) serialization.

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.

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

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

Change records for this issue

Login or register to post comments

Comments

#1

Posted by YesCT on October 11, 2012 at 7:56pm

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.

#2

Posted by plach on November 5, 2012 at 12:31am
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

#3

Posted by plach on November 5, 2012 at 12:31am
Status:postponed» active

#4

Posted by plach on November 5, 2012 at 4:06am
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 to:Anonymous» plach

#5

Posted by fago on January 6, 2013 at 12:44pm

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

#6

Posted by plach on January 28, 2013 at 12:29am
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.

#7

Posted by fago on January 29, 2013 at 3:28pm

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?

#8

Posted by plach on January 29, 2013 at 4:18pm

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.

#9

Posted by sun on January 29, 2013 at 6:25pm

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.

#10

Posted by yched on January 29, 2013 at 9:31pm

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 ?

#11

Posted by plach on January 29, 2013 at 11:56pm

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

#12

Posted by sun on January 29, 2013 at 11:20pm
Issue tags:+Needs architectural review

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

#13

Posted by plach on January 30, 2013 at 5:12pm
Issue tags:+sprint

Tagging for sprint to get more eyes on this.

#14

Posted by fago on January 30, 2013 at 5:42pm

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

#15

Posted by plach on January 30, 2013 at 11:44pm

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.

#16

Posted by yched on January 30, 2013 at 8:11pm

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

#17

Posted by plach on January 30, 2013 at 11:45pm

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

#18

Posted by plach on January 31, 2013 at 5:05pm
Component:translation_entity.module» entity system

Moving to the right queue.

#19

Posted by plach on February 2, 2013 at 1:20am

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.

AttachmentSizeStatusTest resultOperations
et-api-2013_02_01.log_.txt19.8 KBIgnored: Check issue status.NoneNone

#20

Posted by plach on February 2, 2013 at 1:21am
Status:active» postponed

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

#21

Posted by plach on February 24, 2013 at 9:50am
Issue tags:+Post NG clean-up

#22

Posted by msonnabaum on April 1, 2013 at 7:51pm

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.

#23

Posted by plach on April 2, 2013 at 12:37am

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.

#24

Posted by tim.plunkett on April 2, 2013 at 1:32am

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.

#25

Posted by plach on April 2, 2013 at 1:44am

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.

#26

Posted by tim.plunkett on April 2, 2013 at 3:02am

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.

#27

Posted by plach on April 2, 2013 at 8:17am

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.

#28

Posted by Gábor Hojtsy on April 2, 2013 at 8:36am

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.

#29

Posted by plach on April 2, 2013 at 8:53am

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

#30

Posted by Crell on April 7, 2013 at 1:19am

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:

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

#31

Posted by plach on April 7, 2013 at 11:50am

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

#32

Posted by Crell on April 7, 2013 at 6:14pm

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

#33

Posted by plach on April 10, 2013 at 1:05pm

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

#34

Posted by Crell on April 13, 2013 at 10:48pm

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

#35

Posted by Berdir on April 13, 2013 at 10:50pm

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

#36

Posted by Crell on April 13, 2013 at 11:23pm

Oh. That. FAPI--

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

#37

Posted by fago on April 17, 2013 at 9:56am

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.

#38

Posted by Crell on April 18, 2013 at 11:38pm

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:

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

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

#39

Posted by plach on April 19, 2013 at 1:22am

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

#40

Posted by Crell on April 19, 2013 at 6:30am

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.

#41

Posted by fago on April 19, 2013 at 7:54am

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?

#42

Posted by YesCT on May 4, 2013 at 11:40pm

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

#43

Posted by YesCT on May 4, 2013 at 11:42pm

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

#44

Posted by Crell on May 6, 2013 at 3:59am

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

#45

Posted by plach on May 16, 2013 at 12:03pm
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-....