Toolbar module (and others) should properly build its menu slice
Damien Tournoud - November 2, 2009 - 02:45
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | menu system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | D7 API clean-up |
Description
The toolbar module has been implemented with insufficient knowledge and understanding of how the menu system works. The current code is ugly and very inefficient (both in term of processing power and in term of memory usage), introduces some code duplication and a maintenance hell.
We need to fix that. Now.
This issue is the first step: introduce a clean way to build a slice of a menu.
#1
This patch:
- factor out the common tree-building logic of menu_tree_all_data() and menu_tree_page_data() into a menu_build_tree() function
- uses this new function in the toolbar module
#2
The last submitted patch failed testing.
#3
<?phpcache_clear_all('links:' . $menu_name . ':', 'cache_menu', TRUE);
?>
in function
<?phpmenu_cache_clear()
?>
either remove the ':', or edit your cache cid.
#4
@Makara: you are the best.
Same patch, with a simpler, and better tree cache ID.
#5
The last submitted patch failed testing.
#6
This is a very interesting alpha-quality patch, which actually maps nicely to what I outlined in #618530: System module's "listing" pages (and blocks) should use menu_build_tree().
#7
+++ modules/toolbar/toolbar.module@@ -134,12 +134,13 @@ function toolbar_get_menu_tree() {
$admin_link = db_query("SELECT * FROM {menu_links} WHERE menu_name = 'management' AND module = 'system' AND link_path = 'admin'")->fetchAssoc();
...
+ $data = menu_build_tree('management', array(
+ 'parents' => array($admin_link['mlid']),
+ 'min_depth' => 2,
+ 'max_depth' => 2,
+ ));
Instead of a hard-coded 2, we want to use $admin_link['depth'] here. :)
+++ modules/toolbar/toolbar.module@@ -134,12 +134,13 @@ function toolbar_get_menu_tree() {
+ menu_tree_check_access($data['tree'], $data['node_links']);
+ $tree = $data['tree'];
Do we really want to put this into the caller's responsibility?
How about going with this menu_build_tree(), but perhaps make it private, i.e. _menu_build_tree(), and make menu_build_tree() invoke _menu_build_tree() + additionally invoke menu_tree_check_access() and return the resulting tree data? :)
This review is powered by Dreditor.
#8
now the cid is too long...
#9
This should work.
#10
Sorry, left a stale parameter in there.
#11
So what's left here?
This works beautifully. Aside from that:
1) A bit wonky is the "parents" tree parameter (list). When would I want to pass more than one plid? (Though we can as well ignore that, because it equals the current code.)
2) uhm, nothing. I'll check whether we could use this cool thingy also for contextual links now ;) (separate issue)
#12
+++ includes/menu.inc 5 Nov 2009 18:54:56 -0000@@ -1212,10 +1131,115 @@ function menu_tree_page_data($menu_name,
+ $tree_cid = 'links:' . $menu_name . ':tree-data:' . $GLOBALS['language_interface']->language . ':' . md5(serialize($parameters));
I'm not comfortable with this. There is no guarantee that the tree ID is unique for a set of parameters. The risk of conflict is small, but real.
I'm on crack. Are you, too?
#13
That, I don't get. How can the tree be different for the same parameters?
Access checking happens later only, so I really don't see how this would be possible.
I also don't see a ->random() in there. ;)
It would make me really worry about this function. The result of a function should be the same under the same conditions. I'd consider it a critical bug if a function doesn't return the same for the same arguments.
So can you clarify?
#14
Tagging (and updating the other issue accordingly)
#15
Can we proceed here?
In the meantime, I looked into whether this function could be used for contextual links as well. It probably could be, if the menu system would store a proper plid, depth, and pX columns for dynamic paths/local tasks... which it is currently not. That's surely a separate issue (perhaps related to the existing #550254: Menu links are sometimes not properly re-parented), but this patch would be a major step towards a better solution.
#16
That's not what I'm saying. I'm saying that md5(serialize($parameters)) can be the same for several $parameters. But it's not probably that common, given that $parameters is typically small.
#17
Fixed inline comments.
#18
In $tree_parameters,
- renamed 'parents' to 'expanded'
- renamed 'active_link' to 'active_trail'
because I think that's much more understandable for people who want to use this tree builder.
#19
The last submitted patch failed testing.
#20
Test slave #44 produces non-reliable results.
#21
I RTBC this on the condition that in a followup patch we will review how menu build tree and menu tree data could be merged.
#22
I think this is a great clean-up that should be committed.
One comment though: in the original description, DamZ complains about unnecessary duplication but this patch adds more lines than it removes ... Also, in the original description, DamZ complains that the original code is inefficient (both in term of processing power and in term of memory usage), but I don't see performance numbers on the new code? Do we want to test whether it is actually faster? (It should be.)
#23
That sounds like a 'needs review' to me. :)