Currently, function 'menu_set_location' is a placeholder and as result it's not possible to set custom breadcrumbs. Now this is done (well, at least tried) in blog, comment, forum and taxonomy module.

CommentFileSizeAuthor
#14 brood_01.patch7.56 KBdesbeers
#6 menu_set_item_0.patch1.94 KBchx
#5 menu_set_item.patch1.93 KBchx
#1 bread-menu-01.patch708 bytesdesbeers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

desbeers’s picture

desbeers CreditAttribution: desbeers commented
Status: Active » Needs review
FileSize
bread-menu-01.patch708 bytes

I gave it a shot; attached a patch that restores the custom breadcrumb function; although is by no means like the previous version of 'menu_set_location'.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Priority: Normal » Critical
Status: Needs review » Needs work

MSL does more than just breadcrumb

desbeers’s picture

desbeers CreditAttribution: desbeers commented

Yes, it does a lot more, but frankly; I have no clue what it's doing more 'in practice'. I read the documentation for D5 and it still wasn't clear. A search in core showed me it's really only used for breadcrumbs.

I asked help from Chx on IRC and he had also no clue about it; it's a mysterious function :-)

menu_set_location can't be dumped in my opinion as we are in code-freeze. But something has to be done with it. I suggest to make it 'breadcrumb' only (like I did in the patch); include new doxigen and rework it for D7.

Crell’s picture

Crell CreditAttribution: Crell commented

In D5, menu_set_location() is also how you change what menu blocks and primary/secondary links will be active. It changes the entire, well, menu location. That it affects breadcrumbs display is just one symptom of that.

Losing menu_set_location() would be quite bad, as that's a regression that would hose a site I just launched. :-)

chx’s picture

chx CreditAttribution: chx commented
Title: function menu_set_location is a placeholder » menu_set_item was lost
Status: Needs work » Reviewed & tested by the community
FileSize
menu_set_item.patch1.93 KB

Poor menu_set_item, it got removed but why? I have no idea. It was mighy useful. I have readded with copius comments. I can't see what review could add to the issue so I set it to RTBC as this is a somewhat new function. It was in HEAD but it was removed with the multiple menu patch , I checked the issue and there was no mention of it. It either was oversight or we thought the load functionality supersedes it but apparently that's not the case. If you search on $_GET['q'] in menu.inc you will immediately see that menu_get_item is the only place where it used (aside from menu_set_active_item which is not called by core) and everything else relies on menu_get_item() so this is what Crell wants.

chx’s picture

chx CreditAttribution: chx commented
FileSize
menu_set_item_0.patch1.94 KB

Spaces, capitalization in doxygen.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Indeed, quite simple, thanks, committed.

desbeers’s picture

desbeers CreditAttribution: desbeers commented
Title: menu_set_item was lost » Fatal error because 'menu_set_location' is gone
Status: Fixed » Needs work

Now function 'menu_set_location' in menu.inc is gone, but this function was used in blog, comment, forum and taxonomy module....

Viewing a 'blog' node on localhost/node/% for example gives a fatal error now.

Will look at it later today.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Well, reverted the patch in anticipation of a better one. (bjaspan also reported an issue about this problem in the meantime).

Another schoolbook case on why someone should not RTBC his own patches (and why a core committer should look at patches more thoroughly).

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Title: Fatal error because 'menu_set_location' is gone » Restore 'menu_set_location' functionality
chx’s picture

chx CreditAttribution: chx commented

I should have removed the menu_set_location in a separate patch but I will RTBC my own similar patches in the future. It's a waste of time to try to get a review on such stuff.

desbeers’s picture

desbeers CreditAttribution: desbeers commented

Currently, menu_set_location is 'misused' on several places in core to set a custom breadcrumb. I tested Chx path and it's working well. It sets the location correctly; alters title and navigation-block and set the breadcrumb. But it offers no way to set a 'custom' breadcrumb like we do in blog.module for example.

In 'node.pages.inc' the breadcrumb is set directly with function drupal_set_breadcrumb. The other modules should do the same. I will make a patch for that later today.

bjaspan’s picture

bjaspan CreditAttribution: bjaspan commented

Subscribe.

desbeers’s picture

desbeers CreditAttribution: desbeers commented
Status: Needs work » Needs review
FileSize
brood_01.patch7.56 KB

Attached a patch that removes all the 'mis-use' of the function 'menu_set_location'.

It was only used to set a 'custom-breadcrumb' and with this patch it's setting it in a proper way. Just 'set the breadcrumb'.

Chx's patch attached to #6 is exactly doing what it should and this patch solves the fatal-errors as a result of that.

Modeles and functions affected:

- blog.module (blog_view)
- taxonomy.pages.inc (taxonomy_term_page)
- comment.module (comment_reply)
- forum.module (forum_nodeapi and template_preprocess_forums)

In the php-doc's (that were already there) you can see that it was really just about breadcrumbs and never to 'set-a-location'.

chx’s picture

chx CreditAttribution: chx commented

Reviewing (and no, desbeers have not told me on IRC).

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

Nicely done, thanks. I truly hope there are no other ancient remnants of the old menu system.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Committed, thanks.

stella’s picture

stella CreditAttribution: stella commented

I've updated the "Converting 5.x modules to 6.x" documentation to include a section on this issue, as core modules aren't the only ones affected. Please feel free to modify it if required: Use drupal_set_breadcrumb() instead of menu_set_location() to set custom breadcrumbs.

Cheers,
Stella

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)

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