When i set the option Hide empty terms the complete menu for the vocabulary disappears.

My situation:
I have a vocabulary with three levels of terms. The terms on level 1 and 2 do not have nodes attached, their only purpose is to create a menu hierarchy. The level 3 terms are have a node attached.

Vocabulary:
---level-1................ (0 nodes)......<- Gets excluded
------level-2............ (0 nodes)
---------level-31...... (1 node)
---------level-32...... (1 node)
---------level-33...... (1 node)

The problem is that taxonomy_menu looks only one level deep to decide if a menu item must be excluded or not.
In this case term level-1 gets excluded because term level-2 has no nodes.

Solution: Recursively check all levels.

This problem also exist in earlier versions.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hovel’s picture

FileSize
490 bytes

The following patch solves this problem.

hovel’s picture

FileSize
494 bytes

The previous patch was incorrect.
The following one is the correct one.

hovel’s picture

Version: 6.x-2.4-rc2 » 6.x-2.5
FileSize
458 bytes

This problem still exist in version 2.5
Here is a patch for taxonomy_menu-6x.2.5:

indytechcook’s picture

Oppsss. Missed that one.

indytechcook’s picture

Hey hovel. The patch fails to apply. It was rolled against your root instead of the module root. It's now matter, appears to just be one line.

Is this the correct version of _taxonomy_menu_children_has_nodes that works for you?

/**
 * Helper function to see if any of the children have any nodes
 *
 * @param $tid
 * @param $vid
 * @return boolean
 */
function _taxonomy_menu_children_has_nodes($tid, $vid) {
  $children = taxonomy_get_children($tid, $vid);
  foreach ($children as $tid => $term) {
    if (_taxonomy_menu_term_count($tid) > 0) {
      return FALSE;
    }
    return _taxonomy_menu_children_has_nodes($tid, $vid);
  }
  return TRUE;
}
hovel’s picture

Sorry for that. I am not that familiar with patching.

Yes that is the correct version _taxonomy_menu_children_has_nodes that works for me?

Regards,
Hovel

Summit’s picture

Hi, Is this code committed please?
greetings. Martijn

hovel’s picture

The code is not yet committed!
The problem is still present in versions 2.7 and 2.x-dev.

Can it be implemented please.

Attached the patch rebuild for version 2.7

hovel’s picture

Version: 6.x-2.5 » 6.x-2.8

Version 2.8 is now released but the problem is still not corrected!

indytechcook’s picture

Status: Active » Fixed

Status: Fixed » Closed (fixed)

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

miguel_angel’s picture

Version: 6.x-2.8 » 6.x-2.9
Status: Closed (fixed) » Active
FileSize
1.56 KB

Some things about _taxonomy_menu_children_has_nodes(...) function:

Due to its name, this function would return TRUE if some child has nodes and FALSE if none of the children have nodes.
So the call at line 798:

    //if hide menu is selected and the term count is 0 and the term has no children then do not create the menu item
    if ($num == 0 &&
        variable_get('taxonomy_menu_hide_empty_terms_'. $item['vid'], FALSE) &&
        _taxonomy_menu_children_has_nodes($item['tid'], $item['vid'])) {

        $item['remove'] = TRUE;
        return $item;
    }

and at line 998:

          _taxonomy_menu_children_has_nodes($t['tid'], $t['vid'])

must be negated:

          !_taxonomy_menu_children_has_nodes($t['tid'], $t['vid'])

On the other hand, the original function:

/**
 * Helper function to see if any of the children have any nodes
 *
 * @param $tid
 * @param $vid
 * @return boolean
 */
function _taxonomy_menu_children_has_nodes($tid, $vid) {
  $children = taxonomy_get_children($tid, $vid);
  foreach ($children as $tid => $term) {
    if (_taxonomy_menu_term_count($tid) > 0) {
      return _taxonomy_menu_children_has_nodes($tid, $vid);
    }
  }
  return TRUE;
}

ALWAYS returns TRUE.

