A couple of issues here.

  1. MENU_VISIBLE_IN_TREE is a stale constant currently, because it is not used anywhere.

    # grep -R 'MENU_VISIBLE_IN_TREE' ./*
    .\includes\menu.inc(88): define('MENU_VISIBLE_IN_TREE', 0x0002);
    .\includes\menu.inc(138): define('MENU_NORMAL_ITEM', MENU_VISIBLE_IN_TREE | MENU_VISIBLE_IN_BREADCRUMB);

    However, this constant had a purpose, and we need to restore it.

  2. MENU_VISIBLE_IN_BREADCRUMB not only makes a menu item visible in the breadcrumb, but also in the menu tree.

    Coming from #576290: Breadcrumbs don't work for dynamic paths & local tasks but also from trying to expose all dynamic paths + local tasks in admin_menu (#420816: On-demand loading of dynamic paths and local tasks #1), I expected all local tasks and actions to appear like MENU_CALLBACKs in {menu_links}. They currently do not appear in {menu_links}, so I searched for the trigger that puts MENU_CALLBACKs into {menu_links}.

    This trigger currently is this code in menu.inc:3024 in _menu_router_build():

        $item += array(
          '_visible' => (bool)($item['type'] & MENU_VISIBLE_IN_BREADCRUMB),
          '_tab' => (bool)($item['type'] & MENU_IS_LOCAL_TASK),
        );

    Hence, I enhanced the constants for local tasks and actions:

    define('MENU_LOCAL_TASK', MENU_IS_LOCAL_TASK | MENU_VISIBLE_IN_BREADCRUMB);
    define('MENU_DEFAULT_LOCAL_TASK', MENU_IS_LOCAL_TASK | MENU_LINKS_TO_PARENT | MENU_VISIBLE_IN_BREADCRUMB);
    define('MENU_LOCAL_ACTION', MENU_IS_LOCAL_TASK | MENU_IS_LOCAL_ACTION | MENU_VISIBLE_IN_BREADCRUMB);

    Which made them successfully appear in {menu_links}.

  3. However, that made those links also appear in the regular menu tree output of menus.

    That is, because in menu.inc:2289 in _menu_link_build() the condition only checks for certain item types:

      if ($item['type'] == MENU_CALLBACK) {
        $item['hidden'] = -1;
      }
      elseif ($item['type'] == MENU_SUGGESTED_ITEM) {
        $item['hidden'] = 1;
      }
      $item += array(
        'hidden' => 0,
      );

    Which is where aforementioned MENU_VISIBLE_IN_TREE in 1) comes into the game:

    We only have three states, and these are:

    • 1: A suggested item, disabled by default.
    • 0: A normal item, visible in the tree. (MENU_VISIBLE_IN_TREE)
    • -1: An internal item, not visible in the tree. (MENU_CALLBACKs, MENU_IS_LOCAL_TASK, and MENU_IS_LOCAL_ACTION)

So why would we want to do this, you ask?

For one, above mentioned issue for admin_menu tries to achieve something very similar to contextual links in core: For a certain, dynamic menu router path and given values for the dynamic arguments we want to retrieve a menu tree.

Simply put: Given the path admin/structure/menu/manage/% and the dynamic argument map array('navigation'), we want to retrieve this menu link tree:

admin/structure/menu/manage/%
admin/structure/menu/manage/%/list
admin/structure/menu/manage/%/edit
admin/structure/menu/manage/%/delete

Hence, we just ask for a regular menu tree. #620618: Optimize menu tree building and use it for toolbar plays an important role here too, as it allows to retrieve a menu tree starting from a certain parent link (although it doesn't support a dynamic $map yet).

But currently, that's not possible, because the menu system only stores menu callbacks as hidden links, but not local tasks or any other dynamic items.

Attached patch:

  1. fixes the improper use of above mentioned constants
  2. makes the menu system properly store dynamic paths and local tasks as "normalized" links in {menu_links} (just like menu callbacks were stored already)
  3. does not change the existing behavior for visible links in menu trees (i.e. there is no visual difference after applying this patch and rebuilding the menu)
  4. and additionally ensures that those menu links get a proper plid and pX columns in {menu_links}
