Problem/Motivation

When I translate node, the translated menu link item overwrite the original menu item link.

Steps to reproduce :

In this exemple I have a Drupal 8 site with 3 languages enabled :
French (default)
English
German

  1. Enable the translation for a content type
  2. Enable translation for menu links
  3. Create a node in French and on the node creation form, click "Provide a menu link" and add a menu link
  4. Translate this node and translate the menu link

After saved the translated node, The original menu link has been overwritten by the translated link.

The only way to have a translated menu link is create it directly from the menu configuration .

I fixed the priority major because this bug causes user input to be lost.

Comments

mdespeuilles created an issue. See original summary.

mdespeuilles’s picture

Version: 8.0.0-rc2 » 8.0.x-dev
iMiksu’s picture

Assigned: Unassigned » iMiksu

Lets see if I can write test for this.

kekkis’s picture

The method \Drupal\path\Tests\PathLanguageTest::testAliasTranslation seems to test the same thing. iMiksu discovered that it doesn't however test whether the originally set English alias still works.

iMiksu’s picture

I tried with this diff and it still passed. After wondering for a while, we realised that it's not about the path aliases, but about the menu links as described.

diff --git a/core/modules/path/src/Tests/PathLanguageTest.php b/core/modules/path/src/Tests/PathLanguageTest.php
index a042d83..1451bb5 100644
--- a/core/modules/path/src/Tests/PathLanguageTest.php
+++ b/core/modules/path/src/Tests/PathLanguageTest.php
@@ -86,9 +86,9 @@ function testAliasTranslation() {
     $edit['path[0][alias]'] = '/' . $english_alias;
     $this->drupalPostForm('node/' . $english_node->id() . '/edit', $edit, t('Save'));
 
-    // Confirm that the alias works.
+    // Confirm that the original English alias works.
     $this->drupalGet($english_alias);
-    $this->assertText($english_node->body->value, 'Alias works.');
+    $this->assertText($english_node->body->value, 'English alias works.');
 
     // Translate the node into French.
     $this->drupalGet('node/' . $english_node->id() . '/translations');
@@ -115,10 +115,14 @@ function testAliasTranslation() {
     $english_node_french_translation = $english_node->getTranslation('fr');
     $this->assertTrue($english_node->hasTranslation('fr'), 'Node found in database.');
 
-    // Confirm that the alias works.
+    // Confirm that the French alias works.
     $this->drupalGet('fr' . $edit['path[0][alias]']);
     $this->assertText($english_node_french_translation->body->value, 'Alias for French translation works.');
 
+    // Confirm that the original English alias *still* works.
+    $this->drupalGet($english_alias);
+    $this->assertText($english_node->body->value, 'English alias still works.');
+
     // Confirm that the alias is returned for the URL. Languages are cached on
     // many levels, and we need to clear those caches.
     $this->container->get('language_manager')->reset();
iMiksu’s picture

Assigned: iMiksu » Unassigned

It seems somehow that we should probably create a test alongside with \Drupal\menu_link_content\Tests\MenuLinkContentTranslationUITest::doTestTranslationEdit?

kekkis’s picture

At another look, \Drupal\content_translation\Tests\ContentTranslationUITestBase::doTestBasicTranslation (towards the end of the method, starting around line 193) seems to test that the properties have been translated correctly. Similarly to what we tested out with iMiksu, probably would be a good idea to also include assertions for the original values being what they were before the translation was saved.

mdespeuilles’s picture

After investigations, I think I found the problem :

In the file core/modules/menu_ui/menu_ui.module, in the function menu_ui_get_menu_link_defaults() all queries to load menu item link are not filtering by language..

mdespeuilles’s picture

Status: Active » Needs review
agoradesign’s picture

I've encountered the same problem and I'd like to raise a discussion, if this ain't another critical bug, that should be fixed before releasing 1.0 next week!

The current behaviour is far from that what you would normally expect, when translating a node! And even, if you create the translation manually via admin/structure/menu/manage/MENUNAME, the wrong translation is loaded on the node edit page (always the default) and therefore the wrong one gets saved. This is definitely a huge problem!

Regarding the patch from #8

There are still several things missing here:

First scenario: if you create a node in german with a menu link and NOT manually translate the menu link, then translate the node to english, then there's no existing menu link loaded on the node edit page - which is not per se wrong. But if you now check the "Provide a menu link" box, provide a menu link and save the node, then not the original menu link gets translated, but a second one created instead. This is not desirable, especially as the menu system will always show both links, no matter if they are translated or not. Instead the existing menu item should be translated to english.

Second scenario: create a node in german with a menu link, but now translate the menu item manually via admin/structure/menu/manage/MENUNAME, then go and translate the node to english. You will see the german text of the menu link, as well as the german one gets overriden after save!

What we need

  • Always load the menu item in the correct language, if already exists - if not, I think you should still display the menu item in default language
  • On save: if there's already a translation for the current language, make sure, that this translation gets saved, not the original language
  • On save: if there isn't already an existing translation, then of course also do not override the default language, but instead create a new translation of the menu item!

The attached patch only covers the first aspect and needs to be extended properly. This is the quickfix I applied today to a current project. I first thought, that this would cover the first two aspects, but it doesn't. It only loads the correct translation, if existing. However, it always saves the original version and never creates a new one. So it's even worse than the current situation because it seems that you edit the correct language, but you don't really do,... But it's at least another starting point to solve this problem. Unfortunately I can't provide more help today, as I'm running out of time. So maybe someone can finish this work. If not, I hopefully find more time within the next two days...

agoradesign’s picture

Status: Needs review » Needs work

forgot to change the status...

agoradesign’s picture

Status: Needs work » Needs review
FileSize
1.7 KB

Ok, as this is very important to me, I took another quick look onto it, and it was stunningly easy to fix this problem. Please review!

agoradesign’s picture

FileSize
1.44 KB

Same as #12, but without the unwanted deletion of the new line at the end of the file...

agoradesign’s picture

Issue tags: +D8MI, +language-ui, +multilingual

Added some appropriate tags to this issue

Gábor Hojtsy’s picture

Issue tags: +sprint
Gábor Hojtsy’s picture

Issue tags: +Needs tests

Thanks for working on this! Would be important to spell out the use cases expected to be supported in tests. That would also ensure all works and keep working as expected.

agoradesign’s picture

You're right. I planned to provide some, but didn't found time to do so...

Thomas Factory’s picture

I see the same problem in a German/French website on 8.0.1.

The menu-link of the original language is overwritten with the menu-link of the translation.

This very confusing and very difficult if not impossible to handle for unexperienced users.

reekris’s picture

@agoradesign Thank you for this, just was I was looking for and your patch is working great! Shouldn't this be merged into core since the current behaviour seems very broken?

agoradesign’s picture

@reekris: you're basically right, but there are very strict rules regarding what code is committed to core. One of the goals/rules is that changes and fixes like this always come with a appropriate unit tests, on the one hand to verify that the problem existed without patch and is gone with patch. On the other hand to assure that the same bug will not be introduced again by a future code change.

So, in other words, code quality goes before speed! That is very good and important and is one part of the rock-solid foundation and quality Drupal has and we all love. But sometimes it can drastically slow down the process of bringing such tiny, but very important fixes into the code base.

One thing we could do, is to bump the issue priority to a hígher level. Maybe we should ask Gábor, if he agrees to do so..

On the other side, one of us (including me) could just write appropriate tests, then we could get this committed quite quickly. I have to admit, that I planned to do this long time before, but I got stuck in daily business like the most of us, and especially in the last couple of weeks this problem is kind of out of sight for me, as it neither concerns a current project, nor the patch entry in the make files is too disturbing for me...

Without being able to promise anything - if today I am able to progress well in my current project at work, I may find time in the afternoon to worry about the tests... Anyway, I'll report back!

esolitos’s picture

Title: When I translate node, the translated menu link item overwrite the original link » Multilingual content: Menu link is not correctly stored on translation.

I fixed the title to be more understandable on search.

agoradesign’s picture

Thanks @esolitos.

Ok, I don't have much time now/today, but it should be enough to write the necessary test(s)...

agoradesign’s picture

Finally, here are the tests! Hopefully we can get this committed soon now :-))