The function must take into account in recursive calls if it has found a child with nodes and preserves that information.
The patch I suggest do that:

function _taxonomy_menu_children_has_nodes($tid, $vid, $has_nodes = FALSE) { // I assume there's no children with nodes at first
  $children = taxonomy_get_children($tid, $vid);
  foreach ($children as $tid => $term) {
    if (_taxonomy_menu_term_count($tid) > 0) {
      return _taxonomy_menu_children_has_nodes($tid, $vid, TRUE);  // If one child with nodes exists, consecutive calls preserve that condition
    }
  }
  return $has_nodes;  // So, the final returned value will be TRUE or FALSE as expected.
}

I hope it helps.

P.S.: The bug is present at 6.x-2.9 and 6.x-2.x-dev

miguel_angel’s picture

The same patch improved for performance.

function _taxonomy_menu_children_has_nodes($tid, $vid, $has_nodes = FALSE) {
  if (!$has_nodes) {  // If a child with nodes has just been found there's no need to look deeper
    $children = taxonomy_get_children($tid, $vid);
    foreach ($children as $tid => $term) {
      if (_taxonomy_menu_term_count($tid) > 0) {
        return _taxonomy_menu_children_has_nodes($tid, $vid, TRUE);
      }
    }
  }
  return $has_nodes;
}
indytechcook’s picture

Status: Active » Fixed
puckie’s picture

Status: Fixed » Needs review

I have just made the patch, but it does not work:

Now all taxonomy terms are shown in the menu which have no items , but the terms having items are not shown.

I have tried to insert some "!".signs before the call to "_taxonomy_menu_children_has_nodes", but not the right effect.

Perhaps my tests were not perfect because I wasn't able to take the cvs-version, instead I changed the file "taxonomy_menu.module" as said in this thread.

miguel_angel’s picture

Hi, puckie.

The patch was committed past aug-26. You can download the 6.x-2.x-dev from the project page.

Cheers.

puckie’s picture

Now it works !

I get the error "warning: html_entity_decode() expects parameter 2 to be long, string given in /usr/share/drupal6/sites/all/modules/taxonomy_menu/taxonomy_menu.module on line 577." ca. 10 times.

But this can be because of my failed attemps before.

interestingaftermath’s picture

I'm receiving the same error as #17 and I have yet to get my menus to show up. (Disclaimer: I just installed and am working through the configuration)

miguel_angel’s picture

The issue at #17 is a little bug in the constant name "ENT_COMPAT".

The lines:
576: strip_tags(html_entity_decode($item['description'], ENT_COMPACT, "UTF-8")) :
577: strip_tags(html_entity_decode($item['name'], ENT_COMPACT, "UTF-8"));

Must say:
576: strip_tags(html_entity_decode($item['description'], ENT_COMPAT, "UTF-8")) :
577: strip_tags(html_entity_decode($item['name'], ENT_COMPAT, "UTF-8"));

See: http://es.php.net/manual/en/function.html-entity-decode.php

jvieille’s picture

Confirm that #19 solves #17
Thanks!

13rac1’s picture

Version: 6.x-2.9 » 6.x-2.x-dev
FileSize
703 bytes

Patch attached for ENT_COMPAT problem.

indytechcook’s picture

Status: Needs review » Reviewed & tested by the community

Sorry about the mis apply of the patch. The change has been committed. http://drupal.org/cvs?commit=420310

Setting to RTBC for the node count patch. This will be in the next release

mcaden’s picture

I'm using 6.x-2.x-dev that said the last change from from Sept 13th...I still see this bug.

EDIT: I don't see any bugs in the code. It looks right to me. I ran update.php and cleared cache. I am using the taxonomy_menu_path_ubercart submodule so that might have something to do with this. I'm still looking over the code but I'm not seeing the problem quite yet.

dstol’s picture

Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

@mcaden Any follow up on taxonomy_menu_path_ubercart?

mcaden’s picture

I ended up not using catalog and I styled my views to look just like the catalog. Since I switched everything back to the default view it all seems to be working fine.

