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.

Comments

chasz’s picture

ouch !!! LOL

pwolanin’s picture

hmm, guess I should have checked if you volunteered for "Drupal Tough Love" before posting ;-)

pwolanin’s picture

Title: really unfortunate mis-use of D6 menu system » mis-use of D6 menu system
derhasi’s picture

Status: Active » Needs work

I 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 ;)

function taxonomy_menu_create_menu_items($vid){
	$menu_name = variable_get("taxonomy_menu_menu_name_$vid",'navigation');//the menu the links shall be attached to
	$voc_item = variable_get("taxonomy_menu_voc_item_$vid",FALSE);//Shall Vocabulary have its own item?
	$mlids = variable_get("taxonomy_menu_mlids_$vid",array());//The current Mlids for this vocabulary
	$newmlids = array();
	$path_op = variable_set('taxonomy_menu_path_handler_'. $vid,'default');
	
	$parents = array();
	$curdepth = -1;
	$mlid = 0;
	if ($voc_item){
		$link_path = taxonomy_menu_path($vid,$path_op);
		$mlid_this = array_search('voc',$mlids); 
		//get vocabulary data
		$mlid = 0; // here will come menu_link_save()
		$parents = array($mlid);
	}
	$tree = taxonomy_get_tree($vid);
	$plid = 0;
	foreach ($tree as $i=>$term){
		// test if there's allready a menu item for this Term
		$curmlid = array_search($term->tid,$mlids);
		$item = array();
		//get plid
		if ($term->depth > $curdepth){
			$plid = $mlid;
			array_push($parents,$plid);
		} elseif($term->depth < $curdepth) {
			$plid = array_pop($parents);
		}
		
		$curdepth = $term->depth;
		if (!$curmlid){	
			$item['mlid']=0; // New menu item!
			$item['plid'] = $plid;
			$item['link_path'] = taxonomy_menu_path($term,$path_op);
			$item['link_title'] = $term->name;
			$item['menu_name'] = $menu_name;
			
		} else {
			$item['mlid'] = $curmlid;
			$item['link_path'] = taxonomy_menu_path($term,$path_op);
			$item['link_title'] = $term->name;
			unset($mlids[$curmlid]);
		}
		$item['module'] = 'taxonomy_menu';
		$item['router_path'] = "taxonomy_menu/$vid";

		$mlid = menu_link_save($item);
		$newmlids[$mlid] = $term->tid;
	}
	//delete rest of mlids - non-existing term-ids
	foreach ($mlids as $mlid=>$tid){
		menu_link_delete($mlid);
	}
	//newmlids speichern
	variable_set("taxonomy_menu_mlids_$vid",$newmlids); // have to be saved for later deletion or comparison
}
Afief’s picture

Sweet:-) 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:

  • mlids shouldn't be used with variable_get and variable_set as some menus get rather huge(I've seen menus with over 200 terms, serializing and unserializing large data sets is very unhealthy)
  • you need to take into account dropping multiple menu levels in this case:
    ** term1
    ** ** term2
    ** ** ** term3
    ** term4
    

    Perhaps a recursive helper function would help?

  • the function taxonomy_menu_path would have to know if you're passing a vocabulary id or term id, or am I missing something?

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

Mark Theunissen’s picture

Subscribing. Hopefully I can get involved too at creating the patch, because we may need this for an upcoming project. Thanks.

ckidow’s picture

Subscribing... very very important

ppmax’s picture

I 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

