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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

risoy’s picture

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.

heather’s picture

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

andypost’s picture

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

Gábor Hojtsy’s picture

@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?

Gábor Hojtsy’s picture

Title: When you edit items linked in custom menus, they move to the Main menu » Upgrade 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.

andypost’s picture

@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

risoy’s picture

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.

Jody Lynn’s picture

Status: Active » Needs review
FileSize
1.05 KB

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.

andypost’s picture

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

catch’s picture

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.

andypost’s picture

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

puregin’s picture

Issue tags: +D7 upgrade path
andypost’s picture

Status: Needs work » Needs review
FileSize
1.74 KB

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.

catch’s picture

Priority: Critical » Normal

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

chx’s picture

Issue tags: -D7 upgrade path

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

chx’s picture

Title: Upgrade path missing for more fine grained node menu selection » The menu admin screen allows adding nodes to menus not allowed for nodes
andypost’s picture

Title: The menu admin screen allows adding nodes to menus not allowed for nodes » Upgrade 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

Gábor Hojtsy’s picture

@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.

pwolanin’s picture

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

Gábor Hojtsy’s picture

@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:

   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:

  $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:

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

Gábor Hojtsy’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
1.22 KB

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.

andypost’s picture

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

Gábor Hojtsy’s picture

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

andypost’s picture

Let's test merged patch

andypost’s picture

Missed a second hunk, final patch

Status: Needs review » Needs work

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

andypost’s picture

Status: Needs work » Needs review
Issue tags: +D7 upgrade path
FileSize
2.85 KB

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

andypost’s picture

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()

podarok’s picture

subscribe

anselm.marie’s picture

subscribe

gaele’s picture

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.

gaele’s picture

Title: Upgrade path missing for more fine grained node menu selection » Menu 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

hansfn’s picture

subscribe.

jn2’s picture

subscribe

ceardach’s picture

subscribe

pixelite’s picture

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.

gaele’s picture

Status: Needs review » Reviewed & tested by the community

Upgrade path tested.

gaele’s picture

Issue tags: +Usability, +User interface

-

hcderaad’s picture

subscribe

sapox’s picture

subscribe

catch’s picture

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.

klonos’s picture

mlncn’s picture

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?

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
3.05 KB

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

catch’s picture

Issue tags: +Needs tests

Patch looks good but could use a test.

Shawn DeArmond’s picture

Patch works. Attached is an updated patch with tests.

All menu tests pass.

fgm’s picture

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.

iMiksu’s picture

subscribe

Taxoman’s picture

Subscribing.

Shawn DeArmond’s picture

@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().

Countzero’s picture

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

mlncn’s picture

Status: Needs review » Reviewed & tested by the community

Tested and critically acclaimed.

Shawn DeArmond’s picture

Issue tags: -Needs tests

No longer needs tests

catch’s picture

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.

catch’s picture

Issue tags: +Needs backport to D7

tagging.

ksenzee’s picture

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. :)

catch’s picture

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.

catch’s picture

Status: Patch (to be ported) » Needs work

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

ksenzee’s picture

Status: Needs work » Needs review
FileSize
2.25 KB

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.

catch’s picture

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.

ksenzee’s picture

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.

catch’s picture

clashar’s picture

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.

clashar’s picture

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?

clashar’s picture

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.

David_Rothstein’s picture

+    $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 :)

andypost’s picture

+++ 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.

David_Rothstein’s picture

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

ksenzee’s picture

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.

catch’s picture

Status: Reviewed & tested by the community » Needs work

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

catch’s picture

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.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

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

lyricnz’s picture

sun’s picture

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.

sun’s picture

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.

lyricnz’s picture

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.

lyricnz’s picture

Status: Needs work » Needs review
FileSize
1.83 KB

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.

lyricnz’s picture

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

lyricnz’s picture

Update comment on assert()

lyricnz’s picture

Updated test to check that link exists after node_save

catch’s picture

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.

lyricnz’s picture

Status: Needs work » Needs review
FileSize
1.06 KB

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

lyricnz’s picture

- 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

catch’s picture

Status: Needs review » Reviewed & tested by the community

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

sun’s picture

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.

lyricnz’s picture

Status: Needs work » Needs review
FileSize
3.04 KB
lyricnz’s picture

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

sun’s picture

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.

lyricnz’s picture

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.

lyricnz’s picture

Status: Needs work » Needs review
sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

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

lyricnz’s picture

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

klonos’s picture

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

David_Rothstein’s picture

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?

David_Rothstein’s picture

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

  // @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 :)

sun’s picture

@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.

lyricnz’s picture

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.

lyricnz’s picture

Title: Menu items disappear after upgrade or manual menu entry » Menu D6->D7 upgrade doesn't maintain node-menu configuration
Status: Needs work » Needs review
FileSize
1.84 KB

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.

klonos’s picture

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?

lyricnz’s picture

@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.

klonos’s picture

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

David_Rothstein’s picture

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).

lyricnz’s picture

@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?

sun’s picture

@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.

klonos’s picture

Title: Menu D6->D7 upgrade doesn't maintain node-menu configuration » Menu 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.

David_Rothstein’s picture

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.

xmacinfo’s picture

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?

lyricnz’s picture

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.

xmacinfo’s picture

@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. :-)

lyricnz’s picture

Status: Needs work » Needs review

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

lyricnz’s picture

And again with a test.

lyricnz’s picture

And again with JUST the tests.

Status: Needs review » Needs work

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

chx’s picture

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.

webchick’s picture

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

pizzaklaus’s picture

Status: Fixed » Needs review

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

webchick’s picture

Status: Needs review » Fixed
bryancasler’s picture

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

mlncn’s picture

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...)

bryancasler’s picture

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

webchick’s picture

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

mlncn’s picture

@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)) {
lyricnz’s picture

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.

webchick’s picture

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

aspilicious’s picture

Removing tags (needs backport to D7 queue cleanup)

tim.plunkett’s picture

WilliamB’s picture

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.

lyricnz’s picture

@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)

WilliamB’s picture

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.

jn2’s picture

@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

xmacinfo’s picture

@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?

lyricnz’s picture

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.

ap’s picture

Thanks for the patch :)

xmacinfo’s picture

Issue tags: -Needs backport to D7

Backport already done and available in Drupal 7.4.

http://drupal.org/node/1204634