localized menu link data getting saved back to the DB

Looks good on a code review. Needs testing definitely.

Well I'm not sure what the steps are to reproduce the actual bug here, but I tested creating, editing, resetting menu items etc. and all was fine.

Well, before the patch the 'options' array gets customized with t() and friends based on the current request language. Such as in:

     $item['description'] = t($item['description']);
     if ($link_translate && $item['options']['attributes']['title'] == $original_description) {
-      $item['options']['attributes']['title'] = $item['description'];
+      $item['localized_options']['attributes']['title'] = $item['description'];

Before patch, a localized description ends up as title. pwolanin says that the localized data gets saved into the database if you modify a menu item and save it. (Although the title attribute was said to be a generated attribute which is not saved, so this one might not be a good example after all).

pwolanin: it would be great to see a reproducible case first. On IRC earlier you said the title and description is put into a non-saved value anyway. And these are the only two localized right?

Tried to look deeper into this and track this down. To my observation, menu_link_load() always calls _menu_link_translate($item) on data directly retrieved from the DB, which calls _menu_item_localize() to always overwrite data from $item['options'], so even if $item['options'] is saved localized into the database, it is always rewritten for the more accurate data in the actual request at hand in _menu_item_localize().

I also went on to test this on a personal test site. I have Hungarian translations fully imported. All menu items with built-in titles and descriptions are using the English text in the database, regardless of what I try to edit them (move then around, edit their expanded state, etc). Also, the menu items which are indeed user provided data (like node/add links for node types) show up with the custom text given, as expected. I cannot reproduce any localized data being saved, but even then as said above my code inspection shows that it would be used in the correct localization later.

Reopen if reproduced.

translate the description for admin/build/block == "Configure what block content appears in your site's sidebars and other regions."

move this link to under node/add/page (as in the other issue)

delete the page type.

check the DB - the localized string is now polluting the DB

Change the translation of the description. The new translation is not used.

Tried all of these with a 'custom language' and couldn't reproduce. Checked in menu_links and menu_router and both contained the original string.

Also tried the same thing patched with #10 and it didn't do any harm though.

With a fresh HEAD and clean database I was able to reproduce, sorry about #11.

Followed exactly the same steps with the patch from #10 and verified that it fixes the bug. Ought to be RTBC, could do with one more set of eyes though.

Exact steps taken:

fresh HEAD, fresh DB. Patch with #8.

enable locale module. Create a custom language, set it as default, translate the block menu item description to something obvious.

move the blocks menu item under 'Story'.

Delete the 'story' content type.

Look in the database and see that the "options" column has changed to your translated string.

Revert the #8 patch and patch with #10. Reset the 'block's menu item. Re-create the 'Story' content type, move blocks under it again, delete Story content type again. Look in the DB, the untranslated string is in there.

Ok, here's a summary of my findings related to this issue:

1) without any patch the localized description does not get into the database.

2) However, the title argument in the link on node/built/menu does only contain the English original (which firefox decides to not show on hover).

3) Applying the patch from #8 fixes the title tag, but now the localised string gets into the DB.

4) To fix both bugs, you need to apply the patch from #10.

Considering that the patch works and pwolanin should know what he does in the menu system, I think the patch is good to go.

Reviewed yesterday and today, and all looks reasonable, so committed. Thanks.

Oh my god. This broke my updated drupal.hu site (under local testing). When localized_options is not set (== NULL), I get a WSOD caused by this call to l():

      $breadcrumb[] = l($parent['title'], $parent['href'], $parent['localized_options']);

Not good, not good.

Forgot to add that if I close the function call at right after $parent['href'], all is working well. Otherwise its a WSOD.

Not sure if it's related to this but I keep getting this notice.

"notice: Undefined index: localized_options in /includes/menu.inc on line 1220"

The cause of this must be from the commits that happened from yesterday to today.

I have all core modules enabled and the database is up to date.

Ew.. Fatal error when viewing a node.

Well, my reproduction only happens on book node pages and from an updated install from Drupal 5 (the Drupal 5 site runs just fine on Drupal.hu). So if this is broken data in the database, then the menu update code would be at fault. My reproduction is as shown by dvessel as well.

Checked SELECT * FROM menu_links WHERE options NOT LIKE 'a:%' and got no result (the inverse query returns all rows). So all rows are having arrays serialized. Also, I got no unserialization error here, so I bet no such error exists here. Also since dvessel can reproduce, either both of us have bad data or there is something bad in the code.

Right, it only happens with nodes within a book outline. My install was never updated from 5. It's not clean either. It's been updated from all the 6.x-dev revisions.

I'm not very familiar with db's but I tried testing the string Gabor listed with this and got no result.

  $result = db_query("SELECT * FROM menu_links WHERE options NOT LIKE 'a:%'");
  while ($something = db_fetch_array($result)) {
    print var_dump($something);
  }

Removing the NOT returns a ton of menu data.

This seems to be related:

When adding a node to a book outline:
Fatal error: Unsupported operand types in /includes/common.inc on line 1435

From that point forward:
notice: Undefined index: localized_options in /includes/menu.inc on line 1521.

#25 fixes the fatal error and notice that I'm seeing.

Same here. I still get notices but it could be due to a bad updates.

Looks good, committed #25 to Drupal 6.x, still needs to be committed to 7.

pwolanin, Gabor doesn't have access to commit to HEAD.

pwolanin: Dries specifically said I don't have access to commit to HEAD, and although I did not try, I am definitely not about to break the rules there.

I just synced up with the DRUPAL-6 branch this morning, and I am also getting the notice from #19.

notice: Undefined index: localized_options in C:\Program Files\xampp\htdocs\d6\includes\menu.inc on line 1228.

When I go to admin the primary menu, I get an error similar to the one in #20:
( ! ) Fatal error: Unsupported operand types in C:\Program Files\xampp\htdocs\d6\includes\common.inc on line 1435
Call Stack
# Time Function Location
1 0.0034 {main}( ) ..(null):0
2 0.5845 menu_execute_active_handler( ) ..(null):18
3 0.5907 call_user_func_array ( ) ..(null):347
4 0.5907 drupal_get_form( ) ..(null):0
5 0.5907 call_user_func_array ( ) ..(null):102
6 0.5907 drupal_retrieve_form( ) ..(null):0
7 0.5908 call_user_func_array ( ) ..(null):358
8 0.5908 menu_overview_form( ) ..(null):0
9 0.5955 _menu_overview_tree_form( ) ..(null):45
10 0.5988 l( ) ..(null):72

I think my primary menu contains a single item, labeled front, with a path of

<front>

More info: with a fresh install of the db I am getting the same error when I try and create a primary menu item with the path

<front>

Creating a primary menu item with a different path works fine.

I can recreate this.

#37 works for me.

#37 fixes it for me as well.

Where's $item['options'] coming from? Can we dig deeper to the source of that?

Great, I thought all instances of 'options' were being changed to 'localized_options' based on the tiny patch your posted before but I see they are both required.

Committed #37. This means that #25 and #37 still need to be committed to 7.x.

Committed to CVS HEAD. Thanks.