Localize menu items by node translation set

For 1) shouldn't this be part of the i18nsync module. Could the i18nsync module keep all the menu weights the same for menus on translated nodes as the menu on the original node. The i18nsync module could also keep the parent menus in sync so that the whole menu structure for the translated content is the same as for the original content.

There would need to be a setting somewhere so this could be enabled or disabled as there would probably be situations where you want the menu structure different in other languages.

We do something similar to this in a module we are developing for assisting in translations. http://drupal.org/project/icanlocalize

Thanks for your submission, nedjo. This is right one of the things that irritated me (and doesn't look like correct localization if you know the ui localization workflows.. you much more ask "where can i enter the localized text for this menu entry?" instead of adding it a second time).

The whole menu handling in a bit strange. Menu points disappear (in language A) if they refer to nodes of language B. That's fine. But why are those language node properties not getting propagated to the menu item? (the menu item shows still "all language" when editing properties..)
And finally i don't understand how the language setting per menu are getting used at all. No matter what i choose there a menu item is still visible in different languages (even if the menu item refers to e.g. a module page). Why is there no column for language settings in menu item overview such as "manage content" has? I'd expect these two mechanisms to work identically in general - but they don't.

If we'd have a single (and much more simple) workflow of how things are getting localized, one might understand the whole picture much sooner.

At this moment I'm working in a module to synchronize menus. Hope I get it soon.

subscribe

I wrote a tiny module to achieve the goal. Basically it implements a menu call back "localized/node/%", and you can put that in the menu path. When menu item is built but before rendering I use another hook to find the translation source and the translated copy in current language, then alter the href.

It works fine for me, but i think it's too small to put into another project, maybe someone can test it, or even integrate to the i18n module?

We have just added a similar feature to our module: http://drupal.org/project/icanlocalize

This relies on setting up a menu for each language according to http://drupal.org/node/313302

When this is done, our module will then keep any translated menus in sync with any changes to the original language menu. It will keep the menu order, hierarchy, enabled state and expanded state in sync. It includes options for selecting what to sync when you save changes to the original language menu.

Hi,

Managed to write a module to enable menu translations synchronization. It's roughly done and needs lots of testing and reviewing. I'll try to clean up the code, but first I want to check the module mentioned in #7 and see if it is worth working in this module. At the same time TESTING is needed. PLEASE TRY IT OUT and give me your feedback.

What's missing:
No tests have been made with three or more languages, although the code is written to support them.
Add 'translate' links inside the menu page and a language filter as well.
Remove the translated menu from the parent select box.

How to use:
admin/build/menu/i18nsettings: select which items to synchronize.
admin/build/menu/item/{menu item id}/translate: menu item translation table

Menu items assigned through node edit/add form are synchronized with the menus belonging to the node's translations.

There are lot of issues and stuff to mention about this module. Please review this approach.

I am attaching a patch that implements option (2) from the original post.

Assume you have node/1 in en, and node/2 is its translation to es. If you have a menu item pointing at 'node/1' - when you view the site in es, it will switch to 'node/2'.

This is different from the above sync approaches, by using single menu item, and just modifying it as needed.

The attached patch implements #2 from the OP.

It provides a checkbox for each menu that allows to enable node translation selectively. By default it's unchecked to preserve BC.

The approach implemented is the following: on menu item save if we have a node link its translation set is saved among the item link options. On menu rendering the options are checked to see if there is a translation available in the translation set for the current language. If a suitable translation is found its nid is replaced in the item's href.

Serveral checks are implemented to ensure that the menu item's translation cache is always up-to-date.

the patch :)

This is a version for i18n 6.x-1.4

Attached you can find a revised version (dev and 1.4) which addresses a couple of issues with the menu form item in the node form and with access control.

@Jose Reyero:
Please, let me know if you are interested in including this feature in i18nmenu as it can easily moved to a dedicated project otherwise :)

plach i'll review your suggestions asap.
meanwhile i'm also i18n co-maintainer and try to push such improvements whenever they make sense and don't conflict with special use-cases. (i need to check your solution in deep detail on my own to be sure)
if jose takes the ball i'd also be fine for sure!

@miro_dietiker:

