EntityViewBuilder::viewField() does not respect entity current language when used with an entity reference field

The challenge with viewField() is that view() actually allows to pass in an explicit language and that is also used for references, but viewField() does not.

For example if you have a list in views, then you can select in which language the entity should be displayed, you can for example list all translations on a single page and each entity should be rendered in its language. This would not be able to render the referenced user in the appropriate language for each.

This doesn't make it worse, but it's also not a complete fix.

Don't forget to set the issue to needs review so that the patch can get tested.

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -396,6 +396,9 @@ protected function isViewModeCacheable($view_mode) {
+      $entity = \Drupal::service('entity.repository')->getTranslationFromContext($entity);

This service should be injected instead of statically calling it. The class already contains a create and construct.

Tested the patch, my scenario was that (in layout builder) I displayed a paragraph (entity reference revision) with a translatable text field. In both languages the source languages was displayed, after applying this patch the correct language is used! ++

Applying Correct patch with injecting the entity.repository service. Please review and test my patch.

Some nits:

1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -72,6 +73,13 @@ class EntityViewBuilder extends EntityHandlerBase implements EntityHandlerInterf
   +   * @var \Drupal\Core\Entity\EntityRepository
   

   Typehint against interface:

   Drupal\Core\Entity\EntityRepositoryInterface

   Also in the constructor.

2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -396,6 +405,10 @@ protected function isViewModeCacheable($view_mode) {
   +      $entity = $entityRepository->getTranslationFromContext($entity);
   

   Unnecessary variable declaration. Just call $this->entityRepository->getTranslationFromContext($entity) directly.

Thanks Steven Buteneers,
Here below is the corrected and updated patch.Please review and test it.

Kindly ignore the #7 patch as Interface is still missing in typehint in constructor. Please find correct patch in #9.

Here is the Patch.

Sorry I missed some things in the previous patch, here are my last comments:

1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -72,6 +73,13 @@ class EntityViewBuilder extends EntityHandlerBase implements EntityHandlerInterf
   +  protected $entityRepository;
   

   This should be declared after the $entityManager property (based on naming convention)

2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -72,6 +73,13 @@ class EntityViewBuilder extends EntityHandlerBase implements EntityHandlerInterf
   +  protected $entityRepository;
   
   @@ -84,12 +92,13 @@ class EntityViewBuilder extends EntityHandlerBase implements EntityHandlerInterf
       *   The theme registry.
   

   Docblock needs to be updated as well

3. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -84,12 +92,13 @@ class EntityViewBuilder extends EntityHandlerBase implements EntityHandlerInterf
   +  public function __construct(EntityTypeInterface $entity_type, EntityManagerInterface $entity_manager, LanguageManagerInterface $language_manager, Registry $theme_registry = NULL, EntityRepositoryInterface $entityRepository = NULL) {
   

   I don't think we should make the entity repository nullable. I see this happens with the theme registry (don't ask me why, as they actually try to assign the same service twice anyway).
   Therefore I suggest we inject the entity repository before the theme registry and make it required.

   Also $entityRepository (in the constructor) should be written snake_case to match up with the other parameters in the constructor.

At last: you also need to inject the service in the create function, as is done with the other services as well.

Thanks for the update. I have updated my patch.Please check if anything missing.Also please test and review it.

Kindly review and test my code

Some nits:

1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -84,12 +92,13 @@ class EntityViewBuilder extends EntityHandlerBase implements EntityHandlerInterf
       * @param \Drupal\Core\Theme\Registry $theme_registry
   

   Add docblock for the entity repository here (before the theme registry)

2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -84,12 +92,13 @@ class EntityViewBuilder extends EntityHandlerBase implements EntityHandlerInterf
   +  public function __construct(EntityTypeInterface $entity_type, EntityManagerInterface $entity_manager, LanguageManagerInterface $language_manager, Registry $theme_registry = NULL, EntityRepositoryInterface $entity_repository) {
   

   Inject the entity repository before the theme registry. (note: Also in the create function).

Thanks for the update. I have completed both correction and updated the patch.Please check

1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -81,14 +89,17 @@ class EntityViewBuilder extends EntityHandlerBase implements EntityHandlerInterf
       *   The theme registry.
       */
   -  public function __construct(EntityTypeInterface $entity_type, EntityManagerInterface $entity_manager, LanguageManagerInterface $language_manager, Registry $theme_registry = NULL) {
   +  public function __construct(EntityTypeInterface $entity_type, EntityManagerInterface $entity_manager, LanguageManagerInterface $language_manager, EntityRepositoryInterface $entity_repository, Registry $theme_registry = NULL) {
        $this->entityTypeId = $entity_type->id();
        $this->entityType = $entity_type;
        $this->entityManager = $entity_manager;
        $this->languageManager = $language_manager;
   +    $this->entityRepository = $entity_repository;
        $this->themeRegistry = $theme_registry ?: \Drupal::service('theme.registry');
      }
    
   @@ -100,6 +111,7 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
   

   I disagree with that.

   That would break subclasses that already inject the registry.

   Instead, just like the $theme_registry, the argument needs to be last an optional, with a fallback to \Drupal::service().

2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -396,6 +408,9 @@ protected function isViewModeCacheable($view_mode) {
        $entity = $items->getEntity();
   +    if ($entity instanceof TranslatableInterface && $entity->isTranslatable()) {
   +      $entity = $this->entityRepository->getTranslationFromContext($entity);
   +    }
   

   While touching and injecting this, which is great, should we also update the non-injected calls at the same time?

   Also, the problem is that we have no way to render it in a specific language.

   I think the title should probably say "when used with a non-translatable field", because that's the problem/difference.. for translatable field, $entity is the correct translation if $items is.

   The problem is that this makes it impossible for translatable fields to be rendered in not-the-current-language as we have no $langcode argument and trying to add it would be an API change.

And we need tests :)

Previous Patch got failed as it was generated from 8.4.x branch. Now it has been taken from 8.7.x branch.

1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -41,6 +42,13 @@ class EntityViewBuilder extends EntityHandlerBase implements EntityHandlerInterf
   +  ¶
   

   Empty space should be removed.

2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -81,14 +89,16 @@ class EntityViewBuilder extends EntityHandlerBase implements EntityHandlerInterf
   -  public function __construct(EntityTypeInterface $entity_type, EntityManagerInterface $entity_manager, LanguageManagerInterface $language_manager, Registry $theme_registry = NULL) {
   -    $this->entityTypeId = $entity_type->id();
   +public function __construct(EntityTypeInterface $entity_type, EntityManagerInterface $entity_manager, LanguageManagerInterface $language_manager, EntityRepositoryInterface $entity_repository = NULL, Registry $theme_registry = NULL) {    $this->entityTypeId = $entity_type->id();
   

   As @Berdir mentioned in comment #16 point 1 EntityRepositoryInterface $entity_repository = NULL should be the last argument so it does not break the subclass.

Also, this needs tests because this is a significant change.

This patch adds the changes requested in #22.

I think the title should probably say "when used with a non-translatable field", because that's the problem/difference.. for translatable field, $entity is the correct translation if $items is.

Agree. So, why no add this only to fields that are not translatable? This patch does that.

Tests are tricky here... Should we create a new KernelTest to combine Translations and References right? I couldn't find an example to use as base. @Berdir any hint?

Thanks for patch. solve problem with sub-paragraphs translation

The Patch works perfectly. Thanks!

Thanks for the review @Regnoy and @keynone can we also change the status to RTBC now.

This cannot be RTBC until has some tests.

Thanks! the patch works!

Reviewed en tested, works!

In order to commit a bug fix we need an automated to test to prove that we've fixed the bug and ensure that we don't break it again in the future. For more information about writing tests in Drupal 8 see the following links:

1. https://www.drupal.org/docs/8/testing
2. https://api.drupal.org/api/drupal/core%21core.api.php/group/testing/8.7.x

This didn't apply cleanly for me with the latest 8.7.x-dev. I think it's because of #3023981: Add @trigger_error() to deprecated EntityManager->EntityRepository methods and it's gonna need a re-roll? I wish I was good at rolling patches or I'd take a stab at this... :-/

Pretty sure the fix applied above in the latest 8.7.x-dev gives us similar changes to this one, it seems maybe the only piece that might still be needed is this bit? Line #s prob changed and such since #23 was created though.

@@ -427,7 +439,18 @@ protected function isViewModeCacheable($view_mode) {
    */
   public function viewField(FieldItemListInterface $items, $display_options = []) {
     $entity = $items->getEntity();
-    $field_name = $items->getFieldDefinition()->getName();
+    $field_definition = $items->getFieldDefinition();
+
+    // Even when fields are set to not translatable, they still need to know
+    // which is the language of the current entity being rendered.
+    // This allow to render, for example, referenced entities in the same
+    // language than the current entity.
+    if (!$field_definition->isTranslatable() &&
+        $entity instanceof TranslatableInterface &&
+        $entity->isTranslatable()) {
+      $entity = $this->entityRepository->getTranslationFromContext($entity);
+    }
+    $field_name = $field_definition->getName();
     $display = $this->getSingleFieldDisplay($entity, $field_name, $display_options);

With this bit applied, I am now seeing the translated Paragraph field. I guess it still needs the test to pass and get committed though. Thanks everyone!

Gotta try sometime...I think this re-roll is all that's left of what's needed after the latest 8.7.x-dev core updates

Take 2?

Yay! Just needs a test now. Sorry, this is definitely outside my skillset. But now I can incorporate the patch again. :)

Old Patch no problem, Your patch have problem, i use Layout Builder
EntityRepositoryInterface; << is missing
Call to a member function getTranslationFromContext() on null in Drupal\Core\Entity\EntityViewBuilder->viewField()
After this work like a charm

But, this problem is only use LayoutBuilder module if use.

Update patch for Drupal 8.7

After seeing a comment about this issue in #3051188: Translated content displays its taxonomy terms in the wrong language when layout builder is enabled for that content type

I tested this on Drupal 8.7.x
I reverted Umami's settings to use the same taxonomy terms for all languages #3051465: Revert "Taxonomies are only displayed in English"

Here you can see the bug marked with red arrows (English taxonomy terms displayed inside a Spanish recipe page):

Then I applied patch #39
and here's the page showing Spanish taxonomy terms in the Spanish recipe page:

Patch #39 resolves #3051188: Translated content displays its taxonomy terms in the wrong language when layout builder is enabled for that content type

This cannot be RTBC yet because it needs tests.
Also it should likely be reviewed by an entity maintainer.

Should this instead use the new EntityRepository methods? See https://www.drupal.org/node/3036722.

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -449,6 +449,16 @@ protected function isViewModeCacheable($view_mode) {
+    if (!$field_definition->isTranslatable() &&
+          $entity instanceof TranslatableInterface &&
+          $entity->isTranslatable()) {
+          $entity = $this->entityRepository->getTranslationFromContext($entity);
+    }

Nit: The indentation here is very wonky. Hard to know when the condition has ended and the actual line has begun.

This is the test for the issue. Let see what the testbot says.

The patch works because the tags field provided by the standard profile is set to translatable. Making this field untranslatable replicates the same scenario than described in #4.

This patch includes the fix from #39 with the indentation issues fixed.

#45 Includes tests coverage.
#43 shows exposes the failure (that patch doesn't include the fix).

Regarding question from #41

Should this instead use the new EntityRepository methods? See https://www.drupal.org/node/3036722.

I'm not sure. It seems that code is useful when you want to load an entity giving an ID. But at this level of code, the entity is already available with $items->getEntity();

public function viewField(FieldItemListInterface $items, $display_options = []) {
  $entity = $items->getEntity();

We only need to load an existent translation for an already loaded entity.

1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -449,6 +449,17 @@ protected function isViewModeCacheable($view_mode) {
        $entity = $items->getEntity();
   +    $field_definition = $items->getFieldDefinition();
   +    // Even when fields are set to not translatable, they still need to know
   +    // which is the language of the current entity being rendered.
   +    // This allow to render, for example, referenced entities in the same
   +    // language than the current entity.
   +    if (!$field_definition->isTranslatable() &&
   +        $entity instanceof TranslatableInterface &&
   +        $entity->isTranslatable()
   +    ) {
   +      $entity = $this->entityRepository->getTranslationFromContext($entity);
   +    }
   

   Using the methods mentioned in #41, all these lines can be replaced with
   $entity = $this->entityRepository->getActive($items->getEntity()->getEntityTypeId(), $items->getEntity()->id());

2. The test added in layout builder is great, but should probably get additional test coverage in the entity system, since that is what changed. This can probably be done in EntityViewBuilderTest.

3. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationWithReferencedContentTest.php
   @@ -0,0 +1,193 @@
   +use Drupal\Core\Entity\Entity\EntityViewDisplay;
   

   Not used

4. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationWithReferencedContentTest.php
   @@ -0,0 +1,193 @@
   +use Drupal\node\NodeInterface;
   

   Not used

5. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationWithReferencedContentTest.php
   @@ -0,0 +1,193 @@
   +    // Assert that each content displays the reference in the right language.
   

   Nit: s/content/translation/ s/right/correct

6. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationWithReferencedContentTest.php
   @@ -0,0 +1,193 @@
   +    $assert_session->pageTextContains('Article in english');
   

   Capitalize "English" and all other language names in this test.

7. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationWithReferencedContentTest.php
   @@ -0,0 +1,193 @@
   +    // Now enable Layout Builder for the Article content type. And check the
   +    // same assertions again.
   

   Nit: Remove "now" and make this one sentence.

8. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationWithReferencedContentTest.php
   @@ -0,0 +1,193 @@
   +    // A now check if making the tags field no translatable, still shows the
   

   Nit: Remove "A now" and comma. Change "no translatable" to "non-translatable"

9. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationWithReferencedContentTest.php
   @@ -0,0 +1,193 @@
   +    // translated content.
   

   Nit: Remove "A now" and the comma.

10. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationWithReferencedContentTest.php
    @@ -0,0 +1,193 @@
    +  protected function enableLayoutBuilderForContentType($type) {
    

    For this and the other enable/disable translation methods in this class, the function args aren't necessary since it is always article/tags. The methods themselves could be considered by some to be unnecessary since they are only used once, but they do make the test easier to read so I'm inclined to keep them but it's possible another will feel differently.

11. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationWithReferencedContentTest.php
    @@ -0,0 +1,193 @@
    +   * Makes the entity reference field that links tags, not translatable.
    

    Nit: Remove comma and change "not translatable" to "non-translatable"

12. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationWithReferencedContentTest.php
    @@ -0,0 +1,193 @@
    +    // Create a random taxonomy term with its translation in spanish.
    

    This isn't really random - maybe rephrase as Create a taxonomy term with a Spanish translation.

13. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationWithReferencedContentTest.php
    @@ -0,0 +1,193 @@
    +    // Create a node, with a spanish translation and reference the taxonomy
    

    Nit: Remove comma

14. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationWithReferencedContentTest.php
    @@ -0,0 +1,193 @@
    +    $url = Url::fromRoute("entity.node.content_translation_add", [
    

    Nit: Single quotes

Thanks! @bnjmnm.

Items 1, 3 to 9 and 11 to 14 addressed in this patch.

The test added in layout builder is great, but should probably get additional test coverage in the entity system, since that is what changed. This can probably be done in EntityViewBuilderTest.

I invested around four hours to write a proper patch for this issue. Unfortunately I don't have time to work in a different test right now.

For this and the other enable/disable translation methods in this class, the function args aren't necessary since it is always article/tags. The methods themselves could be considered by some to be unnecessary since they are only used once, but they do make the test easier to read so I'm inclined to keep them but it's possible another will feel differently.

I tried to prioritize readability here.

Thanks for the quick turnaround on that @dagmar!

Here's a patch Addressing two items from my review (needs a subsystem maintainer anyway so I'm not in a position to RTBC)

1. Modified the change I advised in #47.1 as the tests revealed some use cases where the approach as-suggested would not work (the test failures in #48 were due to my bad intel 😏)
2. My explanation for #47.10 was confusing -- I'm fine with the methods staying there for readability, but removed the args since they never act on a different content type/taxonomy, so enableTranslationsForContentType($type) is now enableTranslationsForArticle()

So the additional tests needed (no expectations for @dagmar to do it, btw 🙂) are tests that would have identified this issue without us having to discover it via Layout Builder (it's good the Layout Builder test is there, though)

First I should point out that the functional test added to Layout Builder should be refactored to not use the standard profile - this will need to be changed before this can be RTBC.

Added a kernel test to cover EntityViewBuilder directly.
A few things to mention

1. Had to add an admin permission to EntityTestTranslatableNoUISkip, an entity type I borrowed from the language module. The permission was necessary for the entity to be rendered in an entity reference field. Full disclosure I'm not 100% sure why - it happens in a call to checkAccess() in Drupal\Core\Field\Plugin\Field\FieldFormatterEntityReferenceFormatterBase::getEntitiesToView
2. I'm still a bit curious if there is a solution that can be applied earlier than Drupal\Core\Entity\EntityViewBuilder::viewField - this solution works just fine but it seems odd that the entity returned by the FieldItemList in $items->getEntity() is the default language, even if the field is retrieved from a translated version. I think the solution as-is is just fine, but since a maintainer will be reviewing this anyway, I'd like their thoughts on this.

Changed the deprecated call to getViewBuilder to use the service where the method is not deprecated, and updated LayoutBuilderTranslationWithReferencedContentTest to not use the standard profile.

@bnjmnm thanks for continue working on this. I have a few updated from my side regarding the use of Drupal 8.7 and Layout Builder.

I'm not entirely sure why, but when I try to edit the Layout of a content type using Layout Builder. This code

$entity = !$field_is_translatable && $entity_is_translatable ? $this->entityRepository->getActive($items->getEntity()->getEntityTypeId(), $items->getEntity()->id()) : $items->getEntity();

ends making $entity NULL.

Which later shows a fatal error in the page because.

Error: Call to a member function getEntityTypeId() on null in Drupal\Core\Entity\EntityViewBuilder->getSingleFieldDisplay() (line 525 of web/core/lib/Drupal/Core/Entity/EntityViewBuilder.php)
 #0 web/core/lib/Drupal/Core/Entity/EntityViewBuilder.php(461): Drupal\Core\Entity\EntityViewBuilder->getSingleFieldDisplay(NULL, 'field_featured_...', Array)
 #1 web/core/lib/Drupal/Core/Field/FieldItemList.php(243): Drupal\Core\Entity\EntityViewBuilder->viewField(Object(Drupal\Core\Field\EntityReferenceFieldItemList), Array)
 #2 web/core/modules/layout_builder/src/Plugin/Block/FieldBlock.php(161): Drupal\Core\Field\FieldItemList->view(Array)

Also, the another problem related to the use of entityRepository->getActive trigger this warning:

Warning: array_flip(): Can only flip STRING and INTEGER values! in Drupal\Core\Entity\EntityStorageBase->loadMultiple() (line 265 of web/core/lib/Drupal/Core/Entity/EntityStorageBase.php)
 #0 web/core/includes/bootstrap.inc(587): _drupal_error_handler_real(2, 'array_flip(): C...', '...', 265, Array)
 #1 [internal function]: _drupal_error_handler(2, 'array_flip(): C...', '...', 265, Array)
 #2 web/core/lib/Drupal/Core/Entity/EntityStorageBase.php(265): array_flip(Array)
 #3 web/core/lib/Drupal/Core/Entity/EntityRepository.php(157): Drupal\Core\Entity\EntityStorageBase->loadMultiple(Array)
 #4 web/core/lib/Drupal/Core/Entity/EntityRepository.php(136): Drupal\Core\Entity\EntityRepository->getActiveMultiple('node', Array, Array)
 #5 web/core/lib/Drupal/Core/Entity/EntityViewBuilder.php(458): Drupal\Core\Entity\EntityRepository->getActive('node', NULL)
 #6 web/core/lib/Drupal/Core/Field/FieldItemList.php(243): Drupal\Core\Entity\EntityViewBuilder->viewField(Object(Drupal\Core\Field\EntityReferenceFieldItemList), Array)
 #7 web/core/modules/layout_builder/src/Plugin/Block/FieldBlock.php(161): Drupal\Core\Field\FieldItemList->view(Array)

We upgraded a site from D8.6 to D8.7 and reverting #54 allowed us to keep using the Layout Builder user interface.

I'm almost sure this has to be related with the use of entityRepository->getActive. My patch of #44 doesn't produce the fatal error.

@dagmar if this is not working for you when editing Layout Builder, the test coverage needs to be expanded to cover that use case. Would you be able to review LayoutBuilderTranslationWithReferencedContentTest.php and point out what this test does not cover that would have otherwise surfaced the issue you reported? (field and block types in your layout, etc..)

What's missing is that $items->getEntity() can be null, typically when creating a "faked" field items object without an actual entity, which is I guess what layout builder is doing. The code needs to first check that $items->getEntity()( isn't NULL, in which case we can't detect the correct language and should do some kind of fallback, likely the current language.

#58 Agree with you, have same problem in Commerce Product Variation

The error in #56 should get test coverage, I tried to piece together how to reproduce this based on the other comments but was not able to do so. If someone can provide steps on how to reproduce the error, with just core modules, I can write a test that proves a different approach is necessary. Someone else who has experienced the error is also welcome to write the test 🙂

The patch in #44 that was reported to work also uses $items->getEntity() , which #58 points out can return null, so it seems particularly important to surface these use cases in tests.

@bnjmnm I tried to replicate this as well using a fresh 8.8.x site install. Unfortunately I haven't been able to replicate this either. Maybe is related to something with the upgrade from 8.6.x.

Based on the following I'm setting back to needs review

• I ran into (as far as I can tell) the commerce error reported in #59 without this patch applied. The error I encountered has an existing issue #3029969: "Product variation: Operations links" causes a crash in ProductVariationListBuilder.
• Although it was pointed out in #58 that the use of $items->getEntity() could cause issues, the use of this method to get the entity is not introduced in this patch. In fact, the changes in this patch actually reduces the use of this method. If there are problems stemming from the use of $items->getEntity(), it should get its own issue, including steps to reproduce. I could not reproduce the issue, and expanded the test coverage (as seen in the interdiff) as confirmation of this.
• @dagmar reported back on the error reported in #56 to say that it could no longer be reproduced.

The tests in this patch prove that this patch fixes the reported issue, an I don't see any evidence that it introduces new problems, so I'd like to move this forward so multilingual sites no longer have this bug. If someone is running into problems related to this patch please provide steps to reproduce and/or tests that prove the existence of the problem. While I don't want to introduce new problems with this patch, I also don't want to deprive users of a fix based on something less-than-conclusive.

RTBC.

• I downloaded a fresh new drupal 8.8.x,
• Applied patch #62 as well as the latest patch from #3051465: Revert "Taxonomies are only displayed in English"
• I installed the Umami demo.

I can confirm that in recipe nodes (which use Layout Builder) tags are displaying the correct string in English and in Spanish.

Thanks @bnjmnm

@dagmar reported back on the error reported in #56 to say that it could no longer be reproduced.

Sorry meant to say I cannot reproduce this as part of a fresh install. I faced this issue in production in a really large site, so I can tell this is not ready yet as is. #58 and #59 are mentioning this as well.

@shaal thanks for your feedback. But I'm moving this back to Needs review because we already know that this will break existent installation if this lands as is.

Let's see if @Regnoy or @Berdir can provide some extra indications to provide a full working patch.

@dagmar have error Commerce Product Layout Manager(field Variations) after applay last patch from Support for Layout Builder module
Drupal\Core\Entity\EntityStorageException: No entity bundle was specified in Drupal\Core\Entity\ContentEntityStorageBase->createWithSampleValues()

Here only have error.
Fresh Site Install.
For working site i use patch entity-view-builder-entity-reference-language-2955392-36.patch

I applied @shaal #62 and @bnjmnm #54, but both introduces this error when using default layout builder in a taxonomy with a paragraph.

Call to a member function getEntityTypeId() on null in /var/www/html/ridwan/public_html/core/lib/Drupal/Core/Entity/EntityViewBuilder.php on line 525

Thank you for your hard work on this issue, I can confirm that this is also causing #3072745: Symmetric translation broken? in entity_reference_layout module which is fixed by #62 for me.

We'll test #62 on 8.7.x for the next days and report if it breaks other things. So far it applies cleanly and works without any problems. You are my heros for today after searching for hours... :)

Thank you all, this is truely a major issue and broke translations in entitiy_reference_revisions for us. With the patch in #62 it's fixed for us, we didn't run into any of the problems above yet in the last two weeks.

-.- I should not have written my comment in #67 ... :D Now that I wrote it, we're in trouble. Translation of entity_reference_revision elements is working but therefor layout administration is broken:

The website encountered an unexpected error. Please try again later.
Error: Call to a member function getEntityTypeId() on null in Drupal\Core\Entity\EntityViewBuilder->getSingleFieldDisplay() (line 525 of core/lib/Drupal/Core/Entity/EntityViewBuilder.php).

which doesn't appear if the patch is removed.

On a different node view mode ("teaser") we get:

Auf der Website ist ein unvorhergesehener Fehler aufgetreten. Bitte versuchen Sie es später nochmal.Drupal\Component\Plugin\Exception\MissingValueContextException: Required contexts without a value: entity in Drupal\Core\Plugin\Context\ContextHandler->applyContextMapping() (line 155 of core/lib/Drupal/Core/Plugin/Context/ContextHandler.php).

even after the patch was removed so that might be unrelated.

As I've just seen, our error is the same as #56. We're using 8.7.x, NO commerce and NO fieldblock module.

UPDATE:
I was able to solve the second bug by removing
context_mapping:
entity: layout_builder.entity
view_mode: view_mode

from the core.entity_view_display.node.XX.teaser.yml configuration and re-importing it. The error was gone and I couldn't find any negative impact yet. After re-saving that value was not set again.

The first error definitely only appears with this patch. Perhaps we're missing another patch which has been committed to 8.8.x but not 8.7.x?

This is the third confirmation the patch #62 breaks layout builder (#69, #65, #64).

@Anybody thanks for your feedback, I think could be useful to provide some extra context of your site configuration, like fields your are trying to render in your site, or which modules, are interacting there. In the meantime I guess this still needs work.

I guess this issue #3063839: PHP message: Error: Call to a member function getEntityTypeId() on null in /var/www/html/drupal/web/core/lib/Drupal/Core/Entity/EntityViewBuilder.php on line 525 might be reported by other users of this patch. So we should have a closer look at these lines, which cause the error for me for the default view mode of a translatable node entity type.

   public function viewField(FieldItemListInterface $items, $display_options = []) {
-    $entity = $items->getEntity();
+    $field_is_translatable = $items->getFieldDefinition()->isTranslatable();
+    $entity_is_translatable = $items->getEntity()->isTranslatable();
+
+    // If there is a mismatch between field and parent entity translatability,
+    // use getActive() to ensure entity references are rendered in the parent
+    // entity's language.
+    /* @var \Drupal\Core\Entity\FieldableEntityInterface $entity */
+    $entity = !$field_is_translatable && $entity_is_translatable ? $this->entityRepository->getActive($items->getEntity()->getEntityTypeId(), $items->getEntity()->id()) : $items->getEntity();
+
     $field_name = $items->getFieldDefinition()->getName();
     $display = $this->getSingleFieldDisplay($entity, $field_name, $display_options);

that cause:

Error: Call to a member function getEntityTypeId() on null in Drupal\Core\Entity\EntityViewBuilder->getSingleFieldDisplay() (line 525 of core/lib/Drupal/Core/Entity/EntityViewBuilder.php).

Ok I had a closer look and the problem seems to exist in preview mode in the layout builder, when using this patch because in this line

$entity = !$field_is_translatable && $entity_is_translatable ? $this->entityRepository->getActive($items->getEntity()->getEntityTypeId(), $items->getEntity()->id()) : $items->getEntity();

the

$items->getEntity()->id();

returns NULL instead of the entity ID in some cases in the default view mode (only). The returned example entity simply seems to be "new" and doesn't have an ID. That causes getActive() to fail and also return NULL which means $entity is also passed as NULL.

As a quickfix I added "!$items->getEntity()->id() === NULL" condition to check if the entity is empty, which brings the page back to life:

$entity = !$field_is_translatable && $entity_is_translatable && !$items->getEntity()->id() === NULL ? $this->entityRepository->getActive($items->getEntity()->getEntityTypeId(), $items->getEntity()->id()) : $items->getEntity();

(core/lib/Drupal/Core/Entity/EntityViewBuilder.php, Line 458)

But my experience in this context isn't enough to tell WHY the returned entity is "new" here and what should be the appropriate fix.

the patch seems to be working, thanks guys. hope this gets into core quickly :)

#62 + hack from #72 did the trick for me, thank you!
Could you perhaps check if a new patch with #72 included makes sense to you?

Patch #62 fixed the issue I experienced in Umami.

Comments #64-74 did not provide steps to replicate the issue you are seeing after applying patch #62.
Can you please provide these steps?

Hi @shaal, thank you!

The steps are quite simple:

1. Enable layout builder for a content type
2. Configure the layout / fields
3. Save the layout
4. Click Manage display > Default (other view modes work for us!) > Manage Layout for the content type
5. ERROR

This happens in every combination for every content type we tried.

The content type has translation enabled for most of the fields. Some simply don't make sense. And I guess the error appears because of the preview functionality.

As background information: We're using the following patched (via composer-patches) against core in the environments where we've encountered these problems:

"[8.8 RTBC Views] Cannot use relationship for rendered entity on Views (#2457999)": "https://www.drupal.org/files/issues/2019-05-04/2457999-194.patch",
        "[8.8 RTBC Views + Multilanguage] Views plugin Rendered Entity must add langcode in render function (#2925816)": "https://www.drupal.org/files/issues/2019-03-15/2925816-views_plugin_langcode-9.patch",
        "[8.8 FIXED Views] Exposed filter reset redirects user to 404 page on AJAX view when placed as a block (#2820347)": "https://www.drupal.org/files/issues/2018-07-18/core-views-ajax-path--2820347-96.patch",
        "[8.8 FIXED built-in] UI for publishing/unpublishing taxonomy terms": "https://git.drupalcode.org/project/drupal/commit/8e31070.patch",
        "[8.8 FIXED built-in] Field blocks in the layout builder do not have view mode suggestions (required for fences to work) (#3001313)": "https://www.drupal.org/files/issues/2019-06-17/3001313-41.patch",
        "[8.8 FIXED built-in] Entity Links fields does not have translation support (#2877994)": "https://www.drupal.org/files/issues/2019-05-29/2877994-140.patch",
        "Localization update feature logs 404 errors too agressively":"https://www.drupal.org/files/issues/2018-05-04/locale-add_ignore_404_option-2879998-9.patch",
        "[Layout-Builder] Error: Call to a member function getLabel() after enable layout_builder with media in core (#2985882)": "https://www.drupal.org/files/issues/2018-07-16/2985882-entityfield-2.patch",
        "[Layout-Builder] Callers of LayoutEntityHelperTrait::getEntitySections() do not account for the view mode - InvalidArgumentException: Invalid UUID (#3008924)": "https://www.drupal.org/files/issues/2019-06-20/3008924-5.patch",
        "[ERL / Paragraphs translated paragraph display: #3072745] EntityViewBuilder::viewField() does not respect entity current language when used with an entity reference field (#2955392)": "https://www.drupal.org/files/issues/2019-07-08/2955392-62.patch",
        "[Token] Add a token for the site logo (#2842780)": "https://www.drupal.org/files/issues/2019-07-12/2842780-110.patch"

If you and others are not able to reproduce this problem (we did in several Drupal projects), we should try to find out which configuration leads to these problems.

The specific steps (1-5) listed in #76are already part of of test coverage and those tests pass. We need to know what use cases are present on sites experiencing the problem so it can be added to test coverage. As soon as there are steps that allow us to recreate the problem on a fresh Drupal install (including modules enabled, field type, field config, how it is used in layout, etc), it will be possible to write the tests that will allow this issue to get committed.

@Anybody thank you for providing these clear steps.
@bnjmnm thank you for your continuous support.

I was able to replicate it, on a fresh Umami install:

1. Apply patch #62
2. Apply latest patch from #3051465: Revert "Taxonomies are only displayed in English"
3. Install Drupal Umami drush si demo_umami -y
4. ::Log in as admin
5. Go to /en/admin/structure/types/manage/article/display
6. Check "Use Layout Builder"
7. Save
8. Click "Manage Layout"
9. *WSOD*

#78 is the info we need to get this moving again! (Thanks to everyone pre-#78 for the feedback that helped us get there too!)

If anyone happens to have bandwidth, it would be incredibly helpful to know what specific variables in Umami are causing the error so the bug can be reproduced on a bare Drupal install. Once that info is available I can take care of writing the tests that will be necessary to get this committed.

#79

1. Create a new content type 'Foo' enable it to be translatable
2. Add a entity reference field (i.e. Taxonomy), it should not be translatable.
3. Enable Layout builder
4. Manage layout
5. *WSOD*

#72 was on the right track!

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -456,7 +456,15 @@ protected function isViewModeCacheable($view_mode) {
+    $entity = !$field_is_translatable && $entity_is_translatable ? $this->entityRepository->getActive($items->getEntity()->getEntityTypeId(), $items->getEntity()->id()) : $items->getEntity();

On manage layout the entity would not have a Id, and this line would attempt to get the entity from the repository which fails.

The steps in #80 had the info needed to create a test that reproduces the error reported in #56, #76, etc.

Attached is a test-only patch that should fail without the solution in #80, along with a patch that combines that test with the solution, which should patch. Lets see if the testbot agrees.

+++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTranslationWithReferencedContentTest.php
@@ -0,0 +1,211 @@
+  }

On a local site, I managed to get this test to fail without the patch from #81, this is the test in the TEST-ONLY patch from #82 that is supposed to fail as reproduces the steps described in #80.

I'll try to revisit this some time this week and attempt to make a test that actually fails -- but if anyone else is excited to get this committed you are quite welcome to hop in and get the test from 2955392-82-TEST-ONLY to fail like it should have.

For this to be committed, we need a failing test to prove that the bugs reported in #56, #76etc are in core and not a specific site config (something I have no doubt about, btw, but we need that test to get it through the gate)

Now I finally did run into a case where I could use this as well, so I decided to spend some time with it.

getActive() is IMHO the wrong API here. We might be calling this for an arbitrary specific revision and want to render *that* revision. getActive() loads the revision it thinks is best for this case. So we might end up with $items and $entity from different revisions, which doesn't sound like fun at all :) (That entity might not even have the same translations as the one we currently have).

What we really need is getTranslationFromContext(), which is the same as EntityViewBuilder::view() uses. That removes the need for an extra isNew() check because we don't need the id anymore. I wasn't quite sure what to do with that layout builder test, didn't touch it yet. IMHO it would be fine to remove that now.

Then I also improved the kernel test a bit. Some notes on that:
* The changed logic means the mocking approach that this used didn't really work anyore. What I did instead is change the default language and reset the language manager. That's much easier then.
* That did result in a weird problem where it thought that the site wasn't multilingual anymore. The reason for that is that EN didn't really exist as a configurable language, only the fixed default, and changing the default resulted in having only one language. I've also installed en.
* entity_test_translatable_UI_skip is a really weird entity type that we only use to test some edge case in the content_translation UI. The default multilingual test type is entity_test_mul, so I switched to that. We just need to grant the view test entity translations permission then.

Please test this :)

I tested #85, together with #3051465: Revert "Taxonomies are only displayed in English" and it works as expected, taxonomies are being displayed correctly in Spanish.

Applied patch #85, all our problems were solved. Nice job.

Great, thank you so much @Berdir, I can also confirm #85 works perfectly. Thanks for your time and investigation!

I'll set this RTBC. Is it possible to get this into 8.8 or too late?

For this to be committed, we still need a failing test that reproduces the bug reported in #56, #76 in order to prove that #85 addresses the specific issue reported.

My implementation is very different from the previous patch and does not have the have the issue that previous patches had, so no, I don't think explicit layout builder test coverage is needed. In fact, I think we should remove the existing layout builder test here as well, it wasn't actually failing before and if there is something there that isn't covered by tests yet for the layout builder module then I think that should be done in a separate issue.

That said, we can actually kind of test the scenario that failed, by not saving the relevant entities that we're testing with and it still works fine. A test-only patch of this specific scenario is however not possible because as mentioned, the code that failed on new entities simply doesn't exist anymore.

Thank you @Berdir, #91 fixed #3063839: PHP message: Error: Call to a member function getEntityTypeId() on null in /var/www/html/drupal/web/core/lib/Drupal/Core/Entity/EntityViewBuilder.php on line 525 for us, which appeared with patch #62!

Patch #91 worked for me with Paragraphs.
RTBC

I confirm this patch fixed the problem I was having with translated Paragraph entities on Layout Builder pages. Before the patch, the entities were displayed in the default language only. After the patch, they are displayed in the correct translation depending on the context.

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -456,7 +456,17 @@ protected function isViewModeCacheable($view_mode) {
-    $entity = $items->getEntity();
+    // If the field is not translatable and the entity is, then the field item
+    // list points always to the default translation of the entity. Attempt to
+    // fetch it in the current content language.
+    /** @var \Drupal\Core\Entity\FieldableEntityInterface $entity */
+    if (!$items->getFieldDefinition()->isTranslatable() && $items->getEntity()->isTranslatable()) {
+      $entity = $this->entityRepository->getTranslationFromContext($items->getEntity());
+    }
+    else {
+      $entity = $items->getEntity();
+    }

I think we can improve this by doing

    $entity = $items->getEntity();
    // If the field is not translatable and the entity is, then the field item
    // list points always to the default translation of the entity. Attempt to
    // fetch it in the current content language.
    /** @var \Drupal\Core\Entity\FieldableEntityInterface $entity */
    if (!$items->getFieldDefinition()->isTranslatable() && $entity->isTranslatable()) {
      $entity = $this->entityRepository->getTranslationFromContext($entity);
    }

Less else and less getEntity().

Here's the patch. Leaving at rtbc because this is not really changing the patch at all - just doing less to do the same.

Repeating what I said on slack: The improvement makes sense to me.

Looks good to me, haven't had the time to verify manually that the WSOD bug has been solved though, but by looking at it, it should.

Some minor doc thingies, but leaving at RTBC:

1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -456,7 +456,15 @@ protected function isViewModeCacheable($view_mode) {
   +    // list points always to the default translation of the entity. Attempt to
   

   Should "points always" be flipped to "always points"?

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityViewBuilderTest.php
   @@ -197,6 +202,107 @@ public function testEntityViewBuilderWeight() {
   +   * Test the view_field() method and confirm it is language aware.
   

   Tests the viewField method

@johndevman good points. Fixed.

Just tested #98 manually in some environments again. Confirming RTBC!

Re-testing my scenario with new patch, and it also worked.

Crediting everyone who provided evidence of local testing and/or patch reviews.

@Berdir in #3 you discounted the solution in #2 which feels very similar to the solution in #91 and on. Does the old you still disagree with the new you?

The one important difference compared to #2 is that we now check whether or not the field is translatable, so at least we don't break anything that worked before. The view use case would have always shown the original language for untranslatable fields, now it would always show it in the current language.

I think this is an improvent for the 90% use case, and if someone really has a use case where they want to control the language and show something different, then they can figure out a solution for how to pass that language into the field, I'm not sure what the best approach for that would be, but this doesn't make easier or harder to do so IMHO.

But yes, we took a lot of interesting detours until we got back to a working solution...

Given the number of followers and commenters we're probably above the 90% use-case. Going to commit the fix to 9.0.x and 8.9.x and ask other committers about backport to 8.8.x

Committed and pushed 1e04ea32c6 to 9.0.x and 94493f36dc to 8.9.x. Thanks!

@catch and @webchick +1'd the backport to 8.8.x

hi,

thanks a lot for the effort you guys put in solving this problem.

I use drupal 8.7.10 and without core patches both twig examples below render the default language. With patch #98 from this issue only Example 1 renders fine in all languages. With patch #20 from the duplicate FieldItemList::view() issue both examples render correctly across languages.

Am I using bad practice code? Is there a way to make this work (except for going via rendered entities directly -- with custom display modes if needed -- as in example 1)?

node.field_documentation contains an entity reference to the content type documentation.

Example 1

{% for doc in node.field_documentation %}
{{doc|view}}
{% endfor %}

Example 2

{% for doc in node.field_documentation %}
{{doc.entity.title|view}}
{% endfor %}

@tomsch I'm running 8.9.3 and am able to duplicate your issue with Example 2 not working. I would prefer to directly access the data rather than the rendered entity.

Example 2 isn't meant to work with this, because title then is a translatable field and doesn't trigger the content language fallback. It's not possible to make that work magically without breaking other use cases like actually rendering it in a different language, with is not uncommon with views.

To make that work, you need to add a bit of php code in preprocess, as you can see in the patch, it is basically doing this:

$this->entityRepository->getTranslationFromContext($entity);

So you want to build something like this:

$variables['documentation_nodes'] = [];
foreach ($node->get('field_documentation') as $item) {
  if ($item->entity) {
  $variables['documentation_nodes'][] = \Drupal::service('entity.repository')->getTranslationFromContext($item->entity);
  }
}

That gives you a list of nodes with the best-available translation for the current content language, and you can loop over those in twig.

That said, I would recommend to use view modes. One thing that you're still missing with this is necessary cache tags for example, so if you change those documentation nodes, it won't invalidate that. you can support that too with another php line, but it just works with view modes. Alternatively, try https://www.drupal.org/project/field_formatter to render just a single field from a referenced entity.