Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Broken off from #860974: Menu Links will not import/revert
Need to set customized to prevent rebuild from switching back to previous menu (results in duplicates). That part of the patch has been reviewed and looks good.
However, just realized that this may have issues when updating, as after removing the menu item from the feature, it will not fallback to where it was previously as customized was set (in general, need to consider what happens in different situations with different types of menu links).
Is this acceptable or does further cleanup needed?
Comment | File | Size | Author |
---|---|---|---|
#15 | menu_links_customized-927576-15.patch | 917 bytes | mpotter |
#10 | menu_links_customized-6.x-927576.patch | 952 bytes | kenorb |
#8 | menu_links_customized-927576-8.patch | 898 bytes | jweowu |
#6 | menu_links_customized-927576-6.patch | 998 bytes | ipouw |
860974_menu_links_customized.patch | 381 bytes | hefox | |
Comments
Comment #1
hefox CreditAttribution: hefox commentedThis makes menu items fuax exportables instead of real exportables; to make them back to real, something else has to track that the menu item has been customized by the user, etc. bah.
Setting to needs work for now
Comment #2
ezra-g CreditAttribution: ezra-g commentedThis change is incorporated into the patch at #860974: Menu Links will not import/revert.
Comment #3
DamienMcKennaPer @hefox, lets keep this open to separate out the different sub-issues with menus, mainly so we can deal with smaller patches.
Comment #4
ezra-g CreditAttribution: ezra-g commentedMarked #910456: Custom menu links not saved as a duplicate.
Comment #5
hefox CreditAttribution: hefox commentedIf commit
1) Fix many people's issues
2) Removed menu links will need to be manually removed.
Being that 2) exists for several other components, it seems acceptable to allow it for this one also as current problems can be QUITE bad, however I'd like at least one+ people to agree and not disagree with that.
Comment #6
ipouw CreditAttribution: ipouw commentedWhy not just include the 'customized' value in the export?
Comment #7
Dean Reilly CreditAttribution: Dean Reilly commented2) is already true of non-system menu links anyway so I don't think it's that much of an issue and in fact standardizing what happens to missing links is a good thing. On the other hand setting customized = 1 for every link feels like a bit of a hack and not what it was intended for.
I would tend to agree with ipouw that exporting the customized value is a better solution. It gives the user more control as a developer could change the customized value as needed (although I'm not sure if there is a use case for this). I also feel that this is a better fit with the features methodology in that it should export items as they are not as would be convenient.
This would make the case of what happens to menu links that are removed from features or after features are turned off even more unclear though. Hence, I think a good solution would be ipouw's patch and a proper garbage collector for menu links.
After jotting some ideas down the way I would see the garbage collector working is that each link in the database would have to have something which identified which feature it came from. We can then use a pipe_alter hook to add these links in when the feature is being built. Thus if a feature is updated and no longer contains a specific link it will show as being overridden. Finally, we can write code in the revert and disable functions to clean up any of these orphaned links.
I'm happy to have a go at writing a patch for this but thought I would mention it first and get some feedback as it's a fairly radical departure from what has been talked about so far.
Comment #8
jweowu CreditAttribution: jweowu commentedI'm pretty new to features, but Dean's suggestions in #7 certainly sound like a more robust and user-friendly approach to the problem.
Attaching a Drupal 7 (7.x-1.0-beta4) version of ipouw's patch in #6.
Comment #9
q0rban CreditAttribution: q0rban commentedI think what this issue is saying is that without the customized = 1, duplicate items will get created. Can someone please post explicit instructions on how to replicate that?
> 2) Removed menu links will need to be manually removed.
I agree, this is pretty common for pseudo exportables, such as roles, permissions, menu items, etc. I see no problem here.
I like ipouw's approach, and agree with Dean Reilly's sentiments about the flexibility it affords. Setting to needs review.
Comment #10
kenorb CreditAttribution: kenorb commentedAttached patch for Drupal 6.x.
Tested and it works.
Comment #11
kenorb CreditAttribution: kenorb commentedAh, sorry, it's actually the same as #6.
Comment #12
JvE CreditAttribution: JvE commentedD7 patch works for me
Comment #13
kenorb CreditAttribution: kenorb commentedComment #14
dooug CreditAttribution: dooug commentedWhen will this be ported? I can confirm it works (see my posts in #1330856: Menu links behaves unexpectedly when menu link stored elsewhere in code (e.g. Page manager, views, etc)). Please commit and save anyone else the headache I experienced!
Comment #15
mpotter CreditAttribution: mpotter commentedRe-rolling this for latest 7.x-2.x-dev since this needs to be fixed in D7 first and then backported to D6. By changing this from RTBC to patch-to-be-ported and leaving it in the 6.x queue, this fell in the cracks. Please always be careful about changing issue status and versions!
In any case, Drupal Commons is using this patch, so let's get this reviewed before the 2.0 stable release!
This should at least get it run through the automated tester.
Comment #16
jweowu CreditAttribution: jweowu commentedThe same (trivial) code change worked nicely for me in 7.x-1.0. I dare say this could be RTBC if someone on 7.x-2.x confirms that the patch still works as expected.
Comment #17
hefox CreditAttribution: hefox commentedThis caused some sort of side effect that meant multiple entries for same menu link created if I recall correctly, but that could just be 6.x and could be misremembering
Comment #18
mpotter CreditAttribution: mpotter commentedNot seeing any dup entries for menus, so committing this to 6fdb67b.