Reintroduction of MENU_ITEM_GROUPING [#363951]

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: Restrict access to empty top level administration pages.

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.

Comment	File	Size	Author
	menu_item_grouping_02.patch	8.27 KB	Xano

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

catch

he/him

English

CreditAttribution: catch commented 25 January 2009 at 16:25

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

Comment #3

Xano

he/they

English

Southampton

CreditAttribution: Xano commented 25 January 2009 at 18:01

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.

Comment #5

chx CreditAttribution: chx commented 25 January 2009 at 19:10

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.

Comment #6

chx CreditAttribution: chx commented 25 January 2009 at 19:21

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.

Comment #7

Xano

he/they

English

Southampton

CreditAttribution: Xano commented 25 January 2009 at 19:25

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.

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.
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.
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.