The attached patch reintroduces the good ol' MENU_ITEM_GROUPING that disappeard after Drupal 5. Access control for this will be taken care of in #296693: Hide empty admin categories.

The patch adds functions for listing and theming child menu items, which replace a handful of functions left over from D6, that were for 90% identical to eachother.

Files: 
CommentFileSizeAuthor
menu_item_grouping_02.patch8.27 KBXano
Failed: 9235 passes, 1 fail, 0 exceptions
[ View ]

Comments

Looks like a nice cleanup. Do we already have tests for these pages?

When testing locally the following AdminOverviewTestCase->checkOverview() fails, but when checking the source code of the pages before and after the patch, they are the same.

I dunno why this was split out. One, MENU_VISIBLE_IN_TREE | MENU_VISIBLE_IN_BREADCRUMB | 0x0010 is very ugly. Two, the problem with this reintroduction is that it only works for paths which are rendered by system_admin_menu_block_page otherwise simply it does not work. I recommend wont this and only deal with the parent issue.

To properly do MENU_ITEM_GROUPING you need to know that when you are on the router path marked as MENU_ITEM_GROUPING you will see any children or not. This requires the retrieval of all children of said item and access checking them. What you can do, if you insist on this is to handle M_I_G as expanded inside the menu*data functions and then when actually building the tree, skip them. Note that we have a variable guarding the menu against expanded items and if we have M_I_G on always present core paths then this protection becomes moot and performance suffers. Without doubt, you will add another query at least to every page. You are, of course, free to prove me wrong.

Chx: I appreciate your input, but to be honest the way you talk sounds a little hostile. I am open to criticism, but I don't know _why_ you are against this. If I am doing things wrong, please, tell me and tell me how to make it better.

  1. MENU_VISIBLE_IN_TREE | MENU_VISIBLE_IN_BREADCRUMB | 0x0010 is ugly. OK, why? I have no experience with this whatsoever, and I actually creates this line of code by looking at the other constants. I was hoping that if it were wrong indeed (like I expected), somebody would tell me why since this is magic for me.
  2. system_admin_menu_block_page() is history after this patch. The fun part is that this patch also works for menu items that did not use that function before. Node/add and admin/settings works with this patch as well.
  3. On IRC you were complaining about performance. Understandable, since nobody wants Drupal to get any slower. This patch does not add or modify any serious parts of code. It merely merges existing functions. The other issue, about permissions, would indeed need all children elements every time for access control, like you say.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Closed (won't fix)

Discussed this with chx and this is not really doable.

Assigned:Xano» Unassigned