Sure, take all the time you need. I was just wishing to know if this had any chance to get into i18n (possibly in the next release :)).

I've skimmed through the code.
Found inputs like:

• Using a subselect in i18nmenu_get_node_translations is pretty expensive. Why don't we switch menu links to tnid links instead of nid links? (on save, hook_update) this might reduce complexity of code
• why loading all translations for menu alter? we could limit for current language only. Is there a real need? (is this e.g. needed to have a working menu cache?)
• in i18nmenu_get_node_translations setting $key to FALSE is pretty weird.. we should follow std drupal patterns. In case of $key=FALSE you're even executing an SQL query without any sense.
• additionally check for is_numeric instead of isset($map[1]) only, and make sure there's no further param.

Then some more questions like

• Can you explain me, what actually the language of a menu item (not the language of the node) will do with the current code?
• Do you have any idea what happens with a menu item if the node with the translation source gets deleted?
• So we finally have only one menu item for a tnid? Will the translation happen with i18nstrings, right?
• What happens if a user updates the menu entry in the node edit in a specific language? will it update the translation or will it update the base menu entry?
• If this is performant, why do you even ask a user to enable this functionality? unneeded configurations should be avoided to reduce complexity.

This code contains some complexity and some hook implementations are not documented yet.

• in translated_menu_link_alter you should e.g. explain the triviality like "replace the link with node translation of current language".

About this, the idea is ok though the code looks a bit complex to me. To add to the questions above:

- Are we actually enforcing node access for all translations?
- Wouldn't some options like syncing module weights make more sense in i18nsync?. Would it work for translations on different menus?
- I'd like to explore other options, like icanlocalize solution (#1), they're publishing some new modules atm, so before adding any new feature let's see whether there's already one there,
- It is a bit restrictive if it requires setting language negotiation to 'All content'. I'd rather have a global option that would also disable language conditions for all menu queries (as it impacts all menus it should be a global option).
- Some explanation text would be good for the node's menu options like 'This will apply to all the translations too'

For next release, that will be ASAP, we are focusing on some important bug fixes, so no new features for that one. See #795038: Putting out a new 6.x-1.5 release.

One more thing, it would be great if the menu item was the 'translation set' instead of the node, so we could also support a common child item for all languages like in this case, #810952: Workaround menu item duplication for translated node

I have tried the patch from #13, it works for me.

My comments :
-i don't like to have the obligation to set multilingual without negociation.
Have you a solution to guard content selection?
-i really appreciate the system which disabled the node menu system in translated content.

Great patch which must be integrate in the i18n core.

In the attached patch I tried to implement the suggestion from #16-#18 to use the tnid in the menu item data structure. To address #18 and more generally support active trails the patch introduces a hacky technique borrowed from Menu Trails (see i18nmenu_nodeapi($node, 'view')).

@miro_dietiker:

why loading all translations for menu alter? we could limit for current language only. Is there a real need? (is this e.g. needed to have a working menu cache?)

Yes, the idea is that when saving a menu link item we cache all of its translations so we don't need db queries when actually displaying translated menu items in i18nmenu_translated_menu_link_alter().

in i18nmenu_get_node_translations setting $key to FALSE is pretty weird.. we should follow std drupal patterns. In case of $key=FALSE you're even executing an SQL query without any sense.

I don't get what you mean: $key is never used in the query. Renamed it to $field, anyway.

additionally check for is_numeric instead of isset($map[1]) only, and make sure there's no further param.

Ok for the type check but additional parameters are supported through the following implode() call, this way we can support paths like 'node/1/edit'.

Can you explain me, what actually the language of a menu item (not the language of the node) will do with the current code?

When we have language-specific menu items node translation simply does not kick in.

Do you have any idea what happens with a menu item if the node with the translation source gets deleted?

The menu item gets deleted too and it does not appear in the menu anymore.

So we finally have only one menu item for a tnid? Will the translation happen with i18nstrings, right?

Yes. The item title translation keeps happening through i18nstring.

What happens if a user updates the menu entry in the node edit in a specific language? will it update the translation or will it update the base menu entry?

This cannot happen: the menu form items is not shown for node translations to avoid creating additional menu items.

If this is performant, why do you even ask a user to enable this functionality?