function pptaxonomy_menu_admin_form() {
    $form = array();
    $fields = array('term', 'path');
//this is to remove the 0 element in the arrays
    foreach ($fields as $field) {
      $form['items'][$field][0] = NULL;
    }
    $taxonomylist = array(0 => NULL);
    $taxonomyvidlist = array(0 => NULL);

    foreach (taxonomy_get_vocabularies() as $vocab) {
        $taxonomyvidlist[] = $vocab->vid;
        $taxonomylist[] = $vocab->name;
//this is to make sure the arrays are indexed identically
        ksort($taxonomylist);
        ksort($taxonomyvidlist);

        $form['items']['term'][] = array(
            '#type' => 'item',
            '#value' => $vocab->name,
        );
        $form['items']['path'][] = array(
            '#type'           => 'textfield',
            '#title'          => t('Base path'),
            '#default_value'  => '',
            '#description'    => t('Enter the base path for each item in the menu'),
        );
     }

// Remove our array [0] elements.
    unset($taxonomylist[0]);
    unset($taxonomyvidlist[0]);
    foreach ($fields as $field) {
      $form['items'][$field][0] = NULL;
    }

//collapse fields into trees
    $form['items']['term']['#tree'] = TRUE;
    $form['items']['path']['#tree'] = TRUE;
    
//new form item with all taxonomy vid values
    $form['vid_list'] = array(
        '#type' => 'value',
        '#value' => $taxonomyvidlist,
    );
//new form item with all taxonomy name values
    $form['term_list'] = array(
        '#type' => 'value',
        '#value' => $taxonomylist,
    );
//new form item for all items. prolly should do this for vid instead
    $form['taxonomy_list'] = array(
        '#type' => 'checkboxes',
        '#options' => $taxonomylist,
    );

    $form['submit'] = array(
        '#type'           => 'submit',
        '#value'          => t('Save configuration'),
    );
    return $form;
}

Form theme. Basically make a table of all related elements...I love this technique

function theme_pptaxonomy_menu_admin_form($form) {
    $output = '';
    if(isset($form['taxonomy_list']) && $form['taxonomy_list']['#type'] == 'checkboxes') {
        $header = array(theme('table_select_header_cell'), t('Taxonomy Term'), t('Path'));
        $rows = array();
        foreach (element_children($form['taxonomy_list']) as $key) {
            unset($form['taxonomy_list'][$key]['#title']);
            $rows[] = array(
                drupal_render($form['taxonomy_list'][$key]), 
                drupal_render($form['items']['term'][$key]),
                drupal_render($form['items']['path'][$key]),
            );
        }
        $output .= theme('table', $header, $rows);
    }
    return $output . drupal_render($form);
}

Form submit

function pptaxonomy_menu_admin_form_submit($form, &$form_state) {
    $vids = array();
    $terms = array();
    $paths = array();
    $menus = array();

      foreach(array_filter($form_state['values']['taxonomy_list']) as $index) {
        $vids[] = $form_state['values']['vid_list'][$index];
        $terms[] = $form_state['values']['term_list'][$index];
        if(!empty($form_state['values']['path'][$index])) {
            $paths[] = $form_state['values']['path'][$index];
        }
        
        $menu_item = array(
            'link_title' => check_plain($form_state['values']['term_list'][$index]),
            'link_path' => $form_state['values']['path'][$index],
            'menu_name' => 'primary-links',
        );
        menu_link_save(&$menu_item);
        $tree = taxonomy_get_tree($form_state['values']['vid_list'][$index]);
            if(!empty($tree)) {
                foreach ($tree as $term) {
                    $submenu_item = array(
                    'link_title' => check_plain($term->name),
                    'link_path' => $form_state['values']['path'][$index] .'/'. $term->name,
                    'menu_name' => 'primary-links',
                    );
                    menu_link_save(&$submenu_item);
            }
        }
    }

// report back on our progress and debugging
    if (!empty($vids)) {
      drupal_set_message(t('Vids: ') . theme('item_list', $vids));
      drupal_set_message(t('Terms: ') . theme('item_list', $terms));
      drupal_set_message(t('Paths: ') . theme('item_list', $paths));
      drupal_set_message(t('Menus: ') . theme('item_list', $menu_item));
    }
    else {
      drupal_set_message(t('No terms were selected.'));
    }
//used for making sure the form submits all the values I need
    // print '<pre>';
    // print_r($form_state['values']);
    // print'</pre>';
    // exit;
}
indytechcook’s picture

Here is what I have so far. I added a schema for a table taxonomy_menu.

New taxonomy_menu.install

