Problem/Motivation
The Entity link field uses the wrong translation of the entity. It always uses the original language which is not always desired.
This applies to both deletion and editing.
You can use the Entity operations field instead but that is not always desired to do so.
To reproduce:
- I have downloaded latest 8.4.x-dev release
- I have installed Drupal 8.4 with the Standard profile
- I have added a new language "Swedish" and then enabled that language for the "Article" content type
- I have created a new node Article example and then translating it to Swedish Artikel exempel.
- I have created a new View "Articles" that lists content type articles for both languages
- I have configured the view to use table format. I add "Link to edit Content" and "Link to delete Content" fields
The exported view:
uuid: b6c259e3-3fbc-46b9-9979-8df90333fff2
langcode: en
status: true
dependencies:
config:
- node.type.article
module:
- node
- user
id: articles
label: Articles
module: views
description: ''
tag: ''
base_table: node_field_data
base_field: nid
core: 8.x
display:
default:
display_plugin: default
id: default
display_title: Master
position: 0
display_options:
access:
type: perm
options:
perm: 'access content'
cache:
type: tag
options: { }
query:
type: views_query
options:
disable_sql_rewrite: false
distinct: false
replica: false
query_comment: ''
query_tags: { }
exposed_form:
type: basic
options:
submit_button: Apply
reset_button: false
reset_button_label: Reset
exposed_sorts_label: 'Sort by'
expose_sort_order: true
sort_asc_label: Asc
sort_desc_label: Desc
pager:
type: mini
options:
items_per_page: 10
offset: 0
id: 0
total_pages: null
expose:
items_per_page: false
items_per_page_label: 'Items per page'
items_per_page_options: '5, 10, 25, 50'
items_per_page_options_all: false
items_per_page_options_all_label: '- All -'
offset: false
offset_label: Offset
tags:
previous: ‹‹
next: ››
style:
type: table
row:
type: fields
fields:
title:
id: title
table: node_field_data
field: title
entity_type: node
entity_field: title
alter:
alter_text: false
make_link: false
absolute: false
trim: false
word_boundary: false
ellipsis: false
strip_tags: false
html: false
hide_empty: false
empty_zero: false
settings:
link_to_entity: true
plugin_id: field
relationship: none
group_type: group
admin_label: ''
label: Title
exclude: false
element_type: ''
element_class: ''
element_label_type: ''
element_label_class: ''
element_label_colon: true
element_wrapper_type: ''
element_wrapper_class: ''
element_default_classes: true
empty: ''
hide_alter_empty: true
click_sort_column: value
type: string
group_column: value
group_columns: { }
group_rows: true
delta_limit: 0
delta_offset: 0
delta_reversed: false
delta_first_last: false
multi_type: separator
separator: ', '
field_api_classes: false
edit_node:
id: edit_node
table: node
field: edit_node
relationship: none
group_type: group
admin_label: ''
label: 'Link to edit Content'
exclude: false
alter:
alter_text: false
text: ''
make_link: false
path: ''
absolute: false
external: false
replace_spaces: false
path_case: none
trim_whitespace: false
alt: ''
rel: ''
link_class: ''
prefix: ''
suffix: ''
target: ''
nl2br: false
max_length: 0
word_boundary: true
ellipsis: true
more_link: false
more_link_text: ''
more_link_path: ''
strip_tags: false
trim: false
preserve_tags: ''
html: false
element_type: ''
element_class: ''
element_label_type: ''
element_label_class: ''
element_label_colon: true
element_wrapper_type: ''
element_wrapper_class: ''
element_default_classes: true
empty: ''
hide_empty: false
empty_zero: false
hide_alter_empty: true
text: edit
entity_type: node
plugin_id: entity_link_edit
delete_node:
id: delete_node
table: node
field: delete_node
relationship: none
group_type: group
admin_label: ''
label: 'Link to delete Content'
exclude: false
alter:
alter_text: false
text: ''
make_link: false
path: ''
absolute: false
external: false
replace_spaces: false
path_case: none
trim_whitespace: false
alt: ''
rel: ''
link_class: ''
prefix: ''
suffix: ''
target: ''
nl2br: false
max_length: 0
word_boundary: true
ellipsis: true
more_link: false
more_link_text: ''
more_link_path: ''
strip_tags: false
trim: false
preserve_tags: ''
html: false
element_type: ''
element_class: ''
element_label_type: ''
element_label_class: ''
element_label_colon: true
element_wrapper_type: ''
element_wrapper_class: ''
element_default_classes: true
empty: ''
hide_empty: false
empty_zero: false
hide_alter_empty: true
text: delete
entity_type: node
plugin_id: entity_link_delete
filters:
status:
value: '1'
table: node_field_data
field: status
plugin_id: boolean
entity_type: node
entity_field: status
id: status
expose:
operator: ''
group: 1
type:
id: type
table: node_field_data
field: type
value:
article: article
entity_type: node
entity_field: type
plugin_id: bundle
sorts:
created:
id: created
table: node_field_data
field: created
order: DESC
entity_type: node
entity_field: created
plugin_id: date
relationship: none
group_type: group
admin_label: ''
exposed: false
expose:
label: ''
granularity: second
title: Articles
header: { }
footer: { }
empty: { }
relationships: { }
arguments: { }
display_extenders: { }
cache_metadata:
max-age: 0
contexts:
- 'languages:language_content'
- 'languages:language_interface'
- url.query_args
- 'user.node_grants:view'
- user.permissions
tags: { }
page_1:
display_plugin: page
id: page_1
display_title: Page
position: 1
display_options:
display_extenders: { }
path: articles
cache_metadata:
max-age: 0
contexts:
- 'languages:language_content'
- 'languages:language_interface'
- url.query_args
- 'user.node_grants:view'
- user.permissions
tags: { }
Expected behaviour
When clicking on Edit on the Swedish Article exempel node I should be taken to the Edit form for that translation (Swedish)
When clicking on Delete on the Swedish Article exempel node I should be taken to the Delete form for that translation (Swedish)
Current behaviour
I am taken to the edit form for the english (original translation).
I am taken to delete form to delete both translations.
Proposed solution/idea
Use the EntityTranslationRenderTrait trait to properly render entity row links in the right language.
Add Kernel test for the fields to ensure they work as expected.
Proposed follow ups
Use Rendering language in the view configuration for untranslated entities
Fix translations for \Drupal\comment\Plugin\views\field\EntityLink
if needed
Comment | File | Size | Author |
---|---|---|---|
#140 | interdiff_108_140.txt | 500 bytes | joseph.olstad |
#140 | 2877994-140.patch | 18.62 KB | joseph.olstad |
#118 | 2877994-118.patch | 18.62 KB | Krzysztof Domański |
#118 | views-entity-link-fields.JPG | 13.11 KB | Krzysztof Domański |
#118 | interdiff-113-118.txt | 4.03 KB | Krzysztof Domański |
Comments
Comment #4
rimibhagat CreditAttribution: rimibhagat commentedAny solution for this Issue in 8.4.2 drupal version:
Comment #5
LendudeFix and a test.
Comment #6
LendudeAdding related/duplicate issues. Technically #2447821: Translated nodes in content views are all linking to the current language is the oldest so that should be the one to work on, but the issue summary here is more complete, so started work here.
Comment #9
LendudeChanged so 'hreflang' is only added in multilingual sites which matches the current situation in HEAD.
And the new tests didn't handle being in a subdir well, so lets see if this makes that better.
Comment #10
caspervoogt CreditAttribution: caspervoogt at Plethora commentedI just tested #9 and with that patch the "Link to Content" field now returns the correct URL for my translated nodes as well as my source nodes. I checked hreflangs too, but those are unchanged and still working fine (i.e. 'only added in multilingual sites which matches the current situation in HEAD.'). The patch actually doesn't seem to involve hreflang at all (?).
Comment #11
Lendude@caspervoogt thanks for the manual testing!
The hreflang gets set when you pass a language to the url build. Currently, this doesn't happen. With the patch in #5, this ALWAYS happend, even with non-multilingual sites it would set hreflang="en", see the test fail in the comment test in #5.
So to make this change less disruptive for existing sites, I decided to take that out and only set this when it is relevant, so only when we are dealing with a multilingual site. If we feel this should always be set (is it relevant for usability or SEO?), then we should update the comment test and go for the change in #5.
Comment #12
LendudeBumping this to major since this broke certain configurations in 8.5.0 because of the lack of a workaround when 'Content: path' field got deprecated.
Comment #13
LendudeBit of a clean up. Dedicated test view now and testing for the edit and delete links too.
Comment #14
arakwar CreditAttribution: arakwar commented@Lendude
I applied the patch to my installation and Drupal, and I figured out that: when the "Output the URL as text" is activated, we get the non-translated link, but if we are not activating it it's fine.
We were using this option with the "path" field. Any chance it could work with the "view_node" ?
Comment #15
Lendude@arakwar thanks for the manual tests! Silly me, thinking that would just work.
This fixes the output for 'output url as text', plus a test for it.
Comment #16
caspervoogt CreditAttribution: caspervoogt at Plethora commentedThanks for catching that, arawak. I was about to mention that issue myself (just ran into it), and patch 15 solves that for me (thanks Lendude!).
Comment #17
Lendude@caspervoogt do you feel up to doing a full review so maybe we can move this to RTBC?
Comment #18
johnwebdev CreditAttribution: johnwebdev commentedLooks pretty good to me! Only thing I noticed is:
EntityManagerInterface is depreciated.
Comment #19
caspervoogt CreditAttribution: caspervoogt at Plethora commentedI just reviewed on my end. I had not previously tested the Edit Content link; those work for me. As a test I had a look at my /admin/content view, which does not filter on language and shows nodes of both French and English. My French nodes get French edit links in the /admin/content view I'm using, whether I am viewing the view in English or French (e.g. /fr/admin/content vs /admin/content), so that's good. I then tested on a custom view, which does filter on language, and the Edit Content links are fine there also.
I also checked the Operations field and there the edit link is also correctly translated to the French alias.
Next I checked the View Content link, which showed the correct translated alias whether for French or English.
Lastly I tested using rewrites in a custom text field, placing {{ view_node }} and {{ edit_node }} in there, and those presented no issues. I also tested adding such rewrites on the Link to Content and Edit Content fields themselves, and also found no issues.
So from a functionality perspective I think it's RTBC for sure.
Regarding johndevman's note about EntityManagerInterface being deprecated, that is going to be removed before D9 so it would be good to replace with something else. Out of the files affected by this patch, only core/modules/views/src/Plugin/views/field/LinkBase.php references this. I have attached a patch that takes care of that. Tested and working for me on 8.5.x-dev but should be ok on 8.6.x-dev too.
Comment #21
LendudeThanks for the reviews! @caspervoogt that looks like the interdiff in #19
Took the patch from #15 and applied the changes in #19 and did some more doc block updates.
The reason I took the deprecated EntityManager is because this is type hinted in the EntityTranslationRenderTrait. But if it works with using the EntityTypeManager that's much better.
Comment #22
LendudeStray tab snuck in.
Comment #23
gngn CreditAttribution: gngn commented,
With #22 it goes to something like '/node/123', not to '/de/node/123' (or the url alias without language prefix).
So instead of the translated entity's language
I propose somehting like
I wasn't able to create a interdiff (interdiff got "The next patch would create the file /tmp/interdiff-1.tpymsL"), so I just attached the full patch.
And I don't know how to add a test for "Output the URL as text" ...
Btw: I found an old, unresolved issue 2667086 with a similiar patch in comment #17.
Comment #24
Lendude@gngn thanks for looking at this and finding the other issue.
I'm going to roll this back to the version in #22. The use case in #23 doesn't feel like a bug, but just as a certain way some sites might want to handle this, in most cases I think the link to '/node/123' would be fine.
I don't want this to side track the effort of first getting this bug fixed, after that we can look at the use case in #23 in a follow up.
Comment #25
caspervoogt CreditAttribution: caspervoogt at Plethora commentedSounds like a plan Lendude.
Comment #26
dawehnerDo we need to adapt
\Drupal\comment\Plugin\views\field\EntityLink
too?Comment #27
gngn CreditAttribution: gngn commented@Lendude: Thank you for the feeback - but I do not agree - why is '/node/123' better than '/node/123'?
I thought the issue is about "language correct" links?
Ignoring the Rendering language from the view - that might be another issue.
But I think if a user selects something as Rendering language in the view configuration that we should use this setting.
Or am I getting something wrong?
Comment #28
caspervoogt CreditAttribution: caspervoogt at Plethora commentedIn #23's case, I can see the logic in not including that in this RTBC at least;
The node has no language set yet the view is configured to display the rendering language of the view. But the node has no language... therefore we don't use a language alias in its path.
Speaking just for myself here: on most of the multilingual sites I deal with, there are very few (usually no) nodes with language undefined and whenever I do have that, I never want such nodes to include a language prefix. Normally I don't use a language prefix for the primary language on such a site, so that if a view were to return a link to a language-undefined node using the language of the views row, it would return a incorrect path, e.g. /en/node/123 even though my English nodes are not supposed to have language prefixes and that node has no language defined to begin with.
Comment #29
LendudeProbably, but since that doesn't extend LinkBase lets not do that here, but do so in a follow up.
Added the proposed follow ups to the IS.
Comment #30
LendudeComment #31
gngn CreditAttribution: gngn at Computer Manufaktur GmbH commented@caspervoogt (#28) My experiences with undefined language nodes are quite different - also speaking just for myself here:
That is the way my customers want it...
Comment #32
patrickroma CreditAttribution: patrickroma commentedThis is quite critical for all multilingual sites being highly based on views.(read more Button)
Is there a quickfix for the link to content field available that also works in rewritten fields via placeholder?
Comment #33
Lendude@patrickroma did you try the patch in #24? The best way to get this into core is if people review the patch and provide feedback, that way we won't need a quickfix....
Comment #34
Bessonweb CreditAttribution: Bessonweb commentedHi all,
#24 work for me !
Thanks :-)
Comment #35
caspervoogt CreditAttribution: caspervoogt at Plethora commented#24 is working well for me.
Comment #36
xurubo93 CreditAttribution: xurubo93 as a volunteer commented#24 is working also for me.
Comment #37
jennypanighetti CreditAttribution: jennypanighetti commented#24 works for me as well, thank you!
Comment #38
cbanman CreditAttribution: cbanman at Acro Commerce commented#24 tested and works for me.
Comment #39
gwena@ CreditAttribution: gwena@ commented#24 tested and works for me too.
Comment #40
Yazzbe CreditAttribution: Yazzbe as a volunteer commented#24 fixed the problem for us
Comment #41
johnwebdev CreditAttribution: johnwebdev commentedRename to $entityTypeManager
$entity_manager => $entity_type_manager
Just some small nits. Nice that the patch has been confirmed to work well!
Comment #42
Lendude@johndevman thanks for taking a look at this, and thanks everybody for reporting back that it's working in the field.
Fixed nits. Moving to 8.5.x since this is a bug fix.
Comment #43
Ludo.RI confirm that patch #42 works in 8.5.3.
Tested with "Link to content" in Views.
Very nice! :)
Comment #44
tibbsa CreditAttribution: tibbsa as a volunteer commentedConfirming that patch #42 resolved my use case problem (on 8.5.4), wherein "link to content" links within a view were disregarding the language of the content in question (even though the teaser was otherwise displayed in the correct language).
Comment #45
tibbsa CreditAttribution: tibbsa as a volunteer commentedChanging status to RTBC given that a number of people have reported this as working over the past few weeks, and the latest patch revision made only minor naming changes.
Comment #46
patrickroma CreditAttribution: patrickroma commentedLink to content works. But if you rewrite the output and use the field value as placeholder token {{ path }} translation seems to be ignored. Anyone else having this issue?
Comment #47
patrickroma CreditAttribution: patrickroma commentedComment #48
Lendude@patrickroma when I use the 'Link to content' field I have no {{ path }} token available in my list of 'REPLACEMENT PATTERNS', just '{{ view_node }} == Link to Content', which seems to work fine when I use it as a replacement token for something else, in this case the body field:
The {{ path }} token sounds like a leftover from the old Node path field that was deprecated in 8.5.0. Have you tried replacing it with {{ view_node }} ?
If I'm just missing some steps to reproduce the issue, please provide them.
If this is something that gets broken by applying this patch, please set this back to needs work. If it something that is currently not working and after applying this patch is still not working please consider opening a follow up issue so we address the main issue first.
Setting back to RTBC.
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commentedHi Lendude,
I am a colleague of patrickroma.
I added the field "Content: Link to Content". I wasn't able to replace {{ path }} by {{ view_node }}, because {{ view_node }} does not exist:
When I use the {{ path }} token, detail links for the main language of the Drupal installation will be generated. Instead of using the {{ path }} token, I used the functionality of the field, to render the detail link, but even in that case, Drupal rendered the detail link for the main language:
We are running Drupal in version 8.5.5 with the languages Englisch, Spanish and German.
Thanks,
Timo
Comment #50
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #51
Lendude@timomueller thanks for the update.
I think the most important question now is, can you reproduce this in a clean install or just in your fully configured site? And if you can reproduce this in a fresh install, do you have some steps to reproduce?
If not, I'm thinking your view might still be using the old node_path plugin. Do you have a yml export of the config file for this view? Could you check which plugin_id it's using? Or share the export here?
Comment #52
Anonymous (not verified) CreditAttribution: Anonymous commentedHi Lendude,
thanks a lot for your fast reply!
I've attached a yml configuration file for one of our views (views.view_.mt_benefits.yml)
In this view we use the native functionality of the field "Content: Link to Content" to render the detail page link. Even in that case, the detail page of the site's default language is provided.
Comment #53
Lendude@timomueller hmm it's using the right plugin, really stumped as to why it would show the {{ path }} token still.
As to fixing this, the question still remains: can you reproduce this on a clean Drupal install? Because I'm not having any luck with that. And if we can't get this to break in vanilla core, there is not much we can fix here. Then it might be contrib code or custom code getting in the way.
Comment #54
Anonymous (not verified) CreditAttribution: Anonymous commented@lendude: we applied patch #42 and now everything is working wonderful :)
Comment #55
Lendude@timomueller perfect! Thanks for digging into this!
Back to RTBC I'd say.
Comment #56
mahmoud-zayed CreditAttribution: mahmoud-zayed as a volunteer and at Vardot for Vardot commentedI faced another issue that when content language is Not specified or Not applicable and I have two languages when click link in the second language it's switch interface to the default language.
Suggested solution:
I updated @Lendude patch and I add simple check by Entity language id if it's equal Not specified or Not applicable it will add current interface language to link to prevent switching between languages interfaces.
Here's the updated patch.
Thanks
Comment #57
mahmoud-zayed CreditAttribution: mahmoud-zayed as a volunteer and at Vardot for Vardot commentedSorry just correct patch name.
Comment #58
Lendude@mahmoud-zayed I'm still not convinced this is needed in a bug fix. I don't think that redirecting to the default language is any worse then redirecting to the current UI language. It's just an implementational choice.
If it turns out, this is the right way of doing this, we would need a test for this. Currently the added code doesn't have test coverage, and it should have.
Also, this is over 80 characters long and should probably read something like "When the entity language is set to 'Not specified' or 'Not applicable' we set it to the current active language to prevent unnecessary language switching."
Comment #59
aleksip@Lendude I'm currently working on a multilingual site where the language of almost all content types is set to Not applicable. In other words, the multilingual support required on the site is all about the UI. So I would consider redirecting to the default language and suddenly changing the interface language a bug rather than just an implementational choice.
Comment #60
aleksipI have been testing the patch, and it seems to work with link fields. However, I found that the same problem exists with links in other types of fields such as linked title field and dropbutton field.
As this issue is about link fields, should separate issues be opened for different field types with the same problem or should the issue title and description be amended to cover the same basic problem with all views fields that contain links?
Comment #61
aleksipIt looks like the problem with Dropbutton is that
Links::getLinks()
just uses['alter']['url']
and does not take['alter']['language']
into account.I'd be happy to have a go at a patch to make Dropbutton work, but I can think of several ways to fix this, and am not familiar with the code, so am not sure what is the best approach:
1) Add support for
['alter']['language']
toLinks::getLinks()
.2) Use
FieldPluginBase::renderAsLink()
inLinks::getLinks()
(not so sure about this one, don't know about possible side effects).3) In
LinkBase::addLangcode()
also change the language option in['alter']['url']
. This could be done in the current scope of this issue, but perhaps the original value should be left as is?4) Something else?
Comment #62
Lendude@aleksip please look at the other plugins in new issues, this fix is hard enough to land without the additional scope.
Comment #63
aleksip@Lendude Yes, I have learned that it can be very hard and take a very very very long time to land anything in core. :)
While it would be great to get this fix in, especially as it is very close to being committed(?), maybe a common solution in FieldPluginBase would ultimately be the best, as it is reasonable to expect the same behaviour from all views fields? Or would that not be feasible for some reason?
Added another related issue: #2862511: Link to Entity get invalid language link in views..
Comment #64
aleksipIn case anyone reading/following this issue might be interested, I have now opened two new issues with proposed fixes for Dropbutton and linked entity fields (e.g. title, when content language is 'Not specified' or 'Not applicable'):
Comment #66
Lance Lancelot CreditAttribution: Lance Lancelot commentedIf you do not want to use patches, you have a work around in D8 and views, by using twig.
1. add ID as field and exclude from display
2. create a custom text field and add the following into that:
<a href="{{ path('entity.node.canonical', {'node': nid}) }}">Read on</a>
3. if you want to use the translated link to wrap around an image for example. You can use a custom text field and paste the above twig into it (without the a-tag).
4. use the {{nothing}} replacement variable as url when rewriting your image_field
That works for me.
Comment #67
Bessonweb CreditAttribution: Bessonweb commentedThe #57 patch work for me.
Comment #68
ConradFlashback CreditAttribution: ConradFlashback commented#66 works well!
I wait the final patch, thanks.
Comment #69
yaach CreditAttribution: yaach as a volunteer commented#66 works temporarily for me.
Comment #70
pavlosdan CreditAttribution: pavlosdan as a volunteer commentedThe patch for #42 works well for us as well.
What are the plans for this? Could we get it committed (since it has tests etc) and open #56 as a follow up issue?
Comment #71
ConradFlashback CreditAttribution: ConradFlashback commented[edit] with #66 and more than one views display, the view goes crash.
Comment #72
Bessonweb CreditAttribution: Bessonweb commentedAfter 8.6.1 core update the #57 patch don't work.
The view display only in english in every languages.
I hope this bug will be solved quickly because it's very bad.
I used the title with a classe for hide the text for the moment because the title of the node is not impacted by this bug...
Comment #73
cri2mars CreditAttribution: cri2mars commentedwrong typo sorryComment #74
cri2mars CreditAttribution: cri2mars commentedhi all,
here is how i solved this problem
i have a custom entity called toile, fully translated
a view named gallery, overview of these entities, displayed as a grid, each item containing a link pointing to the entity
the problem was as described that whatever was the translation selected of that view, every link was pointing to the default langage entity... bad :-(
fortunately it can be solved using this method:
1- Add the Entity translation language field, in the fields of your view. note that it have to be below the link field, so you can use the link as a token:
2- Change the link field:
exclude from display
render URL as text (that way the pointing URL can be used as a token in URL rewriting...
3- Then we have to change the settings for the new Entity translation language field that way:
rewrite the field as a custom link using the available tokens below: {{ langcode__value }}{{ view_toile }} in my case
render the field as a custom text (as otherwise the URL should really look ugly as a link... {{ 'Zoom' }} in my case, helpful as it only has to be translated in russian ;-)
4- And finally you can add your view translations, and change the translated text displayed for the link in all your languages if you have to...
In my case it totally solved the problem, i am using 8.6.2 core
hope it was clear and helpful
cheers
Comment #75
NickDJM CreditAttribution: NickDJM commentedTested #57 on 8.5.8 and it solved the issue.
Comment #76
Anas_maw CreditAttribution: Anas_maw as a volunteer commentedI can confirm, patch 57 worked for me, Drupal version 8.6.3
Comment #77
maurizio.ganovelliPatch #57 solves issue (tested on core 8.6.3) for me too.
Comment #78
beloa CreditAttribution: beloa commentedPatch #57 worked to solve the same issue with a "link to content" field (core 8.6.4)
Thanks
Comment #79
paper boyCan confirm patch from #57 fixes the problem on a link to content field for me too (core 8.6.3). Couldn't find any problems caused by the patch so far.
Thanks for the great work!
Comment #80
mahmoud-zayed CreditAttribution: mahmoud-zayed as a volunteer and at Vardot for Vardot commentedComment #81
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #82
alexpottApparently we need a change record. Also @Lendude, views maintainer, says in #58 that the latest patch contains an additional fix that is probably not needed. And if it is we need further test coverage. I think we should open a follow-up and go back to the fix in #42.
Also @mahmoud-zayed generally we try to avoid rtbcing patches we've contributed to.
Comment #83
pavlosdan CreditAttribution: pavlosdan as a volunteer commentedIs the issue described in "Links to entities with language set to 'Not specified' or 'Not applicable' should default to current UI language" the same as #56 or do we need another follow up?
Comment #84
LendudeThank @alexpott for taking a look at this and your feedback.
Re-upping #42
#2991440: Links to entities with language set to 'Not specified' or 'Not applicable' should default to current UI language seems like a good follow up.
Will add a CR for the change to the LinkBase constructor.
Comment #85
LendudeAdded CR : https://www.drupal.org/node/3023427
Comment #86
pavlosdan CreditAttribution: pavlosdan as a volunteer commented#43, #44, #54 and #70 confirm the patch in #42 . We've also been using the patch in production for a few months now. Setting to RTBC.
Comment #87
alexpottAs this is a base class we need to dance the optional / @trigger_error dance. So anything in contrib / custom that extends this does not break.
Something like this:
This feels wrong. The trait uses methods on both EntityTypeManagerInterface and EntityRepositoryInterface - we need to provide the entity manager here :( until there is a version of EntityTranslationRenderTrait that works with the correct services.
So I think we need to inject the entity manager here.
Comment #88
Lendude@alexpott thanks for the feedback.
#87.1 done, not sure about the wording, quick search didn't turn up anything like this is core yet
#87.2
turns out we were already passing the entity manager service, oops! so just updated the docblocks and uses
Comment #89
paper boyPatch from #88 seems to work fine on Drupal 8.6.4.
Comment #90
xurubo93 CreditAttribution: xurubo93 as a volunteer commentedI can confirm patch #88 works on Drupal 8.6.7
Comment #91
isinadinos CreditAttribution: isinadinos as a volunteer commentedUnfortunately #88 is not working on 8.6.10
Comment #93
ious CreditAttribution: ious commented@isinadinos, I got it applied. Here what I got with 8.6.10 with composer -v :
Comment #94
ogggg CreditAttribution: ogggg commented#88 works for me in drupal 8.6.10
Applied with:
patch -p1 < path/file.patch
Comment #95
LendudeThis is failing because of #3023981: Add @trigger_error() to deprecated EntityManager->EntityRepository methods on 8.7.x, this needs to be updated to use the EntityTypeManager, not the EntityManager. So basically we can now address #87.2
Comment #96
LendudeThis addresses #95/#87.2, should be back to green.
Comment #98
LendudeDuh...
Comment #99
DuneBLWith #88: everything is fine
With #98: I get the following error when browsing to a view:
Fatal error: Class Drupal\views\Plugin\views\field\EntityLink contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\views\Plugin\views\field\LinkBase::getEntityManager) in .../drupal8/core/modules/views/src/Plugin/views/field/EntityLink.php on line 15
Comment #100
Lendude@DuneBL that is probably on 8.6.x ? The patch in #98 is 8.7.x only since the dependant changes were only done in 8.7.x in #3023981: Add @trigger_error() to deprecated EntityManager->EntityRepository methods
Comment #101
DuneBL@Lendude sorry for disturbing... you are right, I overlooked the 8.7 spec
Comment #102
Riadh Rahmi CreditAttribution: Riadh Rahmi commentedThe #57 patch worked for me. Thanks mahmoud-zayed.
Comment #104
jess_m CreditAttribution: jess_m commentedHi, I attempted the following patches: #57, #84, #88 and #98 with limited success. For #57, #84 and #88, it generated the link correctly but when you clicked on it I got the one line of death saying the Website encountered an error.
For #98 I received the following error.
Fatal error: Class Drupal\views\Plugin\views\field\EntityLink contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\views\Plugin\views\field\LinkBase::getEntityManager) in (localhost)\web\core\modules\views\src\Plugin\views\field\EntityLink.php on line 15
Temporarily I can use
<a href="{{ path('entity.node.canonical', {'node': nid}) }}">TESTING</a>
for my needs, but I have a site with English and four other languages. We have multiple views that will need to use an entity_link along with other sites in the future that will probably have the same set-up and need. My Drupal version is 8.6.10, all security updates have been implemented as well.I'm not sure what other information to provide so if there is anything you need please let me know. The temp code above will work for now, but we'd like to soon after a solution that is more viable in the long-term. Any help would be appreciated.
Thanks.
Comment #105
Lendude@jess_m the patch in #98 is for 8.7.x only, not sure why #88 wouldn't work for you on 8.6.10, but if you get a white screen after clicking the link, that is not the fault of the link, so probably something unrelated to this.
Comment #106
jess_m CreditAttribution: jess_m commented@Lendude
I think what happened was that #88 was my third patch to try and one of the other patches caused a problem so when I tried #88 it showed up as an error. I wiped out my site as it was then and pulled in a fresh copy of it from scratch, applied the patch and it now works. Thanks!
Jess
Comment #107
markdc#88 works as advertised. Thank you!
Comment #108
vuil#57 works for us (Drupal core 8.5.14). Thank you!
Comment #109
Pascal- CreditAttribution: Pascal- commented#88 solved my issue
Comment #110
LendudeSo we have a fair number of 'Tested By the Community' instances, anybody feel up to putting the R in front of that and setting this to RTBC?
Comment #111
johnwebdev CreditAttribution: johnwebdev commentedHaven't looked through in detail yet, but I'm wondering:
This should probably be 8.8 now? Or do we intend to backport in to 8.7?
Comment #112
ABaier CreditAttribution: ABaier commentedI can also confirm that #88 solved the issue for us, on core 8.6.14. Thanks!
Comment #113
Krzysztof Domański1. Tested manually on 8.8.x-dev.
2. New patch with minor changes in the docs, removal of unnecessary use statement.
3. #111 left unchanged so needs review by the commit.
Comment #114
Utilvideo CreditAttribution: Utilvideo commentedAfter applay Patch
Class Drupal\views\Plugin\views\field\EntityLink contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\views\Plugin\views\field\LinkBase::getEntityManager) in C:\wamp\www\colibri.local\web\core\modules\views\src\Plugin\views\field\EntityLink.php on line 15
Comment #115
Lendude@Regnoy see a bunch of previous comments, you probably applied the latest patch to 8.6.x, use the patch in #88 for 8.6.x
Comment #116
mpp CreditAttribution: mpp at AmeXio for District09 commentedNote that "Link to the Content" is still not working (see #2447821).
Comment #117
pavlosdan CreditAttribution: pavlosdan as a volunteer commentedUsing the "Link to content" field as a replacement for the URL of the title field.
This doesn't seem to work (bottom left of attached screenshot - hovering over the "Chinese" title link).
Is there some processing happening when "Output this field as a custom link" is checked that strips the language?
Comment #118
Krzysztof Domański1. Tested manually on 8.8.x-dev. +1 RTBC.
Back to "Needs review" due to changes in tests.
2. #111 left unchanged so needs review by the commit.
3. The 'test_link_base_links' tests also 'Link to Content'. Needs follow-up if the "Link to content" field not work as a replacement for the URL of the title field.
Comment #120
Lendude#111 is not a problem, this is a bug fix so it should target 8.7.x
Test coverage changes look fine, back to RTBC
Comment #123
joseph.olstadPatch #88 fixes our issue on 8.6.15
I tried the newer patch #118 on 8.6.15 and it crashed on 8.6.15 (even though it applied cleanly on my build). I haven't tested patch 118 on 8.8.x , only testing on 8.6.15
Patch #88 works for us on 8.6.15 thanks!
Seems like an important fix should get attention soon would have saved me a day of work, help save others time as well.
Merci!
Comment #124
joseph.olstadThis should go in right away!
Several other patches are waiting but this one is ready now!
Comment #125
paper boyPatch from #88 still working well with 8.7.1.
I'd really love to see this merged soon. Been applying the patch for months again and again.
Comment #126
joseph.olstadTo the core maintainers, If you are not running a site with more than one language then please assign this issue to a core maintainer that does.
Patch for 8.7.x works as well on the new 8.7.1 release.
Please commit, has tests too. Great work on this!
Comment #127
paper boyTested patch #98 now on 8.7.1, also works without any problems so far.
Comment #129
LendudeUnrelated fail
Comment #130
AnybodyConfirming RTBC for #118 and importance of this issue! This should definitely be part of the next core release. Very essential fix when using multilanguage.
I tested #118 also manually plus tests... and everything is fine.
Comment #132
LendudeUnrelated fail
Comment #133
joseph.olstad***EDIT*** impatience
Comment #134
MiSc CreditAttribution: MiSc commentedIn what way @joseph.olstad was that a helpful comment? In what way are you making this issue moving forward with that comment? In what way have you read the code of conduct?
Comment #135
kristiaanvandeneynde@joseph.olstad how can you be on drupal.org for 8 years and:
Very unprofessional dude. There's a lot of things you can learn from people like Lendude. Politeness being one of them.
To end on a positive note: The patch looks great (at a glance), job well done all!
Comment #136
larowlanI realise this is public in the parent trait - but why? The only call I can see is from
\Drupal\views\Entity\Render\EntityTranslationRenderTrait::getEntityTranslationRenderer
so I think it possible should have been protected, as its API surface we don't need. Should we make the new implementation added in this patch protected (PHP allows that) so we don't add anymore API surface and then create a follow up to investigate the impact of making the other implementations protected - although I suspect that would be an API change for anything that wasn't marked internal. I've asked the same question in #2394883: Language setup for entity and field based rendering in views is independent, confusing UI, lacking test coverage where it was introduced.Comment #137
larowlanAlso, I can't speak for other maintainers, but I look at the RTBC queue in reverse chronological order, i.e the issues that were recently updated are at the bottom of the list and the ones that have been RTBC without comment for longer are at the top. So it may be that the regular comments that don't add anything new actually keep dropping this to the bottom of that sort, meaning it doesn't get attention.
Comment #138
aleksip@larowlan That was a very noteworthy point in #137, thanks! I added it to the Getting an issue addressed sooner (and issue queue etiquette) page.
Comment #139
alexpott@larowlan makes a great point in #136 - it's what has made me think twice when looking at this patch before. See https://www.php.net/manual/en/language.oop5.traits.php#language.oop5.tra... for how to change method visibility.
Comment #140
joseph.olstadok if I understood 136 #2877994-136: Entity Links fields does not have translation support correctly, change from public to protected
Comment #141
JeroenTComment #142
joseph.olstadThe added tests will ensure that we won't regress this in the future. Many people do not have the required skill (or time) to find and apply this fix.
Same patch fixes 8.7.x and 8.8.x branches
If you want I can reroll the fix for 8.6.x as well?
Comment #143
delalis CreditAttribution: delalis commentedRTBC +1
Comment #144
gdaw CreditAttribution: gdaw as a volunteer commentedFixes we got from 88 and 118 are still fixed using 140.
RTBC +1
Comment #145
gdaw CreditAttribution: gdaw as a volunteer commentedComment #147
dsutter CreditAttribution: dsutter as a volunteer commentedRTBC +1
Comment #148
ldenna CreditAttribution: ldenna commentedRTBC +1
Comment #149
ldenna CreditAttribution: ldenna commentedRTBC +1
Comment #150
alexpottCrediting @aleksip, @johndevman, @alexpott, @larowlan for reviews.
Crediting @paper boy, @patrickroma, @Bessonweb, @tibbsa, @ConradFlashback, @DuneBL, @jess_m, @arakwar for trying out the patch, testing and reporting back.
Note I've not crediting people for the numerous +1s to the patch - though thanks for all the reports it is great to know so many people have tried and need this fix.
Committed 3839c0d and pushed to 8.8.x. Thanks!
I think given the number of people applying the patch we should consider backporting to 8.7.x BUT I think a release manager should chime in because we're changing the constructor to a base class.
Comment #152
joseph.olstadThank you @Alexpott!
Comment #153
joseph.olstadCan you please cherry-pick this for 8.7.x as well?**EDIT** ok I see the comment about framework manager review. **EDIT**clean cherry-pick
no conflicts (test results above).
Comment #154
vuilComment #155
vuilComment #156
BerdirComment #157
vuilComment #158
vuilComment #159
cilefen CreditAttribution: cilefen as a volunteer commentedComment #160
larowlanComment #161
catchI'm not keen on backporting the changes to a base class constructor to a patch release, maybe for a fatal error or data loss, but this is neither. Has anyone grepped contrib to check if there would be modules affected?
Comment #162
joseph.olstadIf requiring entity_type.manager was causing problems in contrib we'd have known by now in other projects using entity_type.manager.
as for grepping contrib, contrib isn't views, views is in core, contrib just passes things in and expects output.
Garbage in, garbage out.
As an annecdote, we're currently using the code that was pushed into 8.8.x in 8.7.x (using the patch) with over 300 contrib modules, no issues.
Comment #163
joseph.olstadWhat this patch does:
simple view with links and node fields
display fields:
linked title field to node title
taxonomy term label
filter by node type "article"
filter by current display language
OR rss feed
linked to node content title
taxonomy term label
filter by current display language
Before fixing with patch:
site default language: english
english URL in path:
https://mybrokensite.com/en/viewpage
display row:
<a href="https://mybrokensite.com/en/article-title-in-english">Article title in english</a> example taxonomy term english.
english is correct, displays english title, english taxonomy term label and english link to content /en/node/1234 or url alias path if exists.
site default language: english
french URL in path:
https://mybrokensite.com/fr/viewpage
display row:
<a href="https://mybrokensite.com/en/article-title-in-english">Titre en français sacre bleue</a> étiquette de taxonomie français.
Our use case was an rss feed.
AFTER PATCHING with fix:
site default language: english
french URL in path:
https://myfixedsite.com/fr/viewpage
display row:
<a href="https://myfixedsite.com/fr/titre-en-francais-sacre-bleue">Titre en français sacre bleue</a> étiquette de taxonomie français.
Comment #164
catchIt's a base class, which means contrib and custom code can extend it, potentially overriding the constructor.
Comment #165
joseph.olstadIs there a way to download ALL contrib modules, like a master list of uri to the git repos for all projects on drupal.org?
If there was, I would write a script and download ALL of contrib and do a grep for this base class.
Maybe I would have to write a crawler for drupal.org and crawl all 3+million nodes, look for project pages out of those and add the git repo link for each project that has an 8.x version?
Or is there an easier way? I could write a crawler to do this but hate to write code for nothing if this is already listed somewhere or if someone has a better idea?
Comment #166
init90>Or is there an easier way?
Looks like http://grep.xnddx.ru/ can do it.
Comment #167
Gábor Hojtsyhttp://grep.xnddx.ru/search?text=LinkBase is the site we regularly use to grep for contrib, it has a lot of contrib covered. The following relevant results were found:
Directly extending LinkBase without constructor override (I did not go further (ie. did not look at classes extending these classes)):
These are abstract classes themselves, but none of their extensions seem to override the constructor:
Overrides constructor with further arguments, so people using at least these modules would get WSOD if the patch is merged to 8.7.x:
Comment #168
Berdir> Overrides constructor with further arguments, so people using at least these modules would get WSOD if the patch is merged to 8.7.x:
Well, no, because we make the arguments optional with BC? There are still possibilities that something breaks, but they have been very rare since we started to be more strict about constructor changes and provide BC.
Not saying that this should or should not be backported, just that this statement isn't true. And there's also always custom code to think about, BC isn't just about the known contrib code, if anything, it gives us an idea about how widespread those cases might be :)
Comment #169
joseph.olstadIf I am understanding #167 and #168 correctly, it appears that there is little to no risk that this change would break contrib.
The only breakage that I could see would be unsupported breakage for workarounds to this unexpected behavior, example might be twig extensions that developers have made on their own >IF< they weren't fortunate enough to find this patch I could see people might have written their own twig extensions to override the core behavior (prior to this patch or without this patch).
I recall when looking for a solution to this before I found this patch I was considering making a twig extension to provide the behavior but luckily we found this patch that already has been perfected and a lot of work has gone in (thanks mostly to Lendude!)
Comment #170
paper boyI got #140 running on 8.7.4 without having any problems
Comment #171
joseph.olstadI updated a script that git clones over 24012 contrib projects for Drupal (modules mostly), just need to adjust it to checkout the 8.x branches automatically, then grep all those. There's probably well over 5000 of those projects that have 8.x branches but I haven't checked yet. Still need to update the script to look through 8.x branches.
#1057386-82: Provide a way to download the entire git codebase for all projects
with that said, this is probably overkill for checking this patches impact on contrib.
Comment #172
pjudge CreditAttribution: pjudge as a volunteer commentedAlso confirming #140 works beautifully on 8.7.4.
Comment #173
sumithb CreditAttribution: sumithb commentedThis is to confirm that the #140 works for us also(Drupal 8.7.2). We experienced an issue with the RSS feed link in Views. For us, we created separate RSS views for English and French to push content to MailChimp. The problem we faced was with the French RSS feed whose link instead of pointing to French node was pointing to English. The patch mentioned in #140 worked for us.
Comment #174
vuilI confirm that the #140 works for us (D8.7.5).
Comment #175
mrshowermanWe're using #140 in two projects on D8.7.5 and it works like a charm.
Comment #176
larowlanFrom #161
On that basis, marking this as fixed.
Comment #178
markdatter CreditAttribution: markdatter as a volunteer commentedAny chance this will be released in Drupal 8.8?
Comment #179
joseph.olstad@markdatter , it's already in 8.8.x , 8.8.0 will be released in December I believe, should include this.
I would like to see this fixed in 8.7.x as well, however it's probably not going to happen, 8.8.0 is just around the corner two months away.
Comment #180
jrochate CreditAttribution: jrochate commentedHi
I have patched 8.7.7 with #140 but User Entity still doesn't link to correct current language.
This problem happens on user display render and also on views fields from user entity.
Anonymous or Authenticated logins when they click on a User link that is configured with "Link to content" (user picture) or "Linked to the User" (text field) always go to default lang (no lang path in my case) then the current site selected language.
EDIT: SOLVED!
The fields which we want to "Link to content" or "Linked to the User" must be multilang.
So I was linking for "Full Name" field, and that wasn't multilang (doesn't make sense). But for the links to work language sensitive, the field must also be multilang.
That's it...
Comment #181
joseph.olstad@jrochate, correct me if I am mistaken but if my recollection is accurate this solution resolves field language for linked reference to other entities of type node in a view. For type user likely keep searching the issue queue , someone has surely written a patch. Otherwise please review if the patch was applied correctly and clear caches.
Comment #182
vuilComment #183
Deno CreditAttribution: Deno commentedDo I get this right? This issue is resolved in development version and will be resolved in the next 8.x release?
Comment #184
joseph.olstadFor 8.7.x use the patch, however the comming release 8.8.0 has this fixed
Comment #185
jarekjj CreditAttribution: jarekjj commentedCan anyone confirm if 8.8 fixed the issue for them?
It did not in my case.
Comment #186
joseph.olstadI haven't yet upgraded to 8.8.0, maybe in a few weeks.
However, FOR SURE this is fixed in 8.8.0
Try to apply the patch, it will say it's already applied!
Comment #187
efrainhWhen upgrading from 8.7.6 to 8.8 I got this message:
Reviewing the code it already contains the patch so we don't need this patch in 8.8!!.
Comment #188
joseph.olstad@efrain, please move this discussion over to #2745755: AliasStorage::preloadPathAlias() incorrectly prioritizes und aliases
seems obvious to me that the patch you refer to needs a reroll (if it's still needed, you should verify that, might be a stale issue).
Comment #189
xjmComment #190
Antoniya CreditAttribution: Antoniya commentedCan someone confirm that this issue is fixed in 8.8 for all views, including REST?
I have a REST export on nodes with a link field and all entities referenced by this field show URLs in the original lang, translations are completely ignored.
Comment #191
joseph.olstadMaybe open a new issue for the REST use case with steps to reproduce and link it to here.
Meanwhile maybe try workaround with:
https://api.drupal.org/api/drupal/core%21modules%21views%21views.theme.i...
Comment #192
Antoniya CreditAttribution: Antoniya commented@joseph I'm not sure which workaround you're referring to. I don't think the output of REST views can be modified via template preprocess or am I missing something? 🤔
Comment #193
joseph.olstadAgain, off topic now because your issue is not the same. Alternatively as a workaround, you can also load a REST view in a controller method and for a new route and output a modified json array from that.
ex:
This issue is already closed, please do not put any more responses in this thread other than the link to your somewhat related issue.
I will not respond to any more of your questions in this thread
please open a new issue and put the link here.
Comment #194
drase15 CreditAttribution: drase15 commentedA quick and easy workarround that I use for views pages (need pathauto).
Create your page view as usual and set URL in default language e.g. "/english-title".
Then go to admin - search and metadata - url aliases (/admin/config/search/path)
Add alias, on "system path" add link you have created on view "/english-title" and then on "url alias" add translanted link "/titulo-ingles"
Click save and thats it, now you have your views page translated.
Comment #197
quietone CreditAttribution: quietone at PreviousNext commentedClosed #2667086: LinkBase doesn't respect translations as a duplicate, adding credit.