I cannot find the related issue for this, so I'm assuming it's new. If not, you can mark it duplicate.

I discovered this in Gardens, but also tested this against Drupal 7 HEAD today.

If this page is in a custom menu, when I save it will go into the Main menu. And if I untick the option for main menu, it will also disappear from the custom menu.

The screencast here demonstrates more clearly: http://screencast.com/t/OGI1ZGRjZDg (in d7 head, without path auto)
In more detail: http://screencast.com/t/MDJiNjU3M (from gardens w path auto)

To replicate:

1) Create a custom menu.
2) Link to a page in your custom menu.
3) Edit the item and save.
- You will see the link moves to the main menu.
optionally
4) If you untick "Provide menu link" it disappears from custom menu altogether.

Files: 
CommentFileSizeAuthor
#115 menu-default-for-node-761648-115.patch3.2 KBlyricnz
FAILED: [[SimpleTest]]: [MySQL] 32,049 pass(es), 3 fail(s), and 0 exception(es).
[ View ]
#114 menu-default-for-node-761648-114.patch5.04 KBlyricnz
PASSED: [[SimpleTest]]: [MySQL] 32,029 pass(es).
[ View ]
#101 node-menu-update-761648.patch1.84 KBlyricnz
PASSED: [[SimpleTest]]: [MySQL] 31,840 pass(es).
[ View ]
#100 node-menu-update-761648.patch1.67 KBlyricnz
PASSED: [[SimpleTest]]: [MySQL] 31,790 pass(es).
[ View ]
#91 node-form-menu-exclude-disabled-761648.patch3.07 KBlyricnz
PASSED: [[SimpleTest]]: [MySQL] 29,484 pass(es).
[ View ]
#89 node-form-menu-exclude-disabled-761648.patch3.04 KBlyricnz
PASSED: [[SimpleTest]]: [MySQL] 29,476 pass(es).
[ View ]
#88 node-form-menu-exclude-disabled-761648.patch3.04 KBlyricnz
PASSED: [[SimpleTest]]: [MySQL] 29,477 pass(es).
[ View ]
#85 node-form-menu-exclude-disabled-761648.patch3.02 KBlyricnz
PASSED: [[SimpleTest]]: [MySQL] 29,482 pass(es).
[ View ]
#84 node-form-menu-exclude-disabled-test-761648.patch1.06 KBlyricnz
FAILED: [[SimpleTest]]: [MySQL] 29,478 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#82 node-form-menu-exclude-disabled-761648.patch2.9 KBlyricnz
FAILED: [[SimpleTest]]: [MySQL] 29,298 pass(es), 517 fail(s), and 364 exception(es).
[ View ]
#81 node-form-menu-exclude-disabled-761648.patch2.67 KBlyricnz
FAILED: [[SimpleTest]]: [MySQL] 29,294 pass(es), 513 fail(s), and 363 exception(es).
[ View ]
#80 node-form-menu-exclude-disabled-761648.patch2.72 KBlyricnz
FAILED: [[SimpleTest]]: [MySQL] 29,279 pass(es), 514 fail(s), and 376 exception(es).
[ View ]
#79 node-form-menu-exclude-disabled-761648.patch1.83 KBlyricnz
FAILED: [[SimpleTest]]: [MySQL] 29,275 pass(es), 513 fail(s), and 371 exception(es).
[ View ]
#67 761648-67.node-menu-upgrade.patch2.5 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 29,476 pass(es).
[ View ]
#60 761648-60.node-menu-upgrade.patch2.25 KBksenzee
PASSED: [[SimpleTest]]: [MySQL] 29,403 pass(es).
[ View ]
#57 761648-57.node-menu-upgrade.patch3.92 KBksenzee
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 761648-57.node-menu-upgrade.patch.
[ View ]
#47 761648-node-menu-upgrade_47.patch4.42 KBShawn DeArmond
PASSED: [[SimpleTest]]: [MySQL] 31,579 pass(es).
[ View ]
#45 761648-node-menu-upgrade.patch3.05 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 31,567 pass(es).
[ View ]
#28 761648-node-menu-upgrade.patch2.85 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 29,851 pass(es).
[ View ]
#26 761648-node-menu-defaults.patch2.89 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 29,920 pass(es), 28 fail(s), and 82 exception(es).
[ View ]
#25 761648-node-menu-defaults.patch1.94 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 29,901 pass(es), 28 fail(s), and 82 exception(es).
[ View ]
#22 menu-upgrade-missing.patch1.22 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 29,740 pass(es).
[ View ]
#13 761648-menu-defaults-d7.patch1.74 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 20,406 pass(es), 18 fail(s), and 54 exception(es).
[ View ]
#8 menu.patch1.05 KBJody Lynn
PASSED: [[SimpleTest]]: [MySQL] 19,934 pass(es).
[ View ]

Comments

