Referenced entity in wrong language

(Never mind the .gitlab-ci changes. They were needed to make tests green again and are documented in 🐛 Make Gitlab D11 PHPUnit job run successfully Active )

Reviewed:

• Code is good. (I injected the service after we talked. I also added a tiny change to CustomElementsGenerator for consistency.)
• There is no other processor code that directly manipulates $data->entity or a reference->getTarget().
• There is other processor code that calls CustomElementGenerator::generate($data->entity) without a $langcode parameter.

This extra call (see MR) brings DefaultFieldItemProcessor in line with those other generate() calls, so this MR feels logical. (Note lupus_ce_renderer's CustomElementsController also always calls generate() without a $langcode parameter.)

I did not know what language does, so I needed to think and test, on a clean lupus-decoupled install. Questions that came to mind:

Is this change considered a bugfix and not a behavior change?

I think so. Some behaviour could change, but

• Per above: it just becomes more consistent.
• The current behaviour that returns a translated DE node with an untranslated EN taxonomy term, is clearly wrong.

Are we forcing the 'proper' language?

The current code (that calls EntityRepository::getTranslationFromContext($entity, NULL) -> ConfigurableLanguageManager::getCurrentLanguage(CONTENT-LANGUAGE) forces "the current content language, as negotiated by whatever negotiator is active".

Milan told me that with this MR, he was actually seeing an EN taxonomy term while outputting a DE node, when his own account language was set to EN. That sounded dangerous to me / I don't think we want that. However, I could not reproduce it... and also cannot really explain this, because the 'root' call in CustomElementsController leads to the exact same way of calling getTranslationFromContext() as the changed call in this MR.

An alternative is to force the language from the entity, for any referenced entity. That is: to explicitly pass $data->getEntity()->language()->getId() as an extra parameter to any generate() or getTranslationFromContext() that is called in any processor.

I thought that conceptually, that would be a better solution, after Milan's remark above.

Then I tried paragraph references (ParagraphFieldItemProcessor). Its $data->getEntity() has an activeLancode 'x-default' so I get English paragraph contents, even though we just got this field ($data) from the entity that has activeLancode 'de'. (Whereas in the current code / passing NULL as the language into generate(), I correctly get German paragraph contents.) I don't even understand this.

So if we made the same change in ParagraphFieldItemProcessor, that would lead to bugs. It feels creepy to me that a reference field can work like this, so I changed my mind / want to stay away from this.

@fago for your review, see MR:

• Do you think we can just add this change into stable 8.x-2.x and do a new release? (I think we can and this is 'just' a bugfix, not a behavior change, per previous comment)
• Do you think calling getTranslationFromContext($entity, [ no explicit language ] ) is the correct thing to do? If you know the answer is "yes", you don't need to read my above ramblng.

Yes, that makes sense to fix. But I think it's not right atm.

When another language, not the current content language, is shown, defaulting to the current content language is not what you want. We'd have to forward the selected langcode. So we must pass the entity language as $langcode when calling this.

Next, the bug might exist in other processors also. When we fix it, we should fix it in all of them.
In 3.x, we should make sure our formatters do that correctly. Since 3.x is our main development branch, we should first develop it for 3.x and then backport in 2.x

We'd have to forward the selected langcode. So we must pass the entity language as $langcode when calling this.

Next, the bug might exist in other processors also. When we fix it, we should fix it in all of them.

In principle I agree. This is what I was trying to do, when encountering the problem with paragraph references that I described previously.

Since you want this too, I checked further into what the problem is:

The only way I know, to get "the entity language", is doing $field->getEntity()->language().
But that returns the original untranslated language, for any non-translatable field. So, for untranslatable entityreferences (e.g. paragraphs), that's worse than now: all paragraphs in returned JSON will be untranslated.

$entity = get_some_entity()
// $entity->defaultLangcode:       'en'
// $entity->activeLangcode :       'de'
$field_item_list = $entity->get(NON_TRANSLATABLE_FIELD);
// $field_item_list->langcode:     'en' 
$check_entity = $field_item_list->getEntity();
// $check_entity->defaultLangcode: 'en'
// $check_entity->activeLangcode : 'x-default'

This is not specific to paragraph references; it's for any non-translatable field. The reason is in contentEntityBase::getTranslatedField(), which is always called by $entity->get():

• It calls itself recursively with language 'x-default'
• the recursive call sets the 'x-default' entity in the fieldItemList.

So I'm stuck on this. (I don't believe paragraphs can be the only non-translatable entityreferences...)

I was contemplating writing a helper function to check if $entity->activeLangcode == 'x-default', and then getting the language code from ConfigurableLanguageManager::getCurrentLanguage(CONTENT-LANGUAGE) instead as a fallback. But $entity->activeLangcode is not accessible. So.... mmmh.

So the only thing I know to do is always get it from 'the countext' =~ ConfigurableLanguageManager::getCurrentLanguage(CONTENT-LANGUAGE). Which is what the current MR does.

Note: this is exactly the same for 3.x as for 8.x-2.x.

Setting to Needs Work after editing my last paragraph for my plan... for tomorrow.

Done.

MR 102 is made against 3.x.
It gets the language from the entity where possible. So the behavior is better in this regard, but now also inconsistent. (Reasons were given in #5 and #9.)

MR 101 is still open for comparision; its behavior is 'not better, though more consistent'. It is against 8.x-2.x.

Since both branches work exactly the same, I will commit a similar one to each. That is: e.g. if MR 102 is approved, I will close MR 101 and commit an equivalent of MR 102 to 8.x-2.x too.

I still think we need to do what I was writing before:

We'd have to forward the selected langcode. So we must pass the entity language as $langcode when calling this.

Next, the bug might exist in other processors also. When we fix it, we should fix it in all of them.

For 2.x I think it's good enough to fix this with the call-out to the global or even not fix it -> 3.x is the main branch now.
For 3.x we should fix it properly.

i.e. we must extend our API to accept an optional $langcode parameter, so we can pass it on during processing *explicitly*. This is what core does during rendering as well. I guess this means we need to adjust the processors to do so. A new optional $langcode = NULL parameter is no big change, since it keeps BC when the interface was implemented exactly before.