menu link deleted on programmatically updated nodes [#1247506]

To reproduce:
- Create node and menu link to the node
- Programmatically call node_object_prepare and node_save for this node

Expected result:
- Menu link is kept :)

Result:
- Menu link to node is deleted

After some tracing and debugging this is (what I think is) going wrong...

On node_save the hook menu_node_save the following code is executed:
(menu.inc:2506)

    $link = &$node->menu;
    if (empty($link['enabled'])) {
      if (!empty($link['mlid'])) {
        menu_link_delete($link['mlid']);
      }
    }

If the node is submitted through an edit form then the node->menu is populated with the 'enabled' field. In that case the menu link stays. If the user unticks the "menu enabled" option then the menu link gets deleted after submitting just as expected.

However, if you do a node_save programmatically the node->menu is not (!) populated with the 'enabled' field. And because of that the menu link is deleted (see code above).

The 'enabled' field is only set when the hook menu_form_node_form_alter (menu.module:602) is executed (through an edit form).

Comment	File	Size	Author
#10	1247506-10.patch	386 bytes	vishal9619
#10
#6	menu-set_menu_enabled_when_it_is_enabled-1247506.patch	360 bytes	dman
#6
#1	1247506-1.patch	912 bytes	chrisdejager
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

chrisdejager CreditAttribution: chrisdejager commented 12 August 2011 at 14:22

Status:

Active

» Patch (to be ported)

File	Size
Menu module patch (menu.inc and menu.module)	912 bytes

As it is now, it is logical to delete a menu link (in menu_node_save) which is not marked as enabled. Aside from explicitly disabling a menu link (through an edit form) a mlid could be faulty and should be deleted accordingly.

That's why I build on this and would propose to set the 'enabled' attribute on a menu_link_load (menu.inc:2506):

function menu_link_load($mlid) {
  if (is_numeric($mlid)) {
    $query = db_select('menu_links', 'ml');
    $query->leftJoin('menu_router', 'm', 'm.path = ml.router_path');
    $query->fields('ml');
    // Weight should be taken from {menu_links}, not {menu_router}.
    $query->addField('ml', 'weight', 'link_weight');
    $query->fields('m');
    $query->condition('ml.mlid', $mlid);
    if ($item = $query->execute()->fetchAssoc()) {
      $item['weight'] = $item['link_weight'];
      $item['enabled'] = true; // If we are here we are pretty sure the mlid is valid
      _menu_link_translate($item);
      return $item;
    }
  }
  return FALSE;
}

With this change the menu_node_save will always keep the menu link no matter if the node is saved through an edit form or programmatically.

I would propose to change menu_form_node_form_alter (menu.module) line 627 to:

  $form['menu']['enabled'] = array(
    '#type' => 'checkbox',
    '#title' => t('Provide a menu link'),
    '#default_value' => (bool) $link['enabled'], // instead of => (int) (bool) $link['mlid'],
  );

In this way we use the result of the validation of the mlid in menu_link_load. I think that is more consistent.

Of course I tested this code and it solves my problem.

Comment #2

chrisdejager CreditAttribution: chrisdejager commented 12 August 2011 at 20:56

Priority:

Normal

» Minor

A short follow-up.

I overlooked (as a drupal newbie :)) the fact that normally on a node update one does not need to call node_object_prepare. You can only use node_load. Then the problem does not occur because $node is not populated with the menu link and the menu_node_save code is skipped.

But... I still think it is a minor bug because there could be use cases in which you would want to call node_object_prepare after a node_load. Using node_object_prepare without using a form should not result in a deleted menu link. A lot of node saving code posted around the internet uses node_object_prepare in cases in which it is not needed.

I'm curious what others are thinking...

The origin of this bug adventure is a bug in the Feeds module: http://drupal.org/node/1245094
I will post the patch in a minute there.

Comment #3

chrisdejager CreditAttribution: chrisdejager commented 13 August 2011 at 10:40

Status:

Patch (to be ported)

» Needs review

Comment #4

13 August 2011 at 10:42

Status:

Needs review

» Needs work

The last submitted patch, 1247506-1.patch, failed testing.

Comment #5

dman CreditAttribution: dman commented 15 April 2012 at 23:12

Agreed.
#1534356: Data loss - menu items unintentionally delete themselves if a node is updated via code (not via form UI) . Test case attached.

I endorse the (small and obvious) fix that this proposed patch provides.
It doesn't hurt anyone else, it *does* repair the current destructive behavior, and it removes the ambiguity of the transient 'enabled' flag that sorta turns up only on form UI, but is *expected* to be part of the actual menu data array come save time.

Comment #6

dman CreditAttribution: dman commented 15 April 2012 at 23:23

Version:	7.7	» 7.x-dev
Status:	Needs work	» Needs review

File	Size
menu-set_menu_enabled_when_it_is_enabled-1247506.patch	360 bytes

Same issue apples in D8.
Here's a re-write/replacement of the patch (D7)
This is really all that's needed to make the problem go away.

--- a/modules/menu/menu.module
+++ b/modules/menu/menu.module
@@ -587,6 +587,7 @@ function menu_node_prepare($node) {
       }
       if ($mlid) {
         $item = menu_link_load($mlid);
+        $item['enabled'] = TRUE;
       }
     }

I have a simpletest case that demonstrates it's broken, and that this patch would fix it

This applies to D8 also, and the fix can be applied directly there too