Priority:Normal» Critical

Well documented!

I consider this a critical bug. Cannot release without this working. All non Main Menu items end up in Main Menu or disappear when they are edited and saved.

Thanks @risoy - I thought 'critical' too- but wary of ever marking as such!

Category:bug» support
Priority:Critical» Normal
Status:Active» Fixed

You should enable allowed menus at /admin/structure/types for each content type
Only MainMenu enabled by default
Details http://drupal.org/node/224333#menu_default_node_menu

@andypost: oh, well, I think the issue then is that the settings are now separated but do not default to allow all menus? Looks like this will break nodes on all sites upgrading to D7 with multiple content types. I can see that new D7 installs would only allow the main menu, but updated ones should not work alike IMHO. What do you think?

Title:When you edit items linked in custom menus, they move to the Main menuUpgrade path missing for more fine grained node menu selection
Category:support» bug
Priority:Normal» Critical
Status:Fixed» Active

Ok, I've read through #351249: Finer control over the Parent Menu select box and #716792: Remove 'Default menu for content' setting from structure>menus>settings. and there was only one mention of missing an upgrade path from July 2009. Seems like nobody actually worked on one. Once people upgrade, they'll encounter this issue with nodes jumping around menus. Drupal should either set up the upgrade so that all menus are allowed on upgraded sites or should intelligently figure out which nodes under that node type belong to which menus and set up possible menus that way.

@Gábor Hojtsy, Agreed, I found no upgrade path at all.

I think that usually count of menu items is less then nodes so

SELECT n.type, ml.menu_name FROM menu_links ml JOIN node n ON n.nid = SUBSTR(ml.link_path, 6) WHERE ml.link_path LIKE 'node/%' AND ml.link_path NOT LIKE 'node/%/%' GROUP BY n.type, ml.menu_name;

And then store into variables but I think this query could be very hard for servers with millions of nodes. Anyway this should be measured

Additionally you are allowed to link content to a menu in Structure/Menus that is unavailable for that content type. That assignment will break when the content is edited if the content type does not allow the particular menu.

Status:Active» Needs review
StatusFileSize
new1.05 KB
PASSED: [[SimpleTest]]: [MySQL] 19,934 pass(es).
[ View ]

This patch allows a node to remain in any menu during node edit if it is already assigned to that menu (via direct menu administration or an upgrade). I think this is the simplest fix.

Patch seem reasonable but anyway we need some upgrade path to fill default values of variables

Status:Needs review» Needs work

#8 looks good in general, not just for upgraded sites. We keep finding issues where we let people change settings which suddenly invalidates all their content, and dealing with that gracefully (no batch updates, no magic changing of data) is a good pattern to get into. Especially when the node form isn't the only way to add a menu item pointing to a node. Nitpicks - the comment exceeds 80 characters on one line, and this feels like it needs a test to me.

@andypost, I'm not convinced we need to set variables in the upgrade path - this isn't user data, this is just a setting - when you upgrade major versions of Drupal core, you usually have to tweak new settings to make them fit with site workflow. To compare, with Drupal 5 menu upgrade, you were lucky if you ended up with your Drupal 5 menu structure at all.

@catch thanx a lot for clarification, I've already forget about 5->6 menu nightmare.

Issue tags:+D7 upgrade path

Status:Needs work» Needs review
StatusFileSize
new1.74 KB
FAILED: [[SimpleTest]]: [MySQL] 20,406 pass(es), 18 fail(s), and 54 exception(es).
[ View ]

Suppose menu_edit_item() should be fixed too.
Is there any reason why default variable value is 'main-menu:0' but $default is set to 'navigation:0' in both cases?

Status:Needs review» Needs work

The last submitted patch, 761648-menu-defaults-d7.patch, failed testing.

Priority:Critical» Normal

This is just a normal bug, annoying but not a release blocker, easily fixed by configuration.

Issue tags:-D7 upgrade path

yeah there is no significant subsystem broken here. And has nada to do with D7 upgrade.... just a bug.

Title:Upgrade path missing for more fine grained node menu selectionThe menu admin screen allows adding nodes to menus not allowed for nodes

Title:The menu admin screen allows adding nodes to menus not allowed for nodesUpgrade path missing for more fine grained node menu selection

Re-title back, because menu admin "does not allows for now" but this patch make this change!

Also still open question #13

@chx: well, its an upgrade bug since a new feature in D7 makes existing menu associations get lost when editing nodes. The new menu restrictions feature for node types is not properly initialized in the update. That's the bug. It's not an issue when installing or when updating from a previous D7 (post the introduction of this feature), its an update bug.

So basically we need to take the existing single setting in Drupal 7, and set it for all node types in Drupal 7?

