Hello,

I am not sure whether this is a true bug as I am nowhere close to be a D8 savvy, but here's what I've got: when you're rendering an entity with image field attached to it and when the image formatter is configured to link to the entity, the resulting link does not account in which language it is being rendered and always links to the entity's original language. Alright, now translating this to English, here are some basic steps to reproduce:

  1. Get a vanilla fresh D8.1 release
  2. Enable all the 4 core translation modules
  3. Add a Spanish language to the website
  4. Enable content translation (particularly, we want to enable the translation of Article node type).
  5. Within Article node type only enable title and body translation (the important part is to keep "image" untranslatable)
  6. Go ahead and create an article node (let's make it original in English and then add translation of it in Spanish)
  7. Change the "default" display settings of article node: make sure "image" formatter is set up to link to its entity
  8. Navigate to a full page view of your article node in English. Observe the image to link to the English version of the node (expected result)
  9. Now navigate to a full page view of your article node in Spanish. Observe here the image to link to English version of the node (while the expected result is to have that link to the Spanish version).

(this bug disappears if we enable translation of the image field)

To me it seems more logical to link image to the translation language in which its entity is currently being rendered even if the image field is not translatable.

For some reason (I just can't debug whole drupal core to figure it out) in Drupal\image\Plugin\Field\FieldFormatter\ImageFormatter::viewElements() the parent entity of the provided items list is set up to be in LANGCODE_DEFAULT instead of having explicitly set spanish language (since precisely in this language the entity is being rendered). What I currently do in image formatter method to overcome this issue is to force the parent entity to look up its translation in the language in which rendering is happening.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bucefal91 created an issue. See original summary.

aronne’s picture

Status: Active » Needs review
FileSize
3.02 KB

Hi @bucefal91,
this patch should fix the problem, so let me know.

tstoeckler’s picture

@bucefal91: Judging from the issue summary, you are pretty D8-savvy, I'd say! This is absolutely a legitimate bug.

I'm pretty sure there's either already an issue about this somewhere or at least a very similar one, but I can't find it right now.

@aronne: I think that's the right spot to put the language. However, we shouldn't use the interface language there. Instead the passed-in $langcode variable should be used. You can use the language manager's getLanguage() method to get the respective language object.

aronne’s picture

@tstoeckler: ok i've updated the patch by replacing the getCurrentLanguage() method with the one you've suggested getLanguage($langcode).

Let me know if that's what you want or if you would replace, for some reason, even the $this->languageManager call with a \Drupal::languageManager().

tstoeckler’s picture

Version: 8.1.x-dev » 8.2.x-dev
Issue tags: +Needs tests

No, the patch looks perfect. Thanks! I'm afraid we need some tests, though, before this can get in.

tstoeckler’s picture

Also, not sure why the testbot isn't picking up the existing patches. @aronne can you try uploading the file in #4 with a different filename, maybe without the dots (".") in the filename?

aronne’s picture

@tstoeckler tests are not running because the patch i've made is for 8.1 version, but during file upload the starting testable version was 8.2 so i didn't enable tests for these patches.

tstoeckler’s picture

Ahh, I see. Unfortunately this won't go into 8.1.x anymore, as that is basically frozen (barring security issues). So we'll have to go for 8.2.x for this. I presume there's not that much of a difference in the code, though, so maybe you can just re-upload the patch and enable 8.2.x testing, to see.

aronne’s picture

Ok here it is the new patch for 8.2.x version.
Let's see what tests say :)

tstoeckler’s picture

Status: Needs review » Needs work

Awesome, so we know it doesn't break anything. Now we just need some additional tests for the bug. Marking needs work for that.

bucefal91’s picture

Hello!

I would like to write the test to cover this bug, but I've been struggling for a few hours now with those handful lines of code without any positive outcome whatsoever.

I want to take the following steps:

  1. Add 1 more test method into Drupal\Tests\image\Kernel\ImageFormatterTest Kernel test. This new method will test this bug.
  2. Set up entity display for the image field to output link to its entity.
  3. Within that method I want to create a new entity with a dummy image, then create a translation of that entity.
  4. For both original and translation I want to make sure the image now links to the entity in the appropriate language.

Seems quite easy. But I was able to go only up to the 3rd item: I add a translation, but then it does not contain any data for the image field.

Also, could somebody point me to worthwhile debugging tool during Unit test development?

What I am doing in PHP code is the following:

  protected function setUp() {
    // ....
    // I've added 'language' and 'content_translation' modules. Now I create a 2nd language in the system.
    $this->configurableLanguage = ConfigurableLanguage::createFromLangcode('es');
    $this->configurableLanguage->save();
    // ....
  }

  // ....

  // And this is the new test method that I am trying to add.
  public function testImageFormatterWithLinkToEntity() {
    // First, make sure the formatter is set up to link to the entity.
    $component = ['settings' => ['image_link' => 'content']] + $this->display->getComponent($this->fieldName);
    $this->display->setComponent($this->fieldName, $component)
      ->save();

    // Create an entity.
    $entity = EntityTest::create([
      'name' => $this->randomMachineName(),
    ]);
    $entity->{$this->fieldName}->generateSampleItems(1);
    $entity->save();

    // Create its Spanish translation.
    $translation = $entity->addTranslation($this->configurableLanguage->id());
    $translation->save();

    // At this point $translation->{$this->fieldName} is empty, while I expect it to have the sample data generated a few lines above.

    $build = $this->display->build($entity);
    $this->assertEquals($entity->language()->getId(), $build[$this->fieldName][0]['#url']->getOption('language')->getId(), 'Image is linked to the entity in the appropriate language.');

    // I've tried to "look up" translations in both of the 2 ways here. Through "getTranslation()" and through "getTranslationFromContext()".
    $translation = $this->container->get('entity.repository')->getTranslationFromContext($entity, $this->configurableLanguage->getId());
    //$translation = $entity->getTranslation($this->configurableLanguage->getId());

    // At this point, since $translation->{$this->fieldName} is empty, the $build is also empty. So I can't assert anything.
    $build = $this->display->build($translation);
    $this->assertEquals($translation->language()->getId(), $build[$this->fieldName][0]['#url']->getOption('language')->getId(), 'Image is linked to the entity in the appropriate language.');
  }
bucefal91’s picture

Hehe, with extra 6 hours on the task and studying D8 core, I believe I have the test.

Just disregard the junk I've written in the previous comment.

So, what I've done:

  • Added 2 extra modules into the Drupal\Tests\image\Kernel\ImageFormatterTest test dependencies (language and content_translation), so I can test this bug as it requires translating an entity.
  • Now the test uses EntityTestMul entity instead of EntityTest because the former is translatable and the latter is not.
  • In the set up method I also added the Spanish language so we have 2 languages in the system in order to add a translation later on.
  • I've rewritten $this->display initialization since it was using a deprecated approach.
  • Added extra test method into the class that tests image link. It does the following: create an entity, add translation to it. Then test both original and translation and make sure the image link has appropriate language in each of the 2 cases.

I've tested it locally and without the arrone's fix it fails, whereas with the fix it passes.

aronne’s picture

@bucefal91 awesome work :)

tstoeckler’s picture

@bucefal91: Wow, very impressive work. Also thank you a lot for documenting the steps that you took and the things you struggled with. Not only can it help to inform future DX decisions, but it also encourages other contributors who might be struggling with the same issues. Not many people do this, I really appreciate it!

Some notes on the patch. Only the last part actually warrants a "needs work" from my point of view, but pointing out a couple of other minor things anyway.

   /**
+   * @var \Drupal\language\Entity\ConfigurableLanguage
+   */
+  protected $configurableLanguage;

1. It would be nice to add a little comment like "The language used in the test." above the @var declaration.
2. Minor, but I think it's fine for the variable to be named $language.

-    $this->display = entity_get_display($this->entityType, $this->bundle, 'default')
+    $this->display = $this->container->get('entity_type.manager')

Yay!!

+  /**
+   * Tests the link to entity from image formatters.
+   */

I think that sentence is a little bit unclear. Maybe "Tests the entity link of the image formatter."

+    $build = $this->display->build($entity);
+    $this->assertEquals($entity->language()->getId(), $build[$this->fieldName][0]['#url']->getOption('language')->getId(), 'Image is linked to the entity in the appropriate language.');

I think this test is a little bit too close to the actual code that it tests. I think it would be nicer to render the formatter, for example using $output = $this->container->get('renderer)->renderRoot($build[$this->fieldName][0]); and then assert that the correct URL is contained in the output.

bucefal91’s picture

Hello,

Thanks for the warm feedback :) I maintain a few D7 contrib modules here, so I know that sometimes it is difficult to review someone's patch without verbal explanations from the author.

I've incorporated 1) and 2) from tstoeckler's comment. I'd like to counterargument about rendering the $build array. Since we claim to test image formatter (and not the rendering mechanism) then I think we should only assert correctness of the build array (and making sure the array is rendered into correct HTML is a task for another test, the one that tests rendering). But that's just my vision, if you think it's more appropriate to assert it on the renderer output, then I'll get it done.

Also, since this new test method declares to test the link to entity, then I think it's logical to assert not only language but also the correct route in the Url object, which I have included in this new patch. Otherwise it just appears odd: "we test image link to entity" but in fact all we test is the language in that link (no matter where the link may point) :)

bucefal91’s picture

Any feedback, folks? :)

