Problem/Motivation

Links to content translations on translate tabs are incorrect as of #1987882: Convert content_translation routes to a new style controller. They always lead to the entity in the current language. Screenshot is for nodes, but same applies to menu items, shortcuts, etc. Screenshot from a site where all three languages had their path prefixes configured. The bug is not path prefix configuration dependent.

Proposed resolution

Fix. Write tests. Review. Commit.

Remaining tasks

Most of them.

User interface changes

Links will be correct.

API changes

Likely none.

CommentFileSizeAuthor
#2 translate-links.patch3.93 KBGábor Hojtsy
Content | s5b5dcd20db24bc4.s2.simplytest.me 2014-08-22 11-50-27 2014-08-22 11-53-15.png202.99 KBGábor Hojtsy
Translations of Content ... | s5b5dcd20db24bc4.s2.simplytest.me 2014-08-22 11-48-24 2014-08-22 11-50-10.png132.44 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue summary: View changes
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: Entity translations cannot be edited anymore, wrong links » Entity translations cannot be edited anymore, view links are also incorrect
Status: Active » Needs review
FileSize
translate-links.patch3.93 KB

This is the patch I originally thought would be good, but it only fixes symptoms on the overview screen. Not in views of course. Not sure how this all worked before?! At least the test modifications here can be used as-is, they will test all entity types that inherit from this test baseclass, eg. nodes, menu, shortcut, etc.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: Entity translations cannot be edited anymore, view links are also incorrect » All links lead to same entity translations, may result in data loss
Issue summary: View changes

Removed the views use case because that was broken before and down to other things happening in views. Will look up which issue exactly.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: All links lead to same entity translations, may result in data loss » All links lead to same entity translations
Priority: Critical » Major

The delete links are translation specific so the data loss argument does not stand :/

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: All links lead to same entity translations » All links lead to same entity translations in translation overviews
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

The views problem is covered in #2217569: Fields row plugin: Translation is non-uniform for base fields, Field UI fields, links; no way to choose "this row's language". Posted the views screenshot there.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Related issues: +#2323935: Translations page for content created prior to enabling shows "unpublished" for English versions

Also there's related bugs:
#2323935: Translations page for content created prior to enabling shows "unpublished" for English versions
#2251907: Wrong status on Translate tab for entities - suppose this one could be closed as duplicate once "add" addressed in previous issue

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

@andypost: I don't think either of those are duplicates of this one.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito commented
Status: Needs review » Reviewed & tested by the community

If I'm right I broke this at #1987882-195: Convert content_translation routes to a new style controller. test_coverage++

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Tested the patch with simpletest.me and found that redirect after adding a translation is strange.
Once term is created in en and then I add translation for ru, after submit I redirected to edit for en - that's confusing.
Suppose that's not a regression but it makes sense to fix here

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

@andypost: what does it have to do with the translation overview screen?!

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

@Gabor link to add is generated on overview page, and I suppose to add "destination" for add link here

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Reviewed & tested by the community » Needs review

I think we should keep using LanguageManagerInterface::getSwitchLinks() otherwise sites using session language selection won't work.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

@plach: I was half-way back-refactoring this code... But then... Why do we need the links from switch links?! It should be enough to link to view/edit, etc. pages by specifying a path/route and a language and the outbound URL modifiers will do what they need, no? They should. Otherwise why don't we need to use the switch links at *every* place where we need to link to language specific versions? Why are they needed here as a special case? OutboundPathProcessorInterface is implemented by LanguageNegotiationSession, so the same patch should work for that use case, no? Well, in practice it looks like it does not work... While trying this on a local site. But that is quite shocking to me. How are we supposed to link to translated entities then? No way to just pass in a language to a URL generator, but always need to refer back to languagemanager's switch links? Really?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

Unfortunately the only reliable way to obtain language-aware URLs is getting language switch links. This has been true since D7 and is precisely the reason why I always wanted to work on #1137074: Make obtaining language-aware URLs less painful, but I could never get to it. Probably soon will be the moment. Anyway, to fix this for now we need language switch links, hopefully we can get rid of the language manager method (or probably making it wrap the new code, to preserve BC) and get a more sane DX afterwards.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Sigh, that means that fixes we thought were correct in #2149649: Entity rendering/theming does not use the active entity language to render links etc. are not general enough then... :/ The main problem is factoring this back to switch links will make it use paths all over again which the whole factoring away from paths to routes was supposed to avoid. Sounds insane to factor code from routes back to paths now... getLanguageSwitchLinks() on the language manager is path based. So to make that route based sounds like resolving #1137074: Make obtaining language-aware URLs less painful as part of this issue or postponing this one on that which means almost no way to translate entities for quite a long while (at the rate of these issues being fixed). And I hoped I can show the multilingual UI with a straight face in Amsterdam :/

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Reviewed & tested by the community

Ok, let's fix this for the 80% of use cases now and address the remaining 20% in #1137074: Make obtaining language-aware URLs less painful.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented

+1 to RTBC.

+++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
@@ -84,6 +84,9 @@ public function overview(Request $request, $entity_type_id = NULL) {
+          array(
+            'language' => $language,

@@ -91,6 +94,9 @@ public function overview(Request $request, $entity_type_id = NULL) {
+          array(
+            'language' => $language,

@@ -98,6 +104,9 @@ public function overview(Request $request, $entity_type_id = NULL) {
+          array(
+            'language' => $language,

easy to get conflicted and hard to reroll.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

I guess this works for now. This seems extra cumbersome to have to remember to add that parameter manually (esp. since you already gave it a language 3 lines above that). It's not clear to me in what way #1137074: Make obtaining language-aware URLs less painful would help with the DX here.

However, the issue is borderline critical and the patch fixes it (with test coverage so we don't break it again) so...

Committed and pushed to 8.x. Thanks!

  • webchick committed 5c61480 on 8.0.x 
    Issue #2325977 by Gábor Hojtsy: Fixed All links lead to same entity...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

@webchick: no this is actually the simple way to do links, but it does not work for all cases of language negotiation setup, notably for session language negotiation. (I think it should btw, but it clearly does not).

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: -sprint

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

That the link to the translation itself got the language option is correct, but the the add, edit, and delete links this is not correct IMO. Translators should not be forced into a context switch unnecessarily. The actual problem is that the links point to the entity edit forms in the first place instead of the translation edit forms. See #2451693: Operations links on the translations overview page do not link to the correct route