commerce_kickstart_menus_menu_link_alter() should not change the plid of customized items.

Hi,
short explanation:
On links where the "customized" bit is set, the user has intentionally put this link in a specific location, under a specific parent item. Changing the plid via hook_menu_link_alter() is not an option.
http://drupalcontrib.org/api/drupal/contributions%21commerce_kickstart%2...

I don't know if this causes any real-world problems, but I would assume it does.

--------------

Longer explanation:
I came through this while trying to rewrite _menu_navigation_links_rebuild() for performance sake.
#1010480-64: Optimize _menu_navigation_links_rebuild()

During this work I bumped into hook_menu_link_alter().
This is a beast of a hook. I will explain why
http://drupalcontrib.org/api/drupal/drupal%21modules%21system%21system.a...

Where is the hook called?

The problem with the hook:
The hook is fired during menu_link_save(). This is called in multiple occasions:

• During _menu_navigation_links_rebuild(), for "non-customized" (*) links that are being reset to the newest state from menu_router.
• During _menu_navigation_links_rebuild(), for "customized" links that are moved around because the parent has changed or disappeared.
• On form submission, for then "customized" links, when the user wants to update this link.
• On form submission, for any links, when the user has modified a parent, and the children need to be moved around.

Maybe I missed some.

(*): A "customized" link is something a user has edited. During _menu_navigation_links_rebuild(), non-customized links are updated with data from menu_router: They get an updated title, etc. Customized links are not updated from menu_router, except for the router path.

What happens in hook implementations?

I would classify the implementations of hook_menu_link_alter() into these two classes:

• Implementations that look at plid, depth and menu_name, and adjust other values.
• Implementations that change the plid and menu_name (and thus the depth):
  commerce_kickstart_menus
  page_manager (that's disabled and tagged as "concept code")
  user (setting the plid to 0 - only for non-customized items).

Having both of these in one hook makes for a perfect chicken-and-egg problem.
(This alone could be solved by proper sorting of those hook implementations.)

Besides, this makes it really hard to work on _menu_navigation_links_rebuild():
- It is hard to predict where a menu item will end up in the tree, if in the last moment some module can send it somewhere else.
- We don't want modules to do select queries on a half-broken or out-of-date menu_links table.

If I were to decide that for core:
- hook_menu_link_alter() should be forbidden to set plid.
- Instead, core _menu_navigation_links_rebuild() should respect a 'parent_path' setting in menu_router. This can be set to FALSE, to set the plid to 0.

(this is just the background story - it does not all need to be fixed within kickstart)