Switch from field-level language fallback to entity-level language fallback

The link in #1 points back to this issue. Was it intended to point to a different one?

I was going to add language fallback support in the next iteration of #1810370: Entity Translation API improvements but I am fine also with two separate issues. I thought about this for some time and I think we no longer need language fallback with field granularity: in D8 all fields are explictly attached to an entity translation, thus there is no way for a field translation to be missing and another one to be available. We can have an empty field translation, but this should not trigger language fallback IMO.

That said, I think that all we need is falling back to the most appropriate entity translation for a particular context. A very simple implementation of this logic would be:

<?php
  class Entity implements EntityInterface {
    // ...
    
    protected $languageManager; 
    
    public function getCurrentTranslation($languageManager = NULL) {
      $languageManager = isset($languageManager) ? $languageManager : $this->languageManager;
      // LanguageManager::getFallbackCandidates() is the OO version of language_fallback_get_candidates().
      $context = array('entity' => $this);
      $candidates = $languageManager->getFallbackCandidates(Language::TYPE_CONTENT, $context);
      array_unshift($candidates, $languageManager->getLanguage(Language::TYPE_CONTENT)->langcode);
      
      foreach ($candidates as $langcode) {
        if ($this->hasTranslation($langcode)) {
          return $this->getTranslation($langcode);
        }
      }
    }
  }

  // node.module
  function node_page_view(EntityInterface $node) {
    // In procedural code we may need to call this explicitly, but in many scenarios
    // we should be able to pass on the current translation object, hence just
    // $node->label() would work.
    drupal_set_title($node->getCurrentTranslation()->label());
    
    // ...
  }
?>

The example above assumes we already have the #1810370: Entity Translation API improvements.

I'll work on this as soon as we have an agreement.

Here is a first one: it relies on #1810370: Entity Translation API improvements so I provided also a full diff to send it to the bot.

Ok, small improvements. Now bot show me the failures!
(The first one is the one to review)

Good news, no mopre failures than the parent issue: #1810370-100: Entity Translation API improvements. Working on the following plan:

fago: ping
[13:56]	plach	fago: I'd need a quick feedback about https://drupal.org/node/2019055#comment-7534677 before diving deeper into work
[13:58]	fago	plach, hi
[13:58]	fago	*reading*
[13:58]	plach	thanks
[13:59]	fago	plach, what if an entity translation is in-complete? Wouldn't it be useful if I can just e.g. two fields and have all others being NULL fallback?
[14:00]	fago	plach, e.g. think of an image - usually I might keep the default, but if there is some language in there I can upload an alternative it gets picked?
[14:01]	plach	fago: you probably can do it though the API but I din't think there's a way to make this happen through the UI
[14:01]	plach	a value for the translation will be saved anyway, although empty I guess
[14:02]	fago	plach, I think it should save nothing if it's empty, but howsoever - if the API is there that'S enough I think. Contrib could take care of an UI then.
[14:03]	fago	plach, so I think the language fallback rules should be the same for the whole entity (i.e. not per field), but applying them I think we should do per field
[14:03]	fago	plach, it would be even neat if we'd have language fallback rules per entity type as that could be theoretically respected during querying also (-> contrib?)
[14:03]	plach	fago: so we keep the current approach but we check translation availablity per field?
[14:05]	fago	plach, that would make sense to me, yep
[14:05]	fago	plach, I wonder how I'd disable/enable the language fallback?
[14:07]	fago	plach, then I guess the patch should mark field_language() and related stuff as deprecated
[14:07]	plach	fago: ok, but I think it makes no sense to support this at UI level: if I create a translation and I don't provide a value I don't want a value in a different language to appear: the form will be prepopulated with the original values, I can just keep them if I want them to show up
[14:07]	plach	If I erase them I'm explicitly saying I don't want to see them in the translated content
[14:08]	fago	plach, hm, true. IT would have to be a separate checkbox
[14:08]	plach	fago: in contrib?
[14:08]	plach	* in contrib!
[14:08]	fago	plach, yep, contrib could figure that out I think.
[14:09]	plach	fago: about disabling fallback we could support it at configuration level, but I wouldn't provide a UI to set it (I'll check with Gabor)
[14:09]	fago	plach, Also, if you avoid copying the value, it takes any changes to other language into account.
[14:10]	fago	plach, yep - I've not been bothering about that, I mean how'd I do it on the API level?
[14:11]	plach	fago: right now we have a variable_get()
[14:11]	plach	the variable is owned by the Field API
[14:11]	plach	I guess just moving it to config?
[14:11]	plach	and moving to the entity system?
[14:12]	fago	plach, yes, so how woud I do it with new API? $entity->getTranslation() would come with fallbacks, right? How can I disable them, e.g. for ET-forms?
[14:12]	plach	or we could provide an entity key and and alter it torugh config in contrib?
[14:13]	plach	fago: right now we have *$entity->getCurrentTranslation()* wich triggers fallback
[14:13]	fago	plach, oh, got that
[14:15]	plach	fago: probably we don't want a configuration woned by the entity system, I'd go for entity key + info alteration in contrib
[14:15]	plach	* owned
[14:16]	fago	plach, that's quite a confusing name compared to getTranslation() though
[14:16]	plach	fago: any suggestion? I strongly feel they should be separate methods
[14:16]	fago	plach, entity-key named like what?
[14:17]	plach	fago: 'fallback' ?
[14:17]	plach	'language_fallback' ?
[14:18]	fago	plach, you mean in entity-info ? hm, having that as config would make sense to me.
[14:19]	plach	fago: we need it at bundle level, do you want to have a configuration owned by the entity system, really?
[14:19]	plach	anything else right now is info + (alter + config)
[14:20]	fago	plach, hm, I'm unsure. Just thinking how I'd use it - I'D like to configure it?
[14:21]	plach	fago: contrib would provide UI to configure it, store the result in config and alter bundle info based on that
[14:21]	plach	this what we are doing in ET to avoid the entity system know anything about ET
[14:21]	fago	plach, maybe it could work similar as the rdf-mappings, which are config per bundle also? Could it be ET-owned?
[14:22]	fago	plach, I see, that should work I guess
[14:22]	plach	fago: to be ET-owned we need a hook or something like that to avoid hard-coding an explict dependency
[14:22]	fago	plach, yep, hook+ET config sounds good
[14:23]	plach	fago: but then given that ET won't provide a UI for it, can't it be just contrib config?
[14:24]	fago	plach, hm. what's weird about a hook is that we can have just one set of fallback rules, not multiple ones... so it really can be just one module defining it
[14:26]	plach	fago: are you referring to language_get_fallback_candidates()?
[14:28]	fago	plach, in general - we can only have one set of fallback rules applied, so if multiple module try to provide one we have to pick? Or do we just have a default and allow modules to alter?
[14:30]	fago	plach, so we could have system wide default language fallback based on config, e.g. owned by language , and just allow altering that by contrib?
[14:30]	plach	fago: the current patch lets any module to alter the fallback candidates array, each one can implements its own fallback logic, obviously they have to play well together
[14:30]	plach	we are passing a context
[14:30]	plach	so modules might be able to able to act only when they are interested
[14:30]	plach	passing an operation name might be useful
[14:30]	plach	fago: **
[14:31]	plach	^^
[14:31]	fago	plach, true
[14:31]	fago	plach, ok, works for me. I think some sort of system-wide defaults would be useful, but it's really not a must have
[14:32]	plach	fago: we have those: we use language weight
[14:32]	plach	it's even configurable throu the UI
[14:32]	fago	plach,well that's something
[14:32]	fago	plach, seems reasonable then
[14:32]	plach	cool
[14:32]	plach	will work on this today

As this discussed with the D8MI team, this is part of 4 critical issues for D8MI:

• #1497374: Switch from Field-based storage to Entity-based storage
• #1810370: Entity Translation API improvements
• #2019055: Switch from field-level language fallback to entity-level language fallback
• #2004626: Make non-configurable field translation settings available in the content language settings

Still many things to fix but I wanted to see how many fails we have.

Just a reroll against head. I expect more or less the same failures. Fixing some missing stuff meanwhile.

