Updated: Comment #166

Suggested commit message

Issue #2019055 by plach, fago, berdir, yched, Gábor Hojtsy, kfritsche, jibran: Switch from field-level language fallback to entity-level language fallback.

Problem/Motivation

Currently language fallback is supported only for fields provided by the Field API. This means that in any context where language fallback is required (the most common use case is entity rendering), Field API fields for which there is no translation available in the active language will be replaced by an existing translation. However this will not happen for any other field attached to the entity being displayed.

Additionally the current code provides 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.

The D7 code was initially wrote 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 we 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 we didn't take into consideration was that empty fields values are not stored, hence they appear as missing values. The unwanted consequence was we unwillingly introduced a new "feature", that is language fallback at field level. In fact 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. Instead we should leave translators the ability to enter empty values if they need to, but this is now impossible without disabling field language fallback altogether.

Goal

Providing a reusable/swappable language fallback system that coupled with an entity language negotiation system can improve DX when dealing with multilingual entities.

Proposed resolution

The current solution is based on the Entity Translation API. Basically everything boils down to retrieving the most appropriate entity translation available for a particular context. A $context array holding arbitrary data useful to describe the context where fallback is being perfomed can be optionally passed. The most common case is the "entity_view" context, but we can have for instance a "views_query" context or a (not present in this issue) "entity_serialize" context:

<?php
  $translation
= \Drupal::entityManager()->getTranslationFromContext($entity, 'it');
 
// $translation might or might not be italian depending on its availability
 
$value = $translation->field_foo->value;
?>

The current language fallback code has been moved to the language manager and is now responsible for building a list of language fallback candidates, possibily based on a given context. The returned values are alterable through dedicated hooks.

EntityManagerInterface::getTranslationFromContext() performs entity language negotiation by inspecting the language fallback candidates and returning an existing translation object.

This was discussed in Prague. The discussion was attended by: attrib, berdir, das_peter, fago, Gábor Hojtsy, plach, swentel, tstoeckler, webflo, yched.

Remaining tasks

  • Ensure there is consensus on the current approach or explore new ones
  • Complete test coverage
  • Complete PHP docs and inline comments
  • Fix failing tests
  • Reviews

(follow-up to be created) Evaluate whether it makes sense to provide a generic language fallback API that could be used for implementing field-level fallbacks in contrib.

(follow-up to be created) Evaluate whether it makes sense to perform entity language negotiation in the route param converters.

User interface changes

None

API changes

  • The current language fallback code has been moved to the language manager.
  • Field-level language fallback is no longer available.

Original report by fago

Right now we've language fallback implemented for field API fields, however with entity fields being translatable as well, we really to support language fallbacks on entity field level as well. Then, we can probably remove the field API variant in favour of the general one.

I think we should remove any possibility to have run-time alterations language fallbacks that depend on context, but get a general list of language fallback rules (per entity type, per field?), which then could be applied to queries as well — later on (contrib?, d9?).

As this is rather important to have for multi-lingual content, setting to major.

Files: 
CommentFileSizeAuthor
#193 et-language_fallback-2019055-193.patch100.23 KBplach
PASSED: [[SimpleTest]]: [MySQL] 59,916 pass(es).
[ View ]

Comments

fago’s picture

fago’s picture

Issue tags:+Entity Field API
effulgentsia’s picture

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

fago’s picture

Issue tags:+Entity Field API

thanks, fixed that. This happens to me too often :/

plach’s picture

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.

plach’s picture

Assigned:Unassigned» plach
Issue tags:+sprint, +language-content

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

plach’s picture

Status:Active» Needs review
StatusFileSize
new135.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new16.82 KB

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.

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-7.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new111.79 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new7.3 KB
new18.7 KB

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

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-9.patch, failed testing.

plach’s picture

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

Status:Needs work» Needs review
StatusFileSize
new140.5 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new47.36 KB

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

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-13.patch, failed testing.

plach’s picture

StatusFileSize
new48.02 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

plach’s picture

Status:Needs work» Needs review

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.

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-15.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new54.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-18.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new57.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Added some test coverage and fixed some bugs. Bot?

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-20.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new57.33 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.9 KB

This should be better.

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-22.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new66.93 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-24.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new61.75 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-26.no_perfomance.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new61.79 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-26.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new7.69 KB

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.

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-26.no_perfomance.patch, failed testing.

Gábor Hojtsy’s picture

plach’s picture

Added an issue summary describing the current status.

plach’s picture

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?

plach’s picture

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

fago’s picture

First off - thanks for all the great work, I'm looking forward to having that issue solved! :-)

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.