@pwolanin: well, there is no previous setting, since there was no such feature; at least looking at the patch that was committed (http://drupal.org/files/issues/351249_parent_menu-86.patch) this was the D6 code:

<?php
  
foreach ($menus as $menu_name => $title) {
   
$tree = menu_tree_all_data($menu_name, NULL);
   
$options[$menu_name . ':0'] = '<' . $title . '>';
   
_menu_parents_recurse($tree, $menu_name, '--', $options, $item['mlid'], $limit);
   }
?>

This is the new D7 code:

<?php
  $options
= array();
   foreach (
$menus as $menu_name => $title) {
    if (isset(
$available_menus[$menu_name])) {
     
$tree = menu_tree_all_data($menu_name, NULL);
     
$options[$menu_name . ':0'] = '<' . $title . '>';
     
_menu_parents_recurse($tree, $menu_name, '--', $options, $item['mlid'], $limit);
    }
   }
?>

Basically, the D6 code showed all menus, while D7 only shows a limited number of menus based on node type settings AFAIS. The default menu list for a node type only contains one item though, main-menu:0:

<?php
variable_get
('menu_parent_' . $type, 'main-menu:0');
?>

So I think the update function would:

(a) iterate through all node types
(b) check if the node type has this setting already (we are now required to make D7-D7 updates, so it might have)
(c) if it does not, then fill it in with a list of all menu roots on the site

Priority:Normal» Major
Status:Needs work» Needs review
StatusFileSize
new1.22 KB
PASSED: [[SimpleTest]]: [MySQL] 29,740 pass(es).
[ View ]

Here is a proposed patch. I still believe this is a major issue that people upgrading from Drupal 6 will face. Suddenly their previously fine nodes will loose their menu items when saved with jus a pristine upgrade path. We removed one setting and introduced another one without any connection inbetween them, and with much stricter defaults that the previous setting used. I'd almost go as far as to say this is critical but lets try if its getting some attention this way. Please review!

Note that the actual Drupal 7 menu module code still has this code comment related to this phenomenon: @todo This will fail with the new selective menus per content type. around the code patched above by @andypost. That is kind of an interesting workaround, and if you are not happy with the upgrade path suggestion, we can talk about that again obviously.

Gabor, I think we should merge my patch with your upgrade function.

Anyway @todo hunk needs workaround - case for example: when nodes created by some code with pointing an actual menu parent

@andypost: yeah, can you help merging the patches and reporting it? Thanks!

StatusFileSize
new1.94 KB
FAILED: [[SimpleTest]]: [MySQL] 29,901 pass(es), 28 fail(s), and 82 exception(es).
[ View ]

Let's test merged patch

StatusFileSize
new2.89 KB
FAILED: [[SimpleTest]]: [MySQL] 29,920 pass(es), 28 fail(s), and 82 exception(es).
[ View ]

Missed a second hunk, final patch

Status:Needs review» Needs work

The last submitted patch, 761648-node-menu-defaults.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+D7 upgrade path
StatusFileSize
new2.85 KB
PASSED: [[SimpleTest]]: [MySQL] 29,851 pass(es).
[ View ]

After diggin deeper I found that upgrade should be different.
1) Upgrade only when D6 defaults was overriden
2) Upgrade only if D6 menu exists in menu_get_menus() else node form would use default main-menu:0 (for example primary or secondary menus)

Also patch changes current D7 behavior and fixes bug when node has menu item but menu options been disabled for the content type

1) Create Basic page and link it to main menu
2) disable all menus for "Basic page"
3) Edit created page (no menu edit displayed) and save - after patch we could still see menu settings
4) node is unlinked from menu - after patch node still in menu

Should we display menu edit if no menus assigned for the content type but node has menu_list already? probable we need to change logix at menu_node_save()

subscribe

subscribe

Priority:Major» Critical

I applied the patch, but did not test the upgrade path, so I won't set this RTBC.

I did however test the case clearly described in #7: "Additionally you are allowed to link content to a menu in Structure/Menus that is unavailable for that content type. That assignment will break when the content is edited if the content type does not allow the particular menu."

The latter is solved by this patch.

A lot of people are complaining about this behaviour in #957784: Saving node_form removes any menu items that are not in one of the Available menus, see e.g. #18 over there. So marking this critical.

Title:Upgrade path missing for more fine grained node menu selectionMenu items disappear after upgrade or manual menu entry

So
- after either a D6-D7 upgrade or a manual menu entry in Structure/Menus
- nodes end up with a link in a menu that is unavailable for their content type
- which means after a node edit their menu entry automatically disappears

subscribe.

subscribe

subscribe

I tested the patch in #28 when upgrading a site from Drupal 6 to 7 and it prevented the menu items from disappearing.

The menu items that already existed in the primary/secondary links remained after editing the nodes linked to from those menu items. The menu settings appear when I edit those pages and I can edit the 'Menu link title'.

Just to clarify: the menu settings for content types that appear in the menus are unchanged. When I create a new page, for example, the menu settings tab isn't there. If I remove an item from a menu without updating the menu settings for that content type, I can't add that node back to the menu.

