Revisit UUID handling for menu links

hi @ciss, I see that you took out the if module_exists 'uuid' , a bit risky, tests might fail because of that.

actually tests might not fail, but test this out generating a feature with these items involved and disabling the 'uuid' module, you might see errors.

I see some flaws that were simply taken from the old version in #2353585: UUID export and import of menu items linking to nodes, terms and users. Not sure if it is in scope to fix these here.

Strict comparison

A number of places where strict comparison could be used. (=== instead of ==).
(this replacement is not always safe, but in these cases it is)

Early return

_features_clean_menu_link_uuid_path() could use early return instead of nested if blocks.
The same applies, even more so, to _features_set_menu_link_uuid_path().

Variable names

The variable name $uri for an array is a bit silly because normally I would expect this to be a string.
Instead, I would name this $parts or $fragments.
On the other hand, the same variable name is used in uuid_uri_array_to_data() in uuid module itself, for the same thing.
What a sad world we live in.

Symmetry

I see a lack of symmetry:

• For uuid path -> internal path, we have _features_clean_menu_link_uuid_path() which operates on the path (string).
  This function also sets the 'router_path'.
• For internal path -> uuid path, we have _features_set_menu_link_uuid_path(), which operates on the entire menu link array.
  Unlike the above, this does not care about the 'router_path'.

I think it would make sense for both functions to consider (or set) the router path.
If the router path is not 'uuid', then no transformation should happen.
If the router path is 'uuid' but the uuid module is disabled, then.. not sure. I guess this case will usually not happen because the feature should have a dependency on uuid. I hope it does!

Doing this would require both functions to operate on the menu link array, and not just on the link path.

Conflict with #3082974

EDIT: This seems unjustified, see below

I am sure that this PR will conflict with #3082974: Break long statements to multiple lines (ternaries, conditions, function calls)..
It would be sweet if we could land this first :) just needs someone to review.
See also the pinned issues.

Not sure if it is in scope to fix these here.

I see two options:

1. Separate issues for each change beyond the main refactoring.
2. Separate commits in the same issue, using a feature branch.
   A range of commits can be exported as a patch with git format-patch.
   The main requirement here is that testbot does not get confused by this, and verifies that the sequence of commits is green.
   Or even better, each step in the range should give us a green test result.
   (I personally don't mind to look at github for this)

Conflict with #3082974

I notice that the other issue does not (yet) change features.menu.inc.
Perhaps it should?
I think it is ok, we don't need to do everything at once.

So this concern was unjustified.

Is the UUID integration covered by tests?

Not sure. Let us know what you find :)

Would it theoretically be possible to implement this via hooks on behalf of uuid, instead of relying on module_exists() calls?

Could be interesting, but indeed would be a follow-up I'd say.
If we introduce a hook, we should define a broader use case. E.g. to define different replacement patterns for different paths beyond just entities?

Scope concerns were the reason why I tried to keep the changes to a bare minimum.

I think we should focus on refactoring here.
I think at least some of my points can be addressed, especially the "symmetry" point, because this problem did not exist previously.

Symmetry

Perhaps we could use "symmetric" function names like "decode"/"encode"?
Not sure, maybe you have a better idea :)

And btw, is there a valid case where we want to store the actual uuid paths, and not have them replaced with internal paths on revert?
I think we can cover this by checking whether the 'uuid' key is set.

And btw I think here is another flaw in the existing implementation:
We set a key 'uuid', which sounds like this is the uuid for the menu link, but in fact this is just the uuid for the entity.
So more accurate would be to set two keys 'entity_uuid' and 'entity_type'. Without the type, the uuid is kinda pointless.

And another point.
_features_set_menu_link_uuid_path()
This has a special case for system path 'uuid', in which case it replaces the uuid path with an internal path.
Why do we need this? Why can't we leave those paths alone?

If we add tests, we should make sure to also cover such edge cases.
A round-trip from database to feature to database should leave the menu links unmodified..

And of course:

Is the UUID integration covered by tests?

Additional tests are always welcome. Moar better!

I think we need to step on the brakes. Imo this should have never been committed to Features. There were several calls in the original issue to implement the changes in uuid_features instead, which unfortunately got ignored.

@donquixote What's your take on the situation?

I only joined the list of maintainers very recently, and I have not studied the menu links integration in detail yet.
I imagine I would have complained more before committing this, from looking at the code. But I don't know yet if I would have fully rejected the idea.

