Context Reaction: Set menu trail

I'm looking to replace my use of Menu Trails with context and the one missing piece is a reaction that sets the menu trail (as opposed to the active class).

Menu Trails (usage stats: over 8,000) allows designation of parent menu menu trails for node pages by node type and by taxonomy term. Typical use case: provide a persistent contextual sidebar menu (e.g., using Menu block). Menu trails more or less works, but lacks a lot of the flexibility that using Context would provide (and has significant outstanding bugs).

There would be several options for adding this functionality to context:
* Add into Context core as a new reaction.
* Add into Context core as a variation of the existing Menu reaction.
* Implement in a new contrib module.
* Implement in an existing contrib module, maybe Menu trails.

To get started, here's a patch that implements the first of these options, providing a new context reaction, "Menu trail."

Previous version had superfluous code.

See also http://drupal.org/project/context_menu_block.

Subscribe

This was posted to the Context: Menu Block issue queue here: #890584: Bundle this functionality in context itself?

To summarize my response there:

I don't think the issue Context: Menu Block solves would be solved by this patch. The original issue from which the module was borne (#751700: Setting active with Context Module) wasn't a confusion between active and active-trail: the use case for Context: Menu Block was that the active menu must be set, but Menu Block creates its own menu tree that's completely unaware of Context. Context: Menu Block merely alters the tree Menu Block creates to add the active class back in based on the current available contexts.

I think if you're going to combine functionality, it should probably be done in Menu Block, but if you wanted to somehow roll this into Context, telling people they need to redo their contexts (and themes) to use active trail rather than active menu is less than ideal. Rather, Context should implement hook_menu_block_tree_alter() to set the active menu.

But then we get to the question of whether Context should support every other contrib module under the sun. For that matter, should Menu Block have to account for every other contrib module? I don't think contrib modules should have to, which is why Context: Menu Block is being maintained (although I wouldn't complain if one did opt to support the other).

Tracking.

Re: #9, I don't think this feature is too contrib-specific to be in context. It's not really contrib-specific at all--as far as I can tell, the patch adds functionality that is responsive only to the core menu system. Also, I don't see why this patch means anyone would need to modify existing contexts or themes. It simply provides an additional feature; it doesn't change existing features.

I'll try upgrading to Context 3 to test this patch.

Be sure to use the -p0 option when you apply this patch; otherwise the new plugin is created in the wrong directory.

This patch (#4) meets my needs perfectly. (Wish I'd tested it yesterday before I spent hours trying to get this functionality a different way.) +1.

xjm: yes, it would provide an additional feature, but it's also being contended that this patch obviates Context: Menu Block (see: #890584: Bundle this functionality in context itself?) which I do not believe it does.

Re: #14 - Currently, the context_menu_block module does not wholly fix #2. So, this patch is still beneficial to Menu Block, even if it does not resolve #1. It also works well in combination with context_menu_block.module.

In any case, I think it is an extremely useful feature by itself. I'm already using it in five different contexts on my current site (only two of which are menu_block-related). I agree wholeheartedly with your summary of the patch.

Awesome, this just saved my day. Thanks!

Patch at #4 works really well. It probably should be committed to module or build as a separate module.

Tagging.

I have primary links displaying above secondary links, which are the second level of the primary links.

If I use the menu class reaction I only get active related css on the second level and if I use the new menu trail reaction I only get active related classes on the first level.

If I use them both together I get what I want.

Is this how it is supposed to be?

#5 the module fixed the menu trail problem when using menu block module with context module

subscribing

The current patch causes tab links to follow the active menu trail's setting instead of the node being viewed. So, for example, the edit tab will take you to the edit screen for the active menu trail instead of the edit screen for node being viewed. Since my use of this patch is confined to a context that is triggered solely by one content type, I did a hack in page.tpl.php to get the nid and and manually write the edit links for that content type.

I was thinking this might be an issue of module weighting? But I'm about 90% done with the site I'm working on and did not want to rock the boat :)

Just an FYI. If I have time to fix this, I'll post something. Or, is this intended behavior? (I hope not...)

jyg

@jyg the patch following active-trail is intended. Context: Menu Block follows the active menu.

This patch incorrectly sets the active trail condition when on node edit forms - if there are multiple contexts that specify 'set on node form' for a given content type, the active trail will be set on the first of these contexts alphabetically, regardless of the node type.

please commit this to 6.x-3.dev

I do not want menu_block and menutrail or any other modules if i am using Context.
Patch from #4 helped me strongly. don't want to think about patching during each update.

subscribing

subscribe

