Allow setting the active menu trail for dynamically-generated menu paths.

menu_set_item?

This goooblybegedook caused the functionality to be missed from D6.

You want the menu open as if the current page would have a link under a certain parent.

That's all.

http://drupal.org/node/545052

In a hook_init():

$item = menu_get_item();
$item['href'] = '<the href of an existing menu link>';
menu_set_item(NULL, $item);

I don't see any drawback to this, but maybe there are some.

RE #5 -- I do, its a really strange way to accomplish a common need.

known workaround. can't be critical.

Damien's solution works great. I use it on pretty much every site I'm rolling out these days. It is kind of a weird solution though, I know. I got it from the menu trails module.

Maybe the menu system should support something like this with a cleaner solution, but for now I don't think this is a bug and isn't really a feature request since we can do it.

sub

No, this is only a very partial workaround -- and its one that only seems to work under the simplest conditions.

For one reason or another, hook_nodeapi(when $op = 'view', $page = TRUE) is where I remember it working best.
I think this because that state is relatively late in the page building processing so modules that may have tried to influence the active trail beforehand are overruled. There are contribs that attempt to deal with it -- but they only work for node paths -- and only half the time.

I actually think this is still a critical bug. However, I won't touch the status until I investigate it again in d7, and can provide an actual analysis based on code. My guess is that a correct solution may involve a hook since menu trails are so sensitive to what stage in the bootstrap -> page render process you are in especially given that many pages (e.g. page manager in panels) are not quite the same pages you'd assume.

Update: Okay -- more info. Panels node overrides were broken for a mysterious reason (very common when using this technique). This ended up being the solution. Pretty ugly -- but there's no alternatives known.

function menutrails_init() {
  // Allow Menu Trails to set the active menu and breadcrumb 
  // if the node_view is overwritten by the Panels module
  if (module_exists('panels') && 'node' == arg(0) && is_numeric(arg(1))) {
    // Get the node object and just call menutrails_nodeapi() 
    // to get Menu Trails do its node related stuff
    $node = node_load(arg(1));
    if (FALSE !== $node) {
      menutrails_nodeapi($node, 'view', NULL, TRUE);
    }
  }
}

They have a similar work around for OG. If there's a better hook that works then hook_init -- i haven't found it.

Was looking into this.. set up a menu:
-node/1
--taxonomy/term/2
---node/3 [this is our test of correct behavior]

Used the alleged workaround to set taxonomy/term/2 as the parent in nodeapi from node/2/view

function funklab_node_view($node, $view_mode) {
  if($view_mode == 'full' && $node->nid == 2) {
    menu_set_active_menu_names(array('main-menu'));
    $item = menu_get_item();
    $item['href'] = 'taxonomy/term/2';
    menu_set_item(NULL, $item);
  }  
}

Sometimes you gotta post images to really show how f#cked things are.
1. Okay, so picture one is the correct behavior where a node is really a term page's parent. Note the breadcrumbs.
2. Picture two is the behavior you get when you do this hack. Note the incorrect breadcrumb. The breadcrumb thinks its actually on taxonomy/term/2 not node/2 -- and its quite correct about that. After all we set an active menu item, not the parent of the current item! And there's no way to set the parent!
3. Picture three is a total WTF -- that's the actual page that picture 2 pretended to be. Note the breadcrumb is busted because taxonomy_term_page sets its breadcrumb in the page callback.

So no, we cannot set a term parent, we can only expand a menu tree by pretending to *be* the parent item. This behavior is total WTF. I agree with crell: setting it back to critical ( a regression from d5 to d6).

Note that Chx said this all along: http://drupal.org/node/520106#comment-1814730

sub

So after speaking with Crell on IRC we came full circle to a place in the code that chx had happened to mention earlier: http://api.drupal.org/api/function/menu_tree_page_data/6

My view is that the main problem with menu_set_active_trail is its name not that it doesn't work. I'm cooking up a solution that involves a hook ( a hook because this function fires and forgets and other functions cannot influence its return value at a certain point).

The goal is to make the functions that receive data from http://api.drupal.org/api/function/menu_tree_page_data/6 believe that an items in the database when in fact its not. Must. Find. Chx.

This patch merely shows two methods i'm experimenting with for setting a menu parent dynamically. This approach could be madness, so please review the place I'm setting the trail.

// basic principles of how the hook might work.
function funklab_menu_trail_alter(&$args) {
  if ($args[0] == 'node/6') {
    // objects like $node, and $user would be available through menu_get_object();
    $args[0] = 'taxonomy/term/4';
  }
}

// calling function
function funklab_node_view($node, $view_mode) {
  if($view_mode == 'full' && $node->nid == 7) {
    menu_set_dynamic_parent('taxonomy/term/4');
  }  
}

While menu_set_dynamic_parent($system_path) (not set on the name!) looks better from a DX standpoint, I actually lean more towards the hook method. The main reason being that menu_set_dynamic_parent() might seem like it could be called anywhere, and that's not at all the case. If we use a hook then themers won't be tempted to confuse themselves while looking up docs and doesn't require you understand how the menu is built from start to finish.

One thing to note is that this doesn't alter the breadcrumb trail unlike the hack method. I'm investigating that as well...

Two bugs are quite related -- unclear yet whether they dupe since breadcrumbs are currently separate from the mechanism that apparently decides whether an active trail expands a menu tree:
#689890: Breadcrumbs don't try hard enough.
#576290: Breadcrumbs don't work for dynamic paths & local tasks

subscribing

