[drupal-devel] menu system idea
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Instead of defining menu's as we do now (using 'path' => ) , why don't we skip the entire building a tree thing and just define the menu entries as trees to begin with? ie : $menu['node']['add']['blog'][ITEM] = array(title => t('blog entry'), access => user_access('edit own blog')); $menu['blog'][ITEM] = array(title => t('blogs'), callback => 'blog_page', access => user_access('access content'), type => MENU_SUGGESTED_ITEM); (ITEM would just be a define('ITEM', '__item') to avoid namespace conflicts.) This would mean we could 'build' the menu, just by : $_menu = array(); foreach (module_implements('menu') as $module) { $_menu = array_merge_recursive($_menu, module_invoke($module, 'menu')); } Furthermore, we could solve the multiple menu tree requirement by defining menu items for the primary navigation such as : $menu[PRIMARY]['node']['add']['blog'] = array(title => t('new post!'), access => user_access('edit own blog')); It might also simplify the menu system to the point where mere mortals can understand it =) - -- Adrian Rossouw Drupal developer and Bryght Guy http://drupal.org | http://bryght.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (Darwin) iD8DBQFCtJp7gegMqdGlkasRAsY/AJwJARgxJz3zgNQnySSM54XqYi3bDgCgo7ml ewl/EPrMJRsafBJrqQdQxaA= =wlf3 -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 And here's an example of how you would get the right menu callback : <?php define('item', '__item'); $menu[item] = array('title' => 'root page', 'callback' => 'root_page'); $menu['node'][item] = array('title' => 'listing', 'callback' => 'node_listing'); $menu['node']['add']['blog']['here'][item] = array('title' => 'blog entry', 'callback' => 'some_func' ); $menu['node']['add'][item] = array('title' => 'create content', 'callback' => 'node_page'); $path = explode('/', $_GET['q']); $q = $path; while (sizeof($q)) { $vars = array(); $page = array(); $current = $menu; foreach ($q as $i) { if (array_key_exists($i, $current)) { $page[] = $i; $current = $current[$i]; } else { break; } } if (array_key_exists(item, $current)) { $callback = $current[item]['callback']; $args = array_slice($path, sizeof($page)); break; } array_pop($q); } print_r(array('callback' => $callback, 'args' => $args)); ?> Another thing that's interesting about this code, is that we can set up a root menu handler ie $menu[item] = array(/* stuff */); Which might end up being a cleaner way of doing the variable_get ('site_frontpage') stuff. On 19 Jun 2005, at 12:04 AM, Adrian Rossouw wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Instead of defining menu's as we do now (using 'path' => ) , why don't we skip the entire building a tree thing and just define the menu entries as trees to begin with?
ie : $menu['node']['add']['blog'][ITEM] = array(title => t('blog entry'), access => user_access('edit own blog')); $menu['blog'][ITEM] = array(title => t('blogs'), callback => 'blog_page', access => user_access('access content'), type => MENU_SUGGESTED_ITEM);
(ITEM would just be a define('ITEM', '__item') to avoid namespace conflicts.)
This would mean we could 'build' the menu, just by :
$_menu = array(); foreach (module_implements('menu') as $module) { $_menu = array_merge_recursive($_menu, module_invoke($module, 'menu')); }
Furthermore, we could solve the multiple menu tree requirement by defining menu items for the primary navigation such as : $menu[PRIMARY]['node']['add']['blog'] = array(title => t('new post!'), access => user_access('edit own blog'));
It might also simplify the menu system to the point where mere mortals can understand it =)
- -- Adrian Rossouw Drupal developer and Bryght Guy http://drupal.org | http://bryght.com
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (Darwin)
iD8DBQFCtJp7gegMqdGlkasRAsY/AJwJARgxJz3zgNQnySSM54XqYi3bDgCgo7ml ewl/EPrMJRsafBJrqQdQxaA= =wlf3 -----END PGP SIGNATURE-----
- -- Adrian Rossouw Drupal developer and Bryght Guy http://drupal.org | http://bryght.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (Darwin) iD8DBQFCtK9rgegMqdGlkasRAp4+AKCjrOeURlhdKEdFwePqZDWCLjMI5QCfRErf fehRWUg9pGhkS2/Au9cRi+s= =Wt8F -----END PGP SIGNATURE-----
Adrian Rossouw wrote:
It might also simplify the menu system to the point where mere mortals can understand it =)
I appreciate the effort to simplify the menu system, but your examples of multi-dimensional arrays make my brain hurt even more than the current menu system arrangement. :-/ -- Chris Johnson
Chris, Op zondag 19 juni 2005 06:34, schreef Chris Johnson:
Adrian Rossouw wrote:
It might also simplify the menu system to the point where mere mortals can understand it =)
I appreciate the effort to simplify the menu system, but your examples of multi-dimensional arrays make my brain hurt even more than the current menu system arrangement. :-/
$menu[item] = array('title' => 'root page', 'callback' =>'root_page'); $menu['node'][item] = array('title' => 'listing', 'callback' =>'node_listing'); $menu['node']['add']['blog']['here'][item] = array('title' =>'blog entry', 'callback' => 'some_func' ); $menu['node']['add'][item] = array('title' => 'create content', 'callback' => 'node_page'); $menu['node'][item] would replace your current 'path' => 'node/item' Is the part to look at, The other part is, afaik only to show how easy it is to handle the menus in the rest of the code, not in your code! Alternatively, Adrian, if people really do not like this new idea of your (I like it), an option would be to add extra logic in core that first iterates over all menus, and creates your multidim. arrays from the 'path/to' variables? Regards, Bèr -- [ Bèr Kessels | Drupal services www.webschuur.com ]
On Sun, 19 Jun 2005, Adrian Rossouw wrote:
Instead of defining menu's as we do now (using 'path' => ) , why don't we skip the entire building a tree thing and just define the menu entries as trees to begin with?
ie : $menu['node']['add']['blog'][ITEM] = array(title => t('blog entry'), access => user_access('edit own blog')); $menu['blog'][ITEM] = array(title => t('blogs'), callback => 'blog_page', access => user_access('access content'), type => MENU_SUGGESTED_ITEM);
I don't know why, but I think that nested arrays make the code more difficult to read. Cheers, Gerhard
The nested arrays don't bother me. How much work would one have to do in this direction before we would be able to 1) measure performance and 2) rule out any "gottchas" that would make it less attractive than it initially seems? Robert
On 19 Jun 2005, at 10:26, Robert Douglass wrote:
The nested arrays don't bother me. How much work would one have to do in this direction before we would be able to 1) measure performance and 2) rule out any "gottchas" that would make it less attractive than it initially seems?
I agree. We can't really tell it is an improvement until it has been tested. I think it actually might fix a number of gottchas. Example: because modules get loaded in random order, a menu item's parent might only become available after the child has been loaded. Therefore, the current menu system first stores all menu items in a sequential array, and only builds the hierarchy of menu items after all modules have returned their menus. The current menu system has two important drawbacks: 1. it is slow -- we had to add menu caching, but even with caching it is slow. 2. it is complex -- it tends to be difficult to make simple changes. -- Dries Buytaert :: http://www.buytaert.net/
On Sun, 19 Jun 2005, Dries Buytaert wrote:
On 19 Jun 2005, at 10:26, Robert Douglass wrote:
The nested arrays don't bother me. How much work would one have to do in this direction before we would be able to 1) measure performance and 2) rule out any "gottchas" that would make it less attractive than it initially seems?
I agree. We can't really tell it is an improvement until it has been tested. I think it actually might fix a number of gottchas. Example: because modules get loaded in random order, a menu item's parent might only become available after the child has been loaded. Therefore, the current menu system first stores all menu items in a sequential array, and only builds the hierarchy of menu items after all modules have returned their menus.
I don't see how this could be improved using Adrian's arrays.
The current menu system has two important drawbacks: 1. it is slow -- we had to add menu caching, but even with caching it is slow.
If Adrian's scheme can improve performance, I'd be all for it. I'd still the code is ugly, but ...
2. it is complex -- it tends to be difficult to make simple changes.
One change I'd like to see is that permissions also apply to child menu items. For example the user permission "access administration pages" applies to the path "admin", but not to any of the child menu items. Cheers, Gerhard
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 19 Jun 2005, at 11:17 AM, Gerhard Killesreiter wrote:
On 19 Jun 2005, at 10:26, Robert Douglass wrote:
The nested arrays don't bother me. How much work would one have to do in this direction before we would be able to 1) measure performance and 2) rule out any "gottchas" that would make it less attractive than it initially seems? First step is speaking to JonBob about this, as he has a much better understanding re: the gotchas of the menu system than any of us.
He might have considered the same, but decided against it.
I don't see how this could be improved using Adrian's arrays. Oddly enough, i harbour no love for multi-dimensional arrays =)
I do however think this is one of the cases where they will actually provide a more natural access method to the data they contain. For instance, if you want to get all the child pages for node , you can just cycle through $menu['node'], and print all the children that have 'item' elements. Same with drawing the local tasks. I just think that using a tree to store a .. you know.. tree, results in clearer, and easier to debug code.
The current menu system has two important drawbacks: 1. it is slow -- we had to add menu caching, but even with caching it is slow.
If Adrian's scheme can improve performance, I'd be all for it. I'd still the code is ugly, but ... The data we are manipulating is much simpler, and doesn't need to be 'built' into anything. The data that has been defined by the module developer is the data used to traverse and render the menu system.
2. it is complex -- it tends to be difficult to make simple changes.
One change I'd like to see is that permissions also apply to child menu items. For example the user permission "access administration pages" applies to the path "admin", but not to any of the child menu items.
The following addition to the code i sent earlier does just that. I would also like to be able to add additional properties to menu items, and save it somewhere in the database. For instance, I would like to be able to set a 'section' bit on a menu item, and then have that cascade down. That could be used to set all admin/ pages to use a different theme for instance. define('item', '__item'); $menu[item] = array('title' => 'root page', 'callback' => 'root_page', 'access' => true); $menu['node'][item] = array('title' => 'listing', 'callback' => 'node_listing', 'access' => true); $menu['node']['add']['blog']['here'][item] = array('title' => 'blog entry', 'callback' => 'some_func' ); $menu['node']['add'][item] = array('title' => 'create content', 'callback' => 'node_page', 'access' => false); $path = explode('/', $_GET['q']); $q = $path; while (sizeof($q)) { $vars = array(); $page = array(); $access = $menu[item]['access']; $current = $menu; foreach ($q as $i) { if (array_key_exists($i, $current)) { $page[] = $i; $current = $current[$i]; if (array_key_exists(item, $current)) { if (array_key_exists('access', $current[item])) { $access = $current[item]['access'] && $access; } } } else { break; } } if (array_key_exists(item, $current)) { $callback = $current[item]['callback']; $args = array_slice($path, (sizeof($page)) ? sizeof($page) : 1); break; } array_pop($q); } print_r(array('callback' => $callback, 'args' => $args, 'access' => ($access) ? 'true' : 'false')); - -- Adrian Rossouw Drupal developer and Bryght Guy http://drupal.org | http://bryght.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (Darwin) iD8DBQFCtV8LgegMqdGlkasRAjXTAJsG1a38qnmkQxcQ3oD6OflImVH2yQCeOrAL U36y7nx2/wkP7kZeMisOmt8= =JnSz -----END PGP SIGNATURE-----
Adrian Rossouw wrote:
Oddly enough, i harbour no love for multi-dimensional arrays =)
I do however think this is one of the cases where they will actually provide a more natural access method to the data they contain.
For instance, if you want to get all the child pages for node , you can just cycle through $menu['node'], and print all the children that have 'item' elements.
Same with drawing the local tasks.
Maybe really good documentation would make it easier to grok.
I just think that using a tree to store a .. you know.. tree, results in clearer, and easier to debug code.
I certainly agree with that. But I usually think of trees as being, well, trees, with pointers to parents and children. You've cleverly mapped this onto multidimensional arrays. I tend to think of trees as having 2 dimensions, depth and breadth. I guess my brain is dimensionally challenged. :-)
1. it is slow -- we had to add menu caching, but even with caching it is slow.
If Adrian's scheme can improve performance, I'd be all for it. I'd still the code is ugly, but ...
I'm all for performance improvement, too. If this scheme makes for faster page generation, I'll bite the bullet and learn to work with it. -- Chris Johnson
One change I'd like to see is that permissions also apply to child menu items. For example the user permission "access administration pages" applies to the path "admin", but not to any of the child menu items.
Yes it does apply to chil menu items unless those children declare their own access control check.
On Sun, 19 Jun 2005, Moshe Weitzman wrote:
One change I'd like to see is that permissions also apply to child menu items. For example the user permission "access administration pages" applies to the path "admin", but not to any of the child menu items.
Yes it does apply to chil menu items unless those children declare their own access control check.
I thought so as well, but the security problem in dba.module tought me otherwise. Maybe it appies only to direct children? Cheers, Gerhard
Took a first look at the idea. I think I rather like it. Things to watch out for (these may or may not be problems with this approach): - The menu system needs to handle modules being loaded in any order (children defined before parents) - TMSNTH parent modules not being loaded at all, and children need to "fall up" to higher levels in the tree in this case. - TMSNTH moving around and renaming items. I don't think this approach is better or worse in this regard; we still have to go through the whole process of assigning mids and pids, AFAICT. - TMSNTH the addition of new, custom items. Ditto from above. I'd be curious to see benchmarks. I don't feel that this code is too much more or less legible than the current approach.
participants (8)
-
Adrian Rossouw -
Bèr Kessels -
Chris Johnson -
Dries Buytaert -
Gerhard Killesreiter -
Jonathan Chaffer -
Moshe Weitzman -
Robert Douglass