Posted by hefox on September 30, 2010 at 2:04pm
15 followers
| Project: | Features |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | patch (to be ported) |
Issue Summary
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?
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| 860974_menu_links_customized.patch | 381 bytes | Ignored: Check issue status. | None | None |
Comments
#1
This 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
#2
This change is incorporated into the patch at #860974: Menu Links will not import/revert.
#3
Per @hefox, lets keep this open to separate out the different sub-issues with menus, mainly so we can deal with smaller patches.
#4
Marked #910456: Custom menu links not saved as a duplicate.
#5
If 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.
#6
Why not just include the 'customized' value in the export?
#7
2) 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.
#8
I'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.
#9
I 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.
#10
Attached patch for Drupal 6.x.
Tested and it works.
#11
Ah, sorry, it's actually the same as #6.
#12
D7 patch works for me
#13
#14
When 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!