We considered using this module as an example of how NOT to write menu code for the presentation in Szeged, but another module got the honor.
see: http://szeged2008.drupalcon.org/program/sessions
http://szeged2008.drupalcon.org/files/menu-szeged2008.ppt
http://szeged2008.drupalcon.org/files/menu_chx.zip
The code is really misguided:
http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/taxonomy_me...
In particular you do:
foreach (taxonomy_get_vocabularies() as $vocab) {
and then another loop later.
You do not need a router item for each vid, since they all have identical callbacks, etc.
You should define the one or two generic router item with a wildcard, and then save links for each. Saving a router item for each is going to bloat the router table and may have performance/memory townsides.
use menu_link_save() and/or menu_link_maintain() to jsut save links. See, for example, admin_menu or the core aggregator module.
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | taxonomy_menu-drupal6_menu-304379-18.patch | 27.44 KB | Mark Theunissen |
Comments
Comment #1
chasz commentedouch !!! LOL
Comment #2
pwolanin commentedhmm, guess I should have checked if you volunteered for "Drupal Tough Love" before posting ;-)
Comment #3
pwolanin commentedComment #4
derhasi commentedI prepared some work for including proper D6 menu handling, so I started with some code, that could be implemented to generate the menu-structure. It was nice if you gave me some answer whether this is the right direction to forward this module, and if I seem to understand the menu-implementation ;)
Comment #5
Afief commentedSweet:-) that's quite similar to white I wrote(except for the taxonomy_menu_path which now makes lot of sense, sorry for not seeing it in the other issue)
A few things though:
Perhaps a recursive helper function would help?
Do you mind if I work on the patch along with you or do you prefer making the changes yourself?
Thanks for the patch:) hope it evolves to be committable soon
Comment #6
Mark Theunissen commentedSubscribing. Hopefully I can get involved too at creating the patch, because we may need this for an upcoming project. Thanks.
Comment #7
ckidowSubscribing... very very important
Comment #8
ppmax commentedI like this module but (unrelated to this post) decided I wanted to rebuild it for my own purposes. I saw this post and it coincides with my own menu related questions.
Heres a summary of what I did with a little context:
Most of the drupal sites I develop tend to be more "structured" (if thats the right word) around taxonomy, node types, and views, EG nodes can only be accessed by their viewpath/taxonomy term/taxonomy term/node title. I theme all my links so that "taxonomy links" are basically themed links to views with args. Because of this I typically end up manually creating menu items for all the taxonomy trees I use, which is fairly cumbersome, and requires extra maintenance when the client wants to add new terms or whatever. So I checked out taxonomy menu and really liked it--but it didnt behave the way I wanted. So the module I wrote (props where props are due: taxonomy term and image modules) lets you pick a taxonomy term and write in a custom path. I wrote it so these paths get added to primary-links so they can be managed (weight, expanded, etc) in the primary-links UI. Currently Im kinda stuck because while I see my new links in the menu_links table they arent exposed in the primary-links UI (help!). Any comments, feedback, or suggestions very welcome! here's the code:
Form
Form theme. Basically make a table of all related elements...I love this technique
Form submit
Comment #9
indytechcook commentedHere is what I have so far. I added a schema for a table taxonomy_menu.
New taxonomy_menu.install
Changes to taxonomy_menu_create_menu_items from derhasi
New functions to interact with the new table.
There is a function being called "taxonomy_menu_path" but I couldn't find a reference of it anywhere. Luckily my buddy google found this for me. It was posted about the same time derhasi posted his version. http://drupalbin.com/3185
Please comment.
Comment #10
indytechcook commentedIf anyone else is interested, here are more entires from about the redesign from September. I will dive into these tomorrow. http://drupalbin.com/search/node/taxonomy_menu
Comment #11
Mark Theunissen commentedCan those providing patches please do so using
http://drupal.org/patch/create
instead of dumping code... thanks!
Comment #12
indytechcook commentedMark, this code was in not ready for a patch. It was merely to let you know of my progress. I am the new module maintainer. You are welcome to checkout DRUPAL-6--2 branch to see the progress. The current state of this branch is not ready to patch against. I am writing an invoke statement during the next few days that the community can build off of.
Comment #13
Mark Theunissen commentedHi indytechcook, thanks for clearing that up. I need this module fairly urgently for a large project I'm busy with, which is why I've spent a day rewriting it.
Specifically I've trimmed off all the excess cruft that I believe is no longer useful given the fact that we have the Views module. I have removed:
- Views specific code, as Views will now be inherently supported.
- Taxonomy Default specific code, as it didn't do anything.
- Page callback _taxonomy_menu_page() as I think this was an incorrect implementation.
- Definitions of constants like TAXONOMY_MENU_NONE. We now use Views to decide output format, and these are unnecessary.
- _taxonomy_menu_admin_submit() function, because we now use system settings form properly.
Changes to the way it works:
- Most of the code was in the taxonomy_menu.inc file, and module hooks would include this file and call functions from it. This is against Drupal coding standards so the code is now back where it belongs (mostly in the main Module but the admin form still in the .inc file and included the proper way).
- Settings form now correctly calls system_settings_form().
- Turning on taxonomy_menu per vocab is now done from each vocab's edit page.
- Taxonomy menu creates link items using menu_link_save().
It now works with path aliases (and thus with Pathauto). It correctly uses the Drupal 6 menu system as specified in the resources given by this issue's creator.
Just a question about the use of a table to record links between menu link items and terms - my gut solution to taxonomy menus is to have a "rebuild" button for each vocab, that will take the vocab, delete all links originally created by taxonomy_menu and recreate the menu. This doesn't require any database access, and thus I'm curious why did you decide to use a table?
Comment #14
Mark Theunissen commentedI will probably be done tomorrow, can I submit a patch? It will be against the 6.01 branch.
Comment #15
indytechcook commentedMark, this is good work. Please submit a patch. You must have read my mind on several points. A couple of comments.
To keep this module as clean as possible, I don't want to place a dependence on views. The original intent is have the module to work more as an API and implement a hook. Then other modules can extend the functionality and add views/pathauto support (or whatever else comes along).
I implemented the database storing of terms due all of the issues with the current version where the menu and vocabularies becoming unlinked. Especially when upgrading or turning on caching. This puts a consistent place where the data is stored allowing you to do whatever you want to the menu without effecting the relationship (Moving, setting expand, interference with other modules).
My of the questions I would have for you would be answered when you submit your patch so I will hold off.
Please submit your patch. I'd love to see what you have done and I'm sure I will be able to incorporate it.
Comment #16
Mark Theunissen commentedGreat, my responses to your points:
1) Absolutely right, there is no dependency on Views. We simply output the path /taxonomy/term/xxx in the link. This is the default taxonomy list from taxonomy.module. It can be overridden by using the Views module, but this is optional (and automatically transparent). If URL aliases are present, they are used. But in the same way, there's no dependency on Pathauto.
2) Thanks for the explanation, I see why the DB is necessary - there is no other way to store the link between menu links and terms. I have copied your schema definition and install files.
Further thought:
A) A user can select which menu they want their taxonomy tree to be added to. Taxonomy menu shouldn't have to create this menu, rather it should be created by a user as a custom menu, if they require this.
B) In hook_taxonomy(), if a term is added, we add just this term into the menu, with the correct parent, without upsetting any other customisations (i.e. don't rebuild the whole thing). This is done by checking our taxonomy_menu table.
Comment #17
indytechcook commentedSweet. So we are on the same page.
Item A was on my list to implement. Wouldn't be that big of a deal. Someone has already submitted a patch for D5. B is very interesting. I like not having to rebuild the menus. Add it to the table and call menu_link_save.
I'm eager to see your code. I'm building on some code sent to me by the previous maintainer. They created a framework for a "hook_taxonomy_menu_handlers." interesting stuff but needs a lot of work.
Comment #18
Mark Theunissen commentedRight!
1) Patch attached for 6.x-1.01. So far the only $op in hook_taxonomy() that is implemented is the 'insert' one.
2) I'm not sure we need to run db_rewrite_sql() on the SQL. Do we really want people to rewrite the module's interaction with it's own table? As far as I'm aware, this should only be called in cases where permissions are an issue, like querying the node table.
3) The code is almost completely overhauled. But it's still nice and brief. But the patch can't really be read by itself. Needs to be applied.
4) I haven't checked the breadcrumb-setting part of the code (the hook_nodeapi()). It does appear to be working however.
5) Most settings are per-vocab (i.e. go to admin/content/taxonomy/edit/vocabulary/xx to see options). The rest could also be pulled out of the Taxonomy Menu admin page and made into per-vocab settings.
6) The schema is the same as the one you posted in this thread (but note in your hook_schema() function above, you forgot to return $schema).
7) The taxonomy menu is completely rebuilt only when the user selects a different menu from admin/content/taxonomy/edit/vocabulary/xx. Or if they disable, then re-enable it on the same page.
8) Otherwise, rebuild applicable terms only on hook_taxonomy().
Seems to be working nicely in my testing. Since I am going to be using this module in a current project, I should run into issues if there are any myself. Looking forward to your thoughts!
Comment #19
Mark Theunissen commentedActually number (4) above is kinda-working, but sometimes it grabs the wrong vocab.
Comment #20
indytechcook commentedThis is a great start. You probably saved me a week of work. That is awesome. Quick tip is to replace "menu_rebuild();"with "variable_set('menu_rebuild_needed', TRUE);" This is a change in the latest DEV package. It resolves several caching issues.
I'll apply it tonight and commit to HEAD.
Comment #21
Mark Theunissen commentedGreat news! I have done the replacement. Looking forward to moving on with this towards a stable release.
Comment #22
Mark Theunissen commentedDid you get a chance to commit to the DRUPAL-6--2 branch? I'm keen to start providing more patches but I want to work against a more up-to-date CVS.
Comment #23
indytechcook commentedNo because the code didn't work for me. I don't want to commit code that didn't work. I'm almost done with the tweaks and I will commit tonight.
Comment #24
Mark Theunissen commentedOk great, could you be more specific about what didn't work? I can always fix any problems if you point them out.
Comment #25
indytechcook commentedMark,
I committed the changes to 6--2 branch. Here is what I changed:
Added "hook_taxonomy_menu_handlers" See the bottom of the file. Basically a new hook to set the path.
Line 99. Made the default a string. It was having issues comparing to the string on line 109.
Added an option to display the vocab item. I'm working on the code for this now.
Added a "administer taxonomy menu" perm.
TO DO:
1. Implement delete and update on hook_taxonomy
2. Add an option to select the type of path from the list of handlers
3. Implement the use displaying the vocab item
4. enable pathauto by creating new module to demonstrate the use of hook_taxonomy_menu_handlers
4. Implement the legacy path format.
I'm working on 2 - 5. Can you work on 1?
Comment #26
indytechcook commentedI made a commit today to change how the module interact with the database. (Totally untested). This will help prepare the module for transition to D7. Next I am going to slightly alter the _taxonomy_menu_add_item function provided by Mark to be a controller for all interaction with the menu items. If you can't tell, I'm trying to make this as OO as possible.
Mark, don't worry about the other updates I asked for until I get this done because they are all based upon it.
Comment #27
Mark Theunissen commentedOk no problem. I've taken a glance over the handler code and it looks sensible!
I'm a bit iffy about such a complicated database access system for such a simple table, but I haven't done any D7 work so not sure how this is going to help. How does it ease the transition? Is there a D6 -> D7 module upgrade guide available yet?
Comment #28
indytechcook commentedI dreamed about the database access system (yes, I dream in Drupal) and I think I agree with you. I come out of the enterprise application world and tend to make everything a little over complicated. I'm still getting used to writing small application and websites. This tends to be on the extreme overkill side, doesn't it?
While I am very proud of that work, I think for the good of the module, it should be removed (tear).
I do think that the menu interaction should be a controller though. Thoughts? I think less duplicate code is better.
Comment #29
Mark Theunissen commentedI agree on removing the DB abstraction... ;)
As for the controller, sounds fine, but I don't get 100% what you intend doing, so once there's a bit more done, if you want me to comment, will be glad to take a look.
Comment #30
indytechcook commentedOk. I committed some more changes.
Removed the DB abstract layer. I kept the database.inc file to help keep the functions accessing the db separate for readability.
menu_link_save has the ability to do adds and updates so I changed the add_item module to do either and placed a function in front to take the request and format it correctly.
I added the ability for other modules to hook into the creation of the menu_items. Not sure if there is much use for this but it seemed easy (if I did it correctly) since we already had a hook for the path.
Of course, none of the code is tested at all. I plan on doing that tomorrow night.
TO DOS:
1. enable pathauto by creating new module to demonstrate the use of hook_taxonomy_menu_handlers
2. Implement the legacy path format.
3. Rework the creation of the breadcrumb
We are getting closer!
Comment #31
Mark Theunissen commentedI see you fixed the following :
$old_menu = variable_get('taxonomy_menu_vocab_' . $vid, 0);
by changing it to
$old_menu = variable_get('taxonomy_menu_vocab_' . $vid, '0');
i.e. changing the int to a string... cool, that one was annoying me.
Comment #32
indytechcook commentedI committed the first working version. Please verify. Yeah!!!
Now comes the fun part of adding more features.
TO DOS:
1. enable pathauto by creating new module to demonstrate the use of hook_taxonomy_menu_handlers
2. Implement the legacy path format.
3. Rework the creation of the breadcrumb
4. Ability to turn off description
Comment #33
Mark Theunissen commentedvariable_set('menu_rebuild_needed', TRUE); vs. menu_cache_clear($menu_name);
I see they're currently both used but in different places?
to line 475 of taxonomy_menu.module.
Otherwise things appear to be working! Thanks for all the work!
Comment #34
Mark Theunissen commentedp.s. will provide a patch once the source is up to date
Comment #35
indytechcook commentedvariable_set('menu_rebuild_needed', TRUE); vs. menu_cache_clear($menu_name);
Menu_cache_clear clear the cache for only that menu. Saves on processing time. I'll be sure to make the changes everywhere.
Has you can tell I still has some cleanup to do. Thanks a lot for this. I worked several hours on it last night and made some changes that weren't ready to commit yet. I added the delete stuff and changed the processing a little. I added hooks for insert, editing and deleting. This will turn the main module into an API and require other modules to do the actual work. I used your function as the main function to interact with menu_save. I really like it and it works great with the API. It's a little hard to explain. I'll have something committed tonight.
Sorry to make so many changes but I find a better idea then roll with it before I realize what I did. I will also provide a patch between the current commit and my version if that helps.
Thanks again for you help Mark.
Comment #36
Mark Theunissen commentedOk, so does that mean my patch for bringing the module up to Drupal's coding standards #373376: Drupal coding standards review is out of date now?
Do we really need such a complex API system for such a simple function? Surely if people want to hook into the Taxonomy menu change processing, they can just use core's hook_taxonomy().
Comment #37
indytechcook commentedI've very close to be able to release a dev. I'm taking a half day of work today (work form home in the afternoon) to go home and finish it up.
The API will allow plugins to be shared with the community without creating more contributed modules. Most people will be fine with the default usage, but this module does get enough requests for customization that I think this is needed.
I do again apologize for committing before I was ready. It will be run through coder before released also.
Comment #38
Mark Theunissen commentedOk sure thing.
Just wanted to note that there are few modules that make taxonomy breadcrumbs, so perhaps it's better to specialise in doing Taxonomy Menu and leave the breadcrumbs to other specialised modules.
Specifically this one looks good (untested):
http://drupal.org/project/taxonomy_breadcrumb
Comment #39
indytechcook commentedI agree on your breadcrumb. Initial version is committed. A DEV package will be created overnight. See taxonomy_menu_default folder for an example plugin.
I had one issue that I could not figure out yet. When an menu link is created for a vocabulary, I didn't know path to use. I tried just using '#' but when the menu cache was cleared or rebuilt, it remove the entry from the DB. This may have to be part of hook_menu and allow a custom page like the old module.
TO DO:
Fix link for vocabulary item.
Create a plugin for legacy path support
PathAuto Plugin
Write Development Documentation
Comment #40
Afief commentedindytechcook,
about the vocabulary item, the census back in the old days was that it should display taxonomy/term/1+2+3+4(all terms inside the vocabulary) by default. Other modules can of course (ab)use it however they want.
Great work you guys have been doing. because of you Taxonomy Menu is starting to shine, if I meet either of you the beer is on me.
Comment #41
indytechcook commentedThanks Afief. I'll add that to taxonomy_menu_default.module.
Comment #42
indytechcook commentedUpdates to the vocabulary have been made.
Now that version 6.x-2.x-dev is a package, I am going to set this issue to fixed. Please log all new issues against the new version for easier tracking.
Comment #43
indytechcook commented