Marking this issue as a duplicate since a one line modification to sun's patch: #576290: Breadcrumbs don't work for dynamic paths & local tasks allows a technique to set the node's trail to the admin menu with both menu tree and breadcrumbs synced perfectly. [see: http://drupal.org/node/576290#comment-2515376]

We'll have a fix once #576290: Breadcrumbs don't work for dynamic paths & local tasks gets through.

sub

Subscribing.

sub

.

#576290: Breadcrumbs don't work for dynamic paths & local tasks passed tests for the first time and needs someone less obsessed with this issue than me to RTBC it or at the very least give a "I understand what's going on, and I see the light that this patch sheds." This bug will resolve quickly if someone is qualified, and willing to do that.

Sub

In preparation for the new "major" issue priority, I'm tagging this (not actually critical, but still important) issue as priority-major.
See: http://drupal.org/node/669048#comment-3019824 (#669048: We should have four priorities not three (introduce a new 'major' priority)) for more information about the coming major issue priority.

Ok, here's a first-draft, guaranteed-to-not-work patch.

The basic idea is that since menu_set_active_trail() is mostly useless, we should make it the go-to function when you want to alter the breadcrumbs and also the active trail used for trees generated by menu_tree_page_data(). The patch also adds a hook_menu_tree_alter() that allows alteration of the tree if you want to insert/modify items to the tree before node access and before it gets baked into the cache.

+1

And here's a patch without the PHP parse error. I told you it wouldn't work! ;-)

Also updated the new API docs.

rerolling #34, I'm not getting as many failures when running the tests locally

JohnAlbin: there are lots of 'Undefined index: localized_options' failures in the tests. When menu_get_active_breadcrumb() calls menu_get_active_trail, $active_trail seems to get populated with a mix of both menu links and router items. Router items don't have a localized_options key.

I tried to go further but menu_tree_page_data() makes my head hurt. Can you describe your approach in more detail?

subscribing

subscribing

#36: menu-set-active-trail-520106-34-reroll.patch queued for re-testing.

Changing to major as per tag.

Tag update

subscribe

I hope very much this will get fixed for D7. I think the concept of a single active trail is not ideal, though, and it would be better to be able to set multiple contexts for the current item. I admit I have spent no time at all considering the practicalities, but I think a node/view/panel should be able to have multiple parents - different vocabularies, site sections and so on.

A useful if horrible trick for D6 I found that's a bit easier to use than the ones mentioned is to do this:

$original_path = $_GET['q'];
$_GET['q'] = --- new path to be the desired context --
... generate menu ...
$_GET['q'] = $original_path;

You can call this in a custom block, wrapping a call to the menu_block, or at the theming stage. One advantage is that it can be done multiple times as needed.

hm, the menu_set_active hack in #5 appears to not work in D7 anyway. If this is true, it might merit raising the priority to critical again. [EDIT - nevermind, I think I was misled by a cache somewhere. Calling menu_set_item() in a hook_init() still works.]

I'm not sure I understand how this patch solves the issue - could someone post a summary? How does the proposed hook_menu_tree_alter() relate to the basic task of setting an active trail?

Here's an updated patch.

BTW, this patch seems to require this issue to be fixed before all of its test failures shake out: #233807: No navigation links on 404 pages

Looking at this patch, it looks like it is possible that #907690: Breadcrumbs don't work for dynamic paths & local tasks #2 might resolve this issue to some extent, too.

+++ includes/menu.inc
@@ -1057,7 +1057,19 @@ function menu_tree_page_data($menu_name, $max_depth = NULL) {
+  $active_trail = menu_get_active_trail();

This can too easily lead to infinite recursion, as menu_set_active_trail() calls back into menu_tree_page_data() for an automatically figured out $menu_name.

Furthermore, I suspect a major performance decrease with this change, since building the active trail is a very slow operation.

Powered by Dreditor.

A performance decrease? The first time menu_get_active_trail() runs, it caches its result. And, even without this patch, menu_get_active_trail() is called on almost all page requests (since its the primary source for breadcrumbs), so this patch doesn't change that.

But perhaps you mean the way menu_set_active_trail() will work after #907690: Breadcrumbs don't work for dynamic paths & local tasks #2 lands. I'll have to reevaluate this patch after that lands since I agree that 907690 should land first.

#907690: Breadcrumbs don't work for dynamic paths & local tasks #2 landed.

In the name of damage control and webchick's call to elevate issues to critical I am presenting this trivial patch which will allow you to set whatever active trail you want for the current menu tree. This is not the best solution we could invent as you need to figure out the plids yourself but it's too late for anything nicer.

Looks reasonable, but how actually fixes? This hook would be called only once, so there's no context to detect actual trail to set

The context is the page. Enhanced docs and hook name 'cos webchick asked to do that.

It's really unfortunate that we didn't give this a better architecture in D6

I'd wish to either rip out menu_set_active_trail() or hold out for a better architecture. chx is beating me down though.

+++ modules/system/system.api.php	2010-11-14 20:32:45 +0000
@@ -1186,6 +1186,25 @@ function hook_translated_menu_link_alter
+ * Alter the current page menu tree parameters like the active trial.

Typo: "trial" -> "trail"

Doesn't qualify for a "needs work", but if someone happens to reroll...

Powered by Dreditor.

I'd like to leave this a few hours for JohnAlbin to chime in. But this addresses my earlier (offline) feedback that hook_menu_tree_page_data_parameters_alter() is a ridiculous name, and the PHPDoc seems fairly clear now what this hook is for.

This patch is also technically edging into "feature" territory, as it were, since this architecture limitation also exists in D6. But if we're going to fix it in D7, now or never.

That typo would plague me: I rerolled the patch to fix it. Marking as "needs review" just to trigger the testbot.

As far as I know, the bot takes RTBC patches as well.

+++ includes/menu.inc	2010-11-14 20:27:35 +0000
@@ -1207,6 +1207,7 @@ function menu_tree_page_data($menu_name,
+        drupal_alter('menu_tree_active_trail', $tree_parameters, $cid);

Can we rename to hook_menu_tree_page_data_alter() or something? While we're indeed solving the issue at hand, this alter hook technically allows for much more, as you can tweak all tree parameters for any menu tree that is output.

Overall, two directions are possible: 1) Reduce the alter hook invocation to solve the issue at hand, or 2) Keep the current code but name it accordingly (and perhaps also provide means to identify whether we're actually dealing with the menu that contains the active trail... AFAIK, that's close to impossible in current HEAD without introducing a major performance decrease... which in turn makes me question the actual usefulness of this patch, sorry :-/ )

