Trying out the new Menu Links support described here http://drupal.org/node/633804 I was unable to get the Menu Links to import/revert. Currently working with a fresh copy of Drupal 6.17 and the current Features dev.

I tried it two ways:

1. Made an sql dump, built the menu, exported the feature, imported the sql dump, turned on the feature, tried to revert Menu Links listed as OVERRIDDEN.

2. Built the menu, exported the feature, removed some menu items that were exported, enabled the feature, tried to revert Menu Links listed as OVERRIDDEN.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yhahn’s picture

Whoops, can you see if this commit fixes things? http://drupal.org/cvs?commit=396186

yhahn’s picture

Status: Active » Fixed

I believe this is fixed now, please open a new ticket if it reoccurs.

cabbiepete’s picture

Still occuring for me. I find I can only get the menu imported at all when enabling the menu.

Have issues with the items it is importing also but there are other bugs covering those so will update there.

cabbiepete’s picture

FileSize
2.03 KB

Attached the menu custom and menu links file out of the feature export that I am testing with.

yhahn’s picture

Status: Fixed » Postponed (maintainer needs more info)

Hm, using the files you've provided I get a menu called "M&G Administration Menu" with a handful of menu items as well.

Any more information you can provide as to why this might not be working for you?

hefox’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
381 bytes

Not sure if this will fix current issues, but it fixes mine

Link get saved correctly, but soon as a menu rebuild comes along, get reverted back, cause not set to 'custimized'.

In the original hook, the actually menu hook was altered so customized = 0 was good, but now since the actual menu router items are not changed, need to mark it as custmized or menu rebuild can wipe changes sometimes.

I'm also suspicious of this line,

if (!$existing || in_array($link, $menu_links)) {

Particularly the $link, $menu_links, should likely be

in_array($identifier, array_keys($menu_links))

But haven't gotten around to testing that, so just posting the first for now.

lobo235’s picture

FileSize
2.13 KB

I have a feature that contains our primary links menu and the feature when enabled/reverted does not restore the menu links correctly. I have attached the feature tarball. I am using Features version 6.x-1.0 and Drupal core 6.19.

jgraham’s picture

I was experiencing a similar issue. The patch provided in #6 applies cleanly to 1.0 and resolves the issue.

Without having dug into this too deeply I think in our case the issue was triggered for any link that already had an entry registered in menu_router. We have a few features with additional navigation options exposed in additional menus. This issue seemed to trigger consistently for items that were essentially duplicate navigational entries and not necessarily unique.

hefox’s picture

That is what was happening with also, jgraham; if it existed by default in another menu via hook_menu, it'd change to that menu, leading to duplicate menu links in the other menu >.O The patch just tells menu router that the links are custimized, so no touchie.

scottrigby’s picture

Status: Needs review » Reviewed & tested by the community

@hefox ty - #6 sorts this issue for us. Also, prior to this, drush cc would reset the menu items back to the module that already inserted them initially via hook_menu - and drush fr would place them back in our custom menu - causing a duplicate each time, but also requiring constantly reverting the feature. lovely. I'm mark RTBC, hopefully not prematurely ;)

jhedstrom’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
871 bytes

When exporting menu links with a parent item that was not defined by a feature (in this case, it was defined by a view), the fix in #6 didn't fix the issue for me. The attached patch gets this working, and still contains the change from #6.

hefox’s picture

jhedstrom: #910604: parent_path set to path not in hook_menu_default_menu_links causes menu link to not revert. Patch does the same TMK, but changes the first if statement instead making a new one.

Also, #920888: Issues when adding a non-customized menu link may be relavent.

(I'm a bit ornery at times about making a new issue for different bugs, or didn't really associate this with the other two.)

archnode’s picture

I did a little bit of testing on the issue because of a problem that might be related:

I tried to add a custom menu item as a parent item of a menu item that was provided by views. Both menu items have the same path. When I add the new menu item it marks the feature that provides the view with the menu item as overridden.

I can't export the custom menu item in a feature, it doesn't get displayed in the feature creation screen.
Neither one of the patches provided worked for me.

hefox’s picture

Keeping track of different issues in one issue is too complicated, so going to use this to post to other issues.

#927576: Menu links not set as customized, revert when menu rebuilt -- (#6 patch) Items not set as customized (see issue for why I broke it off, despite being reviewed).

#927566: Add link title to menu link identifiers to make them more unique. -- (#13) Archnode's issue (I had given it some thought a few weeks ago, but it's more complicated then it sounds initially).

#910604: parent_path set to path not in hook_menu_default_menu_links causes menu link to not revert -- (sorta #11) parent path not handled by features issue.

#920888: Issues when adding a non-customized menu link -- Export having values that for some reason the rebuild didn't like.

(jhedstrom, could you please review 910604?)

greg.harvey’s picture

I am getting a fifth, more generic case ... we have menus already on a stage site, when we change those menu items and re-export our feature, copy it up to stage and revert all features our menu items do not update. The only way to revert them to the feature's captured state in code is to physically delete each affected menu item, which could be hundreds.

Is anyone else seeing this? Is that what the patch here is supposed to address?

DamienMcKenna’s picture

Am seeing some oddity here too, will report what I find.

ezra-g’s picture

hefox’s picture

As mentioned in 14, keeping track of the various reasons why menu links don't import/revert correctly was getting messy in this issue, so created the other issue for that specific bug.

As being mentioned in #744450: Why would a feature not revert?, there's some many ways for features/feature components to not import/revert/etc., so these general issues turn fugly.

I believe I asked yhahn's opinion about how best to handle the issues before splitting it up, but it was a while ago.

Would it be objectional to undo the status change? (and mark this active, as I likely should have done earlier.).

ezra-g’s picture

Status: Needs review » Needs work

I'm running the conglomeration of patches here. I think we need an additional change which I'll describe before I reverse the other patches and re-roll this one ;):
When we do $existing = features_menu_link_load($identifier);, the subsequent conditional should only respect existing links if they are not customized, since one expects a customized menu link to be reverted to what's defined in the feature, even if the exported one is itself a custom link.