The last submitted patch, 23: 2599594-23-tests.patch, failed testing.

nagwani’s picture

Looked at this with @xjm. The patch applies cleanly on 8.0.x but doesn't apply on 8.1.x

nagwani’s picture

The patch in #23 does not seem to fix the issue. Steps followed as mentioned in the summary. Tested on 8.0.x branch.

  1. Enable the translation for a content type
  2. Enable translation for menu links
  3. Create a node in French and on the node creation form, click "Provide a menu link" and add a menu link
  4. Translate this node and translate the menu link
imkartik’s picture

Assigned: Unassigned » imkartik

Looking into this..

imkartik’s picture

Assigned: imkartik » Unassigned
Status: Needs review » Needs work

Changing the status..

agoradesign’s picture

The comment in #26 surprises me - we are using this patch already successfully in 3 multilingual sites, without having any problem + the unit test shows that the problem disappears with the patch.

Just tried it again on a local install - works like expected here too

xjm’s picture

(Updating issue credit for the major triage.)

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

Without enabling translation for menu links, it does indeed not work well, but if translation is enabled for menu links, it works flawlessly. I just tried on a test site with all the steps to reproduce.

There is no information on the UI in that case, that the menu change applies to all languages, but there was none earlier either, so this is not a regression and not caused by this patch. I would vote to get this in as this is one of the biggest outstanding multilingual content creation bugs and would make menu item creation for translations work finally as expected.