Is there any technical reason why the functionality in Context Menu Block cannot be integrated directly into the Context module?

@real_ate no, and speaking as the maintainer of Context: Menu Block, I'd actually rather prefer it.

But, as evidenced by how long this issue has been going on (and the issue that sparked the creation of Context: Menu Block, #751700: Setting active with Context Module), it was far easier to just make a separate integration project than it was to get code added to Context.

had to add a (empty) function "condition_form" to context_reaction_menu_trail.inc to make the patch from #4 work on Context 6.x-3.0

Will this patch be integrated in the next release? Considering the functionality it offers, it would be great!

subscribing.

better to commit it with this minor bug on node form then to patch each update...

Tried this patch from #4 and got:
Call to undefined function menu_trail_options()

I can't find that function in any of the code I have...what am I missing?

Subscribing, greetings, Martijn

Patch in number 4 does not work for me on 6.x-3.0

Fatal error: require_once() [function.require]: Failed opening required './sites/biloba.aegirwip.praece.com/modules/context/plugins/context_reaction_menu_trail.inc' (include_path='.:/usr/local/lib/php') in /var/aegir/platforms/pressflow-6.20.97-dev/sites/biloba.aegirwip.praece.com/modules/ctools/includes/plugins.inc on line 747

#36: See my comment in #12. You need to use the -p0 option for the file to be created in the correct location.

subscribe

subscribe

sub.

Same deal with us.

Trying to get context to play nice. Also looked at Menu_position which lights up the trail but no active state for the selected menu. That would be a nice option to select.

I would love to see context work without any additional modules or hacks.

+1

I had some problems using Context and Context menu block. In time before a solid solution is in place consider using Menu position, it did the trick for me :)

The only ways I can replicate this issue is if a content type is told to set the active-path to two separate paths with no other arguments in the context. This doesn't seem to be the module's fault as it's been provided with bad input. The other way I was able to replicate this issue was to tell the content type to have to different active paths with an added dependency ( such as taxonomy terms ) to differentiate which active path to set and neglected to check off the all conditions required box. Unless I missed the specific use case I would say this patch functions as intended unless you want it to do error checking and to look for contradictions. But in the instance where there are two possible contexts for the path to be set it seems reasonable that the module would pick which ever one it sees first.

That makes sense. Is there any reason not to incorporate the patch? It would be nice to have menu trails working finally.

Subscribe - stumbling into the same issue in D7 with the Nice Menus module (embedded in the template via its' own theme function)

+1

Any movement on this? It sounds like _not_ adding the patch is a much greater problem than the small issue that it may introduce in some cases. Could the maintainers please speak to the current status of this issue and what they feel needs to be done in order to get this patch committed?

Here's a new version of #4, just updated to be Git- and 7.x-3.x-compatible.

This solves an issue I was having that I don't think has been raised so far- my theme uses menu_tree_all_data() to build the menu, instead of $vars['main_menu'], so setting the active menu item in Context had absolutely no effect. Setting the active menu item using the new 'menu trail' plugin solves this. See #1098660: Megamenu: parent link not active when visiting child page for details.

Whoops, forgot to add the menu trail plugin file to the patch, try this one...

Hi Dane, thanks for your effort!

I tried your patch for this scenario: I have one panel that should work in two languages, which it won't due to this bug: #609542: Active trail isn't set on all menu items pointing to the current path - so, I want to set the active trail via Context using path + language as conditions, and menu trail as reaction.

What I get with this patch that I didn't have before is that I get the class "active" on the parent menu item, but nothing more.

What I expected from a set active trail are the classes "active-trail" and "active" (and probably "expanded") on the actual menu item I am on, and the class "active-trail" on the parent item. I believe these are the classes Drupal should respond with if the active menu trail is correctly set?

@vegardjo - sorry, I have only a very limited understanding of the menu / theme system, so I can't really answer your question - all I really did here was to update and apply an earlier patch :)

@Dane - ok, so have I :)

However, my understanding is that "setting the active trail" is more than adding classes to menu items, it's something Drupal does with the menu_set_active_trail function (http://api.drupal.org/api/drupal/includes--menu.inc/function/menu_set_ac...), and classes are added to the active menu item and parents due to that. And this is also what I hope could be accomplished via Context, but I have no idea if that is possible or out of scope for Context.

The way to control this (UI wise) would probably not be to specify the "active trail" as such, but specifying the active menu item would result in a valid active trail.

of course it is not about classes only. For example you need it to have menu items expanded.

There was a little error in file context.plugins.inc. Two times: $registry['reactions']['menu']. So only the menu trails functionallity survives.