So, we should do if ((!$existing && !$link['customized']) || in_array($link, $menu_links)) {.

ezra-g’s picture

Status: Needs work » Reviewed & tested by the community

My apologies: I incorrectly marked this patch as "needs review" due to a code change that was in the scope of the patch at http://drupal.org/node/968826#comment-3804498, which I have re-rolled. Got confused with all these Features menu patches :).

In my experience this patch resolved the issue of menu links not being reverted and allowed me to successfully export hierarchical (parent and children) menu links.

It seems this patch fixes the issues described by folks in the issue, and seems unlikely that without being affected by these issues firsthand and being familiar with the menu system, that folks will take the time to review this issue.

Marking it RTBC in the hopes it gets committed and the benefit of more widespread testing in dev.

ezra-g’s picture

To be clear, I'm referring to #11.

wizonesolutions’s picture

Looking good! Let's get it committed!

...and, once again, I'm willing to help commit to 6.x. I noticed the 6.x -dev version isn't currently published...

kndr’s picture

#11 works for me. Thanks!

kasperg’s picture

#11 worked for me as well.

justafish’s picture

Any fix the 7.x branch? This appears to be broken for me in the latest beta.

scottrigby’s picture

@ezra-g - nice one. #11 also works for 6.x-1.0

Lc5’s picture

#11 works for me to.

KentBye’s picture

We're shooting a features video for Drupalize.me and we ran into this issue with 7.x-1.0-beta2
We created a menu item through a view and reverting wasn't removing it. Clearing the cache did help either.
We did find a brute force method of removing the menu item it by turning off the feature, deleting the View, and then turning the feature back on.

daveparrish’s picture

For version 6.x-1.0 I had the problem where views menu settings would take precedence over menu link settings. I had a menu item created by views and hidden in by the menu link settings. When I would revert the menu link settings the menu link settings would take effect but if I cleared the cache then the view settings would take effect.

My fix was to make sure all the settings were in the views for the weights and I simple removed menu items from views that were supposed to be hidden in menu link settings.

It seems to me that menu link settings should take precedence over the views settings since it seems that is the way it works in the Drupal by default.

bsnodgrass’s picture

I made changes to the menu link in a dev site. The menu link is coming from a view and the view is in another feature. I have the menu link also exported in another feature. Code looks as expected:

  // Exported menu link: menu-members-menu:secb-certificant-list
  $menu_links['menu-members-menu:secb-certificant-list'] = array(
    'menu_name' => 'menu-members-menu',
    'link_path' => 'secb-certificant-list',
    'router_path' => 'secb-certificant-list',
    'link_title' => 'Certificant List',
    'options' => array(
      'alter' => TRUE,
      'attributes' => array(
        'title' => '',
      ),
    ),
    'module' => 'system',
    'hidden' => '0',
    'external' => '0',
    'has_children' => '0',
    'expanded' => '0',
    'weight' => '-43',
  );

When I apply the features to staging, each feature shows as default but the existing menu 'link_title' in the database is actually overriding the feature.

hefox’s picture

Status: Reviewed & tested by the community » Active

All the patches are covered by other issues, this is being used more for a tracking issue.

Most people's main issue seems to be the customized issue #927576, which needs to decide whether to just commit it to fix the major bugs and deal with the fallout, or try and find some other way to handle it.

ice5nake’s picture

The inability to revert menu links has been a hindrance in our adoption of features.

Here's my drush features-diff output

Component: menu_links
        ),
      ),