tstoeckler’s picture

Title: Poor language handling in formatter (when linking to entity) » Wrong language handling in ImageFormatter when linking to entity
Related issues: +#2648288: StringFormatter generates links in wrong language when linking to entity

Re-titling and adding related issue.

That issue has a patch which solves the same issue (for a different formatter) but in a slightly different way. I think we should make that consistent, although I'm not sure which approach is better. I'm leaning slightly to the approach there, but not sure.

bucefal91’s picture

Hello!

Then also this ticket and the #2645922: Image field generates only default language URL when linking to corresponding entity. are duplicates. Are they not?

As far as the 2 alternative solutions, I am geared towards aronne's solution, i.e.

$url = $entity->toUrl('canonical', [
  'language' => $this->languageManager->getLanguage($langcode),
]);

Because semantically it sounds to me as "get me a URL to this entity in this language". Whereas I interpret the $entity->hasTranslation($langcode) solution as "get me the translation. Then get me URL to that entity". We do not need to look up whole translation Entity for we are not going to use any of it, all we need is to get a URL pointing to that translation.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Peacog’s picture

Status: Needs review » Needs work

The last submitted patch, 20: image-poor_language_handling_formatter-with-tests-2798977-20.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Peacog’s picture

Sorry, ignore that last patch. Here's a working one.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mpp’s picture

