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'];
}
Comment | File | Size | Author |
---|---|---|---|
#19 | 326201.patch | 1.33 KB | jhedstrom |
#14 | drupal-menu-submit-7.x_1.patch | 1.54 KB | webchick |
#7 | drupal-menu-submit-7.x.patch | 1.54 KB | jhedstrom |
#7 | drupal-menu-submit-6.x.txt | 958 bytes | jhedstrom |
#2 | drupal-menu-submit-7.x.patch | 465 bytes | AlexisWilke |
Comments
Comment #1
grendzy CreditAttribution: grendzy commentedI 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.
Comment #2
AlexisWilke CreditAttribution: AlexisWilke commentedLooks 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
Comment #4
grendzy CreditAttribution: grendzy commentedmaybe a victim of #335122: Test clean HEAD after every commit ?
Comment #5
pwolanin CreditAttribution: pwolanin commentedthe 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().
Comment #6
AlexisWilke CreditAttribution: AlexisWilke commentedpwolanin,
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
Comment #7
jhedstromI'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
Comment #9
AlexisWilke CreditAttribution: AlexisWilke commentedjhedstrom,
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
Comment #10
jhedstromAll tests still pass for me on this one. Alexis, do you have any idea which test may be focusing on not passing by reference?
Comment #11
AlexisWilke CreditAttribution: AlexisWilke commentedI'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
Comment #12
catchOne 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.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedNeeds 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.
Comment #14
webchickRe-uploading the patch to see if testing bot is happy now.
Comment #15
webchickNice. 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?
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedFeel 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.
Comment #17
AlexisWilke CreditAttribution: AlexisWilke commenteddarthclue,
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.
Thank you.
Alexis Wilke
Comment #18
Gábor HojtsyAgreed, looks good, committed to Drupal 6. Bumping back to Drupal 7 for test case writing.
Comment #19
jhedstromHere'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).
Comment #20
AlexisWilke CreditAttribution: AlexisWilke commentedjhedstrom,
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
Comment #21
sun.
Comment #22
sun.core CreditAttribution: sun.core commentedTests don't qualify as critical.
Comment #24
RaulMuroc CreditAttribution: RaulMuroc commented#14: drupal-menu-submit-7.x_1.patch queued for re-testing.
Comment #25
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #26
ZenDoodles CreditAttribution: ZenDoodles commentedIf 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.
Comment #29
jhedstromThis 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.
Comment #30
AlexisWilke CreditAttribution: AlexisWilke commented