Issues such as
#1905268: Disabled menu items
#1693074: Change menu translation approach (i18n_menu_translated_menu_link_alter() skips item subtree processing)
#1230034: Menu link disappears when publishing a node by the moderation dropdown or by the moderation view link
#1451336: Reverting a node revision deletes menu item
#1534356: Data loss - menu items unintentionally delete themselves if a node is updated via code (not via form UI) . Test case attached.
#1947340: Expire menus incompatible with Panelizer
#1247506: menu link deleted on programmatically updated nodes
#1245094: Node menu link deleted on update
are appearing because third party modules implementing this hook seem to expect it only to run upon menu link display, whereas it also runs upon saving a menu link through the node edit form. The documentation says this hook should only affect display and not loading/saving, and upon reviewing several contrib modules which use this hook, none of them seem to expect this behavior: http://drupalcontrib.org/api/drupal/drupal!modules!system!system.api.php...
This means the menu item is being altered in unpredictable ways (such as being set to "hidden"/disabled when the menu item had been explicitly enabled/"not hidden" before).
When loading the node edit form, in menu_node_prepare, menu_link_load is being called to load the existing menu link into the node object to be saved in $form_state. From there, in _menu_link_translate, hook_translated_menu_link_alter() is invoked for the existing menu link for the node, meaning the link can be unpredictably altered while it is being saved into $node->menu.
This same (altered) menu item is later saved in menu_link_save() (called from menu_node_save() when the form with the altered menu link in the $form_state is submitted).
1. Documenting the current behavior and reviewing contrib modules invoking this hook, submitting fixes to the separate modules.
or
2. Not running hook_translated_menu_link_alter() if it's being invoked from the node edit page.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 2136815-16.patch | 6.43 KB | manuel garcia |
| #13 | 2136815-13.patch | 6.02 KB | stefan.r |
| #5 | drupal-do-not-invoke-hook_translated_menu-on-node-edit-2136815-3.patch | 668 bytes | stefan.r |
Comments
Comment #1
stefan.r commentedComment #2
stefan.r commentedComment #3
stefan.r commentedThe attached patch would be an API change but on the other hand it seems like the current behavior upon node edit is unexpected and undocumented.
Comment #4
stefan.r commentedComment #5
stefan.r commentedThis should skip the menu link being edited...
Comment #6
stefan.r commentedComment #7
stefan.r commentedComment #8
kopeboyPlease review this, I'm tired of wasting days on duplicate translated or not translating menu links!
Comment #9
develcuy commentedI'm the maintainer of Menu Token module, this patch will help 16k sites using Menu Token to prevent duplicated menu items or just broken translations. Thanks in advance!
Comment #10
develcuy commentedComment #12
David_Rothstein commentedChecking for the node/X/edit URL seems a little iffy to me... What if there's a link to the node being displayed somewhere else on that page, but not being edited?
Also, aren't there other ways to reproduce this => basically any time you call menu_link_load() followed by menu_link_save()?
Perhaps we need a new function, something named along the lines of menu_link_load_untranslated(), and the idea is you would call that function instead of menu_link_load() if your code is loading a menu link for the purpose of editing it rather than displaying it?
Comment #13
stefan.r commented@David_Rothstein yes I like that idea.
This is not only a problem on menu link save, it's also a problem in the menu parent selector (and likely all other widgets that display menu links). See #2529360: i18n_menu erroneously marks some menu links as disabled on parent link selector in node edit page. This patch passes along a parameter for those cases as well.
If we think this is the right way to go I can build a test using a test module with a hook_translated_menu_link_alter() invocation to confirm this fixes the issue.
Comment #15
stefan.r commentedAnyone willing to have a look at those test failures?
Comment #16
manuel garcia commentedReroll, small conflict on includes/menu.inc