menu_tokens function should not change the node object [#1317926]

Problem/Motivation

menu_tokens function changes the node object with menu_node_prepare() if it has not a menu variable.

    if (!isset($node->menu)) {
       // Nodes do not have their menu links loaded via menu_node_load().
       menu_node_prepare($node);
    }

A loaded node object (with node_load()) has not menu variable, even if the node has a menu item. menu_node_save() does nothing, if the node object has no menu variable, but if it has, it will create, change or delete a menu item. In this case it will delete the menu item, if it exists.

So if a module loads a node, prepare it, generate node tokens then save it, menu_tokens will remove the node's menu item. Like in this case: #1230034: Menu link disappears when publishing a node by the moderation dropdown or by the moderation view link

Proposed resolution

The menu_tokens() function needs an 'mlid', and I think, the best way to get it in this case is still the menu_node_prepare() function, so my patch just using a new variable for the mlid, and then unset the menu variable, so node object will be unchanged.

I attach also a patch just for the tests, this patch should fail.

Remaining tasks

Review needed.

User interface changes

none

API changes

none

Comment	File	Size	Author
#5	menu_tokens-1317926-5.patch	1.16 KB	pp
#4	menu_tokens-test_only-1317926-4.patch	721 bytes	Désiré
#4	menu_tokens-1317926-4.patch	1.65 KB	Désiré
#3	menu_tokens-1317926-3.patch	1.57 KB	Désiré
#2	menu_tokens-test_only-1317926-1.patch	723 bytes	Désiré
#2	menu_tokens-1317926-1.patch	1.59 KB	Désiré
	menu_tokens.patch	1.59 KB	Désiré
	menu_tokens-test_only.patch	723 bytes	Désiré

Comments

Comment #1

Désiré commented 22 October 2011 at 17:52

Status:

Active

» Needs review

Comment #2

Désiré commented 22 October 2011 at 17:54

Status	File	Size
new	menu_tokens-1317926-1.patch	1.59 KB
new	menu_tokens-test_only-1317926-1.patch	723 bytes

(reupload patches for testing)

Comment #3

Désiré commented 22 October 2011 at 18:20

Status	File	Size
new	menu_tokens-1317926-3.patch	1.57 KB

better tests

Comment #4

Désiré commented 24 October 2011 at 13:59

Status	File	Size
new	menu_tokens-1317926-4.patch	1.65 KB
new	menu_tokens-test_only-1317926-4.patch	721 bytes

After some consultation and code review I've made a new solution and better tests.
(test only patch should fail on tests)

Comment #5

pp commented 24 October 2011 at 14:22

Status	File	Size
new	menu_tokens-1317926-5.patch	1.16 KB

I think it's enough to cloning the $node.

How reproduce this bug?

Comment #6

Désiré commented 25 October 2011 at 07:47

OK, you're patch is cleaner. :)

How reproduce this bug?

It not so simple, because it occurs only if the node object will saved with node_save() after token generation (token_generate()), like in my test:

// Test [node:menu] tokens for a loaded node.
$loaded_node = node_load($node->nid);
$this->assertTokens('node', array('node' => $loaded_node), $tokens);
// Save the loaded node after token generation.
node_save($loaded_node);

There is how was I reproduce it:
1 - create a node, check the 'Provide a menu link' option
2 - then run the following PHP code:

  $node = node_load(1); // replace the correct nid
  token_generate('node', array(), array('node' => $node));
  node_save($node);

3 - reload the page -> the menu item was deleted
(sorry I don't fount simpler reproduction method, but you can see, it's not impossible that a module need implements the steps above)