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:
- Get a vanilla fresh D8.1 release
- Enable all the 4 core translation modules
- Add a Spanish language to the website
- Enable content translation (particularly, we want to enable the translation of Article node type).
- Within Article node type only enable title and body translation (the important part is to keep "image" untranslatable)
- Go ahead and create an article node (let's make it original in English and then add translation of it in Spanish)
- Change the "default" display settings of article node: make sure "image" formatter is set up to link to its entity
- 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)
- 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.
Comments
Comment #2
aronne CreditAttribution: aronne at bmeme commentedHi @bucefal91,
this patch should fix the problem, so let me know.
Comment #3
tstoeckler@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'sgetLanguage()
method to get the respective language object.Comment #4
aronne CreditAttribution: aronne at bmeme commented@tstoeckler: ok i've updated the patch by replacing the
getCurrentLanguage()
method with the one you've suggestedgetLanguage($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()
.Comment #5
tstoecklerNo, the patch looks perfect. Thanks! I'm afraid we need some tests, though, before this can get in.
Comment #6
tstoecklerAlso, 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?
Comment #7
aronne CreditAttribution: aronne at bmeme commented@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.
Comment #8
tstoecklerAhh, 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.
Comment #9
aronne CreditAttribution: aronne at bmeme commentedOk here it is the new patch for 8.2.x version.
Let's see what tests say :)
Comment #10
tstoecklerAwesome, so we know it doesn't break anything. Now we just need some additional tests for the bug. Marking needs work for that.
Comment #11
bucefal91 CreditAttribution: bucefal91 commentedHello!
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:
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:
Comment #12
bucefal91 CreditAttribution: bucefal91 commentedHehe, 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:
Drupal\Tests\image\Kernel\ImageFormatterTest
test dependencies (language and content_translation), so I can test this bug as it requires translating an entity.EntityTestMul
entity instead ofEntityTest
because the former is translatable and the latter is not.$this->display
initialization since it was using a deprecated approach.I've tested it locally and without the arrone's fix it fails, whereas with the fix it passes.
Comment #13
aronne CreditAttribution: aronne at bmeme commented@bucefal91 awesome work :)
Comment #14
tstoeckler@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.
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
.Yay!!
I think that sentence is a little bit unclear. Maybe "Tests the entity link of the image formatter."
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.Comment #15
bucefal91 CreditAttribution: bucefal91 commentedHello,
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) :)
Comment #16
bucefal91 CreditAttribution: bucefal91 commentedAny feedback, folks? :)
Comment #17
tstoecklerRe-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.
Comment #18
bucefal91 CreditAttribution: bucefal91 commentedHello!
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.
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.Comment #20
Peacog CreditAttribution: Peacog as a volunteer commentedPatch needed a re-roll. I also created a version of this patch for Responsive image here: #2889892: Responsive image field formatter links to the wrong entity translation
Comment #22
Peacog CreditAttribution: Peacog as a volunteer commentedSorry, ignore that last patch. Here's a working one.
Comment #24
mpp CreditAttribution: mpp as a volunteer and at AmeXio commentedThanks 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?
Comment #25
svdhout CreditAttribution: svdhout at Calibrate commentedThe 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.
Comment #27
rwam CreditAttribution: rwam commentedI 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.
Comment #28
rwam CreditAttribution: rwam commentedI adapt this solution to the MediaThumbnailFormatter too. May there is a more generic approach to handle all kind of similar issues.
Comment #29
tstoecklerI'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?
Comment #31
Peacog CreditAttribution: Peacog as a volunteer commentedRe-rolled for 8.6
Comment #33
ckaotikI've noticed this for multiple formatters, including
ImageFormatter
,ResponsiveImageFormatter
andStringFormatter
. 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)Comment #34
nuezComment #35
nuezComment #36
fmfpereira CreditAttribution: fmfpereira commented#33 breaks search_api on the following condition:
$items->getParent()->setValue($translation);
will force all translations for a given entity to be indexed on the same language.Comment #42
dqdRegarding #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.Comment #43
Anchal_gupta CreditAttribution: Anchal_gupta at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch against 9.4x. please review it
Comment #44
heni_deepak CreditAttribution: heni_deepak commentedComment #45
dqdThanks 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.