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

/**
 * 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'];
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

grendzy’s picture

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.

AlexisWilke’s picture

FileSize
465 bytes

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.

grendzy’s picture

Status: Needs work » Needs review
pwolanin’s picture

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

AlexisWilke’s picture

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

jhedstrom’s picture

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.

AlexisWilke’s picture

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

jhedstrom’s picture

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?

AlexisWilke’s picture

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

catch’s picture

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.

Anonymous’s picture

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.

webchick’s picture

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

webchick’s picture

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?

Anonymous’s picture

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.

AlexisWilke’s picture

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.

/**
 * 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

Gábor Hojtsy’s picture

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.

jhedstrom’s picture

FileSize
1.33 KB

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

AlexisWilke’s picture

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

sun’s picture

Issue tags: +Needs tests

.

sun.core’s picture

Priority: Critical » Normal

Tests don't qualify as critical.

RaulMuroc’s picture

Status: Needs work » Needs review

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

Tor Arne Thune’s picture

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
ZenDoodles’s picture

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.

alansaviolobo queued 19: 326201.patch for re-testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

This code has changed so much in 8.x, I don't know that this is even relevant anymore. Might only be applicable to 7.x.

AlexisWilke’s picture

Assigned: AlexisWilke » Unassigned

  • webchick committed 610bc6f on 8.3.x
    #326210 by AlexisWhite and jhedstrom: Pass ->menu by reference to allow...

  • webchick committed 610bc6f on 8.3.x
    #326210 by AlexisWhite and jhedstrom: Pass ->menu by reference to allow...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed 610bc6f on 8.4.x
    #326210 by AlexisWhite and jhedstrom: Pass ->menu by reference to allow...

  • webchick committed 610bc6f on 8.4.x
    #326210 by AlexisWhite and jhedstrom: Pass ->menu by reference to allow...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

  • webchick committed 610bc6f on 9.1.x
    #326210 by AlexisWhite and jhedstrom: Pass ->menu by reference to allow...

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.