Files: 
CommentFileSizeAuthor
#33 drupal.menu-visible-in-tree.33.patch22.75 KBsun
Passed on all environments.
[ View ]
#31 drupal.menu-visible-in-tree.31.patch24.48 KBsun
Passed on all environments.
[ View ]
#29 drupal.menu-visible-in-tree.26.plus-reparenting.patch10.26 KBsun
Passed on all environments.
[ View ]
#27 drupal.menu-visible-in-tree.27.patch9.02 KBsun
Failed on MySQL 5.0 InnoDB, with: 15,438 pass(es), 6 fail(s), and 1 exception(es).
[ View ]
#26 drupal.menu-visible-in-tree.26.plus-reparenting.patch10.26 KBsun
Passed on all environments.
[ View ]
#24 drupal.menu-visible-in-tree.24.plus-reparenting.patch10.11 KBsun
Failed on MySQL 5.0 InnoDB, with: 15,330 pass(es), 6 fail(s), and 7 exception(es).
[ View ]
#10 drupal.menu-visible-in-tree.10.plus-reparenting.patch8.75 KBsun
Passed on all environments.
[ View ]
#6 drupal.menu-visible-in-tree.6.patch4.22 KBsun
Failed on MySQL 5.0 ISAM, with: 14,858 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#6 drupal.menu-visible-in-tree.6.plus-reparenting.patch7.2 KBsun
Failed on MySQL 5.0 ISAM, with: 14,784 pass(es), 41 fail(s), and 14 exception(es).
[ View ]
#2 drupal.menu-visible-in-tree.0.patch2.73 KBsun
Failed on MySQL 5.0 ISAM, with: 14,768 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#1 drupal.menu-visible-in-tree.0.patch2.73 KBsun
Failed on MySQL 5.0 ISAM, with: 14,756 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Comments

StatusFileSize
new2.73 KB
Failed on MySQL 5.0 ISAM, with: 14,756 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

d.o infrastructure failure during issue creation due to http://drupal.org/node/631420 - re-uploading the patch.

StatusFileSize
new2.73 KB
Failed on MySQL 5.0 ISAM, with: 14,768 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

It takes 2 hours for me to run the full test suite, so I really hope that a new PIFT report will contain a proper link to the failing test.

Sorry, that was caused by Dreditor - now removed: http://drupal.org/cvs?commit=288838

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

So the only reason why this patch fails is that menu.module's menu_reset_item() invokes menu_get_item($router_path) to find the original router item to reset the link to, but that seems to find the existing, customized link now for any reason... Technically, the test shouldn't be affected by this change, because it is testing a link having a static path (user/logout), so I'm really not sure what goes wrong here.

StatusFileSize
new7.2 KB
Failed on MySQL 5.0 ISAM, with: 14,784 pass(es), 41 fail(s), and 14 exception(es).
[ View ]
new4.22 KB
Failed on MySQL 5.0 ISAM, with: 14,858 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

OK.

So why do I always have to fix completely unrelated things in the menu system when trying to fix one very focused thing? :-(

So here are two patches:

  1. Make menu_reset_item() properly respect the 'menu_name' that is defined in hook_menu() for a certain router path.

    As of now, this function just invokes menu_get_item($customized_link['router_path']) and somehow expects to get a clean router item out of that, but that obviously cannot work, because 'menu_name' as well as other properties are stored in {menu_links}, and the link in {menu_links} has been customized... weird logic.

  2. Further debugging revealed that this should work properly now, but the reset link (still user/logout) was still put into the 'navigation' menu (instead of 'user-menu'), and on top of that, it was placed under an arbitrary parent link.

    Thus, I figured: yeah, don't I know this behavior from somewhere?

    ...and went ahead and additionally applied my latest patch from #550254-48: Menu links are sometimes not properly re-parented.

    And tadaa! Proper re-parenting.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

sun requested that failed test be re-tested.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.75 KB
Passed on all environments.
[ View ]

Finally found the cause for the failing tests.

@pwolanin: Alrighty - now it would be time for a review! :)

Any chance to get this in?

I need to see some of these changes in context - for the most part they look reasonable.

@pwolanin, what are the suggested next steps then?

@Dries - I need to apply and test the patch and/or look at it in a better diff viewer so I can see more of the context of the changes.

(I could use a better diff viewer -- might be a nice DrEditor feature.)

hmmmm... can we proceed here? I really badly need this. And we can most probably leverage this fixed logic for dynamic breadcrumbs and potentially also contextual links (the latter needs investigation).

Subscribing...

Status:Needs review» Needs work

I don't think this should be removed - plid values of 0 will be present for any item at the top level in any menu.

    else {
      // Don't bother with the query - mlid can never equal zero..
      $parent = FALSE;
    }

The changes to menu_reset_item() also look wrong to me - you are loading the whole router in to memory in the patch for no apparent reason.

  $menu = menu_get_router();
  $router_item = $menu[$item['router_path']];
  $new_item = _menu_link_build($router_item);

We should also rename that function (among others).

#19: This is to fix the bug outlined in #550254: Menu links are sometimes not properly re-parented, which apparently is triggered in a steady way with this patch. The comment that "mlid can never equal zero" in itself is wrong, because this code tests plid and plid can very well be zero. Hence, the current code fails to re-parent items that are (currently) on the top-level of a menu.

#20: "for no apparent reason" - I tried to clarify in the comments above that the current code tries to reset a menu link to its original values, including the menu_name, but the original menu_name of a menu link cannot be gathered from anywhere other than the original menu router item definition. The current code is not resetting at all. For the current code to reset, it would have to delete the menu link, so the menu system would re-insert based on the menu router item.

Status:Needs work» Needs review

For now, the raised issues sound bogus, so back to needs review. If you need more clarification let me know. Otherwise, please go ahead with your review :)

