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?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hefox’s picture

Status: Needs review » Needs work

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

ezra-g’s picture

Status: Needs work » Closed (duplicate)

This change is incorporated into the patch at #860974: Menu Links will not import/revert.

DamienMcKenna’s picture

Status: Closed (duplicate) » Needs work

Per @hefox, lets keep this open to separate out the different sub-issues with menus, mainly so we can deal with smaller patches.

ezra-g’s picture

hefox’s picture

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.

ipouw’s picture

Why not just include the 'customized' value in the export?

Dean Reilly’s picture

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.

jweowu’s picture

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.

q0rban’s picture

Status: Needs work » Needs review

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.

kenorb’s picture

Attached patch for Drupal 6.x.
Tested and it works.

kenorb’s picture

Status: Needs review » Reviewed & tested by the community

Ah, sorry, it's actually the same as #6.

JvE’s picture

D7 patch works for me

kenorb’s picture

Status: Reviewed & tested by the community » Patch (to be ported)
dooug’s picture

Priority: Normal » Major

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!

mpotter’s picture

Version: 6.x-1.x-dev » 7.x-2.x-dev
Status: Patch (to be ported) » Needs review
FileSize
917 bytes

Re-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.

jweowu’s picture

The 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.

hefox’s picture

This 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

mpotter’s picture

Status: Needs review » Fixed

Not seeing any dup entries for menus, so committing this to 6fdb67b.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.