APi fixes for menu_tree_data() including depth param
catch - July 3, 2009 - 18:03
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | menu system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Needs Documentation |
Description
Splitting off from #484820: Initial D7UX admin header
You currently can't get a slice of a menu down to a certain depth, only all items below a certain point in the tree - it might make sense to add a $depth param to menu_tree_all_data() to just be able to get certain levels of the tree at any one time.
#1
We shoudl pull in some of the straigh menu APi fixes from #483614: Better breadcrumbs for callbacks, dynamic paths, tabs to make that issue simpler to deal with.
#2
first pass here - passing an array into function menu_tree_data() since that makes the functions more re-usable
#3
first pass here - passing an array into function menu_tree_data() since that makes the functions more re-usable
#4
The last submitted patch failed testing.
#5
<?php+ $result = array_reverse($result);
[...]
+ while ($item = array_pop($result)) {
+ }
?>
I suggest walking the array in the normal order and using unset().
#6
@Damien - but pop is such a nice function name and does just what we want :)
I'm really not such how do do what you suggest in as simple a way. To me this seems ugly and much harder to understand by comparison:
while ($data = each($result)) {
unset($result[$data[['key']);
$item = $data['value'];
}
I chose a reverse and pop because array_unshift() is a rather more expensive operation than pop.
#7
Why not:
<?phpforeach ($result as $key => $item) {
unset($result[$key]);
// ...
}
?>
#8
@Damien - still rather ugly imho - in either case we need to pass the array by reference when we do the recursion. What happens when we come out of the recursive call and we have unset the the array element that the current foreach loop thinks is the current element? I hope PHP handles that gracefully?
#9
A better pass - allows for radicial simplification of _menu_tree_data()
#10
The last submitted patch failed testing.
#11
oops - forgot to use --no-prefix
#12
#13
The last submitted patch failed testing.
#14
fixed book and menu module.
#15
The last submitted patch failed testing.
#16
+++ includes/menu.inc@@ -843,13 +844,16 @@ function menu_tree_output($tree) {
+function menu_tree_all_data($menu_name, $item = NULL, $max_depth = NULL) {
Missing documentation for the $max_depth param.
+++ includes/menu.inc@@ -939,13 +951,16 @@ function menu_tree_all_data($menu_name, $item = NULL) {
+function menu_tree_page_data($menu_name, $max_depth = NULL) {
Also missing @param documentation.
Otherwise looks good apart from that one fail, I don't fully get all the changes here, especially not this evening, but the gnarliest bit is at least a lot less gnarly than what's in there now.
This review is powered by Dreditor.
#17
I think the problem was that book_menu_subtree_data() had a bug- failed to rename a variable.
#18
remove unneeded $result->setFetchMode() calls, fix comment
#19
If those menu tree data functions would additionally allow to specify a starting menu link instead of a menu name, that would heavily trivialize admin_menu as well as http://drupal.org/project/menu_block (which contains some raw menu_*() forks that should be in core)
#20
more feedback from chx, leading to improved and expanded code comments to clarify the algorithm.
#21
slight additional clarification
#22
Very nice code cleanup! I am surprised this could be simplified so much. I got a headache when coded the original, however, it's not an easy thing, this tree traversal because of the way it is. Some time and a "new" coder helped it :)
#23
The last submitted patch failed testing.
#24
Oh, testing bot...
#25
<?php+ $links = array();
+ foreach ($result as $item) {
+ $links[] = $item;
+ }
+ $tree = menu_tree_data($links);
?>
What am I missing here? It seems like we're just making a copy of $result?
#26
@Dries - $reslt is a query result object that implements the Iterator interface so we can call foreach() one it (but not other array functions). By doing the foreach first, we allow the menu_tree_data() function to take a more generic array as a parameter.
#27
The last submitted patch failed testing.
#28
fixed a very minor 1-line merge conflict in book.test (where I am fixing a typo in the test that was failing above).
#29
minor doxygen changes per Dries and added type hinting to the arguments of menu_tree_data()
#30
I think this PHPDoc is sufficient. While I too found this confusing, it's not the job of menu API code to document DBTNG, and the docs that talk about a query result object are enough of a breadcrumb for me to go off searching if I need to for further clarification.
This looks like a nice clean-up to make the menu API more comprehensible to mere mortals. :)
Committed to HEAD. Needs updating in the documentation.