Powered by Dreditor.

Agreed the hook name is not the best; we kicked around a few ideas and this was the closest.

We talked about naming it something more akin to hook_menu_tree_page_data_alter(). The only problem (which chx could elucidate on more) is that menu tree data is actually built in several places, and we're only allowing altering of one of them. Naturally, you might say, "Well let's move it up the stack to where the data gets built then" but we also talked about that, and the problem is that these other places don't get cached, where this one does. So the performance impact here is minimal, and it solves the bug (we think).

I looked long and hard at this patch to see if it could remove the icky, icky cache-munging awefulness in the bowels of http://drupal.org/project/menu_position

And it looks like I can use the hook_menu_tree_active_trail() to add a $tree_parameters['active_trail'] array that forces the path to the one I want.

Over IRC, chx and I discussed how a much cleaner, unwritten, API-changing approach would be better, but it's way too late in the game for that.

So RTBC+1 from me.

chx and I talked about this a bit more. This fix just really doesn't feel right; the fact that we're tossing an alter hook into this deep, dark corner of the menu system that only like 4 people on earth understand and exposing it to module developers gives me the heebie jeebies.

I'm not sure what a different approach would be, but I don't feel comfortable committing this patch unless it's truly our only option.

So we have jumped back to the original approach and JohnAlbin will write a patch that provides an item getter/setter that can be used in lieu of menu_get_item so we can do if (($item = menu_tree_get_item()) || ($item = menu_get_item())) and then issue is simply solved by doing #5 without the side effects you do not want.

Very rough patch showing the direction I'm thinking this evening (not RTBC-worthy). Will come back to it in the morning. Comments requested.

Exactly what i suggested. Go!

We have survived three years with this and I am sure noone will be against adding such a trivial pair of functions to menu.inc especially given that it was here before RC just not 100% ready. Modules now have version dependencies. So, I am demoting from critical.

How about this? Now with simpletests.

Fixed the comment above MenuTreeTestCase.

