Hey guys, haven't actually used this myself, but others on my team are running into a few fringe-cases while trying to use it with features. To explain, features requires and expects that all menu entry paths will be unique. Therefore, having a bunch of entries with "" makes export fail.

Would it be possible to allow "nolink" to be followed by something that can be unique? Like "" or something like that. We've only run into this with features, but I can imagine it might be an issue elsewhere, as I can easily imagine another module assuming that paths will be unique (since they usually are). If we could add some bit of uniqueness after, this would offer a workaround for folks.

Due to a number of other issues, we're actually abandoning our "hardline" feature approach, so we don't need to solve this, but just wanted to give a heads up.

Cheers!

CommentFileSizeAuthor
#18 special_menu_items-features_compat-1355088-18.patch6.03 KBciss
#17 special_menu_items-features_compat-1355088-17.patch5.7 KBciss
#14 interdiff-1355088-9-14.txt4.73 KBciss
#14 special_menu_items-features_compat-1355088-14.patch5.68 KBciss
#10 interdiff-1355088-6-9.txt540 bytesbc
#9 special_menu_items-features_compat-1355088-9.patch3.97 KBbc
#6 special_menu_items-features_compat-1355088-6.patch3.97 KBacrollet
#3 special_menu_exportable-1355088-3.patch4.04 KBJurriaanRoelofs
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gagarine’s picture

gagarine CreditAttribution: gagarine commented
Title: Features compatibility » Features compatibility (path need to be unique)

Interesting... But I think this have to be solved outside this module. See #1283742: UUIDs for Menu Links.

I keep this open, perhaps someone is going to arrive with a great idea.

patcon’s picture

patcon CreditAttribution: patcon commented

Thanks for considering :)

But don't be misled by that single post with an apparent solution. I added something myself, directing that person to the features issue queues, where menus are an ongoing discsussion. It's been awhile since I was following, but for whatever reason, it's hard to get a solid uuid solution with menus (something about how very little associated data is guaranteed to be both persistent and unique? I forget)

Anyhow, I don't believe it's resolved by any stretch. Cheers!

PS - Oh, and the suggestion that was lost by my filter unsuaveness was [nolink#hot_shit_uniqueness] or somthing of that nature :)

JurriaanRoelofs’s picture

JurriaanRoelofs CreditAttribution: JurriaanRoelofs commented
Status: Active » Needs review
FileSize
special_menu_exportable-1355088-3.patch4.04 KB

Hi, thanks for this cool module. This should really be in core because having <nolink> parent menu items in dropdown menus is the best way to make dropdown menus touchscreen friendly.

With this patch you can add arguments to the path, like <nolink>/27 or <nolink>/myblog or whatever, thus allowing for unique menu links. I tested this to work with UUID menu exportables and Im sure it will work with the regular features-native menu exports as well.

Please let me know if you're going to commit this to the module because then I'll be able to use the contrib module in my distributions, which I always prefer over custom(ized) modules.

gagarine’s picture

gagarine CreditAttribution: gagarine commented
Version: 7.x-1.x-dev » 7.x-2.x-dev
Status: Needs review » Needs work

Thanks for the patch. But I don't like you have to manually add /27 . Because you have to remember with number you use or not... -> bad UX.

I think it would be better to automatically create a hash if it's possible. We can even hide it from the user (or like fore node type, it's automatic but you can edit it if you want).

If we arrive to make good UX without ugly hack, I will definitely add this feature.

windmaomao’s picture

windmaomao CreditAttribution: windmaomao commented

without feature, how do we deploy our site ?

acrollet’s picture

acrollet CreditAttribution: acrollet commented
Status: Needs work » Needs review
FileSize
special_menu_items-features_compat-1355088-6.patch3.97 KB

Attached patch implements what I think gagarine was shooting for in #4. All nolink/separator links are stored with a unique path appended, but displayed as /, without the user needing to input or remember unique paths.

gagarine’s picture

gagarine CreditAttribution: gagarine commented

I like #6. Sadly, I don't have time to make extensive test. Specially, I was not able to tell by scanning the code if we do need an upgrade path.

So if some peoples can review this patch I will be happy to commit it.

dema501’s picture

dema501 CreditAttribution: dema501 commented

#6 works for me but i replaced 

-  if (preg_match('/^(<nolink>|<separator>)\/.*$/', $variables['path'])) {
+  if (preg_match('/^(<nolink>|<separator>).*$/', $variables['path'])) {

because path '<nolink>' won't work, only '<nolink>/blahblah' works fine after patching

bc’s picture

bc CreditAttribution: bc commented
Status: Needs review » Reviewed & tested by the community
FileSize
special_menu_items-features_compat-1355088-9.patch3.97 KB

for my own convenience and hopefully others' as well, i'll post a patch patched with dema501's fix in #8

it works well for me. it'd be great for this to be included in 7.x-2.x!

bc’s picture

bc CreditAttribution: bc commented
FileSize
interdiff-1355088-6-9.txt540 bytes

interdiff, because i just learned how to do it :)

bc’s picture

bc CreditAttribution: bc commented
Status: Reviewed & tested by the community » Needs work

After testing the feature creation and enable from scratch, I've found that the above patch doesn't work. I don't know what exactly the problem is, and I need to stop work on this for now.

I tested with both the 1.x and 2.x branches of features, to boot. Not using uuid, though.

FMB’s picture

FMB CreditAttribution: FMB commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

This patch (#9) works beautifully. bc: what might confuse you is that the path argument is not shown in the UI, it is only kept in the database.

ciss’s picture

ciss CreditAttribution: ciss commented
Status: Reviewed & tested by the community » Needs work

Just from glancing over the patch I can see that there are several places to improve it, e.g. using named sub patterns and lazy matching.

ciss’s picture

ciss CreditAttribution: ciss commented
Status: Needs work » Needs review
FileSize
special_menu_items-features_compat-1355088-14.patch5.68 KB
interdiff-1355088-9-14.txt4.73 KB

This patch moves the path check into a separate function and does some general cleanup. Compared to the earlier patch there are no huge functional changes, but the code should be more maintainable now.

I successfully ran a feature export and import of nolink items using this patch.

rooby’s picture

rooby CreditAttribution: rooby commented

I don't think this should be addressed in this module.

It is a features module bug and it makes this module more complex to hack around it.

This is the real issue #927566: Add link title to menu link identifiers to make them more unique..

There are almost certainly other modules out there that have this same issue and you can also get yourself in this spot without even using a menu item customisation module, so it is best this be fixed properly in features instead of a bunch of different ways across a bunch of different modules.

I would recommend closing this as duplciate/won't fix but I'll leave that to the maintainer.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Status: Needs review » Postponed

Marking this as 'postponed' until Features fixes its menu items bug.

ciss’s picture

ciss CreditAttribution: ciss at yousign GmbH commented
FileSize
special_menu_items-features_compat-1355088-17.patch5.7 KB

Untested reroll of #14 against 2.x HEAD (46955e1b, 2015-05-12). Leaving issue postponed.

ciss’s picture

ciss CreditAttribution: ciss at yousign GmbH commented
FileSize
special_menu_items-features_compat-1355088-18.patch6.03 KB

Another reroll of #14 against HEAD (69502551).