AFAIK field API filters out empty values, i.e. they are not saved. So entering empty values won't save them but lead to nothing be saved, i.e. a NULL value appearing after the next load. Imo, that's fine as semantically it means the same: the field is empty, nothing won't be rendered. Consequently, I think we should trigger fallback behaviour as soon as a field is left empty.

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.

Well, they could still receive the regular entity but go via the decorator to access the field items ($item), which then is rendered. $items still has access to $entity as usual, so that should be fine.

return Drupal::entityManager()->getLanguageFallbackDecorator($node)->label();

Yep, that sucks - but the API could look as you suggested without the fallback-object being an entity:

<?php
function node_page_title(EntityInterface $node) {
 
// getCurrentTranslation() returns the decorator, not implementing EntityInterface
  // but still forwarding everything.
 
return $node->getCurrentTranslation()->label();
}
?>

So besides complexity what I dislike about the approach of the current patch is that we introduce state in the entity object (yes, once again ;-). Different to the recently introduced active language we do not clone of the object but directly on the object. So this state-change applies to everyone making use of the "shared" object - that's a problem.

+              // Initialize the language fallback context to mark the object as
+              // fallback-enabled.
+              $translation->languageFallbackContext = $context;

So that's the state - the flag which stores whether fallback is enabled. Also, how do I check whether it's enabled or disable it?

Then, I'm not sure about the method name "getCurrentTranslation" as this would suggest me I get a regular entity translation object - but it is not. Imho we need a method that tells me I get something with language fallback enabled back.

So what about piggy-backing upon regular translation objects (= always use their langcode) and optionally apply language fallbacks? So this is how I'd imagine it:

<?php
 
echo $entity_translation->applyLanguageFallback()->label()
 
// Get field items to render.
 
$items = $entity_translation->applyLanguageFallback()->field;
 
// Imo the following should throw an exception as modifying something with fallback
// enabled would result in unpredictable results - so you shouldn't.
 
$entity_translation->applyLanguageFallback()->field->value = 'foo';
?>

applyLanguageFallback() would return an object using a decorator. Imo it shouldn't be passed around in methods or hooks, if you want to pass your entity on, pass on the entity translation. The code there can itself decide whether language fallback is desired for a certain action or not.

Imho, at the very least we need to clone entity objects before changing their state. Then, the question is whether passing on fallback-enabled objects is something we want to do/encourage - if not, I think a decorator is completely fine and better separates code + state.

plach’s picture

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.

plach’s picture

StatusFileSize
new63.79 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-34.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

plach’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-34.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new59.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-37.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-37.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new60.02 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-39.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Let's get rid of those notices :)

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-39.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new70.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-41.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

yched’s picture

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

yched’s picture

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

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-41.patch, failed testing.

plach’s picture

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.

plach’s picture

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

yched’s picture

Thanks for those answers, @plach.

[edit: removed stupid question, never mind]

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new79.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-48.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This should fix most failures.

Status:Needs review» Needs work
Issue tags:-Needs issue summary update, -API change, -D8MI, -sprint, -language-content, -Entity Field API, -Approved API change

The last submitted patch, et-language_fallback-2019055-48.patch, failed testing.

plach’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Needs issue summary update, +API change, +D8MI, +sprint, +language-content, +Entity Field API, +Approved API change

The last submitted patch, et-language_fallback-2019055-48.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new79.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-52.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

plach’s picture

StatusFileSize
new90.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-53.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-53.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new0 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Merged and (hopefully) fixed failures.

plach’s picture

StatusFileSize
new93.96 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-56.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This time for real :)

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-56.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new94.12 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-58.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Another reroll

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-58.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new94.65 KB
FAILED: [[SimpleTest]]: [MySQL] 58,107 pass(es), 29 fail(s), and 28 exception(s).
[ View ]

This should work better.

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-60.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new98.94 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-62.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Another attempt.

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-62.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new101.36 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-64.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This should hopefully be green again.

amateescu’s picture

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

plach’s picture

StatusFileSize
new100.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-66.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

plach’s picture

Issue summary:View changes

Added issue summary

plach’s picture

StatusFileSize
new100.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-67.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled and updated the issue summary. Reviews welcome :)

plach’s picture

Issue summary:View changes

Updated issue summary

plach’s picture

plach’s picture

Issue summary:View changes

Updated issue summary.

mradcliffe’s picture

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

plach’s picture

StatusFileSize
new107.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-70.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.21 KB

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

plach’s picture

StatusFileSize
new107.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-71.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled

plach’s picture

StatusFileSize
new107.2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-72.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled

Status:Needs review» Needs work
Issue tags:-API change, -D8MI, -sprint, -language-content, -Entity Field API, -Approved API change

The last submitted patch, et-language_fallback-2019055-72.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
Issue tags:+API change, +D8MI, +sprint, +language-content, +Entity Field API, +Approved API change
andypost’s picture

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

