Entity references of untranslatable fields are not displayed in the correct translation

That behaviour was expected, I also mentioned it in the other issue:
#2513094-22: ContentEntityBase::getTranslatedField and ContentEntityBase::__clone break field reference to parent entity

So with my changes you still can see the translated referenced entity on the referer view, but for that to happen you should make the entity reference field translatable.

Making ImageFormatterBase::getEntitiesToView compatible to EntityReferenceFormatterBase::getEntitiesToView.

Created a subclass for EntityReferenceFieldTranslatedReferenceViewTest that sets the fields to untranslatable, reverted the trait from the other patch, fixed the test warning and added a @param for the langcode.

resolved some nitpicks.

I would say, now the changes reflecting ER from the other issue are reverted properly and with the additional changes from plach we get back the right behaviour.

Something bugs me here.

The original code in EntityReferenceFormatterBase::getEntitiesToView() tries to display referenced entities in the same language than the containing entity (which, agreed, is the behavior we want), with this code :

$parent_entity_langcode = $items->getEntity()->language()->getId();
if ($entity instanceof TranslatableInterface && $entity->hasTranslation($parent_entity_langcode)) {
  $entity = $entity->getTranslation($parent_entity_langcode);
}

This worked before #2513094: ContentEntityBase::getTranslatedField and ContentEntityBase::__clone break field reference to parent entity. If it doesn't now, this seems to mean we now have :

$fr_entity = $orig_entity->getTranslation('fr');
$fr_entity->translatable_field->getEntity()->language()->getId() == 'fr' 
$fr_entity->untranslatable_field->getEntity()->language()->getId() == 'en';

Meaning,

$fr_entity->translatable_field->getEntity() === $fr_entity
$fr_entity->untranslatable_field->getEntity() === $orig_entity

That looks very wrong ?
$entity->field->getEntity() sould be === $entity, whether the $entity is translated or not, and the field translatable or not ?

Added $langcode to FormatterInterface, corrected the test.

That looks very wrong ?
$entity->field->getEntity() sould be === $entity, whether the $entity is translated or not, and the field translatable or not ?

We cannot have that: untranslatable field items are shared among translation objects.

I amended the test so it should pass tests instead of causing exceptions (that the exception happens is the whole point of the test, isn't it?). Is no one working on the issue currently or what's the status? I am waiting for a fix so I can continue with this issuse.

Edit: Nope, I think we want the test to pass again?

removed the test.

Here is your feedback implemented.
I had a problem with point 4 though:

viewElements() also has $langcode no longer optional then.

The viewElements() method has to match the FormatterInterface, which we can't modify anymore. So I left the $langcode = NULL there and use the language manager to get the current language in case the the langcode isn't defined.

Edit:

Uhhh...?

Exception message

Here is an updated patch, using \Drupal::languageManager instead of modifying the constructor.
Also fixed a couple of errors I had which complained about the missing $langcode attribute.

Here is the complete version. All core formatters that use viewElements now have the langcode as a parameter, where necessary I added the languageManager()->getCurrentLanguage() to ensure we always have the langcode.

Forgot some usages of LanguageInterface, added them.

Nice!
But if the use statements from #24 are needed why didn't the patch in #23 fail?

The tests all supply a langcode, it seems. When I add the line $langcode = NULL; before any of the missing Interfaces, the tests fail and complain about the missing interface.

Looks good, thanks :)

