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
- Enable the translation for a content type
- Enable translation for menu links
- Create a node in French and on the node creation form, click "Provide a menu link" and add a menu link
- 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.
Comment | File | Size | Author |
---|---|---|---|
#34 | 2599594-33-complete-8.1.x.patch | 6.91 KB | catch |
#33 | 2599594-33-complete-8.1.x-do-not-test.patch | 6.91 KB | agoradesign |
#23 | 2599594-23-complete.patch | 6.92 KB | agoradesign |
#23 | 2599594-23-tests.patch | 5.48 KB | agoradesign |
Comments
Comment #2
mdespeuilles CreditAttribution: mdespeuilles commentedComment #3
iMiksuLets see if I can write test for this.
Comment #4
kekkisThe 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.Comment #5
iMiksuI 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.
Comment #6
iMiksuIt seems somehow that we should probably create a test alongside with
\Drupal\menu_link_content\Tests\MenuLinkContentTranslationUITest::doTestTranslationEdit
?Comment #7
kekkisAt 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.Comment #8
mdespeuilles CreditAttribution: mdespeuilles commentedAfter 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..
Comment #9
mdespeuilles CreditAttribution: mdespeuilles commentedComment #10
agoradesign CreditAttribution: agoradesign commentedI'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
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...
Comment #11
agoradesign CreditAttribution: agoradesign commentedforgot to change the status...
Comment #12
agoradesign CreditAttribution: agoradesign commentedOk, 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!
Comment #13
agoradesign CreditAttribution: agoradesign commentedSame as #12, but without the unwanted deletion of the new line at the end of the file...
Comment #14
agoradesign CreditAttribution: agoradesign commentedAdded some appropriate tags to this issue
Comment #15
Gábor HojtsyComment #16
Gábor HojtsyThanks 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.
Comment #17
agoradesign CreditAttribution: agoradesign commentedYou're right. I planned to provide some, but didn't found time to do so...
Comment #18
Ralf Eisler CreditAttribution: Ralf Eisler commentedI 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.
Comment #19
reekris CreditAttribution: reekris commented@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?
Comment #20
agoradesign CreditAttribution: agoradesign commented@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!
Comment #21
esolitosI fixed the title to be more understandable on search.
Comment #22
agoradesign CreditAttribution: agoradesign commentedThanks @esolitos.
Ok, I don't have much time now/today, but it should be enough to write the necessary test(s)...
Comment #23
agoradesign CreditAttribution: agoradesign commentedFinally, here are the tests! Hopefully we can get this committed soon now :-))
Comment #25
nagwani CreditAttribution: nagwani at Acquia commentedLooked at this with @xjm. The patch applies cleanly on 8.0.x but doesn't apply on 8.1.x
Comment #26
nagwani CreditAttribution: nagwani at Acquia commentedThe patch in #23 does not seem to fix the issue. Steps followed as mentioned in the summary. Tested on 8.0.x branch.
Comment #27
imkartik CreditAttribution: imkartik at TATA Consultancy Services for Pfizer, Inc. commentedLooking into this..
Comment #28
imkartik CreditAttribution: imkartik at TATA Consultancy Services for Pfizer, Inc. commentedChanging the status..
Comment #29
agoradesign CreditAttribution: agoradesign commentedThe 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
Comment #30
xjm(Updating issue credit for the major triage.)
Comment #31
Gábor HojtsyWithout 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.
Comment #32
agoradesign CreditAttribution: agoradesign commentedThat'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
Comment #33
agoradesign CreditAttribution: agoradesign commentedAnd finally, here's the patch for the 8.1.x branch...
Comment #34
catchRe-uploading that for a test run. Haven't reviewed the patch yet.
Comment #36
agoradesign CreditAttribution: agoradesign commented@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
Comment #37
Gábor HojtsyComment #38
Gábor HojtsyAdded another test now that it is on 8.1.x
Comment #39
Gábor HojtsyLooks good on 8.1 too :)
Comment #41
catchOK. 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.
Comment #43
Gábor HojtsyYay, amazing, thanks!
Comment #44
Gábor HojtsyComment #46
rgpublicHmmmm. *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?
Comment #47
rgpublicPS: 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.
Comment #48
Gábor HojtsyComment #49
criscomAlso 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.
Comment #50
criscomOr should we post this problem in the token issue queue?
Comment #51
Gábor Hojtsy@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.
Comment #52
criscomSteps to reproduce:
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
Comment #53
criscomPosted to the token issue queue: https://www.drupal.org/node/2769705
@Gábor: Thanks for the heads-up.
Comment #54
criscomComment #55
Gábor Hojtsy@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.
Comment #56
Brauny CreditAttribution: Brauny commentedI'm seeing this in 8.9.12
BOTH translations (the default and the French translation) now have the French translation's menu link.
Comment #57
tudor.vlas CreditAttribution: tudor.vlas as a volunteer commentedStill an issue in Drupal Core 8.9.12
Same steps as described by @Brauny
Comment #58
Brauny CreditAttribution: Brauny commentedPer Gábor Hojtsy suggestion, I created a new issue for this here: https://www.drupal.org/project/drupal/issues/3197294