errrr... OMG. with your review, not without. Sorry, what a horrible typo! :-/

StatusFileSize
new10.11 KB
Failed on MySQL 5.0 InnoDB, with: 15,330 pass(es), 6 fail(s), and 7 exception(es).
[ View ]

I really, really, really need this patch to go in. To support this, I've revamped the inline comments to be more precise and provide more reasoning.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.26 KB
Passed on all environments.
[ View ]

meh. See, that happens if you want to make a menu system maintainer happy :P

I renamed the argument $item to $link in menu_reset_item(), because I know that you were trying to remove all $items that are no $items elsewhere already.... but I didn't rename all ;)

And since finding this stupid mistake required some debugging, I was able to further optimize the code. :)

StatusFileSize
new9.02 KB
Failed on MySQL 5.0 InnoDB, with: 15,438 pass(es), 6 fail(s), and 1 exception(es).
[ View ]

Playing with this some more, I realized that we can leave the hunk for _menu_router_build() to #550254: Menu links are sometimes not properly re-parented, which means that the two issues can be more or less tackled separately again, and that this patch is self-contained.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.26 KB
Passed on all environments.
[ View ]

I obviously was wrong.

It still seems like we are running an extra, unneeded query for every link that has plid==0?

StatusFileSize
new24.48 KB
Passed on all environments.
[ View ]

ok, removed those unnecessary queries.

Now also added tests to ensure that we don't break this logic again.

These tests pass in a fresh install (i.e. when running tests). Interestingly, the data for those links in {menu_links} of my actual development site was wrong after applying the patch. The new, hidden links had a depth of 1 and no assigned parents at all, until I emptied the {menu_links} table and rebuilt the menu. Which in turn means that the link structure, which is added to menu_test.module in this patch, will serve as perfect testing base for #550254: Menu links are sometimes not properly re-parented

ok, let's see what the test bot says with that minor fix.

StatusFileSize
new22.75 KB
Passed on all environments.
[ View ]

Passed. :)

Also, broken HEAD is fixed now, re-rolled without the contained DatabaseTest fix.

looks RTBC, but will manually test later.

Any news? :)

sun requested that failed test be re-tested.

I really need this patch. It will also help us to resolve #576290: Breadcrumbs don't work for dynamic paths & local tasks, and as mentioned before, contextual links as well as regular local tasks will most likely be able to benefit from this, too, if there is sufficient time to work on follow-ups. If there won't, then at least a contributed module would be able to implement much more sane contextual links/local tasks - as well as other things, like admin_menu will.... but all of that is only possible if this patch lands.

Status:Needs review» Reviewed & tested by the community

tested locally - install and menu functions seem fine.

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks -- also for the extra documentation and tests.

Version:7.x-dev» 6.x-dev
Status:Fixed» Patch (to be ported)

YAYAYAYAY! This finally allows me to remove _all_ menu hacks from admin_menu! As promised, I'll work on #576290: Breadcrumbs don't work for dynamic paths & local tasks later on.

I also asked pwolanin in IRC, whether we could backport this fix to D6... while I was a bit worried about potentially breaking custom code that might contain equally insane workarounds like admin_menu, pwolanin sounded like he would be fine with that.

subscribing...

hope this will be in the next D6.17(?)

subscribe

(might be of-topic / should have pm'ed sun)

@sun #40: just a heads-up... if you do remove any hacks in 6.x (once this is ported), please remember to do it in a new release of admin_menu - perhaps major too - and clearly state that it is meant to be installed in core 6.18 and up (supposing this does get in 6.18). If you fail to do so, I foresee a lot of new issues in admin_menu's queue ;)

Assigned:sun» Unassigned

It's unlikely that I'm going to port this back to D6. D7 works and I'm entirely focusing on it currently.

Looking at this patch once again, it contains quite some bug fixes that may really help people on D6.

subscribe

Important bug fix for D6. thanks everyone. hope to see this fixed on the next minor release

Subscribing.

sun's comment in #40 seems to suggest that a backport to D6 could allow a stable admin_menu 6.x-3.x, which would probably prevent the continued pileup of comments in #246653: Duplicate entry in menu_router: Different symptoms, same issue.

I don't know whether I would be able to backport it correctly, if the two codebases are very different, but I may try.

I'd be curious to know if any work on this has been done so far. Even just getting incomplete patches up would be helpful for moving this issue along, for those of us staying on 6.x for the time being.

Version:6.x-dev» 7.x-dev
Status:Patch (to be ported)» Fixed

Does this mean that there's no intention to backport this to 6.x Daniel? Did you perhaps file a new issue for 6.x?

  1. Yes, at least I don't have an intention to work on D6.
  2. Retrospectively, looking at the changes of this patch once more, I'm not sure whether it would be 100% safe to backport it in the first place; it changes
    • meanings of constants
    • actively uses one constant for its intended purpose, but which wasn't used before (which means that API consumers realized that it wasn't used and possibly didn't use it either)
    • stores links in the menu links table that were not stored before

Status:Fixed» Closed (fixed)

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