plach’s picture

Issue tags:-D8MI, -sprint, -language-content, -Entity Field API, -Approved API change
StatusFileSize
new107.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-76.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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?

plach’s picture

The last submitted patch, et-language_fallback-2019055-76.patch, failed testing.

Status:Needs review» Needs work
Issue tags:-API change, -D8MI, -sprint, -language-content, -Entity Field API, -Approved API change

The last submitted patch, et-language_fallback-2019055-76.patch, failed testing.

plach’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+API change, +D8MI, +sprint, +language-content, +Entity Field API, +Approved API change

The last submitted patch, et-language_fallback-2019055-76.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new107.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-82.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Better reroll

yched’s picture

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)

yched’s picture

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 ?

plach’s picture

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.

yched’s picture

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 ?

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new106.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-87.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new14.19 KB

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.

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-87.patch, failed testing.

plach’s picture

StatusFileSize
new106.52 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-89.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new555 bytes

Fixed a merge issue

yched’s picture

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

Berdir’s picture

Possibly related, might already be (partially) solved by this, but wanted to write the issue down: #2090983: EntityNG::addTranslation() is slow and unnecessarly called (?).

plach’s picture

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

plach’s picture

StatusFileSize
new106.51 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-92.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.5 KB

For reals now

plach’s picture

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.

yched’s picture

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

dsnopek’s picture

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

andypost’s picture

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

kfritsche’s picture

StatusFileSize
new107.2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-98.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-98.patch, failed testing.

plach’s picture

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

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new81.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-101.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new37.48 KB

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

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-101.patch, failed testing.

plach’s picture

StatusFileSize
new81.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-103.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new5.97 KB

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

Berdir’s picture

Status:Needs work» Needs review
  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.

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-103.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new84.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-106.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.48 KB

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

jibran’s picture

  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.

plach’s picture

StatusFileSize
new84.27 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-107.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Testing to see whether a change is actually necessary.

plach’s picture

StatusFileSize
new77.89 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-109.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new33.51 KB

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

plach’s picture

Issue summary:View changes

Updated issue summary.

plach’s picture

Title:Support language fallback on entity fields» Switch from field-level language fallback to entity-level language fallback

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.

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-109.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new4.79 KB
new74.96 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-112.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

plach’s picture

StatusFileSize
new75.02 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-113.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled

yched’s picture

(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

Berdir’s picture

Status:Needs review» Needs work
  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.

Berdir’s picture

Status:Needs work» Needs review

One thing is I think important to consider.

While the API makes a lot of sense, combined with #2072945: Remove $langcode parameter from EntityAccessControllerInterface::access() and friends, #2073217: Remove the $language 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.

Berdir’s picture

Status:Needs review» Needs work

Crosspost with myself. Achievement unlocked.

Berdir’s picture

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

plach’s picture

Cool, will work on this tonight.

plach’s picture

Status:Needs review» Needs work
StatusFileSize
new74.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-120.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new11.39 KB

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.

plach’s picture

Status:Needs work» Needs review
yched’s picture

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.

Berdir’s picture

Status:Needs work» Needs review
+++ 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.

plach’s picture

StatusFileSize
new74.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-124.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new751 bytes

Done

Berdir’s picture

Status:Needs review» Needs work
  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 :)

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new74.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-126.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-126.patch, failed testing.

Anonymous’s picture

Issue summary:View changes

Updated issue summary.

plach’s picture

Issue summary:View changes

Updated issue summary.

plach’s picture

Issue tags:+Prague Hard Problems
plach’s picture

Status:Needs work» Needs review
StatusFileSize
new70.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-129.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new4.06 KB

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

Gábor Hojtsy’s picture

@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 :/

plach’s picture

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

plach’s picture

StatusFileSize
new74.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-132.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new10.89 KB

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

Status:Needs review» Needs work
Issue tags:-API change, -D8MI, -sprint, -language-content, -Entity Field API, -Approved API change, -Prague Hard Problems

The last submitted patch, et-language_fallback-2019055-132.patch, failed testing.

plach’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+API change, +D8MI, +sprint, +language-content, +Entity Field API, +Approved API change, +Prague Hard Problems

The last submitted patch, et-language_fallback-2019055-132.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new74.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-136.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled, test fails still there I guess.

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-136.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new74.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-138.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.15 KB

Let's try this.

plach’s picture

StatusFileSize
new74.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-139.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.7 KB

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.

Gábor Hojtsy’s picture

Issue tags:+blocker

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

yched’s picture

Issue tags:-blocker

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

plach’s picture

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?

yched’s picture

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

plach’s picture

StatusFileSize
new74.79 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-144.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new10.26 KB

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