Mainly to support backward compatibility. Moreover one might wish to keep using the title-translation-only approach.

@Jose Reyero:

the idea is ok though the code looks a bit complex to me

Will you expand on this, please? I'll gladly streamline code if you provide me some guidance.

Are we actually enforcing node access for all translations?

Not exactly: we cannot afford running a complete node access check on every translation. The source translation (which is the node the menu item actually points to) gets a node_access('view')-like check in http://api.drupal.org/api/function/menu_tree_check_access/6. The remaining translations get only a check on their status (see i18nmenu_node_translation() for details).

Wouldn't some options like syncing module weights make more sense in i18nsync?. Would it work for translations on different menus?

Sorry, I can't understand these questions. Would you clarify please?

I'd like to explore other options, like icanlocalize solution (#1), they're publishing some new modules atm, so before adding any new feature let's see whether there's already one there,

We started developing this solution because in the project we are working right now (a large site fully translated in at least 5 languages) the client simply found unacceptable having to replicate the menu items for each language: the menu admin interface becomes unbearably cumbersome and unusable. Hence we are forced to go on with this solution, to have something ready for our deadline. I converted this patch in a standalone module we deployed on the site, however we will keep supporting this patch and we will remove the module if this gets committed.

It is a bit restrictive if it requires setting language negotiation to 'All content'. I'd rather have a global option that would also disable language conditions for all menu queries (as it impacts all menus it should be a global option).

The problem is with the check in http://api.drupal.org/api/function/menu_tree_check_access/6 : all the other options make the menu items disappear for some languages. I'm not sure how to limit i18n_db_rewrite_sql() to act only on non-menu queries: do you have any concrete idea to suggest?

Plach, sounds reasonable and still a way to go after having everything clarified. Thank you for all the updates.

For the menu entry translation:
So it doesn't seem possible to have the menu title editable for current language in the ui.
This means translation still occurs simply via locale interface only (and l10n_client)

Now here's another challenge:
I really can imagine that a page has a footer that needs tnid translation while the same page has a main menu which gets replaced per language. I've seen pages that allow to change language to different main pages via a main menu - not being the primary language switcher. Thus the tnid system needs to be inactivated for those primary menus (or entries).

I'm still not sure about my opinion on how to switch on/off this feature.
I might be more lucky if we could enable this function per menu item (such as a per menu item checkbox - "switch to translation from tnid if available") instead of the global per-menu option. Only if enabled the nid gets permuted with tnid.
This checkbox could also be changed in the node-edit-form as same as in the generic menu system edit item form.

Since node access is dependent the current resulting menu (depending user, languge, ...) - wouldn't it be better to provide a rendered menu object per language (which is access proof) instead of creating a monster amount that is not cleanly checked?

For the menu entry translation:
So it doesn't seem possible to have the menu title editable for current language in the ui.
This means translation still occurs simply via locale interface only (and l10n_client)

On node translations instead of hiding the menu widget, we could replace it with a "menu title" text field which would pass the string to tt().

Now here's another challenge: [...]
I might be more lucky if we could enable this function per menu item (such as a hook - "switch to translation") instead of the global option. Only if enabled the nid gets permuted with tnid.

This would put more burden on the user's shoulders, unless perhaps the current per-menu checkbox acted only as a default value for the proposed per-item checkboxes.

Since node access is dependent the current resulting menu (depending user, languge, ...) - wouldn't it be better to provide a rendered menu object per language (which is access proof) instead of creating a monster amount that is not cleanly checked?

I ain't sure about what you are proposing here: would you rephrase, please?

I would always enable the menu translation system by default (checkbox prechecked) if current node type has enabled translation. Well you're right. one option more all over...

Then having a translated menu title would be very very nice to the user!

We can ignore the last caching and node-access related paragraph.

Jose - what's your preferred level of configurability?

I would always enable the menu translation system by default (checkbox prechecked) if current node type has enabled translation. Well you're right. one option more all over...

I'm worried about backward compatibility: IMO the default menu status should be FALSE at least on updating. For fresh installs we might want to switch to TRUE as default based on the content type multilingual settings.

The attached patch implements #22-#23.