+++ includes/menu.inc
@@ -1120,7 +1152,7 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail =
-  if ($item = menu_get_item()) {
+  if (($item = menu_tree_get_item($menu_name)) || ($item = menu_get_item())) {

Perhaps make menu_tree_get_item() default to menu_get_item() ?

+++ includes/menu.inc
@@ -1160,7 +1192,7 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail =
-          if ($active_link = menu_link_get_preferred()) {
+          if ($active_link = menu_link_get_preferred($item['href'])) {

This change needs a very good reason or should be reverted. menu_link_get_preferred() defaults to $_GET['q'] and is statically cached. $_GET['q'] can be different to $item['href']. Other callers do not pass a path.

Powered by Dreditor.

Perhaps make menu_tree_get_item() default to menu_get_item() ?

At first blush, this looks reasonable. However, menu_get_item() doesn't take a menu_name as an argument, so having menu_tree_get_item($menu_name) return a router item that may not have a link in the requested menu would be a WTF for the menu_tree_get_item() API.

This change needs a very good reason or should be reverted. menu_link_get_preferred() defaults to $_GET['q'] and is statically cached. $_GET['q'] can be different to $item['href']. Other callers do not pass a path.

When menu_tree_get_item() returns a menu item that isn't the path in $_GET['q'], then we need to find the $active_link for that path. That's why its important to make that change to menu_link_get_preferred($item['href']).

If menu_tree_get_item() returns NULL, then menu_get_item() will default to using $_GET['q'] as its path. Which means menu_link_get_preferred($item['href']) is the equivalent to menu_link_get_preferred(), no? Even if the router item's href is not exactly $_GET['q'], since the menu item returned by menu_get_item() shouldn't change per page request, menu_link_get_preferred($item['href'])'s static cache should work just fine.

"2 functions call menu_link_get_preferred()"

Source: http://api.drupal.org/api/drupal/includes--menu.inc/function/menu_link_g...

Current patch updates one, so the other invocation will potentially lead to a duplicate operation for technically the same path under (un)certain circumstances. We could update the other invocation to follow.

However, menu_link_get_preferred() is a very useful API function and I expect that many menu-related and also custom/site-specific modules are going to use it. In turn, all of those would have to understand that they have to pass in the href of the current menu item instead of nothing. In turn, it might be worth to consider to make menu_link_get_preferred() to use the href of menu_tree_get_item() by default, falling back to menu_get_item()'s href, and ultimately falling back to $_GET['q'], in case it can be different to menu_get_item()'s href (not sure whether that is actually possible).

Additionally, we need to take menu_set_active_item() into account, resp., define the expected behavior in case it is used:
http://api.drupal.org/api/drupal/modules--user--user.pages.inc/function/...

If we alter menu_link_get_preferred() to check the menu_tree_get_item() that will have the side affect that it alters the breadcrumb returned by menu_set_active_trail(). And I don't think we want menu_TREE_set_item() to affect breadcrumbs. Maybe we do, but I just want to point out that particular non-obvious result.

I talked to chx in IRC and he said that, yes, $_GET['q'] can be different then the href value of the item returned by menu_get_item().

So, if we want to ensure that we don't muck with the cache of menu_link_get_preferred(), I think I need to change the patch so that it only passes a parameter to menu_link_get_preferred() if the $item was from menu_tree_get_item(). That way, menu_tree_page_data()'s default behavior of getting $item from menu_get_item() will be unaffected.

For the record, here's the patch I was thinking of in #74. But I don't like it anymore. Let me explain in a follow-up patch.

menu_tree_get/set_item() was kind of awkward because it required the user of that function to pass a $item that had many of the specialized array keys that menu_get_item() returns. In real word practice, that would have meant that users would probably have used menu_get_item() to get all those values for the menu item they wanted.

Instead, let's use menu_tree_set_path() and menu_tree_get_path(). Then user only need to specify the path of the menu link they want to be the active item in the generated menu tree. This is a much more natural API function.

And it also simplifies the code greatly. The code in patch #75 was getting awkward. This patch is much cleaner.

It also avoids the cache issue that Sun was bringing up with menu_link_get_preferred(). While the patch does call menu_link_get_preferred($active_path), the $active_path will be NULL if no path has been set with menu_tree_set_path() and, thus, menu_link_get_preferred will end up doing its default behavior.

Sorry for being a pain, but what's the difference to menu_set_active_item() now?

@sun Good questions are not a pain.

menu_set_active_item() internally actually changes the value of $_GET['q']. Which means if you set a path with menu_set_active_item() it actually changes the router item for things like which menu item gets returned by menu_get_item() and (if called earlier enough) also affects what page content is delivered.

The proposed menu_tree_set_path() only changes which path is used to determine the active trail for menu trees. That's it. It has no other effects. If a module also wants to use that same path to affect breadcrumbs, that's relatively easy with our existing APIs plus menu_tree_get_path($menu_name); but that isn't done automatically by design; I want this new API function to do what it says and no more.

The gist of #78 needs to be in the doxygen of menu_tree_set_path(): the doxygen needs to clarify what's the difference between menu_set_active_trail() menu_set_active_item and menu_tree_set_path()

> If a module also wants to use that same path to affect breadcrumbs, that's relatively easy... but that isn't done... I want this new API function to do what it says and no more.

It doesn't seem immediately obvious to me why "setting the menu tree path" wouldn't affect breadcrumbs, so I suspect that this part of the behavior especially needs documentation. I can imagine that an uninformed version of myself would likely try calling this function, then wonder why his breadcrumbs don't match his menus.

Given recent clarifications and questions, I'd expect menu_tree_set_path() to have an impact on the default behavior of menu_set_active_trail(); i.e., the default breadcrumbs when not overridden manually.

The gist of #78 needs to be in the doxygen of menu_tree_set_path(): the doxygen needs to clarify what's the difference between menu_set_active_trail() menu_set_active_item and menu_tree_set_path()

Indeed. :-)

Given recent clarifications and questions, I'd expect menu_tree_set_path() to have an impact on the default behavior of menu_set_active_trail(); i.e., the default breadcrumbs when not overridden manually.

Wow. Fantastic suggestion. Because when I stopped to think about how this should work, I realized the code already does this for the specific use-case that it should work for! :-D Here's the gist of what happens:

1. A module calls menu_tree_set_path('main_menu', 'my/awesome/path'); in order to affect the active trail for any main_menu trees.
2. Time passes…
3. template_process_page() goes to create the breadcrumbs by calling drupal_get_breadcrumb() which calls menu_get_active_breadcrumb() which begat menu_get_active_trail() which gives onto Caesar menu_set_active_trail() and that is actually where the breadcrumbs are determined. Inside menu_set_active_trail(), it determines the preferred menu for the breadcrumb by calling $preferred_link = menu_link_get_preferred(); (without any parameters). It then calls menu_tree_page_data($preferred_link['menu_name'],…); in order to find the active trail.
4. If the preferred link’s menu is the same as one used in a call to menu_tree_set_path(), then our call to menu_tree_page_data($preferred_link['menu_name'],…); will return the trail for the overridden path. For our example in step 1, if the preferred link is in the main_menu menu, then the breadcrumb will be overridden with the breadcrumb to our 'my/awesome/path' path.

What happens if the preferred menu is different then one specified in calls to menu_tree_set_path()? IMO, the breadcrumb is correctly generated from the preferred menu and ignores the irrelevant calls to menu_tree_set_path().

I've updated the docsblocks to mention all this. I've also added two new test cases to check that the breadcrumbs are overridden when the preferred menu matches a menu set in menu_tree_set_path() and not otherwise.

The patch also moves around and clarifies related docs for menu_set_active_trail() and menu_get_active_trail(). And moves a paragraph of docs that was associated with the $path param of menu_set_active_item() to the function itself where it belongs: “Note that this may not have the desired effect unless invoked very early…” ← “invoked” clearly refers to the function, not the parameter.

+++ includes/menu.inc
@@ -1095,6 +1095,47 @@ function menu_tree_all_data($menu_name, $link = NULL, $max_depth = NULL) {
+ * overridden by the corresponsding path returned by menu_tree_get_path().

Typo in "corresponsding".

+++ includes/menu.inc
@@ -2191,38 +2235,35 @@ function menu_get_active_menu_names() {
+ * Any trail set by this function will only be used for functionality that calls
+ * menu_get_active_trail(). Drupal core only uses trails set here for
+ * breadcrumbs and not for menu trees or page content. Additionally, breadcrumbs
+ * set by drupal_set_breadcrumb() will override any trail set here.

I had to learn the hard way that "page content" may not be fully true. I was told that, as of now, menu_set_active_trail() contains a "hidden feature": by altering the active trail, modules are not only customizing the breadcrumb, but also the page title, and at least also the active trail in generated menu trees. Various other menu system and common request path and page API functions (e.g., stuff like drupal_get_title()) indirectly recurse and circle back into menu_set_active_trail(). Kinda messy and really hard to figure out what else actually relies on the active trail.

+++ modules/simpletest/tests/menu.test
@@ -1271,6 +1310,29 @@ class MenuBreadcrumbTestCase extends DrupalWebTestCase {
+  function testBreadCrumbsMenuTreeSetPath() {

It doesn't look like this test actually tests the functionality of setting a custom menu tree path. I'd suggest to do the following:

1) Implement menu_test_init()

2) Add a system variable menu_test_menu_set_tree_path that can be an array containing $menu_name and $path, defaulting to FALSE (not affecting other tests).

3) In the test, invoke some regular paths, assert the expected default behavior.

4) Then, for each path, set the variable to override to something else, and assert the expected custom behavior.

We should make sure to leverage all of assertBreadcrumb()'s features; i.e., not only test the active trail, but also the page title, and possibly another visible menu tree. (see existing breadcrumb tests)

Powered by Dreditor.

It doesn't look like this test actually tests the functionality of setting a custom menu tree path.

Yes, it really does test the functionality. Take a look again; the code comments explain how it works. MenuTreeTestCase::testMenuTreeSetPath() is a minimal, but sufficient test.

I've been moving to a new apartment this week. If the tests need to be expanded, I can try to get my computer set up tomorrow night and work on it, but no guarantees I won't be exhausted. :-p

I had to learn the hard way that "page content" may not be fully true. I was told that, as of now, menu_set_active_trail() contains a "hidden feature": by altering the active trail, modules are not only customizing the breadcrumb, but also the page title, and at least also the active trail in generated menu trees.

Ah, yes, you are correct that menu_set_active_trail() affects the page title. But it doesn't affect the active trail in menu trees. I wish it did! Then we wouldn't need this entire issue! :-D I've updated the docs to reflect that it alters the page title.

I've taken sun's suggestion in #83 and completely re-written the menu tree trail tests to be as robust as the breadcrumb tests.

Otherwise, the patch is the same. Just docblock updates and test updates.

+++ modules/simpletest/tests/menu.test
@@ -1398,3 +1398,215 @@ class MenuBreadcrumbTestCase extends DrupalWebTestCase {
+class MenuTrailTestCase extends DrupalWebTestCase {
...
+  protected function assertTrail($goto, array $trail, $page_title = NULL, array $tree = array(), $last_active = TRUE) {
...
+  protected function getParts() {

huh? Why did you duplicate the entire test methods instead of adding the new test method to the existing test case - or - extending MenuBreadcrumbsTestCase instead of DrupalWebTestCase?

Powered by Dreditor.

Why did you duplicate the entire test methods instead of […] extending MenuBreadcrumbsTestCase instead of DrupalWebTestCase?

Since this is OO, extending the class seemed most natural and is what I tried first. But I discovered that caused my MenuTrailTestCase to re-run all of MenuBreadcrumbTestCase's tests as if they were its own. Hmm… I guess I could override MenuBreadcrumbTestCase's tests with empty methods. Seems slightly hackish, but it will get the job done.

Ok. New patch extends test in an OO way.

meh.

Ah, much better OO approach. :-) But you missed some things in the MenuTrailTestCase::setUp(). Fixored with this patch.

+++ modules/simpletest/tests/menu.test
@@ -6,6 +6,118 @@
+class MenuWebTestCase extends DrupalWebTestCase {
+  function setUp() {
+    parent::setUp(array('menu_test'));
+  }

We actually need a pass-through construct here; i.e., accept $modules, but

$modules = array_merge($modules, array('menu_test'));
parent::setUp($modules);

+++ modules/simpletest/tests/menu.test
@@ -1291,110 +1403,112 @@ class MenuBreadcrumbTestCase extends DrupalWebTestCase {
+class MenuTrailTestCase extends MenuWebTestCase {
...
+    $perms = array_keys(module_invoke_all('permission'));
+    $this->admin_user = $this->drupalCreateUser($perms);

It doesn't look like this test actually needs all permissions (which is the case for the breadcrumbs test). Would be good to be more precise here.

+++ modules/simpletest/tests/menu.test
@@ -1291,110 +1403,112 @@ class MenuBreadcrumbTestCase extends DrupalWebTestCase {
+    $case1_breadcrumb = $home + array(
...
+    $case1_tree = array(
...
+    $case2_breadcrumb = $config + array(
...
+    $case2_tree = $case2_breadcrumb + array(
...
+    $case2_override_breadcrumb = $config + array(
...
+    $case2_override_tree = $case2_override_breadcrumb;

Can we rename the first and the second both to $breadcrumb and $tree, and the second override ones to $breadcrumb_override and $tree_override?

+++ modules/simpletest/tests/menu.test
@@ -1291,110 +1403,112 @@ class MenuBreadcrumbTestCase extends DrupalWebTestCase {
+    // Remove the Home link from the tree arrays.
+    array_shift($case2_tree);
+    array_shift($case2_override_tree);

This tweak could use a better code comment, because I do not understand why "Home" is removed.

Powered by Dreditor.

We actually need a pass-through construct here

Ok. Pass-thru added to MenuWebTestCase::setUp(). Also, realized that MenuWebTestCase doesn't need the menu_test module, so I moved that to its children classes.

It doesn't look like this test actually needs all permissions (which is the case for the breadcrumbs test). Would be good to be more precise here.

Ok. Changed the user to only required two admin-level permissions, needed for the paths it tests under admin/. Also, lowered the required permissions on the new paths in menu_test.module.

Can we rename the first and the second both to $breadcrumb and $tree, and the second override ones to $breadcrumb_override and $tree_override? […] This tweak could use a better code comment, because I do not understand why "Home" is removed.

Ok. Renamed all those variables and simplified the generation of the $tree so it doesn't need an array_shift().

subscribing

Thanks! Looks ready to fly for me now.

Summary for core commiters: (just noticed how long the thread is becoming)

Drupal 5 had the ability to set an active trail that would be used for breadcrumbs and menu trees. Unfortunately, D6 no longer had that ability. Double unfortunately, this was never fixed in D6. But it still remains a serious Usability-related regression.

This bug can most easily be fixed with a small API addition; specifically these two new functions:

/**
 * Set the path for determining the active trail of the specified menu tree.
 *
 * This path will also affect the breadcrumbs under some circumstances.
 * Breadcrumbs are built using the preferred link returned by
 * menu_link_get_preferred(). If the preferred link is inside one of the menus
 * specified in calls to menu_tree_set_path(), the preferred link will be
 * overridden by the corresponding path returned by menu_tree_get_path().
 *
 * Setting this path does not affect the main content; for that use
 * menu_set_active_item() instead.
 *
 * @param $menu_name
 *   The name of the affected menu tree.
 * @param $path
 *   The path to use when finding the active trail.
 */
function menu_tree_set_path($menu_name, $path) {
}

/**
 * Get the path for determining the active trail of the specified menu tree.
 *
 * @param $menu_name
 *   The menu name of the requested tree.
 *
 * @return
 *   A string containing the path. If no path has been specified with
 *   menu_tree_set_path(), NULL is returned.
 */
function menu_tree_get_path($menu_name) {
}

Its not changing the way the existing API works in any way. No place in core calls menu_tree_set_path(), which means menu_tree_get_path() will always return NULL. With this patch, the places in core that call menu_tree_get_path() are designed to work _exactly_ the way they would before this patch if menu_tree_get_path() returns NULL.

The patch is a bit bulky because it does a lot of cleanup of API documentation to help clarify the differences between these two new API functions and existing related API functions. It also refactors some of the breadcrumb tests so that the base functionality can be used for tests for these two functions and also for contrib.

One more follow-up. This is the only hunk from the patch that changes any existing core function. (Everything else in the 22kb patch is the 2 new functions, doc changes, and tests.)

@@ -1139,8 +1180,10 @@ function menu_tree_all_data($menu_name, $link = NULL, $max_depth = NULL) {
 function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail = FALSE) {
   $tree = &drupal_static(__FUNCTION__, array());
 
+  // Check if the active trail has been overridden for this menu tree.
+  $active_path = menu_tree_get_path($menu_name);
   // Load the menu item corresponding to the current page.
-  if ($item = menu_get_item()) {
+  if ($item = menu_get_item($active_path)) {
     if (isset($max_depth)) {
       $max_depth = min($max_depth, MENU_MAX_DEPTH);
     }
@@ -1179,8 +1222,9 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail =
         // If the item for the current page is accessible, build the tree
         // parameters accordingly.
         if ($item['access']) {
-          // Find a menu link corresponding to the current path.
-          if ($active_link = menu_link_get_preferred()) {
+          // Find a menu link corresponding to the current path. If $active_path
+          // is NULL, let menu_link_get_preferred() determine the path.
+          if ($active_link = menu_link_get_preferred($active_path)) {
             // The active link may only be taken into account to build the
             // active trail, if it resides in the requested menu. Otherwise,
             // we'd needlessly re-run _menu_build_tree() queries for every menu

menu_tree_page_data() is the only existing core function that is altered by this patch. Before calling menu_get_item(), it first checks if menu_tree_get_path($menu_name) returns a path for requested menu. As I said in the previous comment, menu_tree_get_path() will always return NULL unless a contrib module calls menu_tree_set_path() since core never uses that function. With $active_path set to NULL, menu_get_item($active_path) and is the same as just calling menu_get_item() without any parameters (see menu_get_item() docs).

The same is also true later in menu_tree_page_data(). Calling menu_link_get_preferred($active_path) is the equivalent of menu_link_get_preferred() with no parameters (see menu_link_get_preferred() docs.)

Thus, the changes to menu_tree_page_data() will have no affect given that $active_path = menu_tree_get_path($menu_name); will always be NULL unless a contrib module uses menu_tree_set_path().

If you scan the rest of the patch, you'll see that the rest of it is only doc changes and test changes.

Also, at the request of webchick, I installed Views and its titles are unaffected by this patch. I created a view with a title, gave it a menu item with a different title, and both the menu item title and the page title were correct after applying the patch.

Have a parameter to the _get() that actually sets is a WTF. I think elsewhere in core, the _get() function wraps the _set() function and passes it NULL or whatever it needs to just return the value.

Also, this function takes a menu link as a param, not a path. That seems like an inconsisentcy with the proposed API addition: http://api.drupal.org/api/drupal/includes--menu.inc/function/menu_tree_a...

Here's an updated patch with the get/set work switched, per Peter's request.

Reply to: comment #98

It looks like menu_tree_all_data()'s $link param is a menu link because book module stores its menu link in $node->book. And its convenient for the book module to pass in a menu link in that param to menu_tree_all_data().

It is, however, not convenient for anyone else. Its easy to find paths or store paths; why force me to query for its menu link? or to create a fake menu link? Especially when menu_tree_page_data() would only use the path from the menu link anyway.

If anything, in D8 we could refactor menu_tree_all_data() so its params are easier to use. But let's not use a menu link in these two new functions.

The way I see it, menu_tree_all_data() is basically deprecated. It serves no real purpose (except caching) now that the complex logic has been factored out in menu_build_tree().

This patch is actually very tiny, changes code in the minimal way possible, but adds a lot of documentation and tests. It blocks John's progress on refactoring menu blocks (and the new menu position dynamic menu item generation) so that those can be one day included in core.

I have nothing to add to this, let's get this in.

Removing the WTF tag. There is absolutely no WTF here, just conscious design decisions that we want to relax here.

This is a patch for D7, correct? Is there any chance of creating one for D6, or is the functionality too different?

Please note that #942782: Custom menus never receive an active trail depends on the breadcrumb simpletests refactoring in this patch.

@kestra

It would take some work to make a D6 patch, but it should be possible. I'm not willing to put the effort in, though. However, you can achieve some of the affect of this patch with the Menu Position module. http://drupal.org/project/menu_position

Restoring tag.

Subscribing.

Subscribing. Looking forward to this.

Taxonomy Menu Trails module is using workaround with menu_set_item(). It would be great to have a completely correct way to set current active trail in core.

This should be commited to HEAD first

Tagging for backport.

#99: 520106-98-dynamic-menu-path.patch queued for re-testing.

#99: 520106-98-dynamic-menu-path.patch queued for re-testing.

Back to RTBC as per #101

Re-rolled #98 as -p1 git patch against branch HEAD.

Corrected and re-rolled.

+++ b/modules/simpletest/tests/menu.test
@@ -5,6 +5,118 @@
+class MenuWebTestCase extends DrupalWebTestCase {
+  function setUp($modules = array()) {
+    parent::setUp($modules);
+  }

@@ -901,8 +1013,9 @@ class MenuBreadcrumbTestCase extends DrupalWebTestCase {
+  function setUp($modules = array()) {
+    $modules[] = 'menu_test';
+    parent::setUp($modules);

@@ -1377,110 +1490,113 @@ class MenuBreadcrumbTestCase extends DrupalWebTestCase {
+  function setUp($modules = array()) {
+    $modules[] = 'menu_test';
+    parent::setUp($modules);

These are not compatible with DrupalWebTestCase::setUp(). No idea why it doesn't complain.

10 days to next Drupal core point release.

Interesting...

Decided to see what would happen if MenuWebTestCase::setUp() and friends were declared as protected, for compatibility with DrupalWebTestCase::setUp(), and all tests failed.

See http://qa.drupal.org/pifr/test/158134

Subscribing

Subscribing.

Just as a quick note - I've found I can actually get Drupal to do what I need (dynamically assign nodes a menu position) by using a combination of Menu Position - http://drupal.org/project/menu_position - and Rules.

@ JohnAlbin

4. If the preferred link’s menu is the same as one used in a call to menu_tree_set_path(), then our call to menu_tree_page_data($preferred_link['menu_name'],…); will return the trail for the overridden path. For our example in step 1, if the preferred link is in the main_menu menu, then the breadcrumb will be overridden with the breadcrumb to our 'my/awesome/path' path.

John, for any node which has no corresponding menu link, prefered link is node/% from navigation menu.
It is not from main_menu (against which I apply menu_tree_set_path()) and thus setting menu_tree_set_path() never affects breadcrumbs.

So my question is - what method do you recommend to get breadcrumbs to do follow the active trail in the circumstances when menu_link_get_prefered() returns menu which is not the menu of our interest?

This issue could use a very good summary

Not sure if this got mentioning, but it seems as if something is going wrong theming wise, too:
The usual $main_menu array we get on our page.tpl.php is empty when I set the path using sth like menu_tree_set_path('main-menu', 'my/path');

Very frustrating. I followed the chain of execution, it starts with menu_main_menu(), which calls menu_navigation_links() which then calls menu_tree_page_data() and so on. What I find interesting is that I get a empty array even on kpr(menu_tree_page_data('main-menu', 0).

What's the deal with that?

@felixSchl

Well, I can't reproduce. I'm using Zen as the base theme, and the code outputting menu links from the page.tpl.php is:

       print theme('links__system_main_menu', array(
          'links' => $main_menu,
          'attributes' => array(
            'id' => 'main-menu',
            'class' => array('links', 'inline', 'clearfix'),
          ),
          'heading' => array(
            'text' => t('Main menu'),
            'level' => 'h2',
            'class' => array('element-invisible'),
          ),
        ));

$main_menu array is never empty on pages where I set active path in this way:

  menu_tree_set_path('main-menu', 'node/' . $nid); /* nid is calculated from Node references */

@OnkelTem, sorry. I mistook the URL alias for the actual path.

menu_tree_set_path('main-menu', 'library/category');
did not work although the node's alias matched.

I worked around this via
menu_tree_set_path('main-menu', drupal_lookup_path('source','library/category'));
which works just fine.

Maybe this even helps someone not make the same stoopid mistake,
Cheers!

#17: menu-trail-alter.patch queued for re-testing.

Nice, and the patch even works in D7 :)
Not sure why @sun thinks the setUp() method should be protected (how should it be called then?).

The attached version should resolve @sun's objections, which (as I now understand) weren't about protected vs. private, but about the parameter signature.

Right, thanks, the visibility is unimportant - it's the signature that's different, and which should normally throw an E_STRICT warning.

I know that @webchick is going to ask for it, so let's shortcut it: We should take over the identical logic/code lines for passing on $modules in the test helper class as in DrupalWebTestCase::setUp():

    $modules = func_get_args();
    if (isset($modules[0]) && is_array($modules[0])) {
      $modules = $modules[0];
    }

20 days to next Drupal core point release.

    $modules = func_get_args();
    if (isset($modules[0]) && is_array($modules[0])) {
      $modules = $modules[0];
    }

Sure.

+++ b/modules/simpletest/tests/menu.test
@@ -5,6 +5,123 @@
+    module_enable($modules, TRUE);
+    parent::setUp();

This cannot work - it's not possible to install or configure anything before setUp().

Just parent::setUp($modules)

20 days to next Drupal core point release.

Just parent::setUp($modules)

Okay.

it's the signature that's different, and which should normally throw an E_STRICT warning.

Good to know. Opened #1273722: Fix FieldAttachTestCase::setUp() signature.

Very nice patch for a very needed feature.

It works for D7. Is this planned to be committed to that branch?

@ramsalt, hence the keyword "needs backport to D7". The workflow is that bugs affection both D8 and D7 should be fixed on D8 first and then backported to D7.

@franz Dammit. This is a feature that solves a very important issue. And for all that are using context the menu reaction will now actually work. But the patch applied nicely on D7 and the feature works as it is now. Cross my fingers for backward compatablilty when it's committed to D8.

@ramsalt, no need to worry, if they both apply webchick will probably commit this to 7.x at the same time.

Would anybody answer my question? The patch doesn't help to set breadcrumb correctly. See #124 for details.

The change to the SetUp() makes sense. Back to RTBC.

@OnkelTem To alter the breadcrumb for that use case, you need to use one of the existing breadcrumb altering API functions. http://api.drupal.org/api/search/7/breadcrumb

starting review

I just updated the issue summary.

This is blocking the release of menu_position 7.x-1.0. And the simpletest breadcrumb refactoring is blocking another issue that is in turn blocking the release of menu_block 7.x-2.0. Anybody remember the issue tag for "blocking release of contrib module"?

Anybody remember the issue tag for "blocking release of contrib module"?

Change from "task" to "bug report" and bump to major. It's already "major" so changing category.

Updated issue summary.

#136: menu_trail-dynamic_local_paths-520106-136.patch queued for re-testing.

Committed to 8.x, moving back to 7.x for webchick to consider.

Yeah, I think this is good to go since it's a pretty straight-forward API addition. Thanks a lot for the really helpful issue summary to make this look a lot less scary than it actually is! And thanks a lot for your patience as well.

Committed and pushed to 7.x. WOOHOO!

And since adding that summary is relatively easy, tagging as Novice.

I just sent an API change notification to the development list, but I assume it's in the moderation queue, as I only just signed up to the list. So, I'm marking as 'needs review'.

Am I right in assuming that the gist of these notifications gets posted to http://drupal.org/update/modules/6/7 with the next release?

Finally, from the summary: "After this patch is backported to Drupal 7, D8 should consider removing menu_set_active_trail()." Does this warrant a new issue?

Looking through your history, I don't see the change notification you mention.

Could you please include a link to it here?

It got refused by mailman, so no link. I'm following up with development-owner@drupal.org...

Just go to http://drupal.org/node/add/changenotice

There is no support for adding change notifications via the development list.

Don't worry about getting it wrong; change notifications can be edited by any Drupal user, once created.

Finally, from the summary: "After this patch is backported to Drupal 7, D8 should consider removing menu_set_active_trail()." Does this warrant a new issue?

Yes, that would be a separate, follow-up issue. But there should be a link from this issue to that one and vice-versa.

@pillarsdotnet: oh, okay, thanks. I'd looked for documentation and only found some examples on the mailing list.

How about: http://drupal.org/node/1298642 ?

If it were me, I'd start out with something like,

Two new API functions were added in order to enable setting the active menu trail for dynamically generated menu paths. They are:

Then I'd start an unordered list, and put each of the two added menu functions inside its own list item, and lhyperlink each one to its API page.

*AFTER* that succinct summary, I might go into details about the history of this feature, but not in the introduction.

updated, as per suggestions.

(chuckle) -- You didn't have to follow *exactly* what I suggested -- Anyhow, I think we can remove the bogosity now.

@pillarsdotnet: well, I'd mostly just plagiarized a paragraph from the summary, so switching sources wasn't too much of a stretch ;)

Also, posted #1299056: Remove misnamed and useless menu_set_active_trail() function. for follow-up.

Hi, newbie here.
How do I implement Damien's solution (#5)?
Could anyone explain the steps required?
I need to set as 'active' the same menu item for two different views (in three languages).
Is this the correct thread?

Thanks!

gejo: in Drupal 7.9 and later you can use http://api.drupal.org/api/drupal/includes--menu.inc/function/menu_tree_s....

Since this issue is closed, a better place to request additional support would be http://drupal.org/support or http://drupal.stackexchange.com/.

For those who arrive from Google in the future -
Menu Trail by Path module makes use of the new menu_tree_set_path() discussed in here.

Added target link.