Thanks for the patches, I had also noticed this issue in ResponsiveImageFormatter.

Since ResponsiveImageFormatter is using the exact same code, shouldn't we be using a private helper to get the translation to avoid code duplication?

svdhout’s picture

The patch at https://www.drupal.org/project/drupal/issues/2883450#comment-12116998 fixes the problem for me, but takes a better approach in my opinion.

The processOutbound method in the LanguageNegotiationUrl plugin can add the current language when the content has language not specified or not applicable.
That means the rewriting is correct, and not every formatter needs to be rewritten.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

rwam’s picture

Status: Needs review » Reviewed & tested by the community

I like the solution provided in #22 and it works well with Drupal 8.5.1. And I would like to see this fix or an other solution in one of the next releases too.

rwam’s picture

I adapt this solution to the MediaThumbnailFormatter too. May there is a more generic approach to handle all kind of similar issues.

tstoeckler’s picture

Component: image.module » entity system
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs subsystem maintainer review

I'm still not sure whether this patch is the right approach or whether the solution from #2648288: StringFormatter generates links in wrong language when linking to entity is better.

Maybe we can get the opinion of another entity API maintainer.

To recap, the question is:

If a langcode is passed explicitly into FieldWidgetInterface::view() what should be the behavior when generating links to entities?
A) Just pass the language as a URL option. This has the downside that even if there is a translation in that language the untranslated label will be used, not the translated one.

B) If there is a translation in that language, just fetch that and use it to build the URL. This has the upside of using the translated language and not having to muck with the language manager manually, but the downside is that if there isn't a translation in that language but people might still want to have the language in the URL object.

Having written this, maybe the correct solution is to do to both? I.e. fetch a translation if it exists, but unconditionally add the language to the URL?

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Peacog’s picture

Status: Needs review » Needs work

The last submitted patch, 31: image-poor_language_handling_formatter-with-tests-2798977-31.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

ckaotik’s picture

I've noticed this for multiple formatters, including ImageFormatter, ResponsiveImageFormatter and StringFormatter. Its quite tedious to update all formatters and the root of the issue seems to come from FieldItemList "forgetting" that its parent entity is a translation for untranslatable fields.

As a completely different approach, how about we ensure that the FielditemList passed to formatters always retains its translation? That way we won't have to update various (core/contrib/custom) formatters, and the existing behavior for FieldItemList and ::toUrl etc. can remain unchanged. I've attached a tiny patch that does this, looking forwards to your feedback. (Still needs tests, though, if the apporach is okay'ed)

nuez’s picture

fmfpereira’s picture

#33 breaks search_api on the following condition:

  • A Rendered HTML output is added to the index.
  • The target view mode has at least one entity reference field formatter.

$items->getParent()->setValue($translation); will force all translations for a given entity to be indexed on the same language.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dqd’s picture

Status: Needs review » Needs work

Regarding #36 I'll set it to "Needs work". Can still reproduce that image links to content do not respect language in latest D9 stable test enviroment for content rendered via manage display view modes, while linked titles do. But I am not sure if we need more reproducing tests in the new Drupal 9 eco system regarding HOW the image has been implemented. Is is a media reference image?(EDIT: cannot link to content) Or an image field. Are they rendered via Layout Builder or directly or via Display Suite? I actually would consider this even as a "Major" prio but leave it as is for now.

Anchal_gupta’s picture

Rerolled patch against 9.4x. please review it

heni_deepak’s picture

Version: 9.4.x-dev » 9.5.x-dev
dqd’s picture

Thanks for working on this. But again, I recommend strongly to ask for more user tests to reproduce this issue. Because it is not clear enough now in D9 cycle IF it still exist, and IF from where it is caused. We need more reproducing reports on this here, imho. Would love to read from more users how they run into this issue in details.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.