<     'parent_path' => 'behavioral-sciences-and-education/staff',
      'router_path' => 'behavioral-sciences-and-education/open-positions',
<     'weight' => '-50',
    ),
    'primary-links:business-administration/open-positions' => array(
      'menu_name' => 'primary-links',
      'module' => 'system',
<     'options' => array(
<       '0' => array(
<         '0' => 'SBA Open Positions',
<       ),
<     ),
      'parent_path' => 'business-administration/staff',
      'router_path' => 'business-administration/open-positions',
<     'weight' => '-49',
---
>     'weight' => '-50',
    ),
    'primary-links:humanities/open-positions' => array(
      'menu_name' => 'primary-links',
      'module' => 'system',
<     'parent_path' => 'humanities/staff',
---
>     'options' => array(
>       '0' => array(
>         '0' => 'Humanities Open Positions',
>       ),
>     ),
      'router_path' => 'humanities/open-positions',
<     'weight' => '-50',
    ),
    'primary-links:science-engineering-technology/open-positions' => array(
        ),
      ),
<     'parent_path' => 'science-engineering-technology/staff',
      'router_path' => 'science-engineering-technology/open-positions',
<     'weight' => '-50',
    ),
  )
Grayside’s picture

@ice5nake menu_links being a poorly implemented system, you might need to manage them via update hooks and leave them out of your Feature export.

ice5nake’s picture

@Grayside, Any chance you can point me in a direction to the specifics of using update hooks?

ice5nake’s picture

Here's another thought, would this Feature request help solve this issue? http://drupal.org/node/968826

Would having uuid numbers for the menu links allow for proper reverting of the menu links. Conceptually it seems like it should.

mpotter’s picture

Status: Active » Closed (duplicate)

Closing this since there are other issues about adding unique ids to menu items and there is a chance that menu links may be dropped from Features entirely.

ezra-g’s picture

Status: Closed (duplicate) » Needs review