Missing stuff:

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -135,12 +136,34 @@ class EntityNG extends Entity {
+    $this->languageFallbackManager = \Drupal::service('language_fallback_manager');

Proper fallback manger injection.

+++ b/core/lib/Drupal/Core/Entity/EntityNG.php
@@ -565,7 +621,54 @@ protected function initializeTranslation($langcode) {
+    // TODO
+    $context = array();

Proper $context initialization in EntityNG::getCurrentTranslation().

+++ b/core/lib/Drupal/Core/Language/NoFallbackManager.php
@@ -0,0 +1,43 @@
+    return array(Language::LANGCODE_NOT_SPECIFIED);

Test coverage, including for the no fallback manager.

+++ b/core/modules/field/field.api.php
@@ -465,6 +465,8 @@ function hook_field_attach_view_alter(&$output, $context) {
+ * TODO remove
+ *

Comments and PHP docs.

Fixed the first two points. Let's see what the bot thinks about this.

Added some test coverage and fixed some bugs. Bot?

This should be better.

Here is my latest work. I expect more failures as properly detecting a NULL value is not exactly easy. I am going to talk to @fago about this tomorrow.

This reverts EntityNG::__isset() changes and tries a different approach: syncing entity values each time they are modified, which allows us to use $entity->values to detect NULL fields.

For perfomance reasons values syncing is limited to multilingual entities. I am attaching a patch removing this check to test the new code more thoroughly.

Tagging as per explicit approval from @Dries in #2004626-38: Make non-configurable field translation settings available in the content language settings.

Added an issue summary describing the current status.

I had an email discussion with @fago about this, posting the relevant excerpts with his permission :)

(me)

I've spent the lats two weeks thinking of/experimenting with two different approaches, both derived from the new ET API improvements that just went in. Both involve having a fallback-enabled translation object that can be passed around as a regular entity/translation object. Enabling fallback means passing on instantiation a $context array with arbitrary data that describe the context where fallback will be needed. The most common case is the "entity_view" context, but we can have for instance a "views_query" context or a (not present in the patch) "entity_serialize" context.

The two approaches differ in the following way:

1) In the first one a fallback translation object is a "snapshot" of the entity object when the fallback object is instantiated. It is a deep-clone of the entity objects and shares no data with it and should be treated as read-only. This is the easiest to implement, as we don't have to worry of making fallback objects and regular translation objects play well together. In fact they are not sharing the same field data, so we dont care about keeping in-sync with modifications in the regular translation objects.

2) In the second scenario fallback objects and translation ones share field data and must be kept in sync. This is the trickiest approach but has the advantage of not implying a different DX when dealing with fallback objects: in monolingual contexts the two work exactly the same way.

At the moment I am definitely leaning towards the second, as the first one implies a burden for the developer that might be totally unjustified in monolingual contexts: having to distinguish between "frozen" fallback objects and regular translation objects in monolingual scenarios is more than what is reasonable to ask to the average devloper.

However the second has a big problem: while in the first approach we can identifying NULL values that should have a fallback in EntityNG::getTranslatedField(), in the second approach, where field data can be altered by regular translation objects, we need to detect NULL values every time we access a field. I managed to find a solution that should have an acceptable performance, but the problem I am facing right now is that I can't tell whether a field is NULL buy just inspecting it, I need to get its items and look into those. But it seems that accessing a NULL field item is enough to make the parent field appear as "set":

<?php
$value = $entity->field_foo->value;
// $isset is TRUE even if $value is NULL.
$isset = isset($entity->field_foo);
?>

And unfortunately inside EntityNG::__get() I cannot access $translation->field_foo->value reliably, but $field->getValue() returns a not NULL value for it. I hope I made myself understood, you can check
EntityNG::__get() and EntityNG::__isset() in the lastest patch (https://drupal.org/files/et-language_fallback-2019055-24.patch) if you need clarifications.

(fago)

So, I'm wondering about multiple things. Let's go through point by point:

a) Do we need to bake the fallback handling into the entity classes? I mostly worried about the complexity in there - the logic ing __get() is already quite complex and I fear that making it even more complex isn't a good way forward. Instead, cannot we separate the fallback logic into a decorator class?
Yes, you cannot pass a decorate into a function expectiong $node, but I don't think you should be able too. While for translations, that's reasonable but for objects involving context-dependent fallback routines I don't think it is. Shouldn't it be enough to be able to render that object?

b) So regardless of a) to the problem of NULL values - yes accessing something creates an empty field - it's still empty, but it's going to bet set. While that's not ideal, I think that's the best we can do for a reasonable good DX, i.e. you can always access $entity->field_foo->value? Still, the semantic meaning of the field being empty is valid. So maybe, should the translation fallback kick in if something is empty? Semantically, I think it should but I fear that won't make things faster. :/

So what we could do here is keeping track of which fields per language are set and react to changes to a field via onChange() - so we always have that information available fast. Having multiple uses of a field fast is what's most important thing I think?

However, as said could we go with a separate decorator class for that? This would mean we'd have to register them in the entity so they can be notified of changes to an entity field for keeping track empty fields per language.

(me)

> a) Do we need to bake the fallback handling into the entity classes?[...]

I understand your concern, initially this scared me too, but I think this is the only way to enure a consistent DX between monolingual and multilingual environments. Otherwise we will have to force developers familiarize with yet another concept that makes sense only in a minority of contexts, that is multilingual sites having entities with missing fields. In my experience this is an uncommon scenario: most of the times either you have a whole translation or you don't have it.

> I mostly worried about the complexity in there

Well, once the BC-decorator stuff is gone we will have just an additional if branch and the test will be just an isset(), so the overhead for non-fallback entity objects should be minimal.

> Yes, you cannot pass a decorate into a function expectiong $node, but I
> don't think you should be able too.

From a theoretical POV I agree, but we can't predict all the possibile use cases, and this sounds as a very strong limitation if we are missing something relevant.

Moreover there's the DX argument above: we'd have to introduce some kind of RenderableEntity decorator which, in practice, would be a black box object people would have to rely on without any apparent benefit in most scenarios. This sounds to me as a perfect recipe for people to forget about using it.

Instead fallback objects are instantiated when negotiating the translation language (see EntityNG::getCurrentTranslation()), which IMHO is a sufficiently common/understandable use-case for people to grasp, even if they usually don't code for multilingual environments. Moreover we can pass already instantiated fallback objects around, so in most cases people might not even know they are dealing with them.

Obviously we must be careful and pass around those only in the proper contexts, but I already made sure a fallback object cannot be stored: a regular entity object is transparently retrieved in the storage controller.

> b) So regardless of a) to the problem of NULL values - yes accessing
> something creates an empty field - it's still empty, but it's going to
> bet set. [...]
> So what we could do here is keeping track of which fields per language
> are set and react to changes to a field via onChange() [...]

Yesterday I went exactly this way: on multilingual entities the $values array is updated every time a field is changed, and if its value is just a single NULL item, the whole field value is set NULL. This is consistent with what happens with EntityNG::getTranslatedField(), AAMOF tests are looking good, just a few failures in FieldSqlStorageTest (see https://drupal.org/node/2019055#comment-7653177).

From the performance POV, checking whether entities are multilingual should limit the overhead in the most common scenarios. We can look for further optimization for multilingual environments later, I guess.

(fago)

>> a) Do we need to bake the fallback handling into the entity classes?[...]
> I understand your concern, initially this scared me too, but I think this is the only way to enure a
> consistent DX between monolingual and multilingual environments. Otherwise we will have to force developers
> familiarize with yet another concept that makes sense only in a minority of contexts, that is multilingual
> sites having entities with missing fields. In my experience this is an uncommon scenario: most of the times
> either you have a whole translation or you don't have it.

Yes - usually you have complete translations, so why do we need the falback handling in the entity class
then? Isn't that mostly for rendering things with the render controller?

>>I mostly worried about the complexity in there
> Well, once the BC-decorator stuff is gone we will have just an additional if branch and the test will be
> just an isset(), so the overhead for non-fallback entity objects should be minimal.

As long as it's not a magic isset(), yep. However, I'm also worried about the class becoming un-maintainable
/ a complex beast.

>> Yes, you cannot pass a decorate into a function expectiong $node, but I
>> don't think you should be able too.
> From a theoretical POV I agree, but we can't predict all the possibile use cases, and this sounds as a very
> strong limitation if we are missing something relevant.

Well, I'd even go further and say it should not be possible to pass in a fallback-$node in any
function/method expecting a real $node. What would be the results if you change something that falls back?
That's quite unpredictable and unwanted side-effects might occur I fear. In the case of active langauge -
that's not so unpredictable and people can easily check for the language - if needed, but you cannot easily
check for fall back mechanims applying on a field or not as it depends on the actual field values?