function taxonomy_menu_uninstall() {
  // Delete variables
  foreach (taxonomy_get_vocabularies() as $vocabulary) {
    variable_del('taxonomy_menu_show_'. $vocabulary->vid);
    variable_del('taxonomy_menu_show_view_'. $vocabulary->vid);
  }
  variable_del('taxonomy_menu_display_num');
  variable_del('taxonomy_menu_hide_empty');
  variable_del('taxonomy_menu_display_page');
  variable_del('taxonomy_menu_display_descendants');

  drupal_uninstall_schema('taxonomy_menu');
}

function taxonomy_menu_install() {
  drupal_install_schema('taxonomy_menu');
}

function taxonomy_menu_schema() {
  $schema['taxonomy_menu'] = array(
    'description' => t('Links a taxonomy term to a menu item'),
    'fields' => array(
      'mlid' => array(
        'type' => 'int',
        'unsigned' => TRUE,
        'not null' => TRUE,
        'default' => 0,
        'description' => t('The taxonomy terms {menu_link}.mlid'),
      ),
      'tid' => array(
        'type' => 'int',
        'unsigned' => TRUE,
        'not null' => TRUE,
        'default' => 0,
        'description' => t('tid that is linked to the mlid'),
      ),
      'vid' => array(
        'type' => 'int',
        'unsigned' => TRUE,
        'not null' => TRUE,
        'default' => 0,
        'description' => t('vid for the tid'),
      ),
    ),
    'primary key' => array('mlid','tid'),
    'indexes' => array(
      'vid' => array('vid'),
    ),
  );
}

Changes to taxonomy_menu_create_menu_items from derhasi

//**
 *
 * @param <type> $vid
 */
function _taxonomy_menu_create_menu_items($vid){
    $menu_name = variable_get("taxonomy_menu_menu_name_$vid",'navigation');//the menu the links shall be attached to
    $voc_item = variable_get("taxonomy_menu_voc_item_$vid",FALSE);//Shall Vocabulary have its own item?
    $mlids =  _taxonomy_menu_get_mlid($vid);//The current Mlids for this vocabulary
    $newmlids = array();
    $path_op = variable_set('taxonomy_menu_path_handler_'. $vid,'default');

    $parents = array();
    $curdepth = -1;
    $mlid = 0;
    if ($voc_item){
        $link_path = taxonomy_menu_path($vid,$path_op);
        $mlid_this = array_search('voc',$mlids);
        //get vocabulary data
        $mlid = 0; // here will come menu_link_save()
        $parents = array($mlid);
    }
    $tree = taxonomy_get_tree($vid);
    $plid = 0;
    foreach ($tree as $i=>$term){
        // test if there's allready a menu item for this Term
        $curmlid = array_search($term->tid,$mlids);
        $item = array();
        //get plid
        if ($term->depth > $curdepth){
            $plid = $mlid;
            array_push($parents,$plid);
        } elseif($term->depth < $curdepth) {
            $plid = array_pop($parents);
        }

        $curdepth = $term->depth;
        if (!$curmlid){
            $item['mlid']=0; // New menu item!
            $item['plid'] = $plid;
            $item['link_path'] = taxonomy_menu_path($term,$path_op);
            $item['link_title'] = $term->name;
            $item['menu_name'] = $menu_name;

        } else {
            $item['mlid'] = $curmlid;
            $item['link_path'] = taxonomy_menu_path($term,$path_op);
            $item['link_title'] = $term->name;
            unset($mlids[$curmlid]);
        }
        $item['module'] = 'taxonomy_menu';
        $item['router_path'] = "taxonomy_menu/$vid";

        $mlid = menu_link_save($item);
        $newmlids[$mlid] = $term->tid;
    }
    //delete rest of mlids - non-existing term-ids
    foreach ($mlids as $mlid=>$tid){
        menu_link_delete($mlid);
    }
    //newmlids speichern
    _taxonomy_menu_save_mlid($newmlids, $vid);
}

New functions to interact with the new table.

/**
 * function to reteive the menu items for a vocab
 * @param int $vid
 * @return array(mlid => tid)
 */