skylord’s picture

I can confirm last dev works ok with taxonomy_menu_path_ubercart module.

mcaden’s picture

Hmm...I ran into the bug again.

EDIT: Installed from dev (Oct 20th). Still no dice. One of my parent menu items don't show up even though it's children's children's children have nodes.

mcaden’s picture

Priority: Normal » Major
Status: Postponed (maintainer needs more info) » Needs work
dstol’s picture

I'll take a look at this as soon as I've got some time, although patches are always welcome. :)

mcaden’s picture

Whoa there,

So at a glance, the code seemed right to me. But I kept having the problem. I posted here while I kept working on the other stuff I had to do. Everybody else seemed as busy as I was. Now I'm at the point where this is just about the only problem on my site so I sat down and actually looked at this code.

Unless I'm misunderstanding something, it's WAY wrong. Nowhere in this function is it checking to see if a term has nodes. It's only looking for child terms and returning false if there's no child terms.

I've rewritten the function. It returns true if itself, or any child has nodes. It only returns false if it doesn't have any nodes, and if none of its child terms has any nodes.

/**
 * Helper function to see if any of the children have any nodes
 *
 * @param $tid
 * @param $vid
 */
function _taxonomy_menu_children_has_nodes($tid, $vid) {
  $has_nodes = taxonomy_term_count_nodes($tid);
  if($has_nodes > 0)
  {
    return true;
  }
  else
  {
    $children = taxonomy_get_children($tid, $vid);
    foreach ($children as $child_tid => $term) {
      if (_taxonomy_menu_term_count($child_tid) > 0) {
        $child_has_nodes = _taxonomy_menu_children_has_nodes($child_tid, $vid);
        if($child_has_nodes)
        {
          return true;
        }
      }
    } 
  }
  
  return false;
}
dstol’s picture

