menu_set_location (menu_set_active) mangles $_GET/arg()

Changed title and component from comment to contact. ;-)

The patch works perfectly for me, with no apparent side-effects. I say commit it.

a) Said breadcrumb vanishes on my local install when I comment out the menu_set_location line. Stock bluemarine.
b) From a quick look, this is happening because $_GET['q'] is reset (in menu_set_active) to the "fallback" menu which is /contact because there is no explicit declaration of /contact/cid.

Setting status to "code needs work" as I lose my breadcrumb due to the attached patch. Third party mediator required :)

My 10p.
-K
P.S Mangling a $_GET variable directly like this seems like a bad idea.

I can confirm that this also breaks other pages like the comment_reply form which also uses a menu_set_location. The reason why we aren't already seeing breakage is because the functions in question access these arguments via initialised variables in the function parameter list. Any arguments not explicity stated in hook_menu will be lost after the menu_set_location; i.e. hooks along the lines of hook_form_alter etc. will possibly fail.

Grep output:

modules/blog.module:239:    menu_set_location($breadcrumb);
modules/book.module:495:            menu_set_location($node->breadcrumb);
modules/comment.module:473:  menu_set_location(array(array('path' => "node/$nid", 'title' => $node->title), array('path' => "comment/reply/$nid")));
modules/contact.module:412:  menu_set_location($breadcrumb);
modules/forum.module:285:    menu_set_location($breadcrumb);
modules/forum.module:849:  menu_set_location($breadcrumb);
modules/taxonomy.module:1224:          menu_set_location($breadcrumbs);

Setting this to critical for the 4.7 release.

-K

The attached patch fixes the symptoms by avoiding menu_set_location altogether.
Fixes:

• Makes MENU_SUGGESTED_ITEM breadcrumb friendly (which now gives me a home link without the breadcrumb code in contact).
• Replaces menu_set_location with drupal_set_breadcrumb for all instances in core.
• For nodes, the above adds a breadcrumb for the 'node' part of their paths named 'content'. This IMO is correct and how it should be.
• I noticed in forum.module that there is a seemingly rather pointless DB call that loaded the vocabulary name for Forum and uses that as the title. Replaced it with a t('forums'). This DB call is being made on all forum pages.
• Also lowercased 'Home' to 'home' to keep it consistent with all the other breadcrumbs in Drupal.

This is it I think. I've not removed menu_set_location (which is not used anywhere in core now) with this patch mainly due to the following comment in menu.inc:

 includes/menu.inc-231- *
includes/menu.inc-232- * Unlike the rest of the menu structure, the local task tree cannot be cached
includes/menu.inc-233- * nor determined too early in the page request, because the user's current
includes/menu.inc:234: * location may be changed by a menu_set_location() call, and the tasks shown
includes/menu.inc-235- * (just as the breadcrumb trail) need to reflect the changed location.
includes/menu.inc-236- */
includes/menu.inc-237-function menu_get_local_tasks() {

I'd prefer to get an opinion on this before doing anything destructive.

-K

.

I disagree with moving from menu_set_location() to drupal_set_breadcrumb(). The two are not equivalent. Setting the user's location affects the rendering of the menu tree, and the breadcrumb call does not. In fact, I don't think we should be ever using drupal_set_breadcrumb().

Here's a use case to demonstrate:

Suppose we have the menu structure
- Foo
- Bar
- Baz
- Xyzzy
- Plugh

Now suppose we are on a node view page for a node that is not in the menu structure, "Fnord." Suppose some code in a hook_nodeapi() wants to react to this node view and position the user in the menu, as if they were at Foo -> Bar -> Fnord.

Regardless of whether menu_set_location() or drupal_set_breadcrumb() is used, we get the correct breadcrumb trail. But in the former case the menu is expanded properly, whereas in the latter case we end up with

- Foo
- Xyzzy
- Plugh

which is not what we want.

The example structures were mangled, sorry. The first one should be

- Foo
- - Bar
- - Baz
- Xyzzy
- Plugh

and the second one should be

- Foo
- Xyzzy
- Plugh

I don't think menu_set_location ever needs to change the current path; it should be affecting the menu tree only. The attached is worth a shot.

As long as callbacks rely on $_GET['q'] and arg, menu_set_location needs to update the location IMNSHO.

The way I interpret the menu_set_active_location() doxygen, I disagree with chx. It should only be used to make the current page appear somewhere else in the menu, not to invoke pages from another actual page. That would be a bad idea anyway IMO because it violates the good practice that a piece of content has only one unique URL.

If you want to make another page appear as the content of your current page, it is not that much effort to set $_GET['q'] yourself. It is not very elegant, but then this is IMO not a supported feature of the menu system.

I agree with Steven on this one. I don't think changing the function of the page is part of what menu_set_location is supposed to do. One should probably do a drupal_goto() for this.

applied