Wrong parent_identifier created for menu items

In menu_links_features_identifier() the $clean_title is computed like so:
https://git.drupalcode.org/project/features/blob/7.x-2.11/includes/featu...

/**
 * Callback for generating the menu link exportable identifier.
 */
function menu_links_features_identifier($link, $old = FALSE) {
  // Add some uniqueness to these identifiers, allowing multiple links with the same path, but different titles.
  $clean_title = features_clean_title(isset($link['title']) ? $link['title'] : $link['link_title']);

The parent identifier, without this patch, is/was computed like this:

         $clean_title = features_clean_title($parent['title']);

With this patch it would be like this instead:

         $clean_title = features_clean_title($parent['link_title']);

It seems both of them are wrong, and the correct thing would be like so:

         $clean_title = features_clean_title(isset($parent['title']) ? $parent['title'] : $parent['link_title']));

Or perhaps not?

See also these comments:
#927566-75: Add link title to menu link identifiers to make them more unique.
#927566-77: Add link title to menu link identifiers to make them more unique. ff

Should the menu exports be rewritten to depend upon UUID? There are just so many different possible flaws in the logic if each menu item is not treated uniquely.

Consider a menu structure like this:

• Fruit
  • Shapes
    • Spherical
    • Cylindrical
  • Colors
    • Green
    • Red
• Vegetables
  • Shapes
    • Spherical
    • Cylindrical
  • Colors
    • Green
    • Red

If this menu structure is exported, all of the third level items from the Vegetables menu will be relocated to the Fruit menu, so Fruit -> Shapes and Fruit -> Colors will have four items each while Vegetables -> Shapes and Vegetables -> Colors will not have any items.