The change got committed to 7.x-2.x, but is not part of any official release tag yet.
This means we make only some people angry, not everybody.
It also means that the next stable release will be delayed until we figure this out :(

Did you experience any real-world problems from this change?
The initial problem reported here was just code repetition, which in itself is not a practical problem.

There were several calls in the original issue to implement the changes in uuid_features

Might have been a good idea.
I am not sure how this would work, in some way the uuid_features would have had to intercept or replace the menu_links component hooks.

Title: Refactor duplicate code for UUID handling » Revisit UUID handling for menu links

Is it a good idea to re-purpose this issue, which already contains a patch with a different purpose? Everyone reading this will see the patch first and get confused.
Perhaps better to open a new issue for this meta discussion?

Two new issues:
#3085959: (meta) PSR-4 directories for (new) test classes
#3085965: Create tests for 'menu_links' component.

Revert f260688 to prevent adoption before all questions have been answered.

This was committed one year ago, so I think we can wait a few weeks more until we have a clear plan.

We might find a solution with a hook within features core, and additional code in uuid or uuid_features.
Perhaps this would allow features that already use this to remain (mostly) as they are.

I had a look at uuid and uuid_features.
Both modules implement hook_features_api() and define NEW components:

uuid_features_api():
- 'uuid_entities'

uuid_features_features_api():
- 'uuid_node'
- 'uuid_user'
[..]
- 'current_search'
[..]

Most of these (but not all) are entity type integrations.

The other hook implementations you mention in uuid and uuid_features are simply the component hooks that implement the component functionality for the components declared in the hook_features_api().

In #2353585: UUID export and import of menu items linking to nodes, terms and users we did NOT define a new component, but enhanced an existing component which is already living inside features core.
This, to me, is a strong argument why this change should be in features core and not in uuid_features or in uuid.
You need to make a strong case if you think this should be moved elsewhere.

Not sure what you mean.
Your observation does not surprise me: the menu link gets a new export id with uuid enabled.
This can cause some confusion.

But on a site where uuid is already enabled, and you expect it to remain enabled on all environments (staging, production, local), it will be (mostly) ok or not?

export menu links unmodified

Not sure what you mean by "unmodified".

Please take note of the missing dependency.

indeed.

(cross posting, going to respond to #26 later)

Btw something I would criticize:
The menu link paths for nodes etc are converted into other valid menu link paths (uuid/*).
This potentially conflicts with menu links that actually contain a uuid/* path. Not very likely because this is only for redirect.. but still.

Also this requires hardcoded assumptions about the default path for nodes, terms, etc.
And it would fail for unconventional paths like node/123/edit.

An alternative would have been something like this:
node/134 -> node/{uuid:node:4299314e-2d09-4322-affe-f01b8e48c1fd}.

Idea: Register replacement mechanisms, perhaps per wildcard loader, that replace uuid vs auto-increment id.

This way we only have to look at the fragment that contains the id, not the entire path.

Of course we should be careful with special characters in the features export id.

Are there any other examples of components that change their behavior depending on what additional modules are active? I can't think of any.

Probably not, at least not like this.
Changing the export id is quite a radical difference.
So yes, I agree this is a problem.

The path, as defined in the UI and stored in menu_links, can no longer be exported while UUID is active

Once uuid is enabled:
- With drush features-export you would have to specify the new export id based on the uuid path. Or maybe a path like "node/123" will be converted? Not sure.
- The ids in the FEATURENAME.info file no longer tell you the actual path.

But:
If you have local and production site, and uuid is enabled, you can still use the feature to sync the menu links. Even better now with uuid, because you no longer need to worry about different auto-increment ids.

So I don't see how it "can no longer be exported".

Are there any other examples of components that change their behavior depending on what additional modules are active? I can't think of any.

Now that I think about it:
Field instances get additional settings for "field condition", which can cause noise and annoying git merge conflicts.
There are probably more examples like this.
These do not change the export id, though.

If I were to design a menu links component from scratch:

Option 1:

• write a module that adds uuids to menu links. These uuids should be independent from the entity uuid of the link path!
  (or alternatively, invent a new menu links system with its own database table etc..)
• only allow menu links with uuid to be exported.
• use only the uuid as identifier.
• provide an API to replace auto-increment ids in link paths, to make them portable.
  This no longer messes with the export identifier, because that is the uuid from the menu link itself.

Option 2:

• Export the entire menu tree (of one menu) as a giant yml structure.
  This way we also get rid of weights!
• provide an API to replace auto-increment ids in link paths, to make them portable.
  This does not mess with the export hierarchy, because the hierarchy is independent of identifiers.

Stuff like this could be done in a 3rd party module, with a fresh and new component, thus avoiding disruption for existing projects.

I think we should instead attempt to minimize disruption from #2353585: UUID export and import of menu items linking to nodes, terms and users, while also respecting those who already use this as a patch or from the -dev branch.

If we are honest, if you were using the menu_links component before this change, your pleasure was probably limited.
You would have needed workarounds to avoid auto-increment mismatch, e.g.:

• regularly copy the production database to your localhost, to sync all node ids (but then you don't need to export your menu).
• store absolute urls (ouch)
• store path aliases, and somehow prevent them from being replaced with the system path.
• only link to panels and views pages instead of entity paths.

So the disruption from #2353585: UUID export and import of menu items linking to nodes, terms and users would only affect functionality that was already broken or deeply flawed.

Perhaps the best we could do, aside of some clean-up and refactoring, would be to make this new behavior optional.
Not sure where you would configure this, e.g. in the feature itself?

Proposal: Optional behavior

Perhaps the best we could do, aside of some clean-up and refactoring, would be to make this new behavior optional.
Not sure where you would configure this, e.g. in the feature itself?

Perhaps like this?
- Introduce a boolean config option which is disabled by default, configurable on site level (variable table).
- When exporting a new link, this config variable determines if the link should be rewritten using the entity uuid, or not.
- When exporting a menu link with rewritten url (and identifier), store some additional information that this was rewritten, distinguishing the item from a link that genuinely wants to link to the uuid path of the entity.
- When loading links from the feature, the site-level configuration setting should make no difference. This way the feature can be installed anywhere with the same effect.

I am still studying the menu_links component, to understand the implications. Not the nicest way to spend my time :/

Introduce a boolean config option which is disabled by default, configurable on site level (variable table).

Or store it in the feature *.info file, see #3089077: Pattern for options in generated *.info files..

Here's something I've come up with - it needs work on the export loading, but the export itself should work. And it includes some WIP test coverage. It's based on entity_menu_links that has been severely stripped down to just the Features pieces, removing the revision and Entity API pieces, which makes it much simpler.

Ok, this has a lot of fixes to #33 and is able to properly export menu items now.

Still to come - menu imports.

Once I complete this it could either be added as an optional submodule of Features or I can upload it as a new module, I don't care either way.

FWIW the file in #34 appears to work for imports too, I just need to work on the test coverage and then handle edge cases.

And now with test coverage for menu loading.

I've also provided the module as a patch against Features, to make it easier to e.g. add to a Drush Make file.

great work!

I would say, let's put this patch in the dev branch and let it sit there for a while.

Hi,
Great to see this new energy and ambition!

I would say, let's put this patch in the dev branch and let it sit there for a while.

I would prefer not to do this, at least until we have a new stable release out.
The last stable is more than one year old now, and since then a lot of adventurous changes were added.

Once I complete this it could either be added as an optional submodule of Features or I can upload it as a new module, I don't care either way.

If you want to publish a new contrib module, you would have more creative freedom, and could define your own release schedule etc.
I would link it on the module page and perhaps in other places, e.g. in the module itself - I am open for suggestions.

If someone thinks this should really be part of features itself, perhaps I could be convinced with good arguments :)

What remains to be done in this issue?
We still need to fix the existing menu_links features component, so that the next stable release won't cause confusion.

Next steps?

I think this issue should be about fixing the existing menu_links integration with maximum BC, while also respecting people who already use the -dev version.

If you want, we could open a new issue where we focus on a potential features_menu_uuid module, leaving it open whether this should be a new dedicated module or a submodule.

Perhaps you feel that you want to have some discussion and proof-of-concept-ing before you or we decide what we are going to do next.

I'd say this submodule belongs with the features project, the code isolation makes it safer being that it's mostly in the submodule.

However, maybe to cut the back and forth, make it it's own project and promote it, however personally I'd prefer it in the features project.

my 2 cents.

I worked on the last stable release, haven't looked at the more recent changes but maybe a new branch for the newest stuff , keep the current stable code in maintenance mode with future version php compatibility and security fixes only.

a few options to ponder. Great work Damien!

I think we need some more manual testing of the patch to cover different scenarios.

Can we continue here?
#3117102: Possible sub-module for menu links + uuid

I'd say this submodule belongs with the features project, the code isolation makes it safer being that it's mostly in the submodule.

The proposed submodule is different from currently existing functionality in features, it is the first time we would add a sub-module, and alter the database schema. So far this happened in separate contrib modules.

New issue, #3162806: Revert introduction of UUID entity path for menu links from #2353585.

see https://drupal.org/project/features_menu_uuid
And reported to be working patches in the features_menu_uuid issue queue

see comment #45 , if you need uuid handling for menu links, use the module mentioned in #45 and please use the two associated patches in the issue queue for that project.

And if anyone's interested in being a comaintainer of that module please let me know, I'm happy to add you, I don't use it on any sites anymore (I no longer work on the site it was built for).

@Joseph: I preemptively gave you full comaintainer access, have fun :)