agoradesign’s picture

That's good news :-)

Now, that this issue gained momentum and correctness of the fix is acknowledged, I'll set up a 8.1.x installation on a local machine and create a patch for that branch - because nagwani mentioned in #25, that it won't apply on 8.1.x

agoradesign’s picture

And finally, here's the patch for the 8.1.x branch...

catch’s picture

Re-uploading that for a test run. Haven't reviewed the patch yet.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: 2599594-33-complete-8.1.x.patch, failed testing.

agoradesign’s picture

@catch: the 8.1.x version differs from 8.0.x. so either change the version number of the issue as well or leave the "do-not-test". But this way it can't work ;)))

If the version of the issue should be changed to 8.1.x, it would be very interesting for me, if someone could clarify the official way on that, to be more efficient the next time. I had this part of the guide in mind, where it is stated to not modify the issue's version: https://www.drupal.org/node/332678#versions

And if, who is supposed/allowed to change the issue number? Everyone or only core contributors? etc

Gábor Hojtsy’s picture

Version: 8.0.x-dev » 8.1.x-dev
Gábor Hojtsy’s picture

Added another test now that it is on 8.1.x

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

Looks good on 8.1 too :)

  • catch committed 02511dd on 8.1.x
    Issue #2599594 by agoradesign, mdespeuilles, nagwani: Multilingual...
catch’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Reviewed & tested by the community » Fixed

OK. Committed/pushed to 8.1.x and 8.0.x

I added review credits for several people (issue credit, left commit message as is), let me know if I missed anyone.

  • catch committed 3666d79 on 8.0.x
    Issue #2599594 by agoradesign, mdespeuilles, nagwani: Multilingual...
Gábor Hojtsy’s picture

Yay, amazing, thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Status: Fixed » Closed (fixed)

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

rgpublic’s picture

Version: 8.0.x-dev » 9.x-dev

Hmmmm. *Exactly* what's described in this bug happens to me even though this bug is supposed to be fixed. I'm on 8.1.3 and a quick glance at the code shows I've got the lines patched that are supposed to be patched. What can I do?

rgpublic’s picture

PS: Through further investigation I found out that my issue doesn't seem to be in menu_ui, because if I insert a "return" in the first line of _menu_ui_node_save (which is the only method the proposed patch above changes), then the menu item link title still changes in both languages if I save the translated node (original language is destroyed). I found out that the culrrit seems to be token_node_menu_link_submit in token.module. If I insert a return in the first line of this method the problem disappears. Don't know what the right fix is though. I think this is a pretty serious issue, because due to this one cannot edit translated pages anymore without destroying the menu of the original language.

Gábor Hojtsy’s picture

Version: 9.x-dev » 8.0.0
criscom’s picture

Also for me on Drupal 8.1.7 this problem still persists. I also have the token module (8.x-1.0-alpha2+15-dev) enabled.

criscom’s picture

Status: Closed (fixed) » Needs work

Or should we post this problem in the token issue queue?

Gábor Hojtsy’s picture

Status: Needs work » Closed (fixed)

@criscom: this issue already has a patch committed. Please open another issues and link it from here. Also please provide exact reproduction steps that we can verify.

criscom’s picture

Steps to reproduce:

  1. Install and enable token module
  2. Enable the translation for a content type
  3. Enable translation for menu links
  4. Set German as default language
  5. Create a node in French and on the node creation form, click "Provide a menu link" and add a menu link
  6. Translate this node and translate the menu link

The menu link of the default language, German, will be overwritten by the changes made to the menu link on the French translation.

If you change the menu link of the default language, German, back to what it was before, the menu link on the French translation will not be changed. But if you edit the French translation and save the changes, the menu link of the French translation will overwrite the German menu link.

In order to fix above issue:

1. Uninstall token module

criscom’s picture

Posted to the token issue queue: https://www.drupal.org/node/2769705
@Gábor: Thanks for the heads-up.

criscom’s picture

Gábor Hojtsy’s picture

@criscom: right, that sounds like not related to this issue, since this did not involve needing the token module at all and also had different symptoms in terms of what could / could not be translated.