I updated the issue summary, noting outstanding concerns (including one I added).

This patch is not the right way. As stated in #55 is will only set an active trail by setting classes. But it won't trigger - for example - a menu_block to show up which was configured as a level 2 (submenu) block and you selected a menu item at that level to be active for a specific node type. This is a very common usecase for nodes who don't have menu items but need to have an active submenu shown which is rendered through menu_block.

I think the solution is to use the new function (as from 7.9) menu_set_active_trail:
http://api.drupal.org/api/drupal/includes--menu.inc/function/menu_set_ac...
Testing this in a custom module at hook_init level works exactly as expected.

However, I wasn't able to use this function in context_reaction_menu.inc because (I think) it's fired too late? Is there a way to do this in context at an earlier stage?

See also: #942782: Custom menus never receive an active trail.

I'm not sure if that's really related (I'm using a core menu) but I think it should be able to fix this easily with menu_set_active_trail,
see also a recent patch in the menu_position module which commited this to fix a similar problem:
#979464: Use menu_tree_set_path() to set the active trail of menu trees

Updated issue summary.

@askibinski: Thanks for the issue link, that indeed looks like what's needed. I've updated the "Remaining tasks" summary accordingly.

Any updates on this?

Here's a patch using menu_tree_set_path. Using page_delivery_callback_alter based on menu_position's implementation.

Last patch was missing the new reaction file

I got this error when trying patch #65 against the git repository.

$ git apply context_menu_position_reaction-835090-65.patch
error: context_reaction_menu_position.inc: No such file or directory

The code is pretty short. I don't have the experience with context to review this, but do have the experience to roll up a patch again and hope that it makes it easier to apply for others.

It's just the reaction file (which was there in 65, but not in a way my git liked).

I do like this approach though. Think it will solve some of our problems when it gets into the module.

New reaction file needs to go in the plugins dir

In context_reaction_menu_position.inc:

      menu_tree_set_path(variable_get('menu_main_links_source', 'main-menu'), $path);
      menu_tree_set_path(variable_get('menu_secondary_links_source', 'secondary-links'), $path);

This seems to cause problems if the links themselves *aren't* in main-menu or secondary-menu.

As a preliminary fix, I changed this to simply query the menu_links table for all pertinent menus.

      $result = db_query("SELECT menu_name FROM menu_links WHERE link_path like :path", array(':path' => $path));
      foreach ($result as $menu) {
        menu_tree_set_path($menu->menu_name, $path);
      }

It is possible (though generally unlikely) that the same path exists in multiple menus. A better approach might be to save the menu name in addition to the path in the reaction setting so that both of these are known when executing.

I also implemented the correct directory as mentioned in #67.

Query should probably use "=" rather than "LIKE."

The patch at #69 looks good to me.

Looks good. Two remaining issues:

• The existing menu reaction is triggered in hook_preprocess_page(), while this patch puts the new menu reaction in hook_delivery_callback_alter(). If there's a reason for this difference, it should be documented in a code comment. If there is not a reason, the two should be consistent.
• The table name in the query is missing braces. Could be rewritten:
  

        $names = db_select('menu_links')
          ->fields('menu_links', array('menu_name'))
          ->condition('link_path', $path)
          ->execute()->fetchCol();
        foreach ($names as $name) {
          menu_tree_set_path($name, $path);
  

#71 First question, I believe #64 addresses this?

#71 Second question, braces added to attached patch. If someone wants to make this DBTNG-compliant, go for it.

#72 worked perfectly... EXACTLY what i was looking for, thank you

Hi, thanks for you patch.

Unfortunately I tried it and it doesn't make any change (even after clear cache twice).

I'm using context 7.x-3.0-beta3 and saw that as from this version the only upgrade is that the active item is highlighted (#1587444: Menu set active trail 'hack' breaks menu_position module in Drupal 7.14)

However, if the active item is not in the first level of the menu, the menu will not expand to show the item related. Is it what your patch was intended to do? If yes, I don't have the chance to see it working

Cheers

I could apply the patch #72 and everything seems to be fine, but the new reaction 'Menu Position' doesn't appear in the reaction list..
Did I miss something ?

I'm on a fresh Drupal 7.14 installation running on localhost, with last Context 7.x-3.0-beta3, and the new file 'context_reaction_menu_position.inc' is in the 'plugins' folder

[Edit] I had to clear the cache twice, then everything went fine.
Thanks a lot for this patch !

#72 didn't work for me : install ok, menu position reaction in the list and correctly set, but again, only the item is active, there's no inheritance and the active trail is not set.

you need to set the trail and the active menu item

Well, in this case, as stated in #74, something is missing : I can only set ONE item state with menu position. And I've not a trail reaction to set up.

#72 working for me :)

#72 works fine for me too.

I my opinion, both active trail and menu position should be merged..

#72 works for me :)

#72 working. Thanks for solving my problem. :)

#72, works me as well, thanks !!

#72 is working for me (thanks!), but loads of warnings in the log:

Strict warning: Only variables should be passed by reference in context_reaction_menu_position->execute() (line 11 of /context/plugins/context_reaction_menu_position.inc).

This is line 11, but I don't understand enough to fix it:

 if ($path = reset($this->get_active_paths())) {
      $result = db_query("SELECT menu_name FROM {menu_links} WHERE link_path = :path", array(':path' => $path));
      foreach ($result as $menu) {
        menu_tree_set_path($menu->menu_name, $path);
      }
    }

#72 Works for me too.

@pushka (#84): I can't reproduce why you get this warning. May you check the return value of $this->get_active_paths() in the case where you get the warning?

@tahiticlic (#78): "And I've not a trail reaction to set up." ... Clearing the cache after applying the patch worked for me.

#72 has been available since May 4 and it just saved the day for me. I'm using Context Beta 4 so I had to manually apply the patch by hand because the exact line numbers were a little off. It wasn't hard though.

Since so many people love this little patch, why hasn't it gotten into Context yet? Any way we can get this into Context Beta 5?

@pushka

i think your problem is a php version problem. you can try something like

$checkpath = reset($this->get_active_paths();
if ($path = $checkpath)) {
      $result = db_query("SELECT menu_name FROM {menu_links} WHERE link_path = :path", array(':path' => $path));
      foreach ($result as $menu) {
        menu_tree_set_path($menu->menu_name, $path);
      }
    }

hope it helps

Thanks for the tip. This worked without error in the end:

    $array = $this->get_active_paths();
    $checkpath = reset($array);
    if ($path = $checkpath) {

Error:

Strict warning: Only variables should be passed by reference in context_reaction_menu_position->execute() (line 11 of /.../modules/contrib/context/plugins/context_reaction_menu_position.inc).

#89 fixed that for me, I think.

Updated patch for the strict warning issue.

Screwed up the creation of the patch, take 2.

Additional patch against 3.0-beta4 (obviously meant for convenience not to be committed).

#94 - works for me.

I had to manually apply this patch though.

IMO - this really should replace the menu active class functionality (as proposed in #80)

RaF,

#94 is really only for use in a makefile, #93 should be the one tested.

Could'nt apply patch #93 against the current dev version (#72 stays functionnal).

Could not apply #93 against dev. But #94 applied cleanly against 7.x-3.0-beta5, and solved my issues with active menu trails. Thanks!

In patch #94, I think Drupal's built-in functions should be utilized, so instead of the following:

$result = db_query("SELECT menu_name FROM {menu_links} WHERE link_path = :path", array(':path' => $path));

this could be:

  $query = db_select('menu_links', 'ml')
      ->fields('ml', array('menu_name'))
      ->condition('ml.link_path', $path, '=');

  $result = $query->execute();

=================================

EDIT:

NO, I'm sorry, I reject that, I realized that it can mean too much unnecessary overhead for a simple task (too much function calls), and this is not needed.
I will also test the patch as soon as I have some time.

I'm sure #93 would have worked at one point or I wouldn't have posted it, but no doubt it would do with a simple re-roll from anyone who has a few seconds (which is not currently me even if I am writing this response). If I do have time in the future and the memory of the issue I will try to come back and re-roll it myself, but quite often Dev can be a moving target so there's no guarantee it will continue to work until it has been commmited, whereas while 3.0-beta4 is not a moving target and patch #94 should always work, it can't ever be committed.

Marking this as needs work as #93, if it is true what everyone is reporting, does need a re-roll for this to be ready.

This patch should apply nicely against the current dev version of context.

Made from #72 (c4rl) which add a new context and #93-94 (joey-santiago / Pushka / Deciphered) which resolves the strict warning issue.

EDIT: sorry, this patch is in fact identical as #94.

manually applied patch #94 on 7.x-3.0-beta6 and worked for me!

Thanks for the patch in #94. It applied gracefuly on beta6.

I can see the new reaction and set it, but unfortunately it doesn't do anything. I added the classic menu reaction as well, which correctly sets the "active' class, but I can't see any 'active-trail' class anywhere.

I'm trying to make it work on a basic custom menu with no fancy stuff, except it's included in a panel (the goal is to set the active trail depending on the variant). Could this be the cause of the problem ?

And BTW, does this have any chance to get committed some day ?

For those of you looking to get this fix on Nice Menus check #1331264: Integrate with Context UI for setting active menu?.

Applied cleanly to 7.x-3.0-beta4 for me as well. Resolved issues I was experiencing with trying to set the active trail through Contexts in combination with Superfish (see #1358066: Active-trail / Superfish and Context module. Worked better than proposed feature in #1862342: Override theme_link instead of theme_menu_link). +1 RTBC

@rocket_nova

If you think this is RTBC please change the status, putting +1 RTBC in your comment doesn't achieve much.

http://drupal.org/node/586396#comment-7064362

Patch in #94 worked great for me. Seems like this has worked for a number of people so updating the status.

Patch #94 works for me also, why cannot it be committed to dev? It does create a context.core.inc.orig file, but it should be easy to re-roll the patch.

Firstly: Patch #94 is not meant to be committed, ever, as it's rolled against a stable release. It's a stopgap patch for people who need it against a stable, instead it's patch #93 that should be being reviewed.

Secondly: Hounding contrib developers about why something can't be committed gets you no love. Contrib developers are people just like you and me who hold down day jobs as well as having there own personal lives, and they can't always guarantee that they can give time back to the community whenever the community demands.

If the patch works for you, let them know, if it's RTBC then there's not much more you can do other than offer your hand as a co-maintainer.

Update remaining tasks to reference recent API changes.

A key point to keep in mind is that "it works" is only one point to be considered in whether a patch is ready to be applied.

If you want to help it along, one step is to review and update the issue summary by reading through the issue and ensuring the summary accurately captures the solution and outstanding issues and so reflects the current status. I've just done so and noted some unaddressed issues previously raised, so setting to needs work.

The identified issues are all easy to assess or address.

Has anyone achieved active trails using three levels in jQuery? I have a section of nodes unattached to a menu but all attempts to expand the menu based on their path has been unsuccessful.

isnt this module what you need:
http://drupal.org/project/context_menu_block

I am not using menu block, so this module doesn't help me. But it does sound like the kind of solution I need!

So, I am having the problem that I think this issue is trying to address, and I came here today hoping to figure out how this issue was going. I read the issue summary and the latest few comments (obviously no one has time to read all 100+ comments on this issue)...

So it looks like we are supposed to be reviewing the patch in #93, but the patch in #93 only removes lines of code, so I cannot see how that is going to help resolve this problem. So... what are we actually supposed to be reviewing here?

@jhodgdon - At a glance it looks like the patch in #93 is backwards (should only be additions). Compare it to #94 and #101.

Could be. Actually I found a solution with Rules Bonus Pack so I am abandoning this issue for the moment anyway.

Yes indeed. Don't know how I rolled that patch, but #93 is most definitely backwards.

This is not working at all.

Repro:

1. Create a view with multiple tabs foo/bar1, foo/bar2, foo/bar3
2. Make foo/bar1 name "Foo 1" the default menu item and create a menu item
3. Create context:
   • with path foo/*
   • Reaction is Menu "Foo 1" active
4. Click on the tabs, "Foo 1" is never active-trail

In module menu_position this works properly.

I created a patch for the core context module which implements a reaction that sets the active menu trail as well as the breadcrumb. All in basically 8 lines of code.

This requires Drupal core 7.12+, but works like a charm.

Patch is missing a file.

More importantly though, there are already working solutions for this issue in previous patches, they may need a reroll at this point, but starting again from scratch just takes us back to square one....

Ugh... my bad, I'm relatively new to git, didn't expect that when I git add files, they don't show up in git diff (eh???).

It's so silly that I didn't notice Jibus's patch at all, looking at it, it's very nearly the same solution as mine, only, I set the breadcrumb as well as the active trail.
I do propose both cases should be supported, it's a fairly common case that one wants to set the menu trail as well as the breadcrumb and it's quite an unnecessary overhead calling two context reactions for pretty much the same task (as well as selecting the same thing twice on the UI).

The thing is, Jibus's solution should probably override the core context_reaction_menu reaction (which is just about useless the way it currently is), while breadcrumb is already a subclass of menu, hence Context does think that setting the breadcrumb should set the menu trail as well, but it simply does not work that way (yet).

OK, combined Jibus's patch and mine. Menu reaction now sets the menu path and menu active trail with this patch, while Breadcrumb pretty much remains the same (setting the breadcrumb only).
Hence, Menu sets the breadcrumb via the active trail as well if so configured by other modules (e.g. menu_breadcrumb‎).
I think this is probably the most generic (and modular) approach possible.

Just tried #124 and it is working for me so far.

Thanks!

+++ b/plugins/context_reaction_menu.incundefined
@@ -50,17 +50,29 @@ class context_reaction_menu extends context_reaction {
+    foreach($this->get_active_paths() as $path) {
+      $result = db_query("SELECT mlid, menu_name FROM {menu_links} WHERE link_path = :path", array(':path' => $path));
+      foreach ($result as $menu) {
+        menu_tree_set_path($menu->menu_name, $path);
+        $item = menu_link_load($menu->mlid);
+        $route = array();
+        while($item) {
+          array_unshift($route, $item);
+          $item = menu_link_load($item['plid']);
+        }
+        array_unshift($route, array(
+          'title' => t('Home'),
+          'href' => '<front>',
+          'link_path' => '',
+          'localized_options' => array(),
+          'type' => 0,
+        ));
+        menu_set_active_trail($route);

Database queries within a foreach loop should be a red flag. Can this be optimized?

Yes, indeed, this can be a bit more optimized, attached a new patch.

Please role against latest dev. You re-introduce fixed bugs here.

Er... sorry but what...?! This is a patch for the latest code in branch 7.x-3.x. Where exactly do I reintroduce bugs?

This patch is NOT roled against latest DEV.

+++ b/plugins/context_reaction_block.css
@@ -48,7 +48,7 @@
-  -webkit-user-select:none;
+-  -webkit-user-select:none;

reintroduced bug

+++ b/plugins/context_reaction_block.css
@@ -114,6 +114,7 @@ body.context-editing div.context-block-region {
+    padding:;

reintroduced bug

Fixed.

@Karsa,

Please always provide Interdiffs when re-rolling patches, it allows reviewers to see only the changes made between the last patch instead of having to dig through the whole patch in the chances that something else slipped in.

• https://drupal.org/documentation/git/interdiff
• http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-...

@Karsa,

You have a lot of changes happening in patch #131. Are you patching to your original patch mentioned at #120? I'm also not sure we want to be on the path of modifying context.core.inc for this feature request to work.

I think the right approach is the simple approach proposed by @Deciphered at #92/#93 & #94. It adds a new reaction instead of modifying an existing one. Changes to an existing reaction could produce unexpected results to people who already use it.

However, I think @Deciphered's patch doesn't quite do everything that it could. I have some code that is dependent on data returned from menu_get_active_trail(). The Menu Position reaction there doesn't affect a call to menu_get_active_trail() -- and I think it should. I found some code in the Menu Position module (menu_position) that I think will do more. The only issue is that I don't fully understand why it works :)
See update at end of comment

Change

class context_reaction_menu_position extends context_reaction_menu {
  /**
   * Set active in all applicable menus.
   */
  function execute(&$vars = NULL) {
    $active_paths = $this->get_active_paths();
    if ($path = reset($active_paths)) {
      $result = db_query("SELECT menu_name FROM {menu_links} WHERE link_path = :path", array(':path' => $path));
      foreach ($result as $menu) {
        menu_tree_set_path($menu->menu_name, $path);
      }
    }
  }
}

to

class context_reaction_menu_position extends context_reaction_menu {
  /**
   * Set active in all applicable menus.
   */
  function execute(&$vars = NULL) {
    $active_paths = $this->get_active_paths();
    if ($path = reset($active_paths)) {
      $result = db_query("SELECT menu_name FROM {menu_links} WHERE link_path = :path", array(':path' => $path));
      foreach ($result as $menu) {
        menu_tree_set_path($menu->menu_name, $path);

        // Not sure how or why this works, but it does. menu_get_active_trail
        // returns correct data.
        $preferred_links = &drupal_static('menu_link_get_preferred');
        $preferred_links[$_GET['q']][MENU_PREFERRED_LINK] = menu_link_get_preferred($path);

      }
    }
  }
}

Without this code in place, the Menu Position module (menu_position) also doesn't properly affect the result of menu_get_active_trail(). It can be found in menu_position_activate_rule().

Obviously those additions need some looking over by someone who better understands the Menu System better than I do.

PS - It appears that Breadcrumb trail is also set by this. So the Breadcrumb reaction isn't needed.

Update: Please ignore this comment. This addition is completely unnecessary. I think I was trying to solve another issue. Adding changes to preferred links will actually end up destroying the correct active trail if your path is on another menu entirely. Patches at #92/#93 & #94 seem to work fine for me.

#131 is broken. Fatal error: require_once() [function.require]: Failed opening required sites/all/modules/context/plugins/context_reaction_menu_position.inc'

Missed to clear caches.

#131 is RTBC.

I can also report that #131 seems to work well. I was able to change the breadcrumb (as seen by the theme) and the active menu trail, as seen by menu_get_active_trail().

My apologies for my garbage comment at #133 :)

..jon

I can also confirm patch at #131 works well.

#131 works for me as well and got Menu Block to follow the Context rules I set up. Looks great.

Changing the behavior of the existing menu condition is going to break sites that rely on the existing behavior, and possibly also cause unexpected side effects to sites that tried the menu condition, found it not to do what they expect, and implemented the functionality by another means.

So while I agree the existing menu reaction has only limited usefulness, I think it's safer to introduce a new reaction on a stable branch. Soooo I guess i'd still +1 101

Maybe the better option than having two reactions is to have one reaction with configurable functionality?

On existing sites, the checkbox would be off, therefore the new functionality would not come into affect, breaking anything, until the user made the choice to turn it on.

I would probably vote for the checkbox to be on by default on new contexts, but as long as it can be turned on I'm happy.

I'm asking me in what century this well working patch get's committed.

#131 works for me. Thanks!

As pointed out in #140, this is not RTBC, this has the potential to break sites that rely on the existing behaviour. This should not be RTBC'd again until that concern has been dealt with.

Can you explain this, please? Something that is broken and does not work cannot used on existing sites.therefore this fix only corrects a completly broken feature and does not change an existing feature as it is not working at all.

If I understand correctly, what @fearlsgroove is concerned about is that if a themer has themed the site based on the current markup, and then this patch 'fixes' this "issue" by adding additional classes, and said classes cause the site to act differently then it previously did (in a negative fashion) then this patch is therefore introducing issues into existing sites for anyone who upgrade the site.

While I accept that that is the risk in open source in general, and that you should always ensure you know what changes an upgraded module introduces and cater for it prior to deploying your new codebase, but that doesn't mean we shouldn't at least consider discussing the options and determining if they should be deal with.

If you all want to push ahead with this anyway, I won't stand in your way, as it doesn't really effect me personally, but as you can all currently use the existing patch, and having the issue marked RTBC doesn't guarantee it's going to get committed, I'd recommend spending a little time thinking about it.

I still don't get how this could ever break anything. This only set's a class on a menu item that is also added if we open a node of a menu subitem. This is not a new class, this class exists in other situations, too. We only add it here because it's missing. If a themer has overridden the function he will never see this class as the api itself is not changing.

Please don't hold up this patch any longer. This cannot have side effects. If you still think it has, please provide an example code to review and/or excact steps how this could ever break something. If this effects only one site we can still go forward and fix these major bug and the one site need to be upgraded. That's life.

I've done quite a bit of work on this patch (sorry - I know you all want it in ASAP!). The problem I have with it is that it doesn't properly cater for situations with multiple menus and Drupal's support for 'active_menu_names'.

The attached patch makes some pretty big changes:

- Allow selecting multiple menu items.
- Store a menu name with the selected menu item, so that you can choose the same menu item across multiple menus, or choose one menu item per active menu.
- Try to honour the active menu names (with a fallback) so that if 'main-menu' is active we'll look for preferred paths in the 'main-menu' first.
- Improved logic for looking up menu paths (using menu_link_get_preferred()).
- Updates to the breadcrumb plugin to match.

I've attached an interdiff between this and the patch at #124 which is the one I based my changes on.

I was encountering this issue with the Context module not correctly setting the active trail when used with the Superfish module (https://drupal.org/project/superfish). I applied Angry Dan's patch in #148 to Context 7.x-3.1, flushed my cache, and it seems to be working great now. Thanks! I'm looking forward to this patch getting implemented.

Update work needed.

Special thanks to "Angry Dan" (I like this name it reminds me Angry Bird)

However I can't understand why this issue continues to be open over the YEARS (Since more than 3 YEARS now).

Please subscribe once and for all, instead of being always updating and patching the site (once with menu position, the next day without, etc.)

I'm angry now ;)

Any chance patch #148 can get re-rolled against 7.x-3.2? I'm getting hunk failed errors.

re-rolled to 7.x-3.2

I have multiple items with the same URL path in a menu, but they have different parents (i.e. same menu, different trail). I cannot, by default, choose which trail is active when visiting the URL in question. I was attempting to use #153 to solve this problem, but it doesn't appear to have any effect. It tried setting both the Menu and Breadcrumb reactions. So either the patch is failing, or I'm missing something. I'm off to try Multiple Node Menu instead.

EDIT: Just in case anyone else runs into this same problem, see a solution I came up with in my answer for Putting Items in Multiple Places in a Menu.

As an aside, I recently become one of this module's maintainers. So if I am indeed missing the point of what we're trying to do here, would someone be kind enough to explain it? Thanks all.

Hi Colan,

Drupal and by virtue this patch doesn't define it's behaviour very well for multiple menu items pointing to the same target. I've ran into similar issues before and more often than not found the only solution was to duplicate the menu link which is far from ideal.

Drupal supports only a single "active trail" - anything else would cause confusion when we come to generate a breadcrumb, but it does allow the same link to reside in the menu in multiple places and will mark all of the entries as "active" when you do so.

I'm sorry to say that I don't think this patch, or indeed this context reaction is able to help you solve your problem, but as you are now a maintainer and as this patch appears to be in use and tested, maybe you could commit it for us?!

Thanks!

Yes, commit please!

Oh yes, please please ! U.U

I would like a commit too please!

Looking at the remaining tasks in the description, it looks like #3 is done, but I'm not sure about #1 & #2. Can someone speak to those? I'd like to make sure we're not missing anything before this gets committed. Thanks.

It's been a while since I wrote that patch now. I don't recall the exact reason for doing it in hook_delivery_callback_alter(), although I do recall being pretty sure that was the right place for it at the time.

I think this is the last hook that executes before menu_get_active_trail() gets called for the first time and therefore if we haven't executed by this time then Drupal will build an active trail only for it to be discarded later by our code. By executing at this early stage of the page build process (but after bootstrap) we pre-load the cache before Drupal has a chance to do it's own expensive evaluation.

In conclusion, I'm pretty sure I chose that hook as the most performant point in page load.

@rjacobs: The above info is good so I'll attempt to commit this soon. Thanks everyone!

Yeeeaaa !

\\o

o//

\ o /

Setting to Needs work for the patch in Comment #153 (the patch in Comment #131 works for me).

The multi-select option box does not show the menu items correctly for me. I have attached screenshots. One is of the Main menu manage page and the other is the same menu rendered in the multi-select option box in the menu reaction in context.

You'll see that the Events top-level item does not appear in the select options (along with the disabled menu items). Now, this may be because Events is a View, I don't know, but it is important that it shows for me because Events is the item I want to have the active menu trail on for this context.

If this problem is due to something on my end, please disregard.

Maybe a followup, You only need to find out the reason for this problem.

So... will it appear finally in the module?

@lahode: Indeed! I just pushed it to the dev branch. It will be in the next stable releases whenever one is cut.

If anyone has any related issues, please don't comment here. Open another ticket. Thanks!

Are we able to create a new release now, please?

There are a couple of other RTBCed issues I'll commit soon, and then cut a release after that. Will be out in the next week or two.

As a general note, this works pretty well, but if you have a few links in a menu which are the same paths, but with different query strings, then only the last of these menu items shows up, which is confusing when setting the context up. (I have a few menu items which point to the same view with exposed filters, and the query strings set default values for some of the exposed filters.)

I'm not quite sure if it's something that needs to be fixed soon, as I was able to inspect the html of the menu options list and find what I wanted, but it did spook me a bit.

This is now in 3.3. Rejoice!

This patch has introduced a bug. See referenced case, please.

This improvement has broken my "active" menu items in my site (probably).

See related please

3 users with the same issue introduced by this commit: see #2374445: Reaction MENU to set active menu item does not work anymore after upgrade to 7.x-3.3.

PLEASE: review or roll back this commit.

Thank you very much.

I kicked out the patch for this, and released a 3.4 with the reversion. It won't be reconsidered until the patch includes fixes for #2358313: Angle bracket in menu names double checkplained and #2374445: Reaction MENU to set active menu item does not work anymore after upgrade to 7.x-3.3.

That is a joke isn't it? Are you crazy? The angle brackets are a one line fix without any impact.

This rollback destroys all settings made. It is highly difficult to configure and one of the major troubles we cannot solve here is how the settings arrays are upgraded.

Sorry, I was a bit too hasty, and forgot about DB changes. It would have been better to not cut a release yet.

Can you still reapply this patch after upgrading? Your DB should be unaffected. I can update the release notes to say that this should be done if that functionality was being used. Or we can revert the reversion and release a 1.5.

Any guidance would be appreciated as I'm not using any of these features.

Please create a new release. Add the angle bracket patch before to get this done.

Done.