Performance: menu translations continuously updated

I can confirm this issue.

What's worse, during updating i18nmenu executes a cascade of DELETE/INSERT calls on every request. A typical devel query dump looks like this, repeated over and over again for each menu item displayed:

0.73	1	i18nstrings_get_source	SELECT s.*, i.type, i.oid, i.property FROM locales_source s LEFT JOIN i18n_strings i ON s.lid = i.lid WHERE s.textgroup = 'menu' AND s.location = 'item:19789:title'
1.22	1	i18nstrings_add_string	DELETE FROM i18n_strings WHERE lid = 10733
0.52	1	i18nstrings_add_string	INSERT INTO i18n_strings (lid, type, oid, property) VALUES(10733, 'item', 19789, 'title')

Clearly, the update of the menu items should NOT happen on every request, but ONLY ONCE, after the item has been created or changed. The attached patch avoids the constant updating of menu items. I'm not sure if new/changed items are still recognized, though, after applying this patch.

subscribe

I confirm

i18nmenu_localize_tree() should have a second argument, $update, which defaults to FALSE.

In i18nmenu_localize_tree():

        if ($update) {
          $tree[$index]['link']['title'] = tt('menu:item:'. $link['mlid'] .':title', $link['link_title'], NULL, TRUE);
        }
        else {
          $tree[$index]['link']['title'] = tt('menu:item:'. $link['mlid'] .':title', $link['link_title']);
        }

In i18nmenu_locale_refresh():

   i18nmenu_localize_tree($tree, TRUE);

We also need a separate function to be called when a single menu item is added or updated.

Here's a patch implementing what I suggested in #4, including processing of menu item inserts and updates via the menu item edit form.

With patch this time.

I missed several things in the last patch. Here is a fuller patch.

Additions:

1. In i18nmenu_menu_link_alter, set a callback to do the actual translation. Ideally we would set a ['title_callback'], but custom title callbacks are not fed the actual item to be translated (just the $map variable representing the current path, with objects substituted). We need the item, so we need to use hook_translated_menu_link_alter(). This is triggered by setting $item['options']['alter'] = TRUE;

A problem was that, since the title callback remained t(), we were still getting rows added to the built in interface group. To prevent this, I've added a title callback that simply returns the original title. This prevents the unwanted t() call.

2. On the menu item overview page, add a 'translate' operation link for each menu item, pointing to the translation interface page for that menu item.

3. Provide help text for the language select element on menu items. I can't figure out yet what selecting a language does or is supposed to do, so I've left this vague, but noted that leaving "All languages" will make the item translatable.

4. Add a call to menu_refresh() to i18nmenu_locale_refresh(). This ensures that existing menu items are altered when i18nmenu is enabled.

I've done some basic testing and with the patch menu item translation seems to be working.

Testing suggestions:

1. Enable menu module. Create several custom menu items to the primary links menu.

2. Install and enable French and Spanish.

3. Enable i18nmenu module. On enabling, you should get messages about the menu items added above being registered for translation.

4. Go to menu overview page for the primary links menu. You should see 'translate' links for each menu item for each enabled language.

5. Click one or two of the menu item 'translate' links and enter translations.

6. Enable the language switcher block.

7. Switch languages. You should see that the menu items you entered translations for are translated.

With patch this time.

subscribing.

Tested this following nedjo's comments in #9 and it works well with primary links when displayed in a block (note it doesn't work for primary links in a theme). Did some manual inspection of locales_source as well and everything looks to be in order.

One issue though - there's no 'translate' link in the menu overview - doesn't look like the form_alter's being called at all? Translating from 'translate interface' works fine though.

The patch from #10 seems to work fine. I've just altered two bits:
- Bringing back the user to the menu admin page after submitting a translation.
- Simplified _i18nmenu_get_item().

While the initial problem of this issue seems to be solved, one problem remains with the new 'translate' link: you can't translate the menu item's description, only its title.

Minor improvements to inline comments. This looks ready to go.

Thanks for the reviews and improvements.

I think it's nearly ready to go except for the 'translate' links that I added. Those are confusing, as smk-ka noted, because they link only to one attribute of the menu, its title. We should either remove these or move them to the edit form.