> Yesterday I went exactly this way: on multilingual entities the $values array is updated every time a field
> is changed, and if its value is just a single NULL item, the whole field value is set NULL. This is
> consistent with what happens with EntityNG::getTranslatedField(), AAMOF tests are looking good, just a few
> failures in FieldSqlStorageTest (see https://drupal.org/node/2019055#comment-7653177).

> From the performance POV, checking whether entities are multilingual should limit the overhead in the most
> common scenarios. We can look for further optimization for multilingual environments later, I guess.

In general doing those updates is feasible even by default I think - writes do not occur often and are not
so performance critical, the reads are + having consistent behaviour is a plus. Howsoever, it's a bit odd
that we map non-NULL field values to NULL here. Cannot we just track whether the field is empty and use that
as fallback criteria? Isn't that the semantically right fallback criteria anyway? If a field is empty in a
translation, it shouldn't be displayed?

@fago:

Yes - usually you have complete translations, so why do we need the falback handling in the entity class
then? Isn't that mostly for rendering things with the render controller?

Yes, that's the main uses case, but another case where applying fallback rules is needed is for instance in node_page_title(). I guess it would be something like the following:

<?php
function node_page_title(EntityInterface $node) {
  return $node->getCurrentTranslation()->label();
}
?>

VS

<?php
function node_page_title(EntityInterface $node) {
  return Drupal::entityManager()->getLanguageFallbackDecorator($node)->label();
}
?>

As long as it's not a magic isset(), yep.

It's not, just an internal entity property :)

However, I'm also worried about the class becoming un-maintainable
/ a complex beast.

Most of the fallback code is confined in the language fallback manager service implementations. The changes in EntityNG involve just an additional method and a couple of ifs.

Well, I'd even go further and say it should not be possible to pass in a fallback-$node in any
function/method expecting a real $node.

I'm not so sure about this. The equivalent of hook_field_attach_view() or hook_field_attach_prepare_view() might behave more consistently if the data structure they receive matches the one that will be used to display the entity.

In general doing those updates is feasible even by default I think - writes do not occur often and are not
so performance critical, the reads are + having consistent behaviour is a plus.

I didn't see a big value in updating the $values array in monolingual contexts but I would not be opposed to it if performance doesn't degrade.

Howsoever, it's a bit odd that we map non-NULL field values to NULL here.

Actually this is exactly symmetric to what we do in EntityNG::getTranslatedField(): if a value is NULL we instantiate a non-NULL field with a single NULL item. This is why I think this makes sense. I don't think there would be another way to detect a NULL field value from a Field object.

Cannot we just track whether the field is empty and use that
as fallback criteria? Isn't that the semantically right fallback criteria anyway? If a field is empty in a
translation, it shouldn't be displayed?

There's a big difference between an empty field translation and a NULL (missing) field translation. In D7 an empty field translation does not trigger fallback and I believe this is the correct behavior, since it respects an explicit choice. AAMOF fallback can be "simulated" by translators by just leaving the original values in place, if they need to for any reason, but they have to be allowed to enter empty values without those triggering fallback if they require to do so.

I had a closer look to the entity/field render-related code. Actually it seems that a decorator might be a good choice, as I wasn't able to identify a recurring pattern for the fallback use-case. Depending on the implemented logic you might want to apply it or not.

The main problem I see with the suggestion above is that there is no way for

<?php
$entity_translation->applyLanguageFallback()->field->value = 'foo';
?>

to throw an exception, as the onChange() method will be called directly on the $entity_translation object, unless we deep-clone the field objects, which might be very bad for performance.

Ok, here is a new approach based on the discussion with @fago. Still needs work but I want to see how many failures we have.

This should be mostly ready from the functional POV. Still a couple of details to be fixed as well as test failures and PHP docs.

Let's get rid of those notices :)

Ok, we should be really done with functional stuff now. Missing test fixes + PHP docs.

Just curious: what becomes of field_language_fallback() and the functions in field.multilingual.inc ?

Also, FYI : opened #2051721: Get rid of field_get_items() - would welcome confirmation of the changes.

Just curious: what becomes of field_language_fallback() and the functions in field.multilingual.inc ?

I am planning to deprecate field_language_fallback() and field_language() in this patch but not removing them to avoid unecessary API changes. We can get rid of those in a follow-up.

In general the functions in field.multilingual.inc will be useless once we have fineshed moving the field.attach.inc code to the entity controllers.

Also, the issue summary needs an update after #32 - #33.

Thanks for those answers, @plach.

[edit: removed stupid question, never mind]

This should fix most failures.

#48: et-language_fallback-2019055-48.patch queued for re-testing.

This one should be green unless some new test has been added meanwhile. Please note that there still some things to fix before this is ready for review, hope to have them done by tomorrow night.

There are still a couple of things I wish to fix (improve test coverage and some minor clean-up) but now we should be mostly ready for review. Hopefully I didn't break anything :)

Merged and (hopefully) fixed failures.

This time for real :)

Another reroll

This should work better.

Another attempt.

This should hopefully be green again.

@yched opened a followup issue for cleaning up what's left in field.multilingual.inc after this patch lands.

Some test coverage improvements and a small clean-up. We should be ready for review now.

This is the follow-up @amateescu was referring to: #2067079: Remove the Field Language API.

Added issue summary

Rerolled and updated the issue summary. Reviews welcome :)

Updated issue summary

Updated issue summary.

I stumbled upon this issue, so here are a few comments.