Wow, mcaden awesome turn around and good find. Just to make my life easier could you submit your changes as a patch (http://drupal.org/patch/create) from the 2.x-dev branch

mcaden’s picture

I'll get CVS installed.

CVS as opposed to SVN or Git is pretty much the reason I don't submit more patches ;)

dstol’s picture

Actually, you can clone the repo using git.

http://git.drupalcode.org/project/taxonomy_menu.git

indytechcook’s picture

So "_taxonomy_menu_children_has_nodes" is just for the child nodes as the name implies.

If you look at _taxonomy_menu_item(), where the number of terms is added, you can see that the _taxonomy_menu_term_count($item['tid']); is called to get the number for that term. They are calculated separately because of the options to "display descendants".

If the "display descendants" is selected, then the number of of nodes is changed to include the children.

The probably is in this if statement:

    if ($num == 0 &&
        variable_get('taxonomy_menu_hide_empty_terms_'. $item['vid'], FALSE) &&
        !_taxonomy_menu_children_has_nodes($item['tid'], $item['vid'])) {

        $item['remove'] = TRUE;
        return $item;
    }

It doesn't respect the "display descendants" setting to hide the term. So the bug in in _taxonomy_menu_item and not _taxonomy_menu_children_has_nodes. Looking at this again, I have no idea what I was thinking when I wrote it. It seems very inefficient to me :)

Here is a dirty fix to that if statement. (Yes, I know you want a patch)

    if (variable_get('taxonomy_menu_hide_empty_terms_'. $item['vid'], FALSE) {
        if (variable_get('taxonomy_menu_display_descendants_'. $item['vid'], FALSE) &&
          ($num == 0 && !_taxonomy_menu_children_has_nodes($item['tid'], $item['vid'])) {

          $item['remove'] = TRUE;
        }
        elseif ($num == 0) {
          $item['remove'] = TRUE;
        }

        return $item;
    }

Cheers,
Neil

indytechcook’s picture

BTW, the change was in _taxonomy_menu_item()

mcaden’s picture

I'm confused with your thought process here:

_taxonomy_menu_term_count gets the number of terms, so you're hiding terms that don't have child terms?

And before my changes, _taxonomy_menu_children_has_nodes never even looked at the nodes, it too only looked for child terms...

mcaden’s picture

Status: Needs review » Needs work

CVS hates me: cvs [checkout aborted]: unrecognized auth response from cvs.drupal.org: cvs [pserver aborted]: /cvs/drupal-contrib checkout contributions/modules/taxonomy_menu: no such repository

Git hates me: warning: remote HEAD refers to nonexistent ref, unable to checkout.

EDIT: got CVS working, connected at the top level directory of drupal-contrib and navigated the repo through tortoisecvs

mcaden’s picture

Ok Nvm, _taxonomy_menu_term_count probably needs to be renamed, I see it gets nodes, not terms. In either case I don't see why not to use taxonomy_term_count_nodes($tid)

I do see what you mean about doing display descendents separately, however. I have display descendents off and modified my taxonomy view to include depth. That way when I click "Sports" on my taxonomy menu I don't get a page that has:

Baseball + Basketball + Football + Rugby + Tennis + ...
(populated with all the above)

Instead I get

Sports
(populated with all the above)

Going this route I need it to take descendents into account for hide terms. I think that's why it's been busted for me even though it has worked for others. So yeah, my patch works great, it's just always checks to see if its children has nodes whether you have 'display descendents' or not.

mcaden’s picture

FileSize
1.56 KB

Here's my changes as a patch. I was thinking about it more and display descendants affects how the link is formed, not how the menu structure is formed. It seems to me you'd want it to ignore display descendants. The description on "Display Descendants" only talks about how the taxonomy link is formed (tid+tid+tid). Whether you have "display descendants" clicked or not, it still generates the full tree. I'm not sure, however, if it affects the display node count, but it doesn't look like this is called for display node count anyway as it's only a boolean function and not returning a count.

mcaden’s picture

Status: Needs work » Needs review
FileSize
1.56 KB

Naming patch appropriately

tadfisher’s picture

Status: Needs work » Needs review

Here's a shorter version of the function that avoids the unnecessary comparisons, flag variables, and control nesting.


/**
 * Helper function to see if any of the children have any nodes
 *
 * @param $tid
 * @param $vid
 * @return boolean
 */
function _taxonomy_menu_children_has_nodes($tid, $vid) {
  if (taxonomy_term_count_nodes($tid))
    return true;
  $children = taxonomy_get_children($tid, $vid);
  foreach ($children as $child_tid => $term) {
    if (_taxonomy_menu_term_count($child_tid)) {
      return _taxonomy_menu_children_has_nodes($child_tid, $vid);
    }
  }
  return false;
}

dstol’s picture

@mcaden, give this one a test would you please?

miguel_angel’s picture

What about this?:

function _taxonomy_menu_children_has_nodes($tid) {
  // let taxonomy module count the nodes
  return (taxonomy_term_count_nodes($tid) > 0) ? true : false;
}
hovel’s picture

Since the issue was raised a lot of effort was spend and suggestions made none of them propperly worked in all situations, including mine. miguel_angel just beat me to it. His solution definitely works. You can take it even further and remove function _taxonomy_menu_children_has_nodes altogether.
Line 796 Replace:

    if ($num == 0 &&
        variable_get('taxonomy_menu_hide_empty_terms_'. $item['vid'], FALSE) &&
        !_taxonomy_menu_children_has_nodes($item['tid'], $item['vid'])) {

With:

    if ($num == 0 &&
        variable_get('taxonomy_menu_hide_empty_terms_'. $item['vid'], FALSE) &&
        !((taxonomy_term_count_nodes($item['tid']) > 0) ? true : false)) {

The same function is used a bit further in the code to get the number of descendants. You can improve the code more by making sure this function is called only ones.

With respect to comment [#34] display descendants,i do not understand what is the purpose of it. Display descendants has nothing to do with Hide empty terms.

Regards,
Hovel

hovel’s picture

Disadvantage of using taxonomy_term_count_nodes() is performance. This function scans the entire tree with $tid as root. Maybe this is why _taxonomy_menu_children_has_nodes() was create in the first place. Here after is a version of _taxonomy_menu_children_has_nodes() which does work (tested today) and which stops at the first node found linked to a term in the tree.

function _taxonomy_menu_children_has_nodes($tid, $vid, $has_nodes = FALSE) {
	$children = taxonomy_get_children($tid, $vid);
	foreach ($children as $tid => $term) {
		if (_taxonomy_menu_term_count($tid) > 0) {
			// One or more nodes linked to this term
			$has_nodes = True;
		}
		else {
			// No nodes linked to this term. Go check its children
			$has_nodes = _taxonomy_menu_children_has_nodes($tid, $vid);
		}
		if ($has_nodes) {
			// Nodes found, break the loop.
			Break;
		}
	}
  return $has_nodes;
}

Or the same shorter:

function _taxonomy_menu_children_has_nodes($tid, $vid, $has_nodes = FALSE) {
	$children = taxonomy_get_children($tid, $vid);
	foreach ($children as $tid => $term) {
		if ($has_nodes = (_taxonomy_menu_term_count($tid) > 0) ? True : _taxonomy_menu_children_has_nodes($tid, $vid)) {
			Break;
		}
	}
  return $has_nodes;
}
miguel_angel’s picture

Hi, hovel!

I can't talk about performance because I didn't benchmarked the query taxonomy_term_count_nodes() does. But you're right, there's no need to go through the entire tree.

I gave a solution to this at #13

Cheers.

Michael Zetterberg fd. Lopez’s picture

None of these seem to work for me very well. I did some digging and found a use case when updating a node. Just edit a node and click save. If it has taxonomy terms associated with it hook_taxonomy_menu_* should fire. But it only fires on the terms you have saved on the node, it does not fire for the parents of the terms. This of course means that the parents will stay hidden and not get updated.

I think taxonomy_menu_handler should recursively go upwards and run hook_taxonomy_menu_* on all parents too. Otherwise their visibility won't change when saving a term on a deeper hierarchy level than 1. Although I do not know if the hook is correct for the parents, but something along those lines.

hovel's fix only works for 1 level of hierarchy. The parent items do not get updated when taxonomy_menu_handler runs (although I do not know if that is the best place for it).

miguel_angel’s picture

Trying to solve the lack for support for a term with multiple parents, I wrote a patch that takes into account the update upwards in the tree.

You can try it and see...

hovel’s picture

Hi,
Miguel_angel,
Your solution in #13 goes only one level deep. My solution #45 goes down the entire tree to find any child term with a node attached.

Michael Lopez,
You state "hovel's fix only works for 1 level of hierarchy". Coul you be mistaken?
The purpose of my solution is just to go recursively down the entire tree! It works for me.
Why shoud the taxonomy_menu_handler go upwards? This function (_taxonomy_menu_children_has_nodes) goes recursively down!

My #45 is a reaction/improvement on earlier comments in this discussion. I use the following in my sites:

function _taxonomy_menu_children_has_nodes($tid, $vid) {
  $children = taxonomy_get_children($tid, $vid);
  foreach ($children as $tid => $term) {
    if (_taxonomy_menu_term_count($tid) > 0) {
      return FALSE;
    }
    return _taxonomy_menu_children_has_nodes($tid, $vid);
  }
  return TRUE;
}

Regards,
hovel

miguel_angel’s picture

Hi, hovel.

If you analyse my code at #13 and your code at #45 you'll realize that they do the same thing:
They are seeking downwards in the tree (from an initial tid) until finding a term with one or more nodes attached.

Michael Lopez talks about another problem: the upwards update.
If you attach a node to a term whose parent term is hidden, the parent is not updated and made visible. So the need of upwards updates.

Cheers.

Michael Zetterberg fd. Lopez’s picture

hovel,
as miguel_angel explains, my client selects a child term only, an no parents. So if the parent is hidden, selecting one of its children does not update the parent, keeping it hidden.

hovel’s picture

Hi Miguel,

I am sorry but your code does not work in the situation for which i raised this issue.

Example Vocabulary:
---level-1................ (0 nodes)......<- Gets excluded
------level-2............ (0 nodes)
---------level-31...... (1 node)
---------level-32...... (1 node)
---------level-33...... (1 node)

See the comment in your code.
level-1 has no nodes so function "_taxonomy_menu_children_has_nodes" gets called.

<?php
function _taxonomy_menu_children_has_nodes($tid, $vid, $has_nodes = FALSE) {
  if (!$has_nodes) {  // $has_nodes = FALSE so enter this if
    $children = taxonomy_get_children($tid, $vid); // There is one child -> term level-2
    foreach ($children as $tid => $term) {
      if (_taxonomy_menu_term_count($tid) > 0) { // term level-2 does not have nodes: SKIP this if
        return _taxonomy_menu_children_has_nodes($tid, $vid, TRUE); // Not executed !!! 
      }
    }
  }
  return $has_nodes; // Return FALSE.
}
?>

Even though there are nodes on terms level-31,32 and 33 the function above does not see this and term level-1 will be hidden.
The code above never goes more than one level deep !!!

Unfortunately this code is (still) in taxonomy_menu-6.x-2.x-dev.

Regards,
hovel

hovel’s picture

Michael,
You are absolutely right. My solution is only a part of the problem. It is affective only when switchin the option "Hide empty terms" on.
The full solution will be more complicated. It is possible to do an upwards update when an item changes from hidden to not hidden. This is not possible when an item changes from not hidden to hidden. The status of a parent may depend on the status of other children.

One posibility is to process the full menu-tree for each change (Similar to switching on "Hide empty terms" when updating the vocabulary).

An other posibility is to keep track of the number of nodes attached to each term. Where this number is the sum of nodes attached to the term and those attached to all children. An item can be hidden when this number is 0. The upwards update would then consist of the adding or subtracting one from this number of each parent.

dstol’s picture

Status: Needs review » Needs work
maximpodorov’s picture

It would be great to be able to delegate the calculation of count of nodes associated with the term to separate modules (by means of hooks). My case is Ubercart store with some out-of-stock products which must be excluded from taxonomy term pages, and empty (in this sense) terms must be excluded from taxonomy menus.

mckinleymedia’s picture

Title: Error in hide empty terms » Yes! It works!

This is awesome! Thank you, thank you, thank you!

nevets’s picture

Title: Yes! It works! » Error in hide empty terms

Please do not change issue titles.

vrivera82’s picture

mckinleymedia

What did yo do?

Jenechka’s picture

FileSize
1.43 KB

Here is the patch from me, that solves the problem.

Jenechka’s picture

Status: Needs work » Patch (to be ported)
dstol’s picture

Status: Patch (to be ported) » Needs review

Updating status

Status: Needs review » Needs work

The last submitted patch, taxonomy_menu.module.patch, failed testing.

johnv’s picture

Title: Error in hide empty terms » 'Hide taxonomy terms' hides empty parents of child terms that do have nodes (II)

more explicite title.
this issue might be a duplicate of #1333246: 'Hide taxonomy terms' hides empty parents of child terms that do have nodes

Jenechka’s picture

Status: Needs work » Needs review
FileSize
2.09 KB

Next attempt with the patch.

Status: Needs review » Needs work

The last submitted patch, taxonomy_menu.hide_empty_terms.patch, failed testing.

Jenechka’s picture

Version: 6.x-2.x-dev » 6.x-2.9
Status: Needs work » Needs review
Jenechka’s picture

Enemy’s picture

Enemy’s picture

work! thanks!

skylord’s picture

Patch for latest dev - if anyone is using this module on legacy sites. :-)

Status: Needs review » Needs work

The last submitted patch, 70: taxonomy_menu.hide_empty_terms-689318-70-d6.patch, failed testing.

dstol’s picture

Status: Needs work » Closed (outdated)

The Drupal 6 version is no longer supported