function _taxonomy_menu_get_mlid($vid) {
  $sql = 'SELECT mlid, tid FROM {taxonomy_menu} WHERE vid = %d';
  $result = db_query(db_rewrite_sql($sql), $vid);
  $output = array();
  while ($data = db_fetch_object($result)) {
    $output[$data->mlid] = $data->tid;
  }

  return $output;
}

/**
 * function to save the menu to term relation
 * @param array(mlid => tid) $menu_terms
 * @param int $vid
 */
function _taxonomy_menu_save_mlid($menu_terms, $vid) {
  //delete the records for the vid
  //then insert the new rows
  //this is safer then updating

  db_query('DELETE FROM {taxonomy_menu} WHERE vid = %d', $vid);

  if (!empty($menu_terms)) {
    foreach ($menu_terms as $mlid => $tid) {
      db_query('INSERT INTO {taxonomy_menu} (mlid, tid, vid) VALUES (%d, %d, %d)',
        $mlid, $tid, $vid);
    }
  }
}

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.

indytechcook’s picture

If 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

Mark Theunissen’s picture

Can those providing patches please do so using

http://drupal.org/patch/create

instead of dumping code... thanks!

indytechcook’s picture

Mark, 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.

Mark Theunissen’s picture

Hi 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?

Mark Theunissen’s picture

I will probably be done tomorrow, can I submit a patch? It will be against the 6.01 branch.

indytechcook’s picture

Mark, 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.

Mark Theunissen’s picture

Great, 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.

indytechcook’s picture

Sweet. 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.

Mark Theunissen’s picture

StatusFileSize
new27.44 KB

Right!

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!

Mark Theunissen’s picture

Actually number (4) above is kinda-working, but sometimes it grabs the wrong vocab.

indytechcook’s picture

This 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.

Mark Theunissen’s picture

Great news! I have done the replacement. Looking forward to moving on with this towards a stable release.

Mark Theunissen’s picture

Did 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.

indytechcook’s picture

No 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.

Mark Theunissen’s picture

Ok great, could you be more specific about what didn't work? I can always fix any problems if you point them out.

indytechcook’s picture

Mark,

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?

indytechcook’s picture

I 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.

Mark Theunissen’s picture

Ok 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?

indytechcook’s picture

I 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.

Mark Theunissen’s picture

I 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.

indytechcook’s picture

Ok. 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!

Mark Theunissen’s picture

I 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.

indytechcook’s picture

I 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

Mark Theunissen’s picture

  • What's the verdict on using:

    variable_set('menu_rebuild_needed', TRUE); vs. menu_cache_clear($menu_name);
    I see they're currently both used but in different places?

  • In taxonomy_menu_maintain(), $op='delete' I think you're forgetting to delete the entry from taxonomy_menu's table.
  • Any reason why $op='delete' is in an if statement instead of in the switch statement below it?
  • In my testing, putting the vocab in it's own item works nicely.
  • Found a nefarious little bug... the has_children property was not being set correctly by menu_link_save(), so when you dragged taxonomy items around, changing the parent-child relations, the has_children value was not set correctly in the database, which was evident in the leaf and expanded list item images displaying incorrectly. Anyway, we need to add this:
$children = taxonomy_get_children($item['tid']);
      if (!empty($children)) {
        $link['has_children'] = 1;
      }

to line 475 of taxonomy_menu.module.

Otherwise things appear to be working! Thanks for all the work!

Mark Theunissen’s picture

p.s. will provide a patch once the source is up to date

indytechcook’s picture

variable_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.

Mark Theunissen’s picture

Ok, 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().

indytechcook’s picture

I'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.

Mark Theunissen’s picture

Ok 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

indytechcook’s picture

I 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

Afief’s picture

indytechcook,

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.

indytechcook’s picture

Thanks Afief. I'll add that to taxonomy_menu_default.module.

indytechcook’s picture

Version: 6.x-1.x-dev » 6.x-2.x-dev

Updates 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.

indytechcook’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.