Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
The menu_set_active_trail() function implies with all its heart that it affects anything that involves the active trail in the menu, when in fact it doesn't actually do anything except affect breadcrumbs.
Breadcrumbs may be set by drupal_set_breadcrumb(), so these two functions are effectively duplicates of each other.
We now have menu_tree_set_path() and menu_tree_get_path() for setting and getting the active menu path.
Proposed resolution
The logic within the redundant and confusingly-named menu_set_active_trail() function should be moved to the menu_get_active_trail() function, where it is used and needed.
Remaining tasks
The patch in #4 is awaiting review.
User interface changes
None.
API changes
The redundant and confusingly-named menu_set_active_trail() function would be removed.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | remove-menu_set_active_trail-1299056-4.patch | 4.88 KB | pillarsdotnet |
| #2 | remove-menu_set_active_trail-1299056-2.patch | 8.86 KB | pillarsdotnet |
| #1 | remove-menu_set_active_trail-1299056-1.patch | 8.83 KB | pillarsdotnet |
Comments
Comment #1
pillarsdotnet CreditAttribution: pillarsdotnet commentedPatch removes the
menu_set_active_trail()function by moving most of its logic into themenu_get_active_trail()function, instead.Comment #2
pillarsdotnet CreditAttribution: pillarsdotnet commented(sigh) one-line correction.
Comment #3
pillarsdotnet CreditAttribution: pillarsdotnet commentedTagging for documentation team review of the updated docblocks.
Comment #4
pillarsdotnet CreditAttribution: pillarsdotnet commentedUpdated patch, made considerably smaller by moving menu_get_active_trail() where menu_set_active_trail() used to be.
Comment #4.0
pillarsdotnet CreditAttribution: pillarsdotnet commentedQuote relevant text from parent issue summary.
Comment #4.1
pillarsdotnet CreditAttribution: pillarsdotnet commentedprettifying.
Comment #4.2
pillarsdotnet CreditAttribution: pillarsdotnet commentedStart using summary template.
Comment #5
chx CreditAttribution: chx commentedComment #6
pillarsdotnet CreditAttribution: pillarsdotnet commentedChange notice added.
Comment #7
sunI see three callers to the getter:
http://api.drupal.org/api/drupal/includes--menu.inc/function/menu_get_ac...
http://api.drupal.org/api/drupal/includes--menu.inc/function/menu_get_ac...
http://api.drupal.org/api/drupal/modules--toolbar--toolbar.module/functi...
Without a setter, there's possibly no way to adjust/override/customize the behavior of these callers.
While there is indeed hook_menu_breadcrumb_alter(), there's no equivalent to that regarding menu_get_active_title() or drupal_get_title(). Especially the behavior of the last caller (Toolbar) would effectively be hard-coded and non-customizable.
Changes done via menu_set_active_trail() stick for the remaining execution of the request, and automatically apply to all possible consumers of the trail.
To me it seems like we're removing the ability to easily customize the active trail including all dependent behavior/functionality.
I think this needs more careful reviews.
Especially maintainers of menu-related contributed modules should chime in here; e.g., @JohnAlbin.
--
P.S.: Change notifications should be created after a change was done, not before.
Comment #8
pillarsdotnet CreditAttribution: pillarsdotnet commentedWhich begs the question -- is there any way to unpublish one, once it's created?EDIT: Figured it out. See #1313844: Please delete http://drupal.org/node/1313534
Comment #9
pillarsdotnet CreditAttribution: pillarsdotnet commentedThe parent issue claims that menu_set_active_trail() does not, in fact, affect the active menu trail; it only affects the breadcrumb trail.
I dunno, personally; I'm just knocking out low-hanging fruit like a good little code-monkey.
Comment #10
JohnAlbinI admit I'm a bit confused why we are removing menu_set_active_trail() but leaving menu_get_active_trail(). Sun's points are valid.
IMO, we need to kill off some pair of menu/breadcrumb/trail functions because we have at least one pair too many. And menu_set/get_active_trail looks like the easiest to knock off.
Comment #11
pillarsdotnet CreditAttribution: pillarsdotnet commentedOkay; removed change notice. Unfollowing since this isn't likely to go anywhere this year.
Comment #12
xjmRelated: #1811792: drupal_set_title() should not be necessary to set the homepage title
Comment #12.0
xjmwordsmithing.
Comment #15
quietone CreditAttribution: quietone as a volunteer commentedStumbled on this while waiting for a long test to run.
menu_set_active_trail and menu_get_active_trail were removed in #2237963: Remove dead menu system code: menu_get_active_trail() and friends. Therefor, marking this closed as a duplicate.