I think all of us in the referenced thread (https://www.drupal.org/project/features/issues/927566) agree that UUID is the right solution, but there hasn't been a concerted effort to make it happen:

Proof-of-concept which adds UUID: https://www.drupal.org/project/features/issues/927566#comment-9690957

I think uuid is the right answer, but it has a much longer path to becoming stable, with both uuid and feature uuid integration at an infant stage. (from 2011: https://www.drupal.org/project/features/issues/927566#comment-5016020)

I agree that UUID has a long way to go before becoming usable, though it would seem like the best long-term solution. (from 2011: https://www.drupal.org/project/features/issues/927566#comment-5016868)

I think UUID is the optimum solution and would like to see it adopted more to try and drive more support to development of the UUID module. (from 2011: https://www.drupal.org/project/features/issues/927566#comment-5018016)

A hash would work but given the similarity in the approaches and the fact drupal is moving towards using UUID more and more I think I would still go with UUID. (from 2011: https://www.drupal.org/project/features/issues/927566#comment-5019002)

Could someone verify this patch as a solution, whilst we wait for UUID to get crackin. (from 2011: https://www.drupal.org/project/features/issues/927566#comment-6006828)

I created a sandbox here for the module I use to export and import menu links with Features.
It uses a UUID but does not depend on or integrate with the UUID module. (from 2013, sandbox is gone: https://www.drupal.org/project/features/issues/927566#comment-7146296)

andrewsuth: it cannot use the menu_id because that is not a unique value. On a different site it might conflict with an existing id. That's the whole problem with anything that uses sequential ids rather than uuids (like menu items). That is why it uses the link url instead. (from 2013: https://www.drupal.org/project/features/issues/927566#comment-7710307)

The fun thing is that if Features depended upon CTools it could have UUID support! UUID support was added to CTools 7.x-1.4 via #2155825: Add UUID generation functionality to CTools. Thanks for referencing other comments, I'll review them tomorrow and try to come up with a solution that leverages CTools.

Hi,
please have a look at #3085880: Revisit UUID handling for menu links.
The menu links integration are currently the reason why I am not releasing a new tagged release.

Some history (from memory)

In the last stable version 7.x-2.11, exported menu links are/were identified by some generated id based on the path and perhaps other criteria
Over time, more criteria were added to make this identifier more unique.

Of course this has some inherent problems:
- No matter which criteria you choose, there could always be two distinct menu links which share all those criteria and thus would have the same export id.
- If you change the values in that menu link, it will no longer be exported in the same generated id. Is this a problem? Not sure.

But there is another problem:
The path itself can contain auto-increment ids. E.g. a link to a node would contain the node id, which we would expect to be different on distinct environments (local, staging, etc).
This was "fixed" in #2353585: UUID export and import of menu items linking to nodes, terms and users, which is now part of the -dev version since a long enough time that we have to assume a significant number of people are already using it. Irresponsible? Perhaps..
Also, some code style fixes have been applied since then. So to revert it would at least be not fully trivial.

As I see it, this implementation is flawed and should not have been committed in this way.
Discussion in #3085880: Revisit UUID handling for menu links. This discussion itself is a while ago, so perhaps I will misrepresent some details.

One flaw I remember is that the entity uuid is saved as $link['uuid'] in the exported link.

function _features_set_menu_link_uuid_path(&$link) {
  [..]
      $link['uuid'] = $entity->uuid;

It seems wrong..
I cannot currently say if this is a big problem or not.

There might also be some BC problems.

Alternatives

For my taste, I would prefer to leave the menu_links implementation alone, or rather, produce a version that preserves maximum backwards compatibility both for users of the last stable version 7.x-2.11 and for users of 7.x-2.x.

If anyone really wants this fixed, I would recommend to write a new implementation from scratch, in a new module, which does not need to concern itself with BC.

Maybe I am missing something here, it has been a while since I last looked at this.

Should the menu exports be rewritten to depend upon UUID? There are just so many different possible flaws in the logic if each menu item is not treated uniquely.

Does your idea involve adding a uuid column to the menu_links table? Or would we have to store this in some other obscure way?
Changing the actual database storage to add UUID support would do more than features would normally do. So perhaps this should happen in a dedicated module e.g. menu_links_uuid.

Or is some contrib module already doing this?

This commit in uuid module looks interesting:
https://git.drupalcode.org/project/uuid/commit/8b6d70f1764a83896e7997121...

Drop menu support - use entity_menu_link.module instead

This points to this module:
https://www.drupal.org/project/entity_menu_links
I see this for the first time now :)
How would this work with features? Not sure, look at #1945760: How to deploy menu_links with this module.

Let's postpone this in favor of the other issue.

$link['link_title'] vs $link['title']

In #3 I found a discrepancy between the code that creates a link identifier, vs the code that creates the parent identifier.

For the link itself:

  $clean_title = features_clean_title(isset($link['title']) ? $link['title'] : $link['link_title']);

For the parent:

         $clean_title = features_clean_title($parent['title']);

This is already a problem, but only half of the story.

menu_link_load() vs features_menu_link_load()

The other difference is where the $link array comes from.

In some cases it comes from menu_link_load($mlid). In this case it will contain the 'link' key from menu_router table. Also it will pass through _menu_link_translate().

• In function menu_links_features_export_options()
• In menu_links_features_export_render() for the parent link.

In other cases it comes from features_menu_link_load($identifier). In this case it does NOT contain the 'link' entry.

• In menu_links_features_export()
• In menu_links_features_export_render() for the link itself.

Or it comes from the exported code in a feature:

• In menu_links_features_rebuild_ordered()

Solution: Always use 'link_title', always use features_menu_link_load().

I think the only safe solution is to always use 'link_title' and never 'title'.
Also always use features_menu_link_load() to avoid

By doing this, it no longer matters how the link was loaded.

Let's postpone this in favor of the other issue.

Let's fix this specific problem here.
The goal (for me) is to make features' own menu_links component more stable and predictable, while avoiding disruptive changes.
The more adventurous and disruptive changes can happen in the features_menu_uuid module.

+  if (!$qr = $q->execute()) {
+    return NULL;
+  }
+  if (!$link = $qr->fetchAssoc()) {
+    return NULL;
+  }

I think we should return FALSE to be consistent with menu_link_load() and features_menu_link_load().

The first if () is mostly to prevent the IDE from complaining about possible NULL returned from ->execute().