Creating a menu does not properly handle the name in menu_edit_menu_submit() for 3rd party modules to work properly

Still in d7

We also need to add 'menu_name' to the $item.
In my case, I got a notice with admin_language, in a hook_menu_link_alter(). (because menu_link_save() is called in menu_edit_menu_submit()).

So here is my patch.

Notice: Undefined index: menu_name in admin_language_menu_link_alter() (line 64 of /var/www/html/sites/all/modules/admin_language/admin_language.module).

I get this error when I try to add a category to the aggregator module. i have installed the patch but i still get the error. the category is created but the error persists.

Just stumbled over this when dealing with this: #2090403: Problem with adding a New Menu
It seems like the patch here would be the right place to fix the issue and it fixes it.
Making the adjustment in Entity menu links just a workaround.

Patch here solved the issue & looks reasonable. Starting a re-test and if it is green I'd say RTBC.

Test's came back green. So as said RTBC.

This is certainly odd but in the confines of a stable release we can't move drupal_alter('menu_link', $item); past the // Load defaults. in menu_link_save. What we can do, however, is to move that defaults array into a new private function, and add it in menu_edit_menu_submit so that menu_link_save gets a fully populated link...

Alternatively this should be a documentation fix because whatever else calls menu_link_save will not have a fully populated link and your alter implementations needs to be coded in a way to deal with that.

Actually let's do both: we should populate but also recommend alterers to do a $link += _menu_link_defaults() the beginning of an alter, costs nothing.

@chx Thanks a lot!
First shot at chx's proposal.

I think this is a good one. Here are the possible problems:

1. Some link alter is using the lack of certain properties to detect something. But what? Link newness? Everyone having two functioning brain cells would use mlid for that and we didn't add mlid. I can't figure a way to break here something.
2. Someone already has a function called menu_link_defaults. Your code is broken, you get to keep both parts. That's an asinine thing to even attempt, the core has a menu module, a bazillion menu_* functions , heck quite a few menu_link_* functions, stay the hell out of those namespaces.

Drafted CR https://www.drupal.org/node/2384123

@naveenvalecha Thanks for the CR! I think it contains all the required information.

Thanks for the CR, it's great! I very slightly edited it. This, however, gave me new insight -- do we really want to suggest in the API documentation to use this function? It'd create contrib modules that fatal with non-latest Drupal 7.x. I have wording for this in the CR, do we want to copy over?

-- do we really want to suggest in the API documentation to use this function?

I think yes bt I'm still new to writing the CR and other core issue queues so lets wait what @das-peter thinks ?

I'm torn apart between having the way to handle this in the API Doc and having a probably not existing function mentioned in the API Doc.
I don't think we've a documentation notion to define something's valid just as of a certain minor version, right? At least I can't remember I've seen such a notion before.
Removed that line of documentation in the updated patch.

Fail was a hiccup back to RTBC.

Updated documentation wording and added link to CR as per hint of chx via IRC.

Fail was a hiccup back to RTBC.

This patch doesn't seem to be fixing the original bug anymore? (Which was basically, as I understand it, that submit handlers which run after menu_edit_menu_submit() don't have any way to know that for a newly-inserted item, the menu name that was actually saved is different from what's in $form_state['values']['menu_name']. Maybe it's just up to them to know that particular detail, but another way to state the problem is that they have no access to the final saved menu/link at all, and it could have changed in various ways as a result of being saved.)

Now the issue with "Notice: Undefined index: menu_name in admin_language_menu_link_alter()" (which was brought up later, and doesn't seem related to the original issue)... I am not sure what benefit we get from fixing it only in this one instance? If any code implementing hook_menu_link_alter() is still going to have to deal with the possibility that they are getting an incomplete menu link, I don't see why passing them a complete menu link in this particular instance changes anything. Shouldn't we either fix it completely (by adding the defaults in menu_link_save() before the hook is invoked) or just leave things the way they are and document the behavior?

I am also not sure there are backwards-compatibility concerns with setting the defaults before invoking the hook. (Or if there are, why is it OK to do that in the most common case, as this patch does, but not other cases, i.e. not in menu_link_save() directly?) To be extra safe I guess we could just add that beforehand but not actually move any of the other code, i.e. continue to try adding the defaults a second time after the hook is invoked, and also leave the code that sets $item['external'] after the hook is invoked. If we did that I don't see any real backwards-compatibility concerns. If existing code were trying to rely on defaults not being set, that would be pretty broken already, right - since in the case of a programmatic menu_link_save() the defaults could already be set even now.

Also, thanks for getting started on the change notice. It looks like a good start. We'll probably have to rewrite it a bit eventually if we change direction here, and one small thing to fix while doing that is the links currently point to the wrong hook (hook_menu_alter() rather than hook_menu_link_alter()).

Hello,

I add a new patch to be compilant with Drupal 7.58.
No changes in the code base.

I add a new patch to be compliant with Drupal 7.63.
No changes in the code base.