Now node translation is by default set to TRUE on new installations. To preserve backward compatibility the patch introduces an update function explictly setting node translation off by default for existing sites.

I tried to address also the content negotiation issue by making i18nmenu replicate menu blocks and temporary disable content negotiation while rendering the menu block.

Testing your patch #25 and wondering how this should work.
I have a node/1 in English and its translation node/2 in Dutch. Furthermore, I have a menu item, set to all languages, with the title Help linking to node/1. If I edit node/2, I see the translation in the menu section of the edit form. So that looks fine. If I switch to Dutch with the language switcher however, the menu item dissappears from the menu.

I guess I'm doing something wrong here. Can you please help so I can test drive your great efforts on this issue.

@narends:

If I switch to Dutch with the language switcher however, the menu item dissappears from the menu.
I guess I'm doing something wrong here.

You need to enable the [i18n] version of the menu block (you will find a "i18n" version of each menu block in the block configuration page). Otherwise you will need to set content language negotiation off.

The update function was wrong: the attached patch should fix it.

Sorry, just found a hunk in the previous patch which performs an unintended change.

is this patch agains the latest dev-version?

@stoltoguzzi:

yes, it should

Hey I've tested the patch and I found a problem.
I've made some menu items in the menu 'primary links'. Then I've translated the items from german to english. So far so good..
If I switch the language now, the translated menu items(english) won't appear.. only if I enable the 'i18n primary links block' or if I go under /admin/build/menu it will be correctly displayed

@NicoH:

see #27-#28

Can't seem to apply the patch from the directory. Am I missing something basic?

  Looks like a unified context diff.
  The next patch looks like a unified context diff.
  I can't seem to find a patch in there anywhere.

I've tested the whole complexity possible (all cases to be expected) with i18nmenu and found many issues with the current patch.

In node edit form
- only show in menu fieldset "Enable translation" if nodetype is translateable. also check nodetype settings in other cases.

In node edit form of a translated node (with enabled node translation)
- title changes to the translated menu item don't update it.

In a node edit form if a translated node (with disabled node translation in source node)
- "Enable translations" is present and does strange things. Note this should either be removed or it should act as tnid node menu translation enable.

In admin/build/menu when editing a menu item
- "Enable node translation" is always unchecked and won't get saved.

If we enable node translation in a node edit (source)
- We should check all translations and remove (disable?) their menu entries

When disabling menu translation and editing a translated node
- node edit still shows menu

I don't understand why we switch off the i18n mode in hook_block. I see no reason to do that (unconditionally).
Note that this is no solution - as NicoH told - since the regular page.tpl.php variable also need to be i18nified. in preprocess_page we're even overriding this by i18nmenu. Currently $primary_links is broken regarding your implementation.

