Original issue
(Fix committed as of #15)
If a i18n menu item is translated using i18n_menu_translated_menu_link_alter()
the flag $item['i18n_menu'] = TRUE;
is set to prevent double localization.
Later if the item is passed to i18n_menu_localize_tree()
the function checks if the flag is set and skips the whole processing if so. (See #360 in i18n_menu.module
Unfortunately this also skips the localization of the subtree if one is present.
I suggest to move the subtree processing out of the condition and add a continue;
for the case the whole item has to be removed.
Attached patch does that and seems to work just fine.
This issue is related to following 6.x issue: #966534: Customized subtree and i18nmenu_localize_tree()
Issues with the fix
- Menu links can become disabled while editing a node (comments #17 and from #24 on)
Comments
Comment #1
Jose Reyero CreditAttribution: Jose Reyero commentedUm, interesting. Yes, the patch makes sense.
However, I was thinking whether we could forget about that 'localize tree' and do all the localization / hiding of menu items on hook_menu_translated_link_alter(), which only needs menu items to be set previously with 'options['alter'] = TRUE.
Please take a look at this other patch.
Comment #3
das-peter CreditAttribution: das-peter commentedSounds reasonable - in fact I already used a similar hack over the last year to support our entity translation integration.
But I decided to upgrade all the modules now ;)
I gave the patch a shot and it seems to work. However I don't fully get the approach yet.
Is the goal of the patch is to make
i18n_menu_localize_tree()
obsolete?If so we aren't quite there because currently only
link_title
is translated -title
isn't.Further it seems like the condition
should use
!empty($item['language'])
I changed all those points in the attached patch.
Another change I made is related to the issue reported by the tests. Since
hook_menu_link_alter
is fired inmenu_link_save
before the defaults are set there are cases in which no module is defined. I guess it's sane to assume that if no module is defined the item should be handled as if "menu" is set as module.Comment #4
Jose Reyero CreditAttribution: Jose Reyero commentedI've started with your patch and ended up doing some partial rework, see new patch which I think will fix other related issues, improve performance (by properly handling menu item strings) and is more understandable than the old code.
Some more cleanup may be possible, though functions like i18n_menu_localize_tree(), not needed anymore by i18n_menu must be kept (though maybe it could be empty) as other modules are using it.
Now all the processing is done in hook_translated_menu_link_alter(), no need for replacing block menus anymore or primary and secondary links, etc... so I guess this should be some performance improvement + a serious logic cleanup.
As the patch is getting big, if you think this works, I'll commit it right away so we can focus on other minor improvements while it gets some testing in the dev version.
Comment #5
das-peter CreditAttribution: das-peter commentedAs discussed in Munich I think it's a good idea to use
hook_translated_menu_link_alter()
instead the other approach.In fact I've already used a similar hack for over a year and it worked like a charm - but of course I just covered a limited range of scenarios.
If you like I give the changes a try in our existing installations and provide feedback - but for that I need a patch :)
Comment #6
Jose Reyero CreditAttribution: Jose Reyero commentedOoops, sorry, forgot the patch again :-(
Comment #7
das-peter CreditAttribution: das-peter commentedThanks for the patch :)
I've tested it on one of our installations, looks very good to me.
The only issue I had was related to the update hook, I've fixed that.
Further I fixed some coding standard issues in the already changed code parts.
Comment #8
Jose Reyero CreditAttribution: Jose Reyero commented@das-peter,
Your changes look good to me, thanks for the code cleanup.
I think this is RTBC but since that's a major rework, I'd like to give it some time in the -dev version to get some testing.
So @das-peter, @webflo, wondering whether we should commit this or put another stable release out first, in case there are other fixed issues we are waiting for.
Comment #9
das-peter CreditAttribution: das-peter commentedSounds like a good idea to give it some time in the -dev version.
Comment #10
Jose Reyero CreditAttribution: Jose Reyero commentedI wish we could have done another maintenance release in the meanwhile but it seems we were all too busy with Drupal 8 in the lat weeks.
Anyway, this has been delayed long enough, so it is to be committed ASAP and we'll do a new release by the end of the year.
Comment #11
Jose Reyero CreditAttribution: Jose Reyero commentedCommitted.
Wondering whether we should add some change notice, as this should get all menu items translated cleanly without the need of any other modules (i18n_contrib/i18n_menu_select).
Comment #13
Jose Reyero CreditAttribution: Jose Reyero commentedI've just noticed items in other languages than current show up "disabled" in the menu edit pages, we need to fix it before release.
Comment #14
Jose Reyero CreditAttribution: Jose Reyero commentedFixed with this patch.
Comment #15
Jose Reyero CreditAttribution: Jose Reyero commentedIt was not committed, done now.
Comment #16
plachRelated issue: #1889084: Menu translation tests broken.
Comment #17
Jose Reyero CreditAttribution: Jose Reyero commentedThis may need some more work.
From https://drupal.org/node/1800450#comment-6938800
Comment #18
BerdirUhm. This completely broke the menu localization. Trying to view the translate tab of a menu results in a fatal error which has been reported here #1889878: Call to a member function format_translation() on a non-object in i18n_string.pages.inc on line 80, git bisect points to this issue :)
The problem is that "$strings = $object->get_strings(array('empty' => TRUE));" does return no strings and then the UI explodes.
Comment #19
Jose Reyero CreditAttribution: Jose Reyero commentedThe solution to this latest error is on the other issue, linked from #18
Comment #20
soncco CreditAttribution: soncco commentedAfter this update, all menus for all languages are displaying in the same place , the translation is not respected. The same problem here #1888448: Menu translation broken after upgrading to i18n 7.x-1.8 and variable 7.x-2.2.
Comment #21
bvanmeurs CreditAttribution: bvanmeurs commentedSame here soncco.
The original issue https://drupal.org/node/1726906 has not been fixed. After updating to i18n-7.18 I still get all menu items.
Comment #22
BerdirAnother related issue I think: #1897696: Inconsistent check for translatable title in _i18n_menu_link_localizable_properties() and _i18n_menu_link_localize().
Comment #23
drzraf CreditAttribution: drzraf commentedafter the update, speaking about the
main-menu
which appear in every page (including administration), all menu items show up in themain-menu
when browsing at/admin/structure/menu/manage/*
.This is related to the hunk provided in comment #14 which was part of the commit.
Comment #24
agnese.stelce CreditAttribution: agnese.stelce commentedProblem mentioned in #13 still persists (disabled menu items). One example here - http://drupal.org/node/1905268#comment-7013904. I encountered with this problem also on node edit form trying to edit node in different language then current. After node save it always leads to disabled menu item for that node.
Comment #25
raino CreditAttribution: raino commentedDisabled menu items problem persists when edit node in different language then current. (translate node).
After node save the menu item is disabled.
Comment #26
martins.bertins CreditAttribution: martins.bertins commentedHere's a patch I created.
Comment #27
cdnsteve CreditAttribution: cdnsteve commentedThis seems to be the same patch on another issue.
It doesn't work for language neutral, see comments here:
http://drupal.org/node/1905268#comment-7257360
Comment #28
GiorgosKI think people should at least be warned right on the project page about this strange bug since this issue is becoming stale but bug still persists
Comment #29
GiorgosKI can't understand the logic behind patch in #26 and it actually does not work so reverted back to needs work
And because this can happen to any of at least 65K live sites and might go undetected or completelly HIDE a main menu or have the webmaster wondering what has gone wrong I am marking this major to get the attention of the maintainers
but feel free to change if I am wrong
I was trying to debug i18n_menu myself but I am not an expert on it and I got completelly lost
so instead of going back to 7.17 I created a TOTALLY HACKISH workaround
The premise of this solution is that when an entity is edited/saved (entity_presave) user can't have access to the HIDDEN option. Hidden is only available on the menu administration page. So I look up the previous value of HIDDEN and replace it.
Please use at your own risk
Comment #30
richsky CreditAttribution: richsky commentedHello, here are my two cents.
Let say I have a custom module providing a link to main menu. It can be localized if it stays in its defaults state. If I customize it, I can't get translation anymore. I know customized links go in menu_links, but I don't get why it doesn't show up in string translation anymore and doesn't show up translated.
This is quite anoying.
Comment #31
GiorgosKis this issue related ?
#1905268: Disabled menu items
Comment #32
mErilainen CreditAttribution: mErilainen commentedI'm not sure if this is the right approach, but at least it doesn't use unsafe database query, and seems to work as it should. I don't think we should display the menu items depending on the interface language, because the node language will determine in which menu it can be placed.
Comment #33
trevortwining CreditAttribution: trevortwining commentedPatch works as advertised. Thanks a bunch!
And yes, the issue in #31 is related AFAIK.
Comment #34
http_teapot CreditAttribution: http_teapot commentedPatch works, thanks!
Comment #35
stefan.r CreditAttribution: stefan.r commented#32 may not be the right approach as menu links are still considered part of the interface. A further issue with #26 and #32 is that they can change a menu item to be disabled if the language of the menu item doesn't match the node language, when it had been explicitly set to be enabled before.
I attached a patch which makes us not process the menu item at all if we're on the node edit page. This is the same approach we take for the menu item admin screen.
Possibly related: #2136815: hook_translated_menu_link_alter also runs upon editing nodes with menu links, not just upon menu link display
Comment #36
Scott Robertson CreditAttribution: Scott Robertson commented#35 did not work for me at all, even after changing arg(1) == 'edit' to arg(2) == 'edit' in the patch.
#32 Seems to be working for me thus far, however all of my non-English menu items still have (disabled) next to them in select lists.
Comment #37
joseph.olstadremoving my own comment
Comment #38
stefan.r CreditAttribution: stefan.r commented#36, it should indeed be
arg(2)
but what exactly is not working? How can I reproduce the failure? What this patch should fix is the menu links becoming disabled upon saving a node which is in a different language than the interface.Revised patch + test attached.
Comment #40
Scott Robertson CreditAttribution: Scott Robertson commentedI tested #38 on a fresh install of Drupal and can confirm that menu items no longer become disabled, so I believe a contrib module was conflicting with the patch on my previous test.
Comment #41
stefan.r CreditAttribution: stefan.r commentedComment #42
JonesUI CreditAttribution: JonesUI commentedI'm still experiencing the disabled menu bug in version 7.x-1.10 so I assume that a fix has never been applied?
Comment #43
weri CreditAttribution: weri commented@mordaga no, the fix get applied when the patch is tested by the community. It would be great when you apply this patch and give some feedback when the patch works or not. Thanks!
Comment #44
JonesUI CreditAttribution: JonesUI commentedI tried to apply the patch but my patching app (OS X command line) didn't like the patches (none of the ones I tried from above). I tried with version 7.x-1.8 (release) because that's what is associated with this issue but no beuno.
Should I be patching against a different version?
Comment #45
Scott Robertson CreditAttribution: Scott Robertson commentedHi mordaga,
You'll want to try apply the patch to the 7.x-dev branch, as patches are always written for the latest dev version, even if the issue is reported for the current release.
Comment #46
GuillaumeDuveauJust applied #38 with success, and it fixes the menu links becoming disabled upon saving a node which is in a different language than the interface.
Comment #47
stefan.r CreditAttribution: stefan.r commentedmordaga, this patch should run against 1.10 as well
Comment #48
GuillaumeDuveau"Reviewed and tested by the community" seems OK, I guess...
Comment #49
AnybodyThe patch works great! Who takes it into the latest dev?
Comment #50
Jose Reyero CreditAttribution: Jose Reyero commentedCommitted, thanks
Comment #52
Anonymous (not verified) CreditAttribution: Anonymous commentedHello people, this code makes problems:
See https://drupal.org/node/2250501
Can we change this lines?
Comment #53
stefan.r CreditAttribution: stefan.r commentedComment #54
stefan.r CreditAttribution: stefan.r commentedSince this had already been marked as closed a while ago let's link this back to the relevant issue and continue discussion there :)
#2250501: Node edit page shows menu item (for node being edited) in all languages
Comment #57
stefan.r CreditAttribution: stefan.r commented@Nemanja the reason that patch fails testing is because it has already been committed
Comment #58
mErilainen CreditAttribution: mErilainen commentedWhat has actually changed? I can still see all the menu items in different languages, using 7.11.
Comment #59
stefan.r CreditAttribution: stefan.r commented@mErlianen: likely you only have this issue on the page itself anymore. Have a look at #2250501: Node edit page shows menu item (for node being edited) in all languages for reference.