Status:Needs review» Reviewed & tested by the community

Upgrade path tested.

Issue tags:+Usability, +User interface

-

subscribe

subscribe

Status:Reviewed & tested by the community» Needs work

Both the menu_get_menus() and node_type_get_types() calls here need to be swapped out for upgrade specific helper functions, I think one already exists for at least node_get_types(). Back to needs work for now.

Subscribing. The way issues disappear from ones tracker when marked duplicate (#957784: Saving node_form removes any menu items that are not in one of the Available menus) is a mild annoyance.

This bug however is a major annoyance. Chance of separating the upgrade bug from the all-the-time D7 bug, or should they stay together?

Status:Needs work» Needs review
StatusFileSize
new3.05 KB
PASSED: [[SimpleTest]]: [MySQL] 31,567 pass(es).
[ View ]

Rerolled. Also fixed the comments and put those updates in a proper updates-7.x-extra group.

Issue tags:+Needs tests

Patch looks good but could use a test.

StatusFileSize
new4.42 KB
PASSED: [[SimpleTest]]: [MySQL] 31,579 pass(es).
[ View ]

Patch works. Attached is an updated patch with tests.

All menu tests pass.

We might want to use entity_uri() in the test instead of presupposing path is 'node/' . $nid, but everything in core still supposes node paths are built this way except node module itself, so it it probably not a priority. Good for me.

subscribe

Subscribing.

@fgm, while you're probably right, the rest of testMenuNodeFormWidget() uses 'node/' . $node->nid, so if it were to be changed, it should probably be changed across the whole function, which is outside the scope of this patch. Best to commit this as-is and create a new non-critical issue to address entity_uri().

Patch from #47 did the trick concerning the disapearing menus. Thanks a lot.

Status:Needs review» Reviewed & tested by the community

Tested and critically acclaimed.

Issue tags:-Needs tests

No longer needs tests

Version:7.x-dev» 8.x-dev

This needs to go into 8.x first, then be backported to 7. It applies cleanly to 8.x so leaving RTBC.

Issue tags:+needs backport to D7

tagging.

StatusFileSize
new3.92 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 761648-57.node-menu-upgrade.patch.
[ View ]

I tried to figure out what was going on in the update function just by reading the comments, and I couldn't do it, so I rewrote most of them. I left the code itself intact though; it looks good, so I'm leaving this at RTBC.

I'm a little unclear on how this should be committed to D8 - it seems like it should go in without the update function, and the update function should be committed only to D7 - but I guess that's Dries's problem to solve. :)

Status:Reviewed & tested by the community» Patch (to be ported)

Bfroehle pointed out in another issue we should be adding or incrementing hook_update_last_removed() in D8. I think we should have a separate patch that does this, so marking to be potted for that.

Status:Patch (to be ported)» Needs work

To be ported hides issues from the default criticals list, so CNW.

Status:Needs work» Needs review
StatusFileSize
new2.25 KB
PASSED: [[SimpleTest]]: [MySQL] 29,403 pass(es).
[ View ]

Here's a version for 8.x that doesn't include an update function. It seems odd to me to increment hook_update_last_removed() before the update actually gets committed to 7.x.

Right this is the same problem we had for Drupal 7 -> 6 and it was a nightmare, that caused the updates to go out of sync many, many times. This is part of the reason I opened #1050616: Figure out backport workflow from Drupal 8 to Drupal 7.

Are you suggesting we commit the fix to 8.x, then 7.x, then another patch to 8.x for the last_removed? We've definitely done that in the past, but I'm not sure it's worth the extra effort.

I don't know what the best approach is any more than anyone else does. :/ I have a hard time believing we can actually keep the updates in sync, so I doubt it really matters if we keep last_removed in sync, but I am perfectly willing to be convinced by optimists.

not sure if I should open a new issue, please say if I should.

Still having problem in D7.
I tried to apply patch 47, I tried manually to include in admin > structure > content types and edit the type to chose write menu, but it didn't help to me.
Maybe problem is somehow related that I'm using panels or smth else.

How could I test, what's wrong?
one panel and one menu are still missing in menu list. I could access my panel and it's shown as in my menu list, but when I go to menu list I can't see that. One menu was for sure before editing, but I can find it nowhere now.

one more thing to mention is that panel is one content type which includes inside itself 2 other content types, maybe that's causing problem?

I found a problem, I had a menu that was referring to path "parent/page". Then I changed the panel's path to "parent/parent2/page". So menu was referring to nowhere. So I changed in DB directly path of menu to "parent/parent2/page". Now it's ok.
There is a bug actually that menu doesn't synchronize with panel. But don't know if it is a bug of menu module or panels.

StatusFileSize
new2.5 KB
PASSED: [[SimpleTest]]: [MySQL] 29,476 pass(es).
[ View ]

+    $all_options = menu_parent_options(menu_get_menus(), $link);
+    $options[$default] = $all_options[$default];

This code assumes the current menu link will always be associated with a menu that the Menu module knows about. However, other modules can have menus too (for example, the Shortcut module), and it won't work correctly there.

I discovered that while marking these two issues as duplicates:
#1061762: Editing node results in menu item being deleted
#1083812: shortcuts disappear when saving the same shortcut content

The attached patch is a reroll of #60 (for Drupal 8) with a small change that should fix that issue. Unfortunately, I don't think we have any way to get the human readable name of the menu in the case where another module manages it, so I just went with a somewhat unfortunate piece of default text ("Current parent") in that case. But it's still better than a PHP notice and a completely empty entry in the dropdown, which is what you would get otherwise :)

+++ b/modules/menu/menu.module
@@ -647,9 +648,15 @@ function menu_form_node_form_alter(&$form, $form_state) {
+    $options[$default] = isset($all_options[$default]) ? $all_options[$default] : t('Current parent');

Using this require special submit processing.

I don't understand what you mean by "special submit processing"? The patch should work fine as is.

Status:Needs review» Reviewed & tested by the community

I'm not sure what "special submit processing" means either, and the patch does work fine as is, so I'm going to mark it RTBC.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 761648-67.node-menu-upgrade.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Usability, +User interface, +D7 upgrade path, +needs backport to D7

#67: 761648-67.node-menu-upgrade.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Patch still applies, according to #69 and #70 this seems rtbc...

Status:Reviewed & tested by the community» Needs work

+++ b/modules/menu/menu.test
@@ -664,5 +664,17 @@ class MenuNodeTestCase extends DrupalWebTestCase {
+    // Assert that Management is selected on the node/$nid/edit page, even though the Management menu is not selected for the node type.
+    $this->drupalGet('node/' . $node->nid . '/edit');
+    $this->assertRaw('<option value="management:0" selected="selected">', t('Menu link parent is consistent'));

1) I disagree with this assertion. My expectation would be that I do not see any kind of possibly related menu links in the node edit form when they are not in the allowed menus.

The only remotely possible exception to this /might/ be when you have menu admin permissions. But even then, I'm not sure whether the exception makes sense.

I think we should understand the node edit form's menu link widget as a configuration-constrained interface to a possible menu link. If the context/configuration does not match, then this widget should not attempt to work with data that it is not supposed to process. That's the entire effin' meaning of configuration, right?

2) The inline comment heavily exceeds 80 chars.

Powered by Dreditor.

I think we should understand the node edit form's menu link widget as a configuration-constrained interface to a possible menu link. If the context/configuration does not match, then this widget should not attempt to work with data that it is not supposed to process. That's the entire effin' meaning of configuration, right?

Furthermore, not respecting configuration also means to disrespect user permissions. We'll only ever be able to make per-menu permissions work, if we apply strict rules for which things we expose to users, and which we do not.

Several conditions are at play to potentially constrain the interface:
1. there may be multiple menu entries that point to this node, not all of which are enabled for this node type
2. the selection of menus on the Menu tab is intended to be a constraint for non-menu-admins, and a convenience for menu-admins
3. users without menu-admin permission shouldn't be able to modify menu links from disabled menus (which were added by admins)

It seems like a more correct approach would be to set $node->menu more appropriately in menu_node_prepare(), rather than mess around so much on the form.

Status:Needs work» Needs review
StatusFileSize
new1.83 KB
FAILED: [[SimpleTest]]: [MySQL] 29,275 pass(es), 513 fail(s), and 371 exception(es).
[ View ]

Here's a patch that limits the menus shown on the menu tab to just those currently enabled for the current content type ($type_menus). This avoids users messing up entries unexpectedly - when a menu with a link is not enabled for the current content type.

The patch also includes the fix from #1156128: When editing a node, the default menu (for content type) is not given priority, otherwise menu matching doesn't work. I'll reroll if that gets in first.

StatusFileSize
new2.72 KB
FAILED: [[SimpleTest]]: [MySQL] 29,279 pass(es), 514 fail(s), and 376 exception(es).
[ View ]

And with a test: 54 passes, 0 fails, 0 exceptions, and 22 debug messages

StatusFileSize
new2.67 KB
FAILED: [[SimpleTest]]: [MySQL] 29,294 pass(es), 513 fail(s), and 363 exception(es).
[ View ]

Update comment on assert()

StatusFileSize
new2.9 KB
FAILED: [[SimpleTest]]: [MySQL] 29,298 pass(es), 517 fail(s), and 364 exception(es).
[ View ]

Updated test to check that link exists after node_save

Status:Needs review» Needs work

+++ b/modules/menu/menu.moduleundefined
@@ -540,18 +540,22 @@ function menu_node_delete($node) {
       // Check all menus if a link does not exist in the default menu.
       if (!$mlid) {
-        $mlid = db_query_range("SELECT mlid FROM {menu_links} WHERE link_path = :path AND module = 'menu' ORDER BY mlid ASC", 0, 1, array(

We should update the comment to say all allowed/configured menus, not sure what's best there but it's no longer all menus.

+++ b/modules/menu/menu.testundefined
@@ -664,5 +664,21 @@ class MenuNodeTestCase extends DrupalWebTestCase {
+    $link = menu_link_load($item['mlid']);
+    $this->assertNotIdentical($link, FALSE, t('Disabled menu entry still exists after save'));
   }
}

I'd use assertEqual($item['mlid'], $link['mlid']); or just assertTrue($link);, the double negative is a bit confusing.

One last thing, for the test bot, could we upload a patch file with only the test hunks, then the whole thing, that will just confirm (without having to run tests locally) that the test catches the original bug.

Powered by Dreditor.

Status:Needs work» Needs review
StatusFileSize
new1.06 KB
FAILED: [[SimpleTest]]: [MySQL] 29,478 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Tests only patch attached.
54 passes, 1 fail, 0 exceptions, and 22 debug messages

StatusFileSize
new3.02 KB
PASSED: [[SimpleTest]]: [MySQL] 29,482 pass(es).
[ View ]

- Fix bug that was causing test failures.
- Update comment to "Check all enabled menus" per #83
- Changed the assertTrue($link, ..) - it returns FALSE if not found

Status:Needs review» Reviewed & tested by the community

It makes sense doing it this way, test fails right, patch looks good, back to RTBC.

Status:Reviewed & tested by the community» Needs work

+++ b/modules/menu/menu.module
@@ -540,18 +540,22 @@ function menu_node_delete($node) {
+      if (!$mlid && count($type_menus)) {

!$mlid will throw a PHP notice when the if condition before is not entered.

Either use empty($mlid) or define $mlid = FALSE; upfront. The latter being preferred, I think.

Powered by Dreditor.

Status:Needs work» Needs review
StatusFileSize
new3.04 KB
PASSED: [[SimpleTest]]: [MySQL] 29,477 pass(es).
[ View ]

StatusFileSize
new3.04 KB
PASSED: [[SimpleTest]]: [MySQL] 29,476 pass(es).
[ View ]

Menu tests: 1373 passes, 0 fails, 0 exceptions, and 332 debug messages
Edit: whoops, same patch.

Status:Needs review» Needs work

Thanks! Some final tweaks after having had a cup of sleep:

+++ b/modules/menu/menu.module
@@ -540,18 +540,23 @@ function menu_node_delete($node) {
+      // Check all enabled menus if a link does not exist in the default menu.

"enabled" => "allowed"

+++ b/modules/menu/menu.module
@@ -540,18 +540,23 @@ function menu_node_delete($node) {
+      if (!$mlid && count($type_menus)) {

!empty() might be slightly cleaner than count() here.

+++ b/modules/menu/menu.test
@@ -664,5 +664,21 @@ class MenuNodeTestCase extends DrupalWebTestCase {
+    $this->assertText('Provide a menu link', t('Disabled menu not shown on node form'));

"Link in not allowed menu not shown in node edit form."

+++ b/modules/menu/menu.test
@@ -664,5 +664,21 @@ class MenuNodeTestCase extends DrupalWebTestCase {
+    $this->assertTrue($link, t('Disabled menu entry still exists after save'));

"Link in not allowed menu still exists after saving node."

Powered by Dreditor.

StatusFileSize
new3.07 KB
PASSED: [[SimpleTest]]: [MySQL] 29,484 pass(es).
[ View ]

You two are just having a laugh, right? Applied changes, apart from leaving the fullstops out of the assert messages. The whole "remove t() and use fullstop" thing is a separate issue.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

Thanks!

Status:Reviewed & tested by the community» Fixed

This looks good. Committed to 7.x and 8.x.

Is there still a problem with D6->7 update, as referred to earlier in this issue?
http://drupal.org/node/761648#comment-4303900

Thanx Dries! One less patch to maintain throughout my D7 setups ;)

Version:8.x-dev» 7.x-dev
Status:Fixed» Needs work

Re #95, yes, it looks like the D7 update function still needs to go in.

I think we also should talk more about the overall fix that went in to D7... The patch that was committed definitely made sense for Drupal 8, but since the patch changed the behavior of $node->menu, isn't that an API change?

Also, less important, but relevant for both D7 and D8:

<?php
 
// @todo This will fail with the new selective menus per content type.
 
if (!isset($options[$default])) {
   
$default = 'navigation:0';
  }
?>

This code was removed by the earlier patches, but not the final committed one. Why?

I assume it's now unnecessary and should be removed... Or if it's not now unnecessary, it's probably still broken :)

@David_Rothstein:

1) Indeed, that update from #57 got lost along the way. @ksenzee seems to have removed it in a follow-up patch on her own.

2) I don't see how this could be an API change. Even if you figure out a way to interpret it as one, it's still an essential bug fix, IMHO.

3) I was the one who scattered gazillions of @todos throughout menu.module in a major D7 clean-up patch that updated Menu module to what it is now. It was impossible to resolve all of those @todos in that clean-up issue. It's quite possible that this patch resolved some of those @todos, but it might make sense to move any potential clean-up work of comments into a separate issue. No need to do that on a #100+ comments issue.

StatusFileSize
new1.67 KB
PASSED: [[SimpleTest]]: [MySQL] 31,790 pass(es).
[ View ]

So, I agree with @sun - this is not really an API change: and in fact the preference for the default menu was broken anyway: #1156128: When editing a node, the default menu (for content type) is not given priority.

So, outstanding work:
- create patch for D6->D7 update fix
- review @todo items for cleanup - new issue.

Here's the patch from module.install #57. I'll test the upgrade path here before sending for review, since the testbot doesn't do this.

Title:Menu items disappear after upgrade or manual menu entryMenu D6->D7 upgrade doesn't maintain node-menu configuration
Status:Needs work» Needs review
StatusFileSize
new1.84 KB
PASSED: [[SimpleTest]]: [MySQL] 31,840 pass(es).
[ View ]

There was a bug in the update process, and the per-type variables were not being set. Also set the default menu-parent, not just the options. Also, variable 'menu_default_node_menu' should be deleted, as it's no longer used in D7.

I'm confused. Leaving aside the d6->d7 upgrade path issue, which patch needs to be applied to D7? ...or does the latest patch cover it all?

@klonos: the patch in #91 was applied to D7 and D8 (this fixes unexpected changes in the menu, when editing nodes that don't have the menus enabled).

Now we're working on the other half of the original issue, which was the D6->D7 upgrade not migrating node-menu settings. They're kindof unrelated, except they were reported together. Right now the patch in #101 needs review.

That's exactly what I wanted to know (since I am not upgrading any of my sites for D6 atm). Thanx Simon ;)

Regarding the API change: $node->menu is a public property that a number of contributed modules use. Previously it recorded whether the node had any menu link pointing to it; now it no longer does. That seems like an API change to me.

Certainly the part of the patch that fixed #1156128: When editing a node, the default menu (for content type) is not given priority should remain, but for D7 it seems like the other logic could be moved into the form_alter function (while still preserving the same behavior as the committed patch), and then we would not be changing $node->menu at all. I don't see how changing the meaning of $node->menu was necessary to fix this bug (although certainly a reasonable change for D8).

@David: I guess the change is (bug aside): previously node->menu would contain "some menu entry", even if that menu was disabled for $node->type. Now it only contains "some menu entry" if it is enabled for $node->type.

So.... a teensy change. But yes.

However, there was no guarantee about exactly which menu entry would have shown up here, if one even existed - and you'll only get *one* menu link, when there are potentially many others as well. Any module that needs to manipulate the menu would probably need to have made explicit calls to retrieve all menu items anyway. IMHO, the only difference in the patch above is *not* providing menu entries that are sortof-invalid anyway. Dries, sun and catch seemed to think it was the right solution, anyway.

Do you know of a module that would be negatively impacted by this change?

Can someone review the upgrade process patch please?

@David_Rothstein:

Previously it recorded whether the node had any menu link pointing to it; now it no longer does. That seems like an API change to me.

Uhm. Where was or is that documented? You make it sound as if this change broke a feature. Reality is, the node type specific menu limitations were poorly introduced for D7. However, the fact they have been introduced explicitly means that the expectations for the menu widget on the node form did change.

There's no way to fix and resolve this and any related issues in any sane way, if the behavior of executed code is not constrained to the configuration that is in place.

Dead simple comparison: If you add a taxonomy term field somewhere and limit it to vocabulary Foo, then it would be a bug if that field suddenly shows you associated terms in vocabulary Bar. Just because it can and perhaps did so in poor ol' D6 is not a valid reason to make it do it and leave it broken.

Title:Menu D6->D7 upgrade doesn't maintain node-menu configurationMenu D6->D7 upgrade doesn't maintain node-menu configuration (f.k.a.: Menu items disappear after upgrade or manual menu entry)

...sorry to butt in, but since there is no way to unsubscribe oneself from issues and given #104, I need a clean & fast way to tell what this issue used to be about besides what it currently aims to fix. Hope you people don't mind the -really- long title.

Do you know of a module that would be negatively impacted by this change?

I don't specifically know of any module that is impacted by this, no.

In #1007910: D6->D7 update doesn't convert 'primary-links' menu to 'main-menu' there is a patch that corrects the D6->D7 menu upgrade.

Is this related?

Status:Needs review» Needs work

They are not duplicates, but should probably be coordinated.
- the other patch "Rename primary-links and secondary-links to main-menu and secondary-menu"
- this patch "Migrate the "Default menu for content" setting to individual node types."
See my comments in that issue. If that patch is going to be applied, it should update 'menu_default_node_menu', and run *before* this patch pushes that setting into all the node-types.

@lyricnz: Thanks for coordinating the two issues together and creating a patch combining the two issues.

If we want to merge the two issues, we should mark one the duplicate of the other. :-)

Status:Needs work» Needs review

I changed my mind on the other issue. Sending this one back to CNR based on #101.

StatusFileSize
new5.04 KB
PASSED: [[SimpleTest]]: [MySQL] 32,029 pass(es).
[ View ]

And again with a test.

StatusFileSize
new3.2 KB
FAILED: [[SimpleTest]]: [MySQL] 32,049 pass(es), 3 fail(s), and 0 exception(es).
[ View ]

And again with JUST the tests.

Status:Needs review» Needs work

The last submitted patch, menu-default-for-node-761648-115.patch, failed testing.

Status:Needs work» Reviewed & tested by the community

#114 is ready to go. It's very simple, well commented and well tested as testified by #115.

Status:Reviewed & tested by the community» Fixed

Awesome work!!

Update function is well-documented, and even includes a test to prove it works. :)

Committed to 7.x. A critical bites the dust! :D

Status:Fixed» Needs review

#57: 761648-57.node-menu-upgrade.patch queued for re-testing.

Status:Needs review» Fixed

Just upgraded my site to the current D7 dev that contains this patch. Ran update.php, cleared my site and browser cache, unfortunately that didn't seem to fix the issue. Still using the solution over here to get around this http://drupal.org/node/865702#comment-4015758

I just re-ran the (menu module - 7000) update, but it didn't help. The menu is still missing from the toolbar. http://awesomescreenshot.com/0caecnncc

animelion: Your site was already D7, correct? This patch will fix the issue for people upgrading from D6 to [correction: greater than] 7.2; you will unfortunately have to use other workarounds (should be able to fix things in the database as well as code... always back up first...)

mlncn, thanks for the reply. My site went from D6.22 -> D7.1 -> D7.dev

Not a huge biggie, I do appreciate the feedback.

Update: Above it says 7.1, I should have put 7.2

Well, wait now. Why wouldn't this be handling 7.1 -> 7.3 upgrades, too?

@webchick - by design if i'm reading this right, i think just to be extra-safe in not messing with existing-site data, but, not sure this check is really necessary

+      // If the site already has a custom menu setting for this node type (set
+      // on the initial upgrade to Drupal 7.0), don't override it.
+      if (!isset($type_menus)) {

That check is designed to prevent damage to the configuration where a user has already configured some menus for a given content type (it might not be just the initial D7 upgrade that did this). Perhaps it could emit a warning in this case - but it shouldn't go overwriting changes explicitly made by a user.

Ok, cool, thanks for the explanation. Let's leave it for now and see if we get other reports.

Removing tags (needs backport to D7 queue cleanup)

I'm facing the same problem, menu item disapearing from custom menu when i edit the node they are refering to.
I have always worked on 7.0.

And i really need to fix that before i release the site.
Any help would be greatly appreciated.

@WilliamB: you need to enable the menu for the content-type you are editing.

Edit the content type, eg /admin/structure/types/manage/page/edit - select the "menu" tab, then enable your custom menu. Otherwise, when you edit content, the control that selects which menu the content is placed in will revert to one of the enabled menus instead (removing it from your custom menu)

My problem is not all nodes of this content type are supposed to be displayed in the menu, and those that are arent't all in the same sub menu.
It's a complex menu with 4 layers.

If i enable my menu in the content type setup, the nodes will be automaticly added to this menu right?

And the users want to be able to set a menu title different from the node title.
If i've to add the node to the menu and then have to edit the menu item name it's not very intuitive.

Nonetheless thanks for your answer.

@WilliamB

Nodes are not automatically added to any menu. All enabling the menu in the content type does is make that menu available to nodes of that type.

The documentation here may help:
http://drupal.org/node/788972
http://drupal.org/documentation/modules/menu

@WilliamB: Looks like your problem is not related to D6 ▶ D7 upgrade. Please consider creating a new issue. Also, in the new issue, can you post steps to replicate your problem?

It's mostly unrelated to D6->D7 upgrade, apart from 7.0 didn't migrate the previously-global setting for "default menu for content" to the per-content-type. Anyway, I don't think there's a new issue here - jn2 and myself have explained the cause to WilliamB. But yes, no more posts to this issue :)

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Thanks for the patch :)

Issue tags:-needs backport to D7

Backport already done and available in Drupal 7.4.

http://drupal.org/node/1204634