The following function copies the 'values'.'menu' in a private variable named $item. By doing so, all the info generated at the time the menu is saved in the database is lost.

I suggest we make $item a reference with & as shown below on the 1st line of the function (that & is not in the CVS.)

Why would you need that? Because I wrote an extension, http://drupal.org/project/cutemenu, and that extension alters the menu by adding a couple of items to the form. When the form is submitted, I need to save the result in my cutemenu table. I need to have the mlid for that purpose. Without this '&' I get 0 when the menu item is just being added. (Editing a menu works without the '&' since the mlid is set at the time the form is generated.)

<?php
/**
* Process menu and menu item add/edit form submissions.
*/
function menu_edit_item_submit($form, &$form_state) {
 
$item = &$form_state['values']['menu'];
 
// The value of "hidden" is the opposite of the value
  // supplied by the "enabled" checkbox.
 
$item['hidden'] = (int) !$item['enabled'];
  unset(
$item['enabled']);
 
$item['options']['attributes']['title'] = $item['description'];
  list(
$item['menu_name'], $item['plid']) = explode(':', $item['parent']);
  if (!
menu_link_save($item)) {
   
drupal_set_message(t('There was an error saving the menu link.'), 'error');
  }
 
$form_state['redirect'] = 'admin/build/menu-customize/' . $item['menu_name'];
}
?>
Files: 
CommentFileSizeAuthor
#19 326201.patch1.33 KBjhedstrom
PASSED: [[SimpleTest]]: [MySQL] 31,577 pass(es).
[ View ]
#14 drupal-menu-submit-7.x_1.patch1.54 KBwebchick
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-menu-submit-7.x_1_0.patch.
[ View ]
#7 drupal-menu-submit-7.x.patch1.54 KBjhedstrom
Passed: 9326 passes, 0 fails, 0 exceptions
[ View ]
#7 drupal-menu-submit-6.x.txt958 bytesjhedstrom
#2 drupal-menu-submit-7.x.patch465 bytesAlexisWilke
Passed: 9326 passes, 0 fails, 0 exceptions
[ View ]
drupal-menu-submit-7.x.patch439 bytesAlexisWilke
Failed: Failed to apply patch.
[ View ]

Comments

