GHOP #131: Port the Menu Tree module to Drupal 6.
| Project: | MenuTree |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | GHOP |
This task is to update the Menu Tree module at http://drupal.org/project/menutree from Drupal 5 to Drupal 6. The Menu Tree module is a simple but powerful module that creates a dynamic "site map" for a web site.
Each new major release of Drupal, we introduce new API features and changes that can cause old modules to stop functioning. Modules need to be upgraded with each new version of Drupal so that they are compatible.
To complete this task, work with the module maintainer to port the code to the Drupal 6 API. Using the 5.x to 6.x upgrade page as a guide, make changes to the 5.x version of the module so that it works under Drupal 6.
The final deliverable will be a patch against the HEAD version of the module posted to the module's issue queue that has been reviewed and marked "Ready to be Committed". See the resources section for more information.
Resources:
* http://drupal.org/handbook/cvs
* http://drupal.org/patch/create
* http://drupal.org/node/114774 (Module upgrade guide)
* http://drupal.org/project/menutree
Primary contact:
Larry "Crell" Garfield
#1
We already talked about this one, so feel free to create @ Google.
#2
Duh. I am dumb. :) Sorry. Too many windows.
#3
The GHOP task associated with this issue can be found at:
http://code.google.com/p/google-highly-open-participation-drupal/issues/...
#4
The attached patch converts the module to 6.x. I also edited the README.txt, so don't forget to have a look at it, too.
#5
Great! I'll have a look at this later today or tonight.
#6
It would be nice to get this reviewed and finished today or by monday at the latest, so the student has a chance to claim another task before the deadline to claim tasks passes.
#7
Fixed some minor space issues pointed out by cwgordon7 on IRC.
#8
My apologies for not reviewing it Wednesday like I said I would. Unfortunately my mail server died then, and I spent the rest of the week trying to get email working again. :-(
Functionality-wise, everything looks good. Nice! However, there are some issues in the code:
As of D6, menus are identified by name, not mid. That means we need to do a few things.
- References to $mid in the admin form should be changed to $menu_name, since that's what is being saved, not an id.
- The README file needs to mention that the paths have changed, which means any path aliases will need to be updated.
- If at all possible, we need an upgrade mechanism for the title/intro text. As is, this would lose that data and leave stale variables in the variable table. That's badness. I don't know if it's possible to map all old menus to new menus or not. If so, great. If not, I'd consider just mapping Navigation, Primary Links, and Secondary Links, since those have known IDs, to be acceptable. There needs to be an upgrade hook to load the old variables, save the corresponding new variables, and then delete the old variables.
Other stuff:
- In menutree_display(), the variable_get() call for intro text should be part of the second block, after the comment that says what it's doing.
- In theme_menutree_tree(), the leading \n is unnecessary. In fact I'd say all of them but the final are unnecessary.
- In menutree_tree_page_data(), do not use an implode() to create database placeholders. db_placeholders() function, which is more secure.
- The do-while loop in menutree_tree_page_data confuses me greatly. $num_rows is a boolean, in which case it should be called $is_foo or $has_foo or something else that is self-documenting as a boolean rather than as a counter. Also, do-while is very rarely used in Drupal. I'm not entirely sure of its purpose here, but it looks like a performance hit since you're issuing a query inside of a loop. That's going to thrash the database. Is there no menu api function to call to get the data you need?
If you stick with a do-while there, make sure to document inline why you're using such an unusual structure, especially since the function documentation says it's nearly the same as menu_tree_page_data() and yet it looks nothing like it internally.
#9
Well, I actually just used lines 850 to 862 of menu.inc for this, so it is the same as menu_tree_page_data() except that I removed all the caching and just-expand-items-in-the-current-breadcrumb things. But I will see if I can find a simpler solution.
I fixed the rest of the issues, but have no CVS access at the moment, so I will provide patch later this day.
#10
The attached patch fixes all issues except the do-while-thing.
#12
You are recursively crawling the tree but why? If you want the whole tree then menu_tree_all_data() is all you need, which grabs the whole menu tree with one query.
#13
On visual inspection I don't see another problem beyond what chx mentioned. Try to clean that up (use the API to get the tree and then theme it appropriately, which may even be another existing theme call) and I'll give it another review tonight if I can.
#14
Somehow I overlooked menu_tree_all_data()... thanks, that simplifies the patch a lot.
#15
theme_menutree_tree() isn't neccessary any more.
#16
Ah, using menu_tree_all_data() dips into the cache, so there's no SQL hit at all. Sweet! :-)
I made a few slight changes:
- Added proper word wrapping to the README file.
- Renamed the update function from _1 to _6001, per new Drupal convention.
Committed, branched, and tagged. New tarballs should be coming out shortly. Thanks! Remember to post the final patch to the GHOP issue as well so that Google gives you credit for it.
Quick, go grab another task. :-)
#17
Automatically closed -- issue fixed for two weeks with no activity.