plach’s picture

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: Exploit hook_entity_prepare_form() to set the form language without having to rely on the current language, which at this point would probably need to rely on explicitly switching content language instead of forcing just a form language change.

yched’s picture

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

fago’s picture

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.

That would make sense to me also. Our way to get the current request entity, really should get it in the usual translation. Then, rendering and forms should just default to that. Consquently, it would make sense to me to remove the $langcode paraemter from the render controller.

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

Yeah, if the caller wants another translation, the caller should pass it. Partly having $langcode parameters instead just seems inconsistent to me. That does not mean, we'd have to convert all code as a part of this issue of course ;-)

ad #145: I'm glad to see more discussion about the method names as you know I've not been happy about the previous ones either.

  1. +++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.php
    @@ -49,6 +51,26 @@ public function getTranslationLanguages($include_default = TRUE);
    +   * @param string $langcode
    +   *   (optional) The language of the current context. Defaults to the current
    +   *   content language.

    Shouldn't that be part of the context? Maybe, I'd like to override the language type being used, but passing a langcode seems weird.

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

    I must say I like the context naming more, +1 on that.

    However, I'm unsure on having the method + the logic on the entity class. It does not seem to be entity-related logic, nor is it obvious that it is an operation that requires data from the entity. (Although it does, it requires its translation languages). Maybe could we have just a helper method on the entity and have the logic elsewhere, as we do for access, validation, storage, ... ?
    Not sure, where it should be though. I cannot really see the need for a separate EntityTranslation controller/handler/service, so it would be best situated at the manager which could have the respective services injected properly?

  3. +++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.php
    @@ -126,7 +137,8 @@ public static function create(ContainerInterface $container, array $configuratio
    +      $container->get('language_fallback_mapper')

    Maybe 'language_negotiant'?

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

    I'd see it as the job of the fallback-mapper to give me the explicit langcode, given a set of available languages I have and the context and target language type. There might be different fallbacks depending on context so just having one desired langcode is probably not enough?

Berdir’s picture

Re ServiceProviderInterface, this was necessary until today, see #1988508: Stop pretending we support Symfony bundles when we really just have service providers. There's a base class now that you can extend and only implement the alter hook.

plach’s picture

StatusFileSize
new80.51 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-148.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new33.53 KB

This should address #146-#148.

@yched:

2. Well, we have other contexts where the operation key makes sense, for instance tokens. The op-specific alter for instance allows CT to alter candidates only when it makes sense.
3. Fixed this: we need to update form language before validation handlers are run, otherwise the entity is not built properly for those.
9. That is going to be addressed in #1966436: Default *content* entity languages are not set for entities created with the API.
10. Yes, a wrong permission string is being used: the changes here are probably exposing this bug and tests now fail without this.
11. Same as 10. Moreover you asked to split that in multiple lines somewhere above :)
12. By acting on all available languages, new translation objects were instantiated and then erroneously stored. Again some change here exposed this bug.
14. Yep, that was a leftover of when supported field-level fallback.
16. Probably this is related to the change to Language::sort().
19. That made more sense when (in the early patches) we had two methods needing to alter their stuff. If we end-up adding back the value map method we should need it again. It seems totally hamless to me. And yes, as explained in 2. we still need the OPERATION-specific variant.

@fago:

Partly having $langcode parameters instead just seems inconsistent to me. That does not mean, we'd have to convert all code as a part of this issue of course ;-)

Entirely removing those might be problematic: for instance we may want to specify an explict translation, but we are no longer able to get a default behavior if we trust the entity translation language. In that case we would be forced to perform entity language negotiation outside. Doing that in the route param converter might mitigate this DX problem probably.

1. @yched above is advocating for the opposite, I adopted his suggestion as it makes the most common usages less verbose.
2. Moving fallback stuff on the entity manager looks indeed cleaner, I don't think we need a helper method on the entity object.
3. No strong preference: I will adopt any solution you and @yched will agree on ;)

Status:Needs review» Needs work

The last submitted patch, et-language_fallback-2019055-148.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new82.69 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-151.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.18 KB

Fixed failing tests

plach’s picture

I'm not clear where we are with this issue: are we happy with the current status? I think exploring the approach where we perform entity language negotiation in route param converters should be a separate issue.

yched’s picture

Thanks for the update, @plach!

So, re #145: Resolving entity language as part of the routing mechanism - I guess that seems like the most reasonable option. I kind of fear it kicks language resolution to the "black magic / dark land that few braves dare looking into" area (controllers will be the entry point for most people looking at code), but doing it below the controllers seems clunky/confusing too, so...
No strong opinion on whether this should be done in this patch, though. Maybe that's tied to the "remove $langcode param in entity_view()" task? @Berdir, what do you think ?

