Reverting a node revision deletes menu item

This is possibly related to a call to menu_node_prepare($node) when the module can't figure out what menu the node is in. Because this writes back into the node, I suspect that this may be fiddling with the menu settings.

I've had a problem with the D6 version of the module calling this, because for nodes that aren't in the menu, calling prepare actually populates the default (primary links) menu as the menu for the node, even though it shouldn't be in the menu at all. It then purges primary links needlessly.

I've looked through the commit history, but can't find the rationale for calling this. Possibly it's to handle the case of brand new nodes being created?

If you want to try removing this call and instead bailing when no menu is set, try this patch. I've attached it for D6 and D7.

This patch worked for me.

Also, with this patch, expire correctly fires when I create a brand new node and expires all the new node's family of menu items--so the menu_node_prepare($node) call does not seem to be there to handle the case of nodes being created. Marking RTBC.

I had a similar issue to do with Expire and Workbench Moderation, in that creating a new draft deleted the existing node from the menu, unless that draft went straight to live.

This patch seems to fix that behaviour.

(One minor thing is that creating a new draft (that isn't live) triggers expiry, when in an ideal world it wouldn't expire all 500 cached nodes due to a new draft that may not go live for sometime - not sure if there's an existing issue on this?)

I had a situation where just editing a node would cause its menu item to disappear, if Expire was enabled. Not great. I haven't tested this patch but it looks good.

Hi all,

I did a little investigation on this:

First of all the background:
We had an issue with disappearing links on a serious project with OG Menus module, when saving a node within multiple menus. We fixed that and we thought it's over. And after some time authors started to report that their links disappear again.
We received those reports every day recently. After some investigations we realized that saving a node throw node form will not make the links disappear. We're using Panelizer and saving a panelizer node will cause the link to disappear.

Debugging pointed to Expire module which was recently installed to handle Varnish Purge urls.

Here is what we have:

When node_form is submitted the link will remain in place since this code will not be called:

if (!isset($node->menu['menu_name'])) {
  menu_node_prepare($node);
}

But sometimes we save the node in other ways, without submitting node_form. Like panelizer does. The problem with the above code that menu_node_prepare will prepare the link, but will not set $node->menu['enabled'] which is usually set when we submit the node_form.
After the node is prepared by expire mode, other hook_node_insert or hook_node_update are called. Like menu_node_save within menu_node_insert and menu_node_update.

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

So menu_node_save will verify if $link['enabled'] is empty and will delete the menu link. Which is ok, since this is core drupal behaviour.

We have to set $node->menu['enabled'] = (int) (bool) $node->menu['mlid']; so this will set the right setting for menu_node_save().

I've attached a patch for that.

Cheers,
Alexei.

#1 did not work for me.

#5 does work and appears very simple.

I've committed asgorobets patch in #5 to 7.x-1.x branch
Would very much appreciate a backport to 6.x-1.x of appropriate.

Hi all,
It's not reproducible for me in D6.
It looks like the problem with:

if (empty($link['enabled'])) {
  if (!empty($link['mlid'])) {
    menu_link_delete($link['mlid']);
  }
}

is not present in D6 version since there is 'Delete this menu item' checkbox instead of 'Provide a menu link'.

It seems I might be a little late here, as I see #5 looks like it's been committed to 7.x-1.x.

However, I'd like to suggest an alternative approach still. The problem, it seems to me, is that we get a node object passed into expire_node from expire_node_update, and as with all objects in php5 this is a reference. As previously mentioned, expire_node calls menu_node_prepare, which messes with the node's menu data ... and further saving has yet to happen on that node, so the "messed-with" menu data on the node gets processed as it's being saved (most likely because "expire" comes before "menu" alphabetically, so menu.module's hook_node_update runs later). In the context where this runs, the node should have no ->menu at all, unless passed into expire_node on the $node object itself. Leaving ->menu empty would result in no changes being made to the menu, whereas partially populating it can be destructive.

(Note - I'm talking about the D7 version here for now just because I've seen this issue mainly in D7, but can easily supply a D6 patch if the approach is accepted.)

#5 to me looks like it's messing further with the node object in order to work around the fact that it's been messed-with. It seems to me less error-prone to just sidestep the messing with the node in menu_node_prepare by cloning the node beforehand. I've looked around a bit to see if anything else is required from the menu_node_prepare, but it really looks like it's just the menu name, so this should work fine also.

Note that token.module had a near-identical issue of menu data loss and took this cloning approach as solution. cf #1317926: menu_tokens function should not change the node object (there are more areas of token affected than are obvious from this patch)

Note also that the only way to get the patch to apply against 7.x is to checkout a revision prior to the application of #5.

Note - I have also tested #5 and it also resolves the symptom for me, but I would personally prefer an approach that doesn't tamper with the node object (as in #9) over further tampering with it in order to mitigate earlier tampering.

I agree that the solution provided in #9 does sound more elegant and future proof. To leave a chance for others to chime in this discussion and make sure I don't have another hasty commit I will leave this as is until my next mini sprint this upcoming weekend.
A new version of this patch for the current .dev and a D6 version would still be appreciated.

Also please test and RTBC any other issue in this module's issue queue if you can find the time.. I think it deserves a beta tag since it's quite an essential module.

Thanks all!

Reverted #5 and Commited #9 to 7.x branch. Would appreciate a backport to 6.x still. Thanks!

I've been testing some more with these latest commits and have noticed these additional notices since applying the patch in #9:

Notice: Undefined variable: menu_node in expire_node() (line 266 of /home/paul/drupaldev/expire71/expire.module).
Notice: Trying to get property of non-object in expire_node() (line 266 of /home/paul/drupaldev/expire71/expire.module).

I've solved this by making the menu_tree_all data operate on the original $node-menu in cases the menu_node_prepare and the cloning of the node object is not being executed.

if (!isset($node->menu['menu_name'])) {
  $menu_node = clone $node;
  menu_node_prepare($menu_node);
  $menu = menu_tree_all_data($menu_node->menu['menu_name']);
}
  else {
  $menu = menu_tree_all_data($node->menu['menu_name']);
}

Please verify this addition is not breaking this fix, coding standards or kills kittens since I must admit I'm not too familiar with the menu api inner workings. Since the fix in #9 was already in I have also committed this additional as well. I have postponed the release I was planning over this weekend for now. Any additional feedback is very much appreciated. Thanks!

Okay, it's been rolled into the beta1 release. Leaving this issue for a backport to D6. Thanks all!

Edit

Drupal 6 is no longer supported. If this issue exists in Drupal 7, please reopen and provide details.