1. +++ b/core/includes/language.inc
   @@ -542,20 +542,14 @@ function language_url_split_prefix($path, $languages) {
   + * @deprecated This has been deprectaed in favor of the language fallback
   + *   manager.
   

   Second use of deprecated is misspelled "deprectaed"

2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -60,6 +62,22 @@ class Entity implements IteratorAggregate, EntityInterface {
   +  /**
   +   * The language fallback manager to be used to determine the current
   +   * translation and field values fallback.
   

   This could be re-worded a bit to not use fallback in the definition of fallback manager.

   I don't know if I understand it appropriately, but maybe:

   "The language fallback manager to be used to determine the current translation and field values when no translation exists."?

Thanks, merged in the latest changes and addressed #69.

Rerolled

Rerolled

#72: et-language_fallback-2019055-72.patch queued for re-testing.

+++ b/core/core.services.yml
@@ -196,6 +196,9 @@ services:
+  language_fallback_manager:
...
+    arguments: ['@language_manager']

+++ b/core/modules/language/lib/Drupal/language/FallbackManager.php
@@ -0,0 +1,138 @@
+class FallbackManager extends NoFallbackManager {
...
+  public function __construct(LanguageManager $language_manager, ConfigFactory $config, ModuleHandlerInterface $module_handler) {
+    parent::__construct($language_manager);

The fallback manager already knows it's language manager so any reason to set both services on entity and storage controller?

+  public function __construct($entity_type, array $entity_info, Connection $database, LanguageManager $language_manager, FallbackManagerInterface $fallback_manager) {
...
+    $this->languageManager = $language_manager;
+    $this->languageFallbackManager = $fallback_manager;

Maybe better use language manager from fallback manager? $this->languageFallbackManager->getLanguageManager()

Rerolled after #1497374: Switch from Field-based storage to Entity-based storage.

@andypost:

The fallback manager already knows it's language manager so any reason to set both services on entity and storage controller?

I considered this possibility too. The main reason I choose not to go this way was that from a very theoretical POV a language fallback manager implementation might not need to depend on the language manager, while the entity will definitely need both in any case (to retrieve languages, for instance). Makes sense?

#76: et-language_fallback-2019055-76.patch queued for re-testing.

Better reroll

Started looking at this - great work, this should let us clean a lot of ugly stuff !
Still wrapping my head around it a bit, and I still need to read the exchanges with @fago higher.

Just a couple non-fundamental remarks for now:

1. +++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
   @@ -66,6 +69,20 @@ class DatabaseStorageController extends FieldableEntityStorageControllerBase {
   +  protected $fieldInfo;
   +  ¶
   

   Adding whitespaces

2. +++ b/core/lib/Drupal/Core/Entity/EntityNG.php
   @@ -569,7 +587,49 @@ protected function initializeTranslation($langcode) {
   +        if (isset($this->translations[$candidate])) {
   

   Just wondering: shouldn't this use hasTranslation() ? (proper encapsulation of the logic around $this->translations ?)

3. +++ b/core/lib/Drupal/Core/Language/FallbackManagerInterface.php
   @@ -0,0 +1,63 @@
   + * Contains \Drupal\Core\Language\FallbackHandlerInterface.
   

   Wrong copy / paste.

4. +++ b/core/lib/Drupal/Core/Language/FallbackManagerInterface.php
   @@ -0,0 +1,63 @@
   +/**
   + * A common interface for language fallback managers.
   + *
   + * Implement this interface to provide the language fallback manager service.
   + */
   +interface FallbackManagerInterface {
   

   "fallback manager" looks like a misnomer IMO. A manager manages other stuff. Those are not "managing" anything, they hand out fallback langcodes according to some implementation logic.
   Seeing those named managers make me look for the objects they manage. We shouldn't use "manage" as a synonym of "do some stuff" IMO.
   Also, the class name should carry the fact that this is about language stuff, without needing to look at the namespace (that's in our code standards)
   What those do is map langcodes to langcodes - so maybe LanguageFallbackMapper / LanguageFallbackMapperInterface ?

5. +++ b/core/lib/Drupal/Core/Language/FallbackManagerInterface.php
   @@ -0,0 +1,63 @@
   +  public function getCandidates(array $context = array());
   

   Method name is confusing, makes you wonder what a Candidate object is. This returns langcodes, so getCandidateLangcodes() ?
   (similarly, the $candidate / $candidates used by calling code to manipulate the results - e.g. in EntityNG::getExistingTranslation() - could be renamed $candidate_langcode IMO)

6. +++ b/core/lib/Drupal/Core/Language/Language.php
   @@ -153,7 +153,7 @@ public function extend($obj) {
   -  public static function sort($languages) {
   +  public static function sort(&$languages) {
   

   I don't get why this is needed / how this is related ?

7. +++ b/core/lib/Drupal/Core/TypedData/LanguageFallbackData.php
   @@ -0,0 +1,380 @@
   +  public function getPropertyLanguage($name) {
   +    $langcode = Language::LANGCODE_DEFAULT;
   +    if (isset($this->context)) {
   +      if (!isset($this->fallbackMap)) {
   +        $this->initializeFallbackMap();
   +      }
   +      if (isset($this->fallbackMap[$name])) {
   +        $langcode = $this->fallbackMap[$name];
   +      }
   +    }
   +    if (!isset($this->languages[$langcode])) {
   +      $this->languages += language_list(Language::STATE_ALL);
   +    }
   +    return $this->languages[$langcode];
   +  }
   

   This method is only ever called to obtain a langcode (calling code gets the ->id of the result). Keeping an internal copy of language_list() objects just to be able to return a Language looks like overhead for not much ?
   #2078625: Field/FieldItem value objects should carry their language went in meanwhile, added Field/FieldItem::getLangcode() rather than getLanguage() for those exact same reasons. Maybe we should move to getPropertyLangcode() here too ?

8. +++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.php
   @@ -35,10 +54,9 @@ public function getTranslationLanguages($include_default = TRUE);
   +   * The returned translation has to be of the same type of this this typed data
   

   Grammar ("same type than..." .)
   + double "this"

9. +++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.php
   @@ -90,4 +138,15 @@ public function addTranslation($langcode, array $values = array());
   +  public function applyLanguageFallback($context = array());
   

   "apply" sounds like you're transforming the object itself, not that you're returning a new one.
   getLanguageFallbackData() would be more accurate for the current architecture of the code ?

10. +++ b/core/modules/content_translation/content_translation.module
    @@ -598,7 +603,7 @@ function content_translation_permission() {
    +  if (($form_controller = content_translation_form_controller($form_state)) && ($entity = $form_controller->getEntity()) && $entity->isTranslatable() && count($entity->getTranslationLanguages()) > 1) {
    

    Can we split that line in two or three ? ;-)

11. +++ b/core/modules/field/field.install
    @@ -434,10 +434,7 @@ function field_update_8003() {
     function field_update_8004() {
    -  update_variable_set('field_language_fallback', TRUE);
    -  update_variables_to_config('field.settings', array(
    -    'field_language_fallback' => 'language_fallback',
    -  ));
    +  // Do nothing: the previous update has been moved. ¶
    

    The comment is misleading - "the previous update" means field_update_8003() :-p
    + if we don't care about migrating 'field_language_fallback' anymore, then it looks like it should be removed from field/config/field.settings.yml ? (and also the old 'field_language_fallback' var should be deleted here ?)
    + trailing whitespace

12. diff --git a/core/modules/language/config/language.fallback.yml b/core/modules/language/config/language.fallback.yml
    +++ b/core/modules/language/config/language.fallback.yml
    @@ -0,0 +1 @@
    +enabled: true
    

    One separate file for a single value ? There is already language.detection.yml with a single value as well... Should we consolidate things a bit ? (maybe in a followup ?)

(also, needs reroll)

Also, wondering - I thought I understood (don't remember how/where/who, though) that the plan was to drop field-level language fallback and keep only entity level fallback ?
Meaning, you ask for the entity in a language L1, we determine an existing translation L (= either L1 if it exists or some other L2 if it doesn't), and you get an entity with fields in either L or 'und' but we don't mix and match and fetch fields in some L3 if they don't exist in L ?
The assumption being: there are no partial entities, if a field is empty in a language, then it's just empty and we don't try to find a replacement in another language.

Did I dream that ? Or maybe that was a discussion with someone else ?

Thanks for the review, I'll address it ASAP!

Did I dream that ? Or maybe that was a discussion with someone else ?

That was my original approach to this issue, then @fago convinced me we might still need field-level fallback somewhere here.

That was my original approach to this issue, then @fago convinced me we might still need field-level fallback somewhere here.

OK. This does add a huge amount of complexity to the system though - including all the discussions about "should this "fallbacked" set of fields be a decorator / should this act as a real entity / we have issues if people start to write to it" in #30 and below.
And yes, that LanguageFallbackInterface object is specifically the part that puzzles / worries me in the current patch :-/.

For instance, the fact that it's the entity itself in monolingual cases and is a different stub object that's totally not an Entity in multilingual cases, feels dangerous. It widens the runtime difference between mono and multilingual sites. opens the door to "my code / contrib module works because I only tested it on monolingual, but breaks on multilingual sites".

Then, the fact that entity rendering currently doesn't use the LanguageFallbackInterface object directly, but still goes through the "old" field_attach_*() with the "old" field.multlingual.inc functions (that are kept alive by returning logic based on the LanguageFallbackInterface object), makes it difficult to evaluate how the "real" final code would work and whether that LanguageFallbackInterface fits the needs. We can't be sure we nailed right it until we use it for real...

More importantly though, I don't get how field-level fallback would be triggered. An empty value is a totally valid value in a translation, it shouldn't mean "try to find a non-empty value somewhere else". Why shouldn't translations be allowed to leave a field empty ?

This should address most of #83. I intentionally left out all the stuff that has an impact on the current architecture, since I need to mull on #86. To shortly summarize my thoughts, I am a bit torn: I hate the amount of complexity we are adding to support a use case which is frankly marginal, to say it politely, at least in my experience. OTOH in D7 we could not properly separate field language fallback from entity language negotiation, hence we introduced a "feature" which we might need to keep support for.

Fixed a merge issue

Thanks @plach

Still some language / grammar issues :

1. +++ b/core/lib/Drupal/Core/Entity/EntityNG.php
   @@ -728,10 +728,13 @@ public function applyLanguageFallback($context = array()) {
        // Entity translation objects do not provide language fallback, hence any
   -    // field has always the active language.
   +    // field has always the active language if the field is translatable.
   

   "...hence any field has always..." is off.

   "... hence a translatable field always has the active language" ?
   (not that I understand the overall sentence ;-), but... )

2. +++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.php
   @@ -54,7 +54,7 @@ public function getTranslationLanguages($include_default = TRUE);
   +   * The returned translation has to be of the same type of this typed data
   

   still wrong - "of the same type than..."

Possibly related, might already be (partially) solved by this, but wanted to write the issue down: #2090983: ContentEntityInterface::getTranslation() should throw an exception when an invalid language is specified.

Just fixed #90 for now. I am going to provide my updated POV on this tomorrow.

For reals now

I spent the last days thinking about this as soon I had a free minute and I concluded that I have no interest in supporting language fallback at field level. Let me explain why.

In this issue we are dealing with the conversion to the D8 API of a D7 subsystem that provided two non-cleanly separated tasks: entity language negotiation and field language fallback.

• Entity language negotiation aims to determine which actual entity (translation) language should be used in a particular context, such as entity rendering. Usually a context implies a language, for instance the current page language, but we might not have an entity translation matching that language, hence we may need to apply some fallback logic to determine which actual language we should use.
• Field language fallback aims to provide an alternative field translation value, should a value for the language implied by the current context be missing. Also in this case some fallback logic needs to be applied.

I initially wrote the D7 code to solve a rendering issue: when displaying an entity in a language for which a translation was not available, field values disappeared. This clearly was not desirable. Since in D7 core there is no concept of entity translation (we have only language-aware fields), the only way I found to fix this issue was applying some fallback rules to every field so that existing values would be rendered. The underlying assumption was that all fields would behave the same way. What I didn't take into consideration was that empty fields values are not stored, hence they appear as missing values. The unwanted consequence was I unwillingly introduced a new "feature", that is language fallback at field level. AAMOF in D7 you can render the french translation of an english node, but if the body translation is empty its original english value will be rendered. This behavior was not intended initially and it does not even make much sense in many scenarios.

In my personal experience I cannot recall a single case where such behavior would be the desired one. Instead I think we should leave translators the ability to enter empty values if they need to, but this is now (both in D8 core and with the current patch) impossible without disabling field language fallback altogether.

The other reason I've started leaning towards this position is that to cleanly implement the approach summarized in the OP, we are adding an insane amount of complexity and code to a system that is already not straightforward. And this is to support a use case that is far from being a common one, at least in my experience. Without that translators needing to show an untranslated value as fallback could simply copy the original values as the translated ones, but this way empty values would stay empty :)

To sum up I would like to propose to go back to my original approach and get rid of the fallback data decorator. That logic can easily be implemented by contrib code relying on the language fallback manager service introduced by this patch, but I'd avoid baking that concept into the core entity system.

Thanks @plach. We'll get to discuss this in Prague, but +3000 on #94.

+1 on what @plach said in #94 - I've done a number of multi-lingual sites and I'd never want individual fields to fallback.

+++ b/core/includes/language.inc
@@ -542,20 +542,14 @@ function language_url_split_prefix($path, $languages) {
+  return Drupal::service('language_fallback_manager')->getCandidates(array('type' => $type));

Should be \Drupal according #2053489: Standardize on \Drupal throughout core

Wanted to review and had first to reroll.
Had some trouble to reroll this, but hopefully this works now.

I will continue review and fixing this later, on the first glance I already see some missing comments in the modified __constructs.

Also fixed #98.

Working on implementing #94 as per the multilingual fields hard problems discussion conclusions.

Here is a patch removing field language fallback. Let's see how many failures we have.

Fixed/removed field translation tests and fixed a stupid mistake.

1. +++ b/core/modules/language/lib/Drupal/language/FallbackManager.php
   @@ -0,0 +1,84 @@
   +class FallbackManager extends NoFallbackManager {
   

   Seems like we should have an interface for this?

   Also wondering if we can come up with a better name than SomethingManager. You should hear @msonnabaum's opinion about naming things Managers ;) No idea what we could name it, we could ask him.

2. +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
   @@ -905,6 +907,20 @@ public function setNewRevision($value = TRUE) {
   +   * {@inheritdoc}
   +   */
   +  public function setLanguageManager(LanguageManager $language_manager) {
   +    $this->storage->setLanguageManager($language_manager);
   +  }
   

   We should probably at least wait until we get the config/content separation in, so that we don't need to add those methods here and then remove them again.

   I'm also not too convinced about using setter injection for a one-off like this that has to go through the database storage controllers :( Seems like that's something that we'll just have to revert once we have a generic solution to inject dependencies into entity and field classes.

   Not sure if someone suggested to do it like this, haven't read the full thread.

Just fixing tests for now, reviews above still to be addressed.

1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -375,8 +407,22 @@ public function language() {
   +   * Implements \Drupal\Core\TypedData\TranslatableInterface::getExistingTranslation().
   +   *
   +   * @return \Drupal\Core\Entity\EntityInterface
   

   {@inheritdoc}

2. +++ b/core/modules/field/field.deprecated.inc
   @@ -829,3 +829,92 @@ function field_get_items(EntityInterface $entity, $field_name, $langcode = NULL)
   + * display language code to be used is just Language::LANGCODE_NOT_SPECIFIED, as no other
   

   80 char limit.

3. +++ b/core/modules/language/language.api.php
   @@ -58,5 +58,77 @@ function hook_language_delete($language) {
   + * @param array $context
   + *   A language fallback context.
   ...
   + * @param array $context
   + *   A language fallback context.
   

   Maybe we have to add $context details here.

Testing to see whether a change is actually necessary.

This should address what was left of #83, #104 and #107. Some answers below:

@yched:

6. I don't get why this is needed / how this is related ?

The default fallback rules inspect languages by their weight. The sort function actually does nothing without the ampersand, now we have test coverage for that :)

@berdir:

1. Seems like we should have an interface for this?

We already have it: it's LanguageFallbackMapperInterface (now).

@jibran:

Maybe we have to add $context details here.

There's a pointer to the language fallback mapper interface, which has full docs about it :)

Updated issue summary.

This is yet another switch field/entity issue now, updated issue summary accordingly :)

I think we just need to figure out the actual EntityNG method names (@fago expressed some concerns about those) and then we should be done.

Completely removed service injection from entities for now. We'll have to deal with that in a separate issue.

Rerolled

(partial review, need to run, stopped in the middle :-/)

1. +++ b/core/lib/Drupal/Core/Language/LanguageFallbackMapperInterface.php
   @@ -0,0 +1,36 @@
   + * Implement this interface to provide the language fallback manager service.
   

   Seems unneeded ? (or we'd be basically adding this kind of documentation in a lot of other interfaces)

2. +++ b/core/lib/Drupal/Core/Language/LanguageFallbackMapperInterface.php
   @@ -0,0 +1,36 @@
   +   *   - langcode: The desired language. Normally this would be the first value
   +   *     of the returned array.
   

   Looks like the "Normally..." part is redundant with the doc for the @return statement, I'd vote for removing it.

   Actually - I'd think that at least $langcode (and maybe $operation ?) would make more sense as standalone parameters for the method. Those will always be present (right ?), and it would make the contract clearer : "I want fallback languages for this language (and this operation)". Let's avoid squashing fundamental parameters into a single array param. DX++

3. +++ b/core/lib/Drupal/Core/Language/LanguageFallbackMapperInterface.php
   @@ -0,0 +1,36 @@
   +   *   - operation: The name of the operation in whose context language fallback
   +   *     is going to be applied, e.g. 'entity_view'.
   

   "in which" ?
   "is being applied" ?

4. +++ b/core/lib/Drupal/Core/Language/NoLanguageFallbackMapper.php
   @@ -0,0 +1,39 @@
   + * A mock language fallback manager to be used in monolingual environments.
   + */
   +class NoLanguageFallbackMapper implements LanguageFallbackMapperInterface {
   

   Nitpick: "monolingual environments" / NoLanguageFallback feels weird. Monolingual is "one single language", not "no language" :-)

5. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
   @@ -66,6 +66,22 @@ public function __construct(EntityManager $entity_manager, FieldInfo $field_info
   +  ¶
   +    // Make the comment inherit the current content language unless specifically
   +    // set.
   +    if ($comment->isNew()) {
   +      $language_content = language(Language::TYPE_CONTENT);
   +      $comment->langcode->value = $language_content->id;
   +    }
   +  ¶
   

   trailing whitespaces

1. +++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.php
   @@ -49,6 +51,36 @@ public function getTranslationLanguages($include_default = TRUE);
   +  public function getExistingTranslation($langcode, $context = array());
   

   Hm.

   getFallbackTranslation()
   getTranslationOrFallback()?
   getAvailableTranslation()
   askMarkBecauseHeAlwaysSaysNamingThingsIsEasy()?

2. +++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.php
   @@ -49,6 +51,36 @@ public function getTranslationLanguages($include_default = TRUE);
   +  public function getCurrentTranslation();
   

   Hm2.

   This seems to be one that you should be using in most cases if you load an entity and want to render/access check it based on the current content language. So we should make it easy to find this one.

   "current" seems to be something that's invented here, especially because it doesn't seem to refere to the current *content* language.

   Default already has specific meaning, so that's a no-go.

   Could we just make the $langcode argument optional and say that's what it defaults to?

   => $entity->getExistingTranslation()? (or whatever it will be named)

3. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
   @@ -66,6 +66,22 @@ public function __construct(EntityManager $entity_manager, FieldInfo $field_info
   +    $comment = $this->entity;
   +  ¶
   +    // Make the comment inherit the current content language unless specifically
   

   Trailing spaces.

4. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
   @@ -66,6 +66,22 @@ public function __construct(EntityManager $entity_manager, FieldInfo $field_info
   +      $language_content = language(Language::TYPE_CONTENT);
   

   Should use \Drupal::languageManager()...

5. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Tests/ContentTranslationUITest.php
   @@ -34,17 +34,17 @@
      function testTranslationUI() {
   -    $this->assertBasicTranslation();
   -    $this->assertOutdatedStatus();
   -    $this->assertPublishedStatus();
   -    $this->assertAuthoringInfo();
   -    $this->assertTranslationDeletion();
   +    $this->doTestBasicTranslation();
   +    $this->doTestOutdatedStatus();
   +    $this->doTestPublishedStatus();
   +    $this->doTestAuthoringInfo();
   +    $this->doTestTranslationDeletion();
   

   +1000.

   Those tests are *so* confusing right now.

6. +++ b/core/modules/language/lib/Drupal/language/LanguageServiceProvider.php
   @@ -0,0 +1,36 @@
   +    $definition = $container->getDefinition('language_fallback_mapper');
   +    $definition->setClass('Drupal\language\LanguageFallbackMapper');
   +    $definition->addArgument(new Reference('module_handler'));
   

   Wondering if using fluent interfaces would be easier to read, that's what we did usually before switching to yaml:
   <?php
   $container->getDefinition()
   ->setClass()
   ->addArgument();

7. +++ b/core/modules/language/lib/Drupal/language/Tests/LanguageFallbackTest.php
   @@ -0,0 +1,95 @@
   +  /**
   +   * Returns the language fallback manager service.
   +   *
   +   * @return \Drupal\Core\Language\LanguageFallbackMapperInterface
   +   *   The language fallback manager.
   +   */
   +  protected function getManager() {
   +    return $this->container->get('language_fallback_mapper');
   +  }
   

   manager => mapper (much better, BTW!)

8. +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
   @@ -11,6 +11,8 @@
    use Drupal\Core\Database\Database;
   +use Drupal\Core\Language\LanguageFallbackMapperInterface;
   +use Drupal\Core\Language\LanguageManager;
    use Drupal\Core\TypedData\TypedDataInterface;
   
   @@ -905,6 +907,20 @@ public function setNewRevision($value = TRUE) {
      /**
   +   * {@inheritdoc}
   +   */
   +  public function setLanguageManager(LanguageManager $language_manager) {
   +    $this->storage->setLanguageManager($language_manager);
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setLanguageLanguageFallbackMapper(LanguageFallbackMapperInterface $language_fallback_mapper) {
   +    $this->storage->setLanguageLanguageFallbackMapper($language_fallback_mapper);
   +  }
   +
   

   Left-overs.

One thing is I think important to consider.

While the API makes a lot of sense, combined with #2072945: Remove the $langcode parameter in EntityAccessControllerInterface::access() and friends, #2073217: Remove the $langcode parameter from the entity view/render system, we do introduce one problem.

We lose the ability to have defaults. If you pass in an entity object into the access or render system, we have absolutely no way of knowing if you did apply some sort of language logic. We don't know if you really want to check/display the entity in that language or the current content language.

So, we need to figure out and document where and by whom the language handling needs to be applied. Because this could result in troubles if people forget to do it and then the wrong language will be displayed while they didn't have to care before.

Crosspost with myself. Achievement unlocked.

#2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface went in, so this will need a re-roll and we can remove the empty implementations in Entity and ViewsUI for the new methods.

Cool, will work on this tonight.

Rerolled and addressed #114 and #115.

@yched:

Actually - I'd think that at least $langcode (and maybe $operation ?) would make more sense as standalone parameters for the method.

I don't agree: no parameter is required or even implied here, AAMOF we don't even have a sensible default to apply for them. If you look at LanguageFallbackMapper::getCandidateLangcodes(), we are explictly taking into consideration the fact that no desired language is specified, which might make sense if one just needs a plain list of fallback candidates.

Nitpick: "monolingual environments" / NoLanguageFallback feels weird. Monolingual is "one single language", not "no language" :-)

You are not reading it right :)
In a monlingual environment you have just one language so "no language fallback" can be applied. I asked Gabor for suggestions and he was pretty much agreeing with this name, alternative suggestions welcome.

@berdir:

askMarkBecauseHeAlwaysSaysNamingThingsIsEasy()?

:)

I spent quite some time thinking of a name conveying the proper concept and I think the current version does the job well: my goal was trying to make the method name very easy and straightforward. This method needs to be called to ensure code will work with an existing/valid/available translation object, no other cognitive burden is needed. The fact that we are applying a fallback logic is an implementation detail, a method name should tell what it does not how it does it.

While the API makes a lot of sense [...] we do introduce one problem [...] we have absolutely no way of knowing if you did apply some sort of language logic [...] we need to figure out and document where and by whom the language handling needs to be applied

After spending a very long time thinking how the Entity Translation API could look like, I realized that the ideal DX in most scenario would be being able to write language-agnostic code wherever possible. I think that if we get it right, what you are outlining above will be the key factor in the API success. By always moving the reponsibility to pass the proper translation object on the caller, we are ensuring that in most scenarios people will not have to deal with language, unless their business logic requires it obviously. I totally agree that we must define very clearly where people should be authorized to deal with an entity (translation) object as-is, but ideally the rule of thumb should be "always unless told otherwise", although I don't think we will be able to achive that. I think the form controller is a good example: it figures out which translation it needs to work on early and all handlers always get the proper translation object. If we can reach the same level of reliability, at least in the most common contexts (rendering, access-checking, token-processing), we should be able to guarantee that a fair amount of code out there won't need to worry about language at all.

Going afk for the next couple weeks, I'll let you drive this home folks :-)
Just crossposting to #2095195: Remove deprecated field_attach_form_*(), that contains an initial version of how the integration of widgets / formatters in forms / entity_views would work after this gets in.

+++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.php
@@ -69,18 +69,7 @@ public function getTranslation($langcode);
-  public function getCurrentTranslation();
+  public function getExistingTranslation($langcode = NULL, $context = array());

Docs will need to be updated for the $langcode change.

Done

1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -48,6 +48,7 @@
       */
      public function __construct(array $values, $entity_type) {
        $this->entityType = $entity_type;
   +
        // Set initial values.
        foreach ($values as $key => $value) {
          $this->$key = $value;
   @@ -261,7 +262,7 @@ public function language() {
   
   @@ -261,7 +262,7 @@ public function language() {
      }
    
      /**
   -   * {@inheritdoc}
   +   * Implements \Drupal\Core\Entity\EntityInterface::save().
       */
   

   Looks like this has some left-overs/accidental reverts in Entity.php

2. +++ b/core/modules/field/field.multilingual.inc
   @@ -50,6 +50,7 @@
     *     fallback logic is applied separately to each field to ensure that there
     *     is a value for each field to display.
   + *
     *   The field language fallback logic relies on the global language fallback
     *   configuration. Therefore, the displayed field values can be in the
   

   Another left-over?

3. +++ b/core/modules/user/lib/Drupal/user/UserStorageController.php
   @@ -8,6 +8,8 @@
    
    use Drupal\Component\Uuid\UuidInterface;
   +use Drupal\Core\Language\LanguageFallbackMapperInterface;
   +use Drupal\Core\Language\LanguageManager;
    use Drupal\Core\Entity\EntityInterface;
    use Drupal\Core\Password\PasswordInterface;
    use Drupal\Core\Database\Connection;
   @@ -56,8 +58,8 @@ class UserStorageController extends DatabaseStorageControllerNG implements UserS
   
   @@ -56,8 +58,8 @@ class UserStorageController extends DatabaseStorageControllerNG implements UserS
       * @param \Drupal\user\UserDataInterface $user_data
       *   The user data service.
       */
   -  public function __construct($entity_type, $entity_info, Connection $database, FieldInfo $field_info, UuidInterface $uuid_service, PasswordInterface $password, UserDataInterface $user_data) {
   -    parent::__construct($entity_type, $entity_info, $database, $field_info, $uuid_service);
   +  public function __construct($entity_type, $entity_info, Connection $database, FieldInfo $field_info, UuidInterface $uuid_service, LanguageManager $languageManager, LanguageFallbackMapperInterface $language_fallback_mapper, PasswordInterface $password, UserDataInterface $user_data) {
   +    parent::__construct($entity_type, $entity_info, $database, $field_info, $uuid_service, $languageManager, $language_fallback_mapper);
    
        $this->password = $password;
        $this->userData = $user_data;
   @@ -73,6 +75,8 @@ public static function createInstance(ContainerInterface $container, $entity_typ
   
   @@ -73,6 +75,8 @@ public static function createInstance(ContainerInterface $container, $entity_typ
          $container->get('database'),
          $container->get('field.info'),
          $container->get('uuid'),
   +      $container->get('language_manager'),
   +      $container->get('language_fallback_mapper'),
          $container->get('password'),
          $container->get('user.data')
   

   Also not necessary anymore.

4. +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
   @@ -11,6 +11,8 @@
    use Drupal\views\ViewExecutable;
    use Drupal\Core\Database\Database;
   +use Drupal\Core\Language\LanguageFallbackMapperInterface;
   +use Drupal\Core\Language\LanguageManager;
    use Drupal\Core\TypedData\TypedDataInterface;
    use Drupal\Core\Session\AccountInterface;
    use Drupal\views\Plugin\views\query\Sql;
   @@ -883,7 +885,6 @@ public function getExportProperties() {
   
   @@ -883,7 +885,6 @@ public function getExportProperties() {
        return $this->storage->getExportProperties();
      }
    
   -
      /**
       * {@inheritdoc}
       */
   

   Another left-over.

After spending a very long time thinking how the Entity Translation API could look like, I realized that the ideal DX in most scenario would be being able to write language-agnostic code wherever possible. I think that if we get it right, what you are outlining above will be the key factor in the API success. By always moving the reponsibility to pass the proper translation object on the caller, we are ensuring that in most scenarios people will not have to deal with language, unless their business logic requires it obviously. I totally agree that we must define very clearly where people should be authorized to deal with an entity (translation) object as-is, but ideally the rule of thumb should be "always unless told otherwise", although I don't think we will be able to achive that. I think the form controller is a good example: it figures out which translation it needs to work on early and all handlers always get the proper translation object. If we can reach the same level of reliability, at least in the most common contexts (rendering, access-checking, token-processing), we should be able to guarantee that a fair amount of code out there won't need to worry about language at all.

Yeah, so that's the part that I don't fully understand yet. Can we look at a few examples and where exactly the content negotiation for access checks and rendering will happen and who will do it?

1. Viewing node/1 (This is the easy one, to get started! :))
2. A few with rendered entity, e.g. the front page.
3. A view that displays entity fields.
4. Custom code that loads a node and displays it, e.g. in a block.

I have a feeling that we could really use some functional tests, e.g. for the second one. Create a few nodes with translations and then look at the frontpage view with different languages. Because that view right now displays duplicates if you do that :)

Just a reroll for now. I am going to address #125 soon.

Updated issue summary.

Addressed the code review in #125. Working on the meaty part now.

@plach: the issue summary says only reviews are missing. You said you are working on the meaty part (without specifying what it is). It is hard to follow the status of this issue :/

I was referring to the last part of #125, but I am a bit swamped these days.

Here it is: it turns out that satisfying bullets #125.1, #125.2 and #125.4 is rather easy and just requires to adjust the entity render controller and some minor stuff. We have both unit and functional tests covering these.

Duplicates aren't something we can fix here, AAMOF they are caused by the way Views joins on node tables and retrieves results. We have #2006606: Views rendering ignores entity language, eg: Identical nodes rendered in views when nodes have translations to fix this, where we probably want to introduce a new Views field group (Content translation matching Content revision) exposing the langcode and default_langcode columns of the data table. After having those an optional filter, like the one in the node admin view, would do the trick by filtering stuff matching the current language or having no language, just when Content Translation is enabled. Another possibile solution would be refactoring Views to make it just select entity ids and then loading entities, but this sounds way more invasive.

Anyway, in this scenario #125.3 would just work, as it already shows translated values (although not filtered by language).

Just a side note: with this one node title translations are displayed correctly in the most common scenarios :)

#132: et-language_fallback-2019055-132.patch queued for re-testing.

Rerolled, test fails still there I guess.

Let's try this.

Cleaning up the stuff below.

1. +++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
   @@ -389,7 +389,7 @@ public function getEntity() {
   +   * Implements \Drupal\Core\Entity\EntityFormControllerInterface::setEntity().
   

   Merge issue.

2. +++ b/core/lib/Drupal/Core/Entity/EntityRenderController.php
   @@ -167,7 +167,9 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
   +      $entity = $entity->getExistingTranslation($langcode);
   

   This could use an inline comment.

3. +++ b/core/lib/Drupal/Core/Language/LanguageFallbackMapperInterface.php
   @@ -0,0 +1,33 @@
   +   * Returns the possible language fallback candidates.
   

   This is a bit uninformative.

4. +++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.php
   @@ -866,11 +871,13 @@ function field_langcode(EntityInterface $entity) {
   +      $name = $this->field_info['field_name'];
   

   Unneeded line

5. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
   @@ -126,7 +126,7 @@ public function page(NodeInterface $node) {
   +    return String::checkPlain($node->getExistingTranslation()->label());
   

   @berdir:

   I'd like to discuss the suppression of EntityInterface::getCurrentTranslation() a bit more, as it seems to me that a line like the following would be way more self-documenting than the one above:

   return String::checkPlain($node->getCurrentTranslation()->label());
   

   The ::getExistingTranslation() method is completely meaningless to my eyes when used without a parameter. I'd definitely like to restore the old approach where the $langcode parameter is mandatory and reintroduce the ::getCurrentTranslation() method as implemented in #113:

   <?php
   public function getCurrentTranslation() {
     $langcode = \Drupal::languageManager()->getLanguage(Language::TYPE_CONTENT)->id;
     return $this->getExistingTranslation($langcode);
   }
   ?>
   

   "current" seems to be something that's invented here, especially because it doesn't seem to refere to the current *content* language.

   I don't understand this argument: "current" refers exactly to the content language of the current context, which is provided by the language manager.

   Aside from this I think we are ready to go.

@Berdir, @fago, @yched: what do you think? This is a blocker to make non-configurable field translations display, so it would be important to get over this soon if we have agreement and the code implements the agreement proper :)

Just went through the first couple lines for now, need to run :-/

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -666,6 +673,40 @@ protected function initializeTranslation($langcode) {
   +    if (!isset($langcode)) {
   +      $langcode = \Drupal::languageManager()->getLanguage(Language::TYPE_CONTENT)->id;
   +    }
   +
   +    if (!empty($langcode)) {
   

   Can't this be simplified a bit ? Can there really be cases where getLanguage(CONTENT)->id would be empty ?
   (also, the combination of isset / empty checks looks needlessly confusing - could we unify on one or the other ?)

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -666,6 +673,40 @@ protected function initializeTranslation($langcode) {
   +      // Retrieve language fallback candidates to perform the entity language
   +      // negotiation.
   +      $context['langcode'] = $langcode;
   +      $context['data'] = $this;
   +      $context += array('operation' => 'entity_view');
   +      $candidates = \Drupal::service('language_fallback_mapper')->getCandidateLangcodes($context);
   

   We intend to use the fallback mapper on other stuff than entities ? If not, the 'data' key might be better/less generically named 'entity' ?

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -666,6 +673,40 @@ protected function initializeTranslation($langcode) {
   +      // Return the most fitting entity translation.
   +      foreach ($candidates as $candidate) {
   +        if ($this->hasTranslation($candidate)) {
   +          $translation = $this->getTranslation($candidate);
   +          break;
   +        }
   

   "Most fitting" is just "the first that is present" ?

Can't this be simplified a bit ? Can there really be cases where getLanguage(CONTENT)->id would be empty ?

Well, it depends on the language manager implementation :)

We intend to use the fallback mapper on other stuff than entities ? If not, the 'data' key might be better/less generically named 'entity

The fallback mapper has no knowledge of the entity system, so theoretically it could be used with other data than entities. Using an 'entity' key would make little sense form an architectural POV IMHO.

"Most fitting" is just "the first that is present" ?

Yes, but only before the alter hook is run. We can change that but it might be inaccurate after the alteration.

@Gabor, @yched, @berdir:

Any comment on #139.5?

- Can't this be simplified a bit ? Can there really be cases where getLanguage(CONTENT)->id would be empty ?
- Well, it depends on the language manager implementation :)

What I mean is : getLanguage() returns something, and we unconditionally take its ->id, so we expect that "something" to be not NULL in the first place. Can this ->id then be NULL ? (Can a non null $language object have a NULL id ?)

Still need to review the rest of the patch, but I tend to agree with #139.5 - i.e. +1 on getCurrentTranslation() vs getExisistingTranslation(NULL is understood as "the current language")

@yched:

Ok, now it makes sense to me, fixed it the way you suggested :)

Also I spoke about #139.5 with @Berdir in IRC and he expressed some concerns about the DX of the current method namings:

it just doesn't seem clear to me, especially when we have getTranslation(), getExistingTranslation() and getCurrentTranslation(). I can't tell the difference between those methods just from looking at the method names

In an attempt to reconcile his POV and mine I tried to rename ::getExistingTranslation() to ::getTranslationFromContext(). Personally I find it works both with the $langcode argument and without, so I am happy enough with it to provide a reroll. How does it look now?

We also had a discussion about #116 (you can find the full log attached).

Berdir's concerns is about figuring out a reliable logic to determine when we should expect a translation object to be passed around and when code should be in charge of instantiating it. He correctly pointed out that in the latest versions of this patch the EntityRenderController accepts a $langcode parameter and determines the translation object to act on based on it. This breaks the rule we previously identified that a piece of code should normally rely on the caller code to get the proper translation object.

I realized this too while working on the latest patches and I came to the conclusion that whenever a known language logic is available, it should be applied to determine the proper translation object. If no context is available to reliably determine the correct translation, just act on the entity object as-is. This translates ;) to the following rule of thumb: "If you have no idea of what is the correct translation to act on that probably means you want to act on the entity object you receive as-is". Obviously we cannot entirely avoid applying some form of language logic somewhere in the call stack, we can just avoid the need to repeat it everywhere.

We took in consideration three examples to proof-check this rule. Both form and render entity controllers usually have a well-defined context, from which it's easy to derive the desired language logic: hence both can determine the translation object to act on by themselves. OTOH an entity access controller implies no clear context and as such it should be passed the correct translation object.

Berdir pointed out, and I believe he might be correct, that the proper place to perform entity language negotiation could be the route param converter, after that every piece of code would automatically get the proper translation object. This sounds a bit scary, as it implies going with the current language in lots of places, but it might be definitely worth a try, as it would avoid the need to explictly perform entity language negotiation in most entity routes.

However this would be another reason to find a clean solution for #1810394: Site configuration with domain based language negotiation results in redirecting authenticated users to a different domain when accessing a content entity route for translation language different from the interface language, which at this point would probably need to rely on explicitly switching content language instead of forcing just a form language change.

Mulling a bit over #145, meanwhile, end of my review.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityFormController.php
   @@ -83,43 +83,22 @@ protected function init(array &$form_state) {
   +  protected function getTranslatedEntity(array &$form_state) {
        $langcode = $this->getFormLangcode($form_state);
   -    $translation = $this->entity->getTranslation($langcode);
   -    // Ensure that the entity object is a BC entity if the original one is.
   -    return $this->entity instanceof EntityBCDecorator ? $translation->getBCEntity() : $translation;
   +    return $this->entity->getTranslation($langcode);
      }
   

   Do we still even need that method ? It's only called in init(), and is simple enough that it could be inlined in there ?

   I'm aware it's semi-related to "who is responsible for setting the entity to the right language ?", but even in the current situation, the method seems extraneous ?

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityFormController.php
   @@ -83,43 +83,22 @@ protected function init(array &$form_state) {
   +      // Imply a 'view' operation to ensure users edit entities in the same
   +      // language they are displayed. This allows to keep contextual editing
   +      // working also for multilingual entities.
   +      $form_state['langcode'] = $this->entity->getTranslationFromContext()->language()->id;
   

   I'm not sure I understand this. If we use 'entity_view' operation even for forms, what is the point of having an "operation" in the context ? Wouldn't we be better off with just 'type' => 'entity' in the context instead ?

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityFormController.php
   @@ -138,6 +117,8 @@ public function buildEntity(array $form, array &$form_state) {
   +    $this->updateFormLangcode($form_state);
   +
   

   Looks weird ? EntityFormController::submit() already calls updateFormLangcode() and then buildEntity(). So updateFormLangcode() would end up being called twice ?
   (also, having buildEntity(), whose advertized job is to return an $entity, perform side effects on $this, is a bit unexpected/confusing)

4. +++ b/core/lib/Drupal/Core/Language/LanguageFallbackMapperInterface.php
   @@ -0,0 +1,33 @@
   + * A common interface for language fallback managers.
   

   leftover of "manager"

5. +++ b/core/lib/Drupal/Core/Language/LanguageFallbackMapperInterface.php
   @@ -0,0 +1,33 @@
   +   * Returns the language fallback candidates matching the given context.
   

   Nitpick : "matching" is a bit misleading.
   "Returns ... candidates for a given context" ?

6. +++ b/core/lib/Drupal/Core/Language/LanguageFallbackMapperInterface.php
   @@ -0,0 +1,33 @@
   +  public function getCandidateLangcodes(array $context = array());
   

   I still don't get the absence of an explicit $langcode param in getCandidateLangcodes(). We're in the interface for *Fallback*Mappers, right ? I don't understand what "fallback" means if it's not in the context of "I want a specific language, give me a list of fallbacks for that language". I don't see why something called FallbackMappers would be used for a different task ?

   - The actual consuming code (other than tests and the BC for language_fallback_get_candidates(), which we could remove IMO ?) always calls that method with an actual langcode.
   - It's even more confusing since Entity::getTranslationFromContext() *does* take separate $langcode & $context params, and puts the former in the latter before calling getCandidateLangcodes() :-/. Which then makes it difficult to properly document the $context param in getTranslationFromContext(), since it's slightly different than the one accepted by getCandidateLangcodes().

7. +++ b/core/lib/Drupal/Core/Language/NoLanguageFallbackMapper.php
   @@ -0,0 +1,39 @@
   + * A mock language fallback manager to be used in monolingual environments.
   

   "manager"

8. +++ b/core/lib/Drupal/Core/Language/NoLanguageFallbackMapper.php
   @@ -0,0 +1,39 @@
   +class NoLanguageFallbackMapper implements LanguageFallbackMapperInterface {
   

   I definitely find it hard/impossible to read NoLanguageFallbackMapper as "no fallback" rather than "no language" :-)
   NoFallbackMapper ?
   MonolingualFallbackMapper ?

9. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
   @@ -66,6 +65,22 @@ public function __construct(EntityManager $entity_manager, FieldInfo $field_info
   +  protected function init(array &$form_state) {
   +    $comment = $this->entity;
   +
   +    // Make the comment inherit the current content language unless specifically
   +    // set.
   +    if ($comment->isNew()) {
   +      $language_content = \Drupal::languageManager()->getLanguage(Language::TYPE_CONTENT);
   +      $comment->langcode->value = $language_content->id;
   +    }
   

   This code is just moved around (and I understand why), but isn't it weird that CommentFormController needs to take care of initializing the langcode of new comment entities ? Should there be a "@todo fix this" on a separate issue ?

10. +++ b/core/modules/content_translation/content_translation.module
    @@ -313,7 +313,12 @@ function content_translation_translate_access(EntityInterface $entity) {
     function content_translation_view_access(EntityInterface $entity, $langcode, AccountInterface $account = NULL) {
       $entity_type = $entity->entityType();
    -  return !empty($entity->translation[$langcode]['status']) || user_access('translate any entity', $account) || user_access("translate $entity_type entities", $account);
    +  $info = $entity->entityInfo();
    +  $permission = "translate $entity_type";
    +  if (!empty($info['permission_granularity']) && $info['permission_granularity'] == 'bundle') {
    +    $permission = "translate {$entity->bundle()} $entity_type";
    +  }
    +  return !empty($entity->translation[$langcode]['status']) || user_access('translate any entity', $account) || user_access($permission, $account);
     }
    

    Uh ? Is that really related ?

11. +++ b/core/modules/content_translation/content_translation.module
    @@ -617,7 +622,10 @@ function content_translation_permission() {
     function content_translation_form_alter(array &$form, array &$form_state) {
    -  if (($form_controller = content_translation_form_controller($form_state)) && ($entity = $form_controller->getEntity()) && !$entity->isNew() && $entity instanceof ContentEntityInterface && $entity->isTranslatable()) {
    +  $form_controller = content_translation_form_controller($form_state);
    +  $entity = $form_controller ? $form_controller->getEntity() : NULL;
    +
    +  if ($entity instanceof ContentEntityInterface && $entity->isTranslatable() && count($entity->getTranslationLanguages()) > 1) {
    

    Same, really related ?

12. +++ b/core/modules/field/field.attach.inc
    @@ -120,21 +120,23 @@ function field_invoke_method($method, $target_function, EntityInterface $entity,
    +        if ($entity->hasTranslation($langcode)) {
    

    field_invoke*() are most probably on their way out after this lands, but could you briefly explain why this change is needed ?

13. +++ b/core/modules/field/field.attach.inc
    @@ -120,21 +120,23 @@ function field_invoke_method($method, $target_function, EntityInterface $entity,
               }
    -          else {
    -            $return[] = $result;
               }
    

    Minor: indent error on the second closing bracket.

14. +++ b/core/modules/field/field.deprecated.inc
    @@ -856,3 +857,92 @@ function field_access($op, FieldInterface $field, $entity_type, $entity = NULL,
    +function field_language_fallback_enabled() {
    +  return language_multilingual() && Drupal::service('language_fallback_mapper')->isEnabled();
    +}
    

    There is no isEnabled() method in LanguageFallbackMapperInterface ?
    Shouldn't we just remove the function, since it is not used and looks like it cannot work anyway ?

15. +++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.php
    @@ -93,6 +94,13 @@ class Field extends FieldPluginBase {
    +   * The language fallback manager.
    

    "manager"

16. +++ b/core/modules/field/lib/Drupal/field/Tests/TranslationWebTest.php
    @@ -104,6 +104,7 @@ function testFieldFormTranslationRevisions() {
    +    ksort($available_langcodes);
    

    Minor, but it would make more sense to move this down to where it makes an actual difference, before the key() call a couple lines below ?
    (also, not sure why this ksort is now needed, field_available_language() doesn't seem affected by the patch ?)

17. +++ b/core/modules/language/language.api.php
    @@ -58,5 +58,39 @@ function hook_language_delete($language) {
    +function hook_language_fallback_candidates_alter(array &$candidates, array $context) {
    +  $fallback_candidates = array_reverse($fallback_candidates);
    

    var name mismatch between param and sample code ($candidates / $fallback_candidates)
    Same in the _OPERATION_ specific hook variant.

18. +++ b/core/modules/language/lib/Drupal/language/LanguageFallbackMapper.php
    @@ -0,0 +1,84 @@
    +class LanguageFallbackMapper extends NoLanguageFallbackMapper {
    

    Why does it extend NoLanguageFallbackMapper ? Doesn't seem to make much sense in itself, and no code is shared anyway.

    (also, NoLanguageFallbackMapper doesn't actually need a $language_manager to do its job, it's only LanguageFallbackMapper that needs it, so it should be in LanguageFallbackMapper's construct rather than NoLanguageFallbackMapper ?)

19. +++ b/core/modules/language/lib/Drupal/language/LanguageFallbackMapper.php
    @@ -0,0 +1,84 @@
    +  protected function alter($type, array &$data, array $context) {
    +    $types = array();
    +    if (!empty($context['operation'])) {
    +      $types[] = $type . '_' .  $context['operation'];
    +    }
    +    $types[] = $type;
    +    return $this->moduleHandler->alter($types, $data, $context);
    +  }
    

    The method looks a bit overkill, could easily be inlined in getCandidateLangcodes ?
    (+ do we really need a separate hook_language_fallback_candidates_OPERATION_alter() hook ?)

20. +++ b/core/modules/language/lib/Drupal/language/LanguageServiceProvider.php
    @@ -0,0 +1,36 @@
    + * Defines the LanguageTest service provider.
    + */
    +class LanguageServiceProvider implements ServiceProviderInterface, ServiceModifierInterface {
    

    Class name looks off in the comment ?

21. +++ b/core/modules/language/lib/Drupal/language/LanguageServiceProvider.php
    @@ -0,0 +1,36 @@
    +  public function register(ContainerBuilder $container) {
    +  }
    

    Minor: we usually leave empty implementations on one single line:
    function foo() { }

    (+ silly question: why does the class implement ServiceProviderInterface if it needs no register() ?)