function i18nmenu_preprocess_page(&$vars) {
...

Then there are some questions
- Will the menu item language be ignored? (should we try to remove language selector here?)
- In menu item edit: should we check for path pattern to "Enable translation" checkbox visibility? (might be done in jquery client-side)
- If disabled node translation the menu system still does strange things. Are we able to simplify something here?

I expect there will be some "details" more after fixing these things. So the module needs another deep review after.

I can see there's a lot of work on this issue and also the feature is a desirable one. But really, this adds an incredible amount of complexity to the (already quite a big hack) i18nmenu.

Also it still has a lot of issues and maybe we should be focusing our energy on Drupal 7 ports better than adding another really difficult to port feature to 6.x

My main question is: can we trim it down and make it simpler? Some ideas for that would be:
- Just global options, no more per menu options.
- Do it on the synchronization layer. I mean we just store nodes/menu items as usual, then after a node/menu item update, we sync all the translation set menu items, no options, no crap it should be "all or none" the same weight, location in the tree (though it it's below another node we should find a suitable parent).
- Then, if we still have one item per node, there's a remaining issue with the menu admin page. I'm thinking we can just apply 'language selection' to that page so you only see nodes for the current language there (currently it is disabled for admin pages). As items for nodes will be 'synchronized' after updating you really just need to edit items in one of the languages.

@plach,
Does this make sense / would still be useful for your user story? Btw, I've added you to the commit list for the module in case you want to commit some quick bug fix.

So as said before the patch on its current approach doesn't make too much sense on my (personal) cost-benefit analysis. However if any of you guys (@plach, @miro_dietiker) want to commit to maintaining the i18nmenu module with this patch on it, you are really welcomed to move on with this and eventually commit it.

@miro_dietiker:

In node edit form
- only show in menu fieldset "Enable translation" if nodetype is translateable. also check nodetype settings in other cases.

The attached patch should fix the issues I was able to identify when translation support is disabled.

- title changes to the translated menu item don't update it.

I could not reproduce this issue: would you please investigate further and provide a more detailed path to reproduce the bug?

In admin/build/menu when editing a menu item
- "Enable node translation" is always unchecked and won't get saved.

This happens only for non-node items and is an intended behavior: it makes no sense to enable node translation on non-node items.

If we enable node translation in a node edit (source)
- We should check all translations and remove (disable?) their menu entries
[...]
Will the menu item language be ignored? (should we try to remove language selector here?)

I ain't sure this is a good idea as IMO node translation should have less priority than the item language: when one explicitly assigns a language to a menu item, node translation has no meaning. Instead we could hide the "Enable node translation" checkbox when an item has a language assigned (I implemented this in the attached patch).

When disabling menu translation and editing a translated node
- node edit still shows menu

This is by design too. If one disables node translation the usual behavior is restored: a node translation form currently presents the menu item element prepopulated with a copy of the source node's menu item.

I don't understand why we switch off the i18n mode in hook_block. I see no reason to do that (unconditionally).

Node translation really does not play well with language selection: any mode different from "off" causes the node menu items to be messed up. IMO menu items should be subject only to their language setting, if specified, otherwise they should be always visible (possibly translated). Please keep in mind that language selection is restored immediately after rendering the menu tree so we should not lose any capability associated with it.

Note that this is no solution - as NicoH told - since the regular page.tpl.php variable also need to be i18nified. in preprocess_page we're even overriding this by i18nmenu. Currently $primary_links is broken regarding your implementation.

You are right. The attached patch should fix this (see i18nmenu_theme_registry_alter() and i18nmenu_navigation_links_prepare()).

In menu item edit: should we check for path pattern to "Enable translation" checkbox visibility? (might be done in jquery client-side)

This surely could improve usability, I'll put it on my TODO list.

If disabled node translation the menu system still does strange things. Are we able to simplify something here?

I really need some detail to answer this.

@Jose Reyero:

My main question is: can we trim it down and make it simpler? [...]
Does this make sense / would still be useful for your user story?

Unfortunately I cannot afford to spend the time needed to implent a completely new approach. Moreover I'm actively using the standalone-module version of this patch in a couple of project and changing approach would mean more testing and training people again. Hence, I'm sorry, but the answer is no.

Btw, I've added you to the commit list for the module in case you want to commit some quick bug fix.

Thanks Jose, very appreciated :)

So as said before the patch on its current approach doesn't make too much sense on my (personal) cost-benefit analysis. However if any of you guys (@plach, @miro_dietiker) want to commit to maintaining the i18nmenu module with this patch on it, you are really welcomed to move on with this and eventually commit it.

First a preamble: since my only viable alternative to getting this into i18nmenu is creating a new project I'd have no problem to become a maintainer of i18nmenu.

However, given that Jose seems inclined to explore different approaches and Miro and I seem not to be on the same line about what should happen when enabling node translation on existing sites and more generally on backward compatibilty, perhaps we should actually release this as a standalone project. There should be no problem in doing this on the technical side and in this scenario we could be sure that the users, by enabling the module, really want node translation on their sites.

Moreover an eye on the future: I think menu node item translation will be far less useful when field translation will be available in Drupal 7, as every node will be available in multiple languages, so this solution would make porting i18nmenu to 7.x easier.

subscribe

@plach,

Ok then, I think the best solution is to make this a different module. I'll add a link from i18n project page.

I'm assuming it will work with i18nmenu (right?) instead of being a replacement for it. If so and there is any change to i18nmenu needed to make the integration simpler, we can work on that

@Jose:

I'm assuming it will work with i18nmenu (right?) instead of being a replacement for it. If so and there is any change to i18nmenu needed to make the integration simpler, we can work on that

Yes, the current standalone versione depends on i18nmenu. I'll publish it ASAP.

@Miro:

If you wish to keep working with me on it you're welcome :)

Here it is: http://drupal.org/project/i18nmenu_node.

As Jose noted in #41 some minor changes can be retained from the latest patch.

Ok,

Committed the small patch in #43 (needed to apply manually)
Added link to the new module to the project page.

So it seems we are all set for now, follow up here: http://drupal.org/project/i18nmenu_node.

Thanks Jose!

plach, fine to see this.
I'll review the module. Note that most of my questions where to get clarification in intention and provide some thoughts. So looking forward to support your progress.

Now that this functionality is separated completely we should about making i18nmenu base code more straight. Is there some simplification possible? (e.g. don't consider menu item node case ever)

Now that this functionality is separated completely we should about making i18nmenu base code more straight. Is there some simplification possible? (e.g. don't consider menu item node case ever)

AFAICT there is not much node-specific code in i18nmenu, just i18nmenu_form_alter() and i18nmenu_nodeapi() and removing them IMO would seriously compromise i18nmenu's functionality.

A possible minor optimization.

plach - what does this fix change in functionality? (what kind of issue does it fix?)

It does not change anything: simply only the needed hook is invoked to load the menu data structure into the $node object instead of all the hooks implementing hook_nodeapi('prepare').

Setting back to active as we need some further optimization to optimally interfer with this module.

If the module about node translation set is missing on a site, we still should optimize the situation.
If i add a node (which has a language) to the menu, it is being added for all languages.

Why does i18ncontent not sync the language into the menu item (also on node updates)?
I'd consider it a very special case that my node from language A in a menu would needed to be shown in language B.
Is this case so common?

This might affect the i18nmenu_node somehow.

This might affect the i18nmenu_node somehow.

I ain't sure about this: if a node menu item has a language assigned i18nmenu_node does not kick in.

plach - cool.
It turns out that my i18n menu does it perfectly right now. Seems to be some edge case of e.g. changing languages in the node.

UPDATE: We need to check if menu item language is also updated if content language is changed.
It could be this (missing menu item language update) was the source of most of our issues.

Then there's a question to the translation system.
Should we avoid passing menu items with assigned languages to the translation layer? If a menu item is language specific it doesn't make any sense to pass it. We even might remove the translations if we change the language into neutral.

We need to check if menu item language is also updated if content language is changed.
It could be this (missing menu item language update) was the source of most of our issues.

At the moment node language is not taken into account (but is preserved) if a node has already a menu item associated or has no translations, hence node language matters only when creating a node translation. The item language has to be changed manually. Supporting language change would mean changing the current behavior.

Should we avoid passing menu items with assigned languages to the translation layer? If a menu item is language specific it doesn't make any sense to pass it.

It's already so: in i18nmenu_menu_link_alter() the menu item is marked to be altered only if it has a language associated. Hence if the menu item has a language associated i18nmenu_translated_menu_link_alter() is not invoked.

We even might remove the translations if we change the language into neutral.

Do you mean removing the string translations? I ain't sure this is a great optimization.

By the way, I found that manually applying #43 introduced a small bug fixed by the attached patch. I committed it to the DRUPAL-6--1 branch.

I'll check some workflows in adding nodes and switching languages and adding menu items to understand if some more updates are needed in i18nmenu from my side.

See #967688-9: Introduce an API function for removing a string translation.

Hello,

Just came across this thread and I'm curious if any progress has been made since 2010??

I have been hoping for functionality like this for a while. I will try patches, but if this has gone somewhere else, I would appreciate a heads-up.

This ended up in a separate module: http://drupal.org/project/i18nmenu_node

But no Drupal 7 love, huh?
Unfortunately Entity Translation hasn't worked out well enough for me in it's current state so I'm stuck with Content Translation.

Thank you for the effort though!!

Changing this to 6.x since the module that came out of this discussion has no D7 port. :-(

I apologize for my misbehavior immediately above - for dense minds like mine: See i18n_menu_node

any hope the separated (must-have) feature provided by i18nmenu_node being merge into i18n_menu ?

Please don't reopen old closed issues, add a new one with a proper updated description. And also explain why it is a problem having the feature in a different module.