I too hit this problem, while working on a module that extends the menu system ( http://drupal.org/project/menu_icons ).

I proposed patch is exactly what I came up with. I can confirm it does work for the menu edit form on admin/build/menu-customize/*.

However, it doesn't help get the mlid when processing the node submit form. I think this patch is also needed:
#298515: Node_save does not update 'menu' array

BTW, I think it makes sense to commit this to 7.x first... but I would dearly love to see this backported to D6.

StatusFileSize
new465 bytes
Passed: 9326 passes, 0 fails, 0 exceptions
[ View ]

Looks like there was a problem with the patch. Is it because the path was wrong?

I'm re-submitting a new one. Just in case.

And I also vote for a backport since I'm using D6 too. 8-)

Thank you.
Alexis Wilke

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

the patch seems reasonable - I'm not sure if there would be a better way to do this. Basically this problem arises since links are not handled as 1st class objects, so don't have a post-save hook like hook_nodeapi_insert().

pwolanin,

Well... if you ask me, I'd scratch 100% the use of any array anywhere in Drupal. You'd pass objects, and those do not require an &. But we're not there yet... 8-)

Now, I'm not the only person who mentioned this problem.

grendzy reported having the same issue when attempting to enhance the menu (links) handling.

My patch makes the function work more or less like the other hook function calls (such as hook_nodeapi_insert() that you mentioned.) There is no other way I could see to fix my problem. But if you have a solution that would not require the application of this patch, then we could look into that instead.

Thank you.
Alexis Wilke

StatusFileSize
new958 bytes
new1.54 KB
Passed: 9326 passes, 0 fails, 0 exceptions
[ View ]

I've stumbled across #72039: Menu items created twice, where the consensus seems to be than the menu module should be smarter about that particular issue. As it turns out, this patch, with the addition of a small tweak to menu_nodeapi_insert() and menu_nodeapi_update() fixes the issue.

Also attached is a back-ported version for 6.x

Status:Needs review» Needs work

The last submitted patch failed testing.

jhedstrom,

I think that there is a test that ensures that the & is NOT there and that's why the patch will fail against the tests. We'd have to verify that, but it is not impossible that the author won't bother until it passes and that means getting the test fixed too. (okay that's really a total assumption!)

That makes it just a bit more complicated to get things to go through!

Thank you.
Alexis Wilke

Status:Needs work» Needs review

All tests still pass for me on this one. Alexis, do you have any idea which test may be focusing on not passing by reference?

I'm not too sure how it is run. They mention the class "File naming" as failing. Does that tell you something?

It could also be that the tests would fail even if the patch was not changing anything in the code! 8-)

We could test that by submitting a patch that only changes a comment! What do you think?

Alexis

One of the testing servers is having trouble with that particular test intermittently - setting back to needs review and resubmitting for testing will normally get you the right results.

Priority:Normal» Critical
Status:Needs review» Reviewed & tested by the community

Needs backport to D6.

At present, it is not possible to modify menu items or interact with them since the mlid is not passed.

I have tested the patch and confirmed that it applies cleanly and works as advertised.

StatusFileSize
new1.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-menu-submit-7.x_1_0.patch.
[ View ]

Re-uploading the patch to see if testing bot is happy now.

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

Nice. I've run into this with Workflow/Actions before. Also has the thumbs-up from the menu system maintainer.

Committed to HEAD. Moving back to 6.x for consideration. (see #7)

Because this is actively holding up D6 contributed modules from working properly, I didn't ask for tests, but I do think we need some to ensure that this stays working as intended in the future. Could one of you come up with a test case based on the modules referenced here that demonstrates the bug?

Feel free to hand me the moron cap ...

I've got no idea how to make a test case that demonstrates the bug. I'll gladly produce it if I know what it is.

darthclue,

Easy! 8-)

In the old (wrong) version, the $form_state is passed read-only and cannot be changed by the sub-functions.

So write a test that calls the function to create a new menu item and check that you get the menu item number in the $form_state (and not null or whatever it is when undefined, but I think that's null).

Does that make sense?

I show the function below. The problem is the copy of the $form_state variable in $item. It was missing the &. So the menu_link_save() function would modify the $item variable but it would not replicate the change in $form_state.

<?php
/**
* Process menu and menu item add/edit form submissions.
*/
function menu_edit_item_submit($form, &$form_state) {
 
// WRONG:
 
$item = $form_state['values']['menu'];
 
// FIXED:
 
$item = &$form_state['values']['menu'];
 
// The value of "hidden" is the opposite of the value
  // supplied by the "enabled" checkbox.
 
$item['hidden'] = (int) !$item['enabled'];
  unset(
$item['enabled']);
 
$item['options']['attributes']['title'] = $item['description'];
  list(
$item['menu_name'], $item['plid']) = explode(':', $item['parent']);
  if (!
menu_link_save($item)) {
   
drupal_set_message(t('There was an error saving the menu link.'), 'error');
  }
 
$form_state['redirect'] = 'admin/build/menu-customize/' . $item['menu_name'];
}
?>

Thank you.
Alexis Wilke

Version:6.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Needs work

Agreed, looks good, committed to Drupal 6. Bumping back to Drupal 7 for test case writing.

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

Here's the start of some tests. They still don't test this exact issue, but as I went in to start on that, I discovered that there wasn't even a test for altering the menu via a node form. This patch adds a simple test for that. To test the issue in this patch, I think that may need to be done via a different approach (perhaps a menu_test_nodeapi_update implementation that would re-save a node after an edit, then verify that the menu item isn't created twice).

jhedstrom,

There are two issues being talked about here. The one I had was only on a menu creation. So if you first create a menu, you do not get the mlid in the array. If the menu already exists, the mlid does not change and thus is already available and stays available in the array.

Thank you.
Alexis

Issue tags:+Needs tests

.

Priority:Critical» Normal

Tests don't qualify as critical.

Status:Needs work» Needs review

#14: drupal-menu-submit-7.x_1.patch queued for re-testing.

Title:menu_submit & mlid on menu creation(Needs tests) menu_submit & mlid on menu creation
Version:7.x-dev» 8.x-dev
Category:bug» task
Issue tags:+needs backport to D7

If I understand correctly, we want to ensure modules (like cutemenu) can act on a newly created mlid when implementing hook_node_submit(). I was initially confused because it seems like the form should not care what the newly created mlid is. I'd have probably used node_presave instead, but the part of beauty of Drupal is the flexibility of approaches...

So, since apparently the form does care now, we need a test which creates a new menu item on node_form_submit(), and asserts $form_state['values']['menu']['link']['mlid'] is set correctly.