Not sure I see the win in moving getTranslationFromContext() from the entity to the entity manager - but if @plach and @fago both think it's better, I have no real objection myself.
(side note: the code sample in the OP needs to be updated for the newest shape of the language fallback API)

I don't understand why the latest patch changes EntityViewBuilder CommentEntityViewBuilder to receive $entity_info as an injected param ? Doesn't seem related at all to this patch ?

+  public function __construct($entity_type, array $entity_info, EntityManager $entity_manager) {
     $this->entityType = $entity_type;
-    $this->entityInfo = entity_get_info($entity_type);
+    $this->entityInfo = $entity_info;
yched’s picture

More remarks on the interdiff in #149:

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityFormController.php
    @@ -17,6 +17,13 @@
    +   * The entity being used by this form.
    +   *
    +   * @var \Drupal\Core\Entity\EntityManager
    +   */
    +  protected $entityManager;

    Comment is wrong.

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityFormController.php
    @@ -163,4 +140,18 @@ public function buildEntity(array $form, array &$form_state) {
    +  protected function entityManager() {
    +    if (!$this->entityManager) {
    +      $this->entityManager = $this->container()->get('entity.manager');
    +    }
    +    return $this->entityManager;
    +  }

    Service locator instead of injection ? Why isn't the manager injected in the ContentEntityFormController controller ?
    Drawback of moving the getTranslationFromContext() method from the entity to the entity manager : now the manager needs to be injected in more places - would be mitigated if this language resolution is pre-done in the routing layer, but meanwhile we add clunky code and dependencies to a couple classes here :-/

  3. +++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
    @@ -269,6 +269,7 @@ protected function actions(array $form, array &$form_state) {
       public function validate(array $form, array &$form_state) {
    +    $this->updateFormLangcode($form_state);

    Does this really make sense for non-content entities ? ContentEntityFormController does it in its own override of validate(), so this code is for non-content entities. Why would non-content entities support switching the language ?

yched’s picture

I'd like to take a step back on the FalbackMapper classes.

We have a service whose job is to return a list of candidate languages for a requested language and a given context. Two implementations are provided:
- default implementation (NoFallbackMapper) is used on monolingual sites, and unconditionally returns array(LANGCODE_NOT_SPECIFIED)
- when enabled, language.module switches in a different service in the container instead (LanguageFallbackMapper), that unconditionally sets the result to "all enabled languages ordered by weight", and calls an alter hook to let modules modify that. In the end, this alter hook is the only place where there can be logic based on $context.

This is a simple case of "trivial unconditional logic + some alter hook", and the actual extensibility and granularity ("what are we doing fallback for ? entities ? other stuff ?") resides in the hook. Isn't the "service + service override" approach way overkill ? It obfuscates the actual logic IMO - took me that long (155 comments in...) to realize that it was in fact as simple as described above...

Couldn't we:
- get rid of the 'language_fallback_mapper' service altogether
- add a getCandidateLangcodes() method directly to \Drupal\Core\Language\LanguageManager, that does:

if (more than one language) {
  // Build the list of languages based on language_list(), run hook_language_fallback_candidates_alter().
}
else {
  // Return LANGCODE_NOT_SPECIFIED.
}

?

plach’s picture

StatusFileSize
new77.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-156.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new20.63 KB

@yched:

Thanks for the new reviews :)

No strong opinion on whether this should be done in this patch, though. Maybe that's tied to the "remove $langcode param in entity_view()" task? @Berdir, what do you think ?

In the latest patches we have a solution that we know will work. While the other alternative might be preferrable I think we need to experiment with it before being sure. I think this issue has been open for way too long time to wait for this experimentation to happen, so I would be really happy if we could move on with this one and try the other approach in a separate issue.

I don't understand why the latest patch changes EntityViewBuilder CommentEntityViewBuilder to receive $entity_info as an injected param ? Doesn't seem related at all to this patch ?

Well, since I had to change the signatures of EntityViewBuilder/CommentEntityViewBuilder anyway to inject the entity manager, I thought it would be ok to inject also the entiy info instead of relying on that stupid procedural helper.

Service locator instead of injection ? Why isn't the manager injected in the ContentEntityFormController controller ?

Because ContentEntityFormController extends FormBase which in turn uses the service locator pattern.

Drawback of moving the getTranslationFromContext() method from the entity to the entity manager : now the manager needs to be injected in more places [...]

Well, in the previous patches we simply skipped DI for entities since there is no way to do that properly atm, so the entity manager would actually simplify that a bit. However if this becomes awkward we can always inject the entity manager in the entity objects and restore the helper method on those.

Does this really make sense for non-content entities ? [...] Why would non-content entities support switching the language ?

I may want to change the language of a View, for instance, but I am not sure it actually works that way. We should check with Gabor.

Couldn't we:
- get rid of the 'language_fallback_mapper' service altogether
- add a getCandidateLangcodes() method directly to \Drupal\Core\Language\LanguageManager, that does:

Yep, that service made way more sense when we had field-level fallback. Moreover #1862202: Objectify the language system is going to introduced a stripped-down version of the language manger for monolingual sites, hence we will be able to restore the no fallback logic there.

Gábor Hojtsy’s picture

Re: non-content entities, config entities cannot load themselves in different languages, they are not aware of the language support underneath them. Yeah, that is not very consistent with how language works on content entities, but at least for now, config entities don't know if they are loaded in their original form or in a translated form, they are just "dumb" classes getting data from the config system, and the config system knows if there are overrides to add. Once the config entity is loaded, it does not really know if its a translation or not. This may or may not change with #2098119: Replace config context system with baked-in locale support and single-event based overrides but I don't think this patch should deal with that at all.

yched’s picture

re: entity-language handling for non-content entity forms: Yes, the patch here just moves some code around. It seems there are simplifications to be made, but it's outside of scope here. I opened #2123867: Simplify/cleanup language handling in EntityFormController.

Will look into #156 more closely tonight - yay for the simplification !
Just: NoFallbackMapper returned LANGCODE_NOT_SPECIFIED, now the new patch returns LANGCODE_DEFAULT ?

plach’s picture

Just: NoFallbackMapper returned LANGCODE_NOT_SPECIFIED, now the new patch returns LANGCODE_DEFAULT ?

Because non-eneglish monolingual sites by default create entities in the default language. Right now both constants have 'und' as value, so the difference is putrely academic. However with #2076445: Make sure language codes for original field values always match entity language regardless of field translatability we will have an actual difference and I think LANGCODE_NOT_SPECIFED would not always work.

plach’s picture

StatusFileSize
new78.12 KB
PASSED: [[SimpleTest]]: [MySQL] 59,996 pass(es).
[ View ]

Rerolled

plach’s picture

@Gabor:

I just posted a patch in #2123867: Simplify/cleanup language handling in EntityFormController, can you check that I am not breaking config entity language assignment please? :)

plach’s picture

Issue summary:View changes

Updated issue summary.

yched’s picture

Last nitpicks...

- About "service locator" in ContentEntityFormController

+ protected function entityManager() {
+    if (!$this->entityManager) {
+      $this->entityManager = $this->container()->get('entity.manager');
+    }
+    return $this->entityManager;
+  }

You replied: "ContentEntityFormController extends FormBase which in turn uses the service locator pattern".
It doesn't appear so ? FormBase uses the static create($container) factory pattern, and child classes can (and some do) override it to extract dependencies in there and pass them to the __construct(). Why couldn't we inject the entity manager this way ?

- Yay for getting rid of the "fallback service", and putting the getCandidateLangcodes() method directly on the LanguageManager, but the method name kind of lacks context now - the word "fallback" doesn't appear anywhere. It's also totally disconnected from the name of the alter hook it triggers (hook_language_fallback_candidates_alter()).
--> getLanguageFallbackCandidates() ? getFallbackCandidates() ?

- Sorry, patch doesn't apply anymore ;-)

Other than that, I think this is ready. I'm fine with exploring the "resolve entity language during route param upcasting" in a separate issue, if @Berdir is ok with it too (although not sure exactly when @Berdir goes afk)

plach’s picture

Issue summary:View changes
Related issues:+#1810370: Entity Translation API improvements, +#1260640: Improve field language API DX
StatusFileSize
new9.1 KB
new78.47 KB
FAILED: [[SimpleTest]]: [MySQL] 57,066 pass(es), 465 fail(s), and 233 exception(s).
[ View ]

It doesn't appear so ? FormBase uses the static create($container) factory pattern

You are right, sorry, I completely missed that :(

plach’s picture

plach’s picture

Issue summary:View changes
StatusFileSize
new2.75 KB
new79.04 KB
FAILED: [[SimpleTest]]: [MySQL] 56,973 pass(es), 463 fail(s), and 232 exception(s).
[ View ]

While updating the issue summary I realized the new method was not declared on the entity manager interface.

plach’s picture

Issue summary:View changes
yched’s picture

Status:Needs review» Reviewed & tested by the community

Yay. As far as I'm concerned, RTBC if green. Thanks!

(I guess test failures will tell us that, but shouldn't we check for classes that subclass ContentEntityFormController and might already implement create() __construct() themselves ?)

yched’s picture

Yay. As far as I'm concerned, RTBC if green. Thanks!

(I guess test failures will tell us that, but shouldn't we check for classes that subclass ContentEntityFormController and might already implement create() __construct() themselves ?)

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 22: et-language_fallback-2019055-22.patch, failed testing.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 26: et-language_fallback-2019055-26.patch, failed testing.

YesCT’s picture

#166 failed with 463 fail(s), and 232 exception(s), lots of "simplexml_import_dom(): Invalid Nodetype to import" but others also..

duplicate tests (or testing each hidden) is related to #2126225: PIFT creating duplicate test records, causing PDO exception during pift-cron

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new20.02 KB
new96.14 KB
FAILED: [[SimpleTest]]: [MySQL] 58,542 pass(es), 270 fail(s), and 115 exception(s).
[ View ]

There may be a d7do bug around but surely many of those failures are caused by me posting patches at 3AM...

@yched was right once more ;)

plach’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityFormController.php
@@ -7,6 +7,7 @@
+use Drupal\Core\Entity\EntityManagerInterface;

And this is unneeded: to be fixed on the next reroll.

+++ b/core/modules/field/lib/Drupal/field/Plugin/views/field/Field.php
@@ -261,14 +261,7 @@ public function query($use_groupby = FALSE) {
+        $langcode_fallback_candidates = $this->languageManager->getFallbackCandidates(array('operation' => 'views_query', 'langcode' => $langcode, 'data' => $this));

And also this line needs to be fixed.

Status:Needs review» Needs work

The last submitted patch, 172: et-language_fallback-2019055-172.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new3.18 KB
new131.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-language_fallback-2019055-175.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Let's try this one

Status:Needs review» Needs work

The last submitted patch, 175: et-language_fallback-2019055-175.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new98.86 KB
PASSED: [[SimpleTest]]: [MySQL] 60,031 pass(es).
[ View ]

Wrong patch, sorry, the interdiff is the same as in #175.

plach’s picture

Issue summary:View changes
plach’s picture

Issue summary:View changes
plach’s picture

Issue summary:View changes
plach’s picture

Issue summary:View changes
plach’s picture

Issue summary:View changes
yched’s picture

Status:Needs review» Reviewed & tested by the community
+++ b/core/modules/forum/lib/Drupal/forum/Form/ForumFormController.php
@@ -121,8 +76,9 @@ public function buildEntity(array $form, array &$form_state) {
+    $term_storage = $this->entityManager->getStorageController('taxonomy_term');
+    $status = $term_storage->save($term);

-    $status = $this->termStorage->save($term);

Minor: no need to fetch the term storage controller to do that, $status = $term->save() would work equally well ?

Other than that, changes look good. RTBC when green again.

plach’s picture

Yeah, I didn't even look at that code :D

yched’s picture

As a side note, this latest batch of changes since #164 shows that DI is a real problem in terms of API breaks: when a parent class needs a new dependency, all child classes that were overriding create() / __construct() because they had dependencies of their own, suddenly break :-(. Those cases are going to be fun to handle once we release...

plach’s picture

Issue summary:View changes

Updated suggested commit message.

yched’s picture

Hem... so I basically tweeted a short version of #185, and it turned into an interesting discussion about DI and base classes, with @msonnabaum & @tim.plunkett advocating that maybe we should move away from constructor injection for classes that are intended to be used as base classes, in favor of service locator-style fetching of dependencies - in other words, exactly what the patch was doing with ContentEntityFormController::entityManager() before I asked for a change in #163 ;-)
(well, with an additional method allowing setter injection for unit testing)

That patch here is green and RTBC now, plus it might be worth having this discussed and agreed upon in a real d.o issue, so I'm leaving this one alone... Worst case, if this gets agreed, the interdiffs in #172 / #175 directly give us the parts that can be reverted later on.

Sorry about that, @plach :-/

Gábor Hojtsy’s picture

@yched: that sounds like could be done in a followup with more of a recorded discussion :) This issue in itself would enable so much more things, eg. to work on displaying the title of translated nodes, which are currently enterable on forms, stored, etc. but not displayed.

fago’s picture

I like the move to the method + alter hook, good improvements!
Here another review:

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -423,6 +423,12 @@ protected function getTranslatedField($property_name, $langcode) {
    +    if ($property_name == 'langcode') {

    This should be already handled onChange(), thus does not make sense here?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -456,4 +456,35 @@ public function getEntityTypeLabels() {
    +   */

    Minor, but should be public function.

    Also, it should not restrict to ContentEntityInterface, should it? I could roll my own entity-class-variant that is translatable, which I would not be able to use then? We have TranslatableInterface so we should check for that, and just type-hint on EntityInterface in that case?

  3. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
    @@ -58,14 +51,29 @@ public static function create(ContainerInterface $container) {
    +    // Make the comment inherit the current content language unless specifically
    +    // set.

    Why is that implemented specifically for comments, isn't that regular content entity form behaviour?

    Update: Clarified in #149 9/10.

  4. +++ b/core/modules/content_translation/content_translation.module
    @@ -323,7 +323,12 @@ function content_translation_translate_access(EntityInterface $entity) {
    -  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);

    Not sure how this hunk relates.

    Update: Clarified in #149 9/10.

  5. +++ b/core/modules/content_translation/content_translation.module
    @@ -627,7 +632,10 @@ function content_translation_permission() {
    +  if ($entity instanceof ContentEntityInterface && $entity->isTranslatable() && count($entity->getTranslationLanguages()) > 1) {

    Maybe stuff for another issue, but shouldn't Content translation check for translatbleinterface instead of content entity interface?

  6. -  public function label($langcode = Language::LANGCODE_DEFAULT) {
    +  public function label($langcode = NULL) {
         $info = $this->entityInfo();
    +    if (!isset($langcode)) {
    +      $langcode = $this->activeLangcode;
    +    }

    That's reasonable behaviour, but not what EntityInterface documents for it. -> Needs update.

fago’s picture

Status:Reviewed & tested by the community» Needs work
yched’s picture

@fago:
3 & 4: See #146, items 9 & 10, answered in #149

fago’s picture

Ah, thanks - I've updated my comment to show which part of the review are already clarified.

plach’s picture

Status:Needs work» Needs review
StatusFileSize
new5.01 KB
new100.23 KB
PASSED: [[SimpleTest]]: [MySQL] 59,916 pass(es).
[ View ]

Thanks for the review!

This should be already handled onChange(), thus does not make sense here?

It does because in the line above we have $this->get($property_name)->setValue($value, FALSE), thus onChange() is not called.

Maybe stuff for another issue, but shouldn't Content translation check for translatbleinterface instead of content entity interface?

Yes, actually a class implementing EntityInterface + TranslatableInterface should be enough for CT to work. Totally another issue though :)

That's reasonable behaviour, but not what EntityInterface documents for it. -> Needs update.

I updated the docs so that they make sense but we should probably get rid of that parameter and just rely on the active language.

fago’s picture

Status:Needs review» Reviewed & tested by the community

It does because in the line above we have $this->get($property_name)->setValue($value, FALSE), thus onChange() is not called.

True, that's a bit weird though and probably stems from an unncessary, previous optimization where onChange() was empty. Howsoever, let's streamline $notify handling of set() methods in a follow-up.

Improvements are good, thanks. Back to RTBC then.

catch’s picture

Title:Switch from field-level language fallback to entity-level language fallback» Change notice: Switch from field-level language fallback to entity-level language fallback
Priority:Critical» Major
Status:Reviewed & tested by the community» Active

Nice to see this one green.

Committed/pushed to 8.x, thanks!

Will need a small change notice I think.

catch’s picture

Missed the suggested commit message (as always), so reverted then re-committed with that. Sorry folks.

plach’s picture

Who-hoo! Enjoy translated node titles :)

yched’s picture

Awesome! @plach++!

plach’s picture

Status:Active» Needs review

Updated the Entity Translation API change notice:

https://drupal.org/node/2040323/revisions/view/2879569/6683781

Also created a new one for the language manager changes:

https://drupal.org/node/2129611

Gábor Hojtsy’s picture

Looks almost good, however the $context and what it may contain is not explained. Also https://drupal.org/node/2129611 needs to link to https://drupal.org/node/2040323 for explanation of that context (where the operation comes from). The operation is not clear from the new change notice. The old one also does not list what those may be (i.e reserved operation names).a

Berdir’s picture

Does the updated change notice also be reflected in https://drupal.org/node/2040721, which was created based on the old change notice?

plach’s picture

@Berdir:

I will update the documentation page as soon as the change notice is ok :)

hass’s picture

Gábor Hojtsy’s picture

@hass: neither. Eg. the translation save problem was reproduced as early as Prague (if not before).

Gábor Hojtsy’s picture

For me as soon as #200 is resolved, this is done :) I don't fully understand what context may be myself, otherwise I would do it :/ Sorry.

plach’s picture

I will take care of that tonight.

plach’s picture

I updated the new change notice with a more accurate description of the context array. The old one has a link to it.

Wim Leers’s picture

Issue summary:View changes
Gábor Hojtsy’s picture

Title:Change notice: Switch from field-level language fallback to entity-level language fallback» Switch from field-level language fallback to entity-level language fallback
Priority:Major» Critical
Status:Needs review» Fixed
Issue tags:-sprint

Changes look good to me, thanks!

plach’s picture

I just updated the documentation page with the latest stuff.

fago’s picture

Status:Fixed» Closed (fixed)

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

SiliconMind’s picture

Now when this has been resolved, any chance that we could also add language fallback for string translations? #2122175: String translation does not honor language fallback