+++ b/core/lib/Drupal/Core/Field/FormatterBase.php
@@ -76,8 +77,11 @@ public function __construct($plugin_id, $plugin_definition, FieldDefinitionInter
+    if (empty($langcode)) {
+      $langcode = \Drupal::languageManager()->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId();
+    }
+    $elements = $this->viewElements($items, $langcode);

Not to be fixed here, but I'm wondering whether it would make sense to have a trait encapsulating this logic once and for all. We have already many instances of it in core...

Here is a freshly rebased patch. This one should apply again.

Another rebase, if someone could finally review this thing and help get this committed, that would be awesome!

Was RTBC before, so back to RTBC if the reroll is green ?

+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
@@ -1,3 +1,4 @@
+

That's not supposed to be there...

Here is the correctly rebased version.

Test green, setting back to RTBC.
Edit: I also checked all changes this time, I'm not just relying on the testbot :).

1. The amount of formatters changed in this patch does not seem to be match the added test coverage. The only tests that seem to be improved are the entity reference tests - the formatter where the bug was noticed. But every other formatter has changed - are we missing coverage?

2. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceFieldTranslatedReferenceViewTest.php
   @@ -10,6 +10,7 @@
   +use Symfony\Component\Validator\Constraints\False;
   

   Not used.

3. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
   @@ -41,6 +42,20 @@ class ImageFormatter extends ImageFormatterBase implements ContainerFactoryPlugi
      /**
   +   * The link generator.
   +   *
   +   * @var \Drupal\Core\Utility\LinkGeneratorInterface
   +   */
   +  protected $linkGenerator;
   

   Whilst this might be correct it is totally out of scope.

Given the solution on this issue I think we have to get this fixed before the release candidate - tagging as such.

Feedback implemented, mostly:

1. The interface change forced me to update a lot of classes that might need testing, I'm not really sure, someone who knows what could go wrong could possibly create an issue to extend test coverage for untranslatable fields and their content in a translated entity (I can understand the issue with entity_references, but I have a hard time to imagine the same scenario for the ImageFormatter, as an example).

2. removed

3. removed

Just to be sure, there are several scenarios we are talking about:

• a) entity reference field is not translatable, pointing to a not translated eg. taxonomy term
• b) entity reference field is not translatable, pointing to a translated eg. taxonomy term (current language included)
• c) entity reference field is not translatable, pointing to a translated eg. taxonomy term (current language NOT included, using the default language of the taxonomy term)
• d) entity reference field translatable, pointing to a not translated eg. taxonomy term (only default language)
• e) entity reference field translatable, pointing to a translated eg. taxonomy term with current or default fall back language

so the bug used to mess up scenario b) resulting in only rendering the default language of the referenced entity (eg. taxonomy term)
hopefully this helps a bit

I had a look at the code base. If we would abstract away the check for the langcode, we'd end up with a much cleaner code. Hope this will help a bit.

updated broken files

Alright, I made the $langcode non-optional, updated all the formatters and added a method which set's the langcode in case it isn't defined yet.
Still no additional tests, I was thinking about adding one for the EntityReferenceTaxonomyTermRssFormatter, but I'd like to have some feedback about the general patch first (like a testbot run) :).

1. +++ b/core/lib/Drupal/Core/Field/FormatterBase.php
   @@ -110,6 +112,15 @@ public function view(FieldItemListInterface $items) {
   +  public function setMissingLangcode(&$langcode) {
   +    if (empty($langcode)) {
   +      $langcode = \Drupal::languageManager()->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId();
   +    }
   +  }
   

   That's... really weird :-)

   - A method named as a setter, that doesn't set anything but instead adjusts the passed param by ref. When you see a setter you expect it changes some state in the object, so that makes you think Formatters hold a state on langcode, that's misleading.

   - That's just helper code for "fallback to some langcode if I don't have one", I'm not sure it belongs to Formatters / FormatterInterface. I'd really like to avoid adding kludge to FormatterInterface.

   - Also, it's feels really a bummer that each (ER) formatter seems to have to call the method in various places (the patch adds calls from within view() implementations and getEntitiesToView() implementations) ?
   What case is it actually needed for ? What is this trying to cover ?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceFormatterBase.php
   @@ -36,24 +37,25 @@
   +   * @param $langcode
   +   *   The language code of entities.
       *
       * @return \Drupal\Core\Entity\EntityInterface[]
       *   The array of referenced entities to display, keyed by delta.
       *
       * @see ::prepareView()
       */
   -  protected function getEntitiesToView(EntityReferenceFieldItemListInterface $items) {
   +  protected function getEntitiesToView(EntityReferenceFieldItemListInterface $items, $langcode) {
   

   phpdoc misses the typehint, and the description is not really accurate. In the context of that method, that's the langcode in which we want to display the referenced entities.

#46 - I've applied this to the head of 8.0.x and tested it as part of my investigation of #2540556 and can confirm that it fixes this issue.

I've renamed the helper method (setMissingLangcode -> languageFallback), better suggestions are welcome.
The method is not in the FormatterInterface anymore.
I only call it once in the FormatterBase now, let's see if it is necessary to call the method in other classes.
I also fixed the missing type in the phpdocblock and added the information that it is the language code of the entities to reference.

More test coverage is necessary, but I'm still not sure how to actually test it. (Currently, one test has the added $translatable = true; property and extends the same test but sets the translatable to false, can I do the same for tests which are also field formatters of entity references?)

Here is the patch, had some problems after some comment suddenly appeared :D.

Yay. RTBC if green

Discussed with @yched and @Berdir - without this change we can;t deliver the expected functionality of entity references in Drupal 8 - this for me is very close to a critical - therefore I think implementing this with a very obvious and easy to fix API break makes sense. Committed d0c1376 and pushed to 8.0.x. Thanks!

I had some issues with untranslatable entities and translatable entity references that is quite related to this, documented this here #2584745: Entity references should be displayed translated on non translatable entity types.