I'd be tempted to remove the 'translate' links - or at least split them into a separate issue.

Putting them on the edit form also sounds like a better option (and might be nice in other places too).

Another issue is the title_callback I introduced--setting it to a callback that just returns the title, to prevent triggering of t().

Probably better would be setting it to check_plain.

I'm looking more into this issue. Probably we need a core patch.

I've re-rolled without the 'translate' links on the menu overview form, and added a @TODO for the no-op title callback - opened a core issue here: #348627: Allow title_callback to bypass t().

I'm not sure about using check_plain here - check_plain will double encode characters if the same text is passed through twice, so we'd need to be sure that's not going to be done again somewhere else further down the line.

We can just set the title_callback to FALSE according to Damien, attached patch does this.

_menu_item_localize() ensures check_plain() isn't called twice:

      // Avoid calling check_plain again on l() function.
      if ($callback == 'check_plain') {
        $item['localized_options']['html'] = TRUE;
      }

Confirmed that locale.inc validates input here and adds a watchdog notice about the attempt, so there's no need for check_plain() in this case.

Patch with two additions from the previous version:

1. Respond to deletion of menu items.

2. Respond when menu items are added, edited, or deleted when nodes are saved or deleted. This change addresses the fact that menu items can be added/edited/deleted directly from the node edit form.

Before applying this patch, I would like to investigate two things:

1. Looking back to records created without this patch applied, I find that menu item records were created but not assigned to the 'menu' textgroup. (Nothing shows up for textgroup.) TBD: can this problem be reproduced (with an unpatched i18nmenu)? If so, what is causing it?

2. When I delete menu items, their data are only partially deleted. The item string no longer shows up, but there is a remnant record without the string that shows up when searching the 'menu' textgroup. Why? Is this a bug in i18nmenu, or am I calling the delete routine incorrectly in the introduced function _i18nmenu_delete_item() ?

nedjo, is it possible that #1 is related to your comment in #9 = "A problem was that, since the title callback remained t(), we were still getting rows added to the built in interface group. "?

Looking through the rest of the patch, the calls to tt() for deletion seem to match the code flow there, haven't gone in to try reproducing yet though.

I am not able to replicate #1 - without this patch, strings are saved in the textgroup "menu". Also, without this patch, I can confirm that menu strings are not deleted (neither in locales_source, nor in i18nstrings).

With this patch,

• records are still properly assigned to the "menu" textgroup in locales_source (just for confirmation).
• all related records in locales_source, locales_target, and i18nstrings are deleted upon deletion of a menu item. (which did not happen before)

Hence, I'd like to mark #22 as RTBC.

However, the culprit is that #22 does no longer fix the performance issue. I tracked this further down to

• tt() properly gets $update = FALSE, and invokes i18nstrings_tt()
• i18nstrings_tt() is properly invoked with $update = FALSE
• i18nstrings_tt() invokes i18nstrings_get_string() to fetch a translation (note that the 3rd and 4th argument for i18nstrings_get_string() are not used)
• However, if no translation exists, the guilty code comes into the game: (note the reversed !$update condition here)
  

    $translation = i18nstrings_get_string($context, $langcode, $string, !$update);
  
    if ($translation === FALSE) {
      // If the string is missing, create it.
      if (!$update) {
        i18nstrings_add_string($name, $string);
        i18nstrings_cache($context, $langcode, $string, TRUE);
      }
      $translation = TRUE;
    }
  

• Thus, i18nstrings_add_string() goes ahead and invokes plenty of DELETE FROMs and INSERT INTOs on each page request for all untranslated strings.

Note that this only happens if the active language is different to the default language.

In attached patch, I simply removed the ! from this condition. However, I have no idea what this change could affect. In general, the logic in i18nstrings_tt() (above snippet) looks totally weird - if it cannot find a translation, it adds the source string as translation or what?

Applied the patch with some changes for the i18nstrings logic:
- The tt function should create the source anyway if missing, even it not udate forced ($update)
- Replaced the function call to remove strings for deleted items.

I know that strings translation thing is far from perfect, this looks like an improvement though, so committed, we can move on from here. The logic is weird, I agree, so I've added some comments here and there to try to add some light into it. I'll add some more comments into that module.

Thanks all for the great work here.