I spoke to mpotter in IRC and we agreed to keep this issue open. The D6 patch is essentially RTBC but needs to be re-rolled and tested against D7 (though there's a chance that the feature will be removed entirely for D7).

hefox’s picture

This issue was closed as it was a broad issue that covered various smaller bugs, and the indivudal bugs had/have their own issues, e.g. http://drupal.org/node/910604

mpotter’s picture

I closed #910604: parent_path set to path not in hook_menu_default_menu_links causes menu link to not revert for inactivity, but if somebody can clarify a patch between that issue and this one that applies to D7 then I'll look at it.

q0rban’s picture

Wow, this is a really confusing set of issues. Trying to grok everything that's going on across all the issues, but it's not easy. Can someone who understands the larger picture please update the summary? As far as I can tell there's really just this issue, about existing menu items not getting reverted, and #927576: Menu links not set as customized, revert when menu rebuilt, where the issue of links not being set as customized is being worked on.

The code itself is confusing as well, and I'm seeing a couple of red flags regarding this issue:

  • The big red flag to me is what is mentioned in comment #6, namely that in_array() is being passed two arrays, so the code to actually save a menu link will only fire if the item doesn't exist already.
  • We are iterating over an ordered list of all default links, disregarding what is contained in $menu_links. If the item doesn't exist, it's saved. Seems like this should only save a menu item that is in the $menu_links array.
  • Lack of commenting, making it hard to read what is going on and fix issues.
  • Not using drupal_static() (minor)

Attached patch addresses the above. The big departure here from the previous version of the patch is that every menu link that is being passed gets saved, instead of only items that did not exist. In addition, menu items that are not passed, are not saved. Because of that, one whole level of indentation was removed, as the if statement becomes unnecessary. It makes the patch look bigger than it is, but it's actually a pretty simple change, just doing an array intersection between the ordered array and the array of actual menu links to be reverted.

q0rban’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
tchopshop’s picture

Issue summary: View changes

I'm trying to save a highly customized Management menu via Features. This is a case where none of the menu links are nodes -- just system paths. I use this for my own responsive Admin menu. I cannot get the feature to revert, which means I have to redo the Management menu each time. Very frustrating. The patch in #40 doesn't work. I've tried all the other patches also.

goldenboy’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev
FileSize
2.33 KB

I changed the version of the bug to 7.x-2.x-dev because the last patch submitted refers to that version.

Hi, I think that the real problem is related to features_menu_link_load function. More precisely, where the link identified by parameter $identifier is compared to the ones extracted by the queries.

Consider the following use case:
A site that exposes 2 organisations in his main menu. The main menu will have following structure:

Org1 -- Social Organization
     `- About
Org2 -- Educational Organization
     `- About

When we export the links in a feature we'll get the following identifiers:

  1. main-menu_org1:path/org1
    1. main-menu_social-organization:path/org1/social
    2. main-menu_about:path/org1/about
  2. main-menu_org2:path/org2
    1. main-menu_education-organization:path/org2/edu
    2. main-menu_about:path/org2/about

Now, we have the feature and we want to import the menu in out production site. This is what happens.
When we revert the feature, features_menu_link_load is called for each exported link by menu_link_features_rebuild_ordered.

Inside features_menu_link_load, after executing the queries, we cycle over the result and, if the identifier isn't the same, we look at the title.

So, in the use case described, the second link with title about is matched and is returned.
It's not what you expect because the second about belongs to another subtree.
Instead, this behaviour could be accepted in case there are 2 links with the same title which are siblings.

So, IMHO, I think we need to consider the parent to solve this bug.

A solution is to add a parameter to the features_menu_link_load function to pass in the parent identifier. Then, inside the function we can load the parent, if exists, and modify the conditional statements to consider the parent when we compare the titles.

Attached there is a patch.

goldenboy’s picture

The last patch broke the export workflow. When features executes menu_links_features_export the title-match-condition doesn't work anymore.

Attached a patch that solve this problem too.

drupov’s picture

Just reporting that the patch from #44 (against 7.x-2.x-dev from 2014-Mar-28) does not solve the issue for me: the feature part containing the menu link on the target site does not get reverted.

dagomar’s picture

Agreed, this patch doesn't revert a menu link.

hefox’s picture

Status: Needs review » Postponed

Let's go back to creating separate issues for the identified issues that are clear on what the condition is. As can seen above, the issue is gaining different patches and reviews for different issues encountered and thus confusing to evaluate each patch

Rafal Lukawiecki’s picture

Is there a newer version of any of these patches—they fail to apply to the current dev branch? I am unable to revert menu links with both the current 7.x-2.11 and 7.x-2.x-dev versions of Features. 2.11 does not think that a menu needs reverting. 2.x-dev agrees they need reverting, but it fails to revert. Any suggestions much appreciated.