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.
Comment | File | Size | Author |
---|---|---|---|
#70 | taxonomy_menu.hide_empty_terms-689318-70-d6.patch | 1.61 KB | skylord |
#64 | taxonomy_menu.hide_empty_terms.patch | 2.09 KB | Jenechka |
#59 | taxonomy_menu.module.patch | 1.43 KB | Jenechka |
#42 | _taxonomy_menu_item-689318-41.patch | 1.22 KB | dstol |
#40 | taxonomy_menu.module.patch | 1.56 KB | mcaden |
Comments
Comment #1
hovel CreditAttribution: hovel commentedThe following patch solves this problem.
Comment #2
hovel CreditAttribution: hovel commentedThe previous patch was incorrect.
The following one is the correct one.
Comment #3
hovel CreditAttribution: hovel commentedThis problem still exist in version 2.5
Here is a patch for taxonomy_menu-6x.2.5:
Comment #4
indytechcook CreditAttribution: indytechcook commentedOppsss. Missed that one.
Comment #5
indytechcook CreditAttribution: indytechcook commentedHey 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?
Comment #6
hovel CreditAttribution: hovel commentedSorry 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
Comment #7
Summit CreditAttribution: Summit commentedHi, Is this code committed please?
greetings. Martijn
Comment #8
hovel CreditAttribution: hovel commentedThe 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
Comment #9
hovel CreditAttribution: hovel commentedVersion 2.8 is now released but the problem is still not corrected!
Comment #10
indytechcook CreditAttribution: indytechcook commentedAdded. http://drupal.org/cvs?commit=379810
Comment #12
miguel_angel CreditAttribution: miguel_angel commentedSome 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:
and at line 998:
must be negated:
On the other hand, the original function:
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:
I hope it helps.
P.S.: The bug is present at 6.x-2.9 and 6.x-2.x-dev
Comment #13
miguel_angel CreditAttribution: miguel_angel commentedThe same patch improved for performance.
Comment #14
indytechcook CreditAttribution: indytechcook commentedCommitted: http://drupal.org/cvs?commit=412330
Cheers
Comment #15
puckie CreditAttribution: puckie commentedI 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.
Comment #16
miguel_angel CreditAttribution: miguel_angel commentedHi, puckie.
The patch was committed past aug-26. You can download the 6.x-2.x-dev from the project page.
Cheers.
Comment #17
puckie CreditAttribution: puckie commentedNow 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.
Comment #18
interestingaftermath CreditAttribution: interestingaftermath commentedI'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)
Comment #19
miguel_angel CreditAttribution: miguel_angel commentedThe 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
Comment #20
jvieille CreditAttribution: jvieille commentedConfirm that #19 solves #17
Thanks!
Comment #21
13rac1 CreditAttribution: 13rac1 commentedPatch attached for ENT_COMPAT problem.
Comment #22
indytechcook CreditAttribution: indytechcook commentedSorry 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
Comment #23
mcaden CreditAttribution: mcaden commentedI'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.
Comment #24
dstol@mcaden Any follow up on taxonomy_menu_path_ubercart?
Comment #25
mcaden CreditAttribution: mcaden commentedI 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.
Comment #26
skylord CreditAttribution: skylord commentedI can confirm last dev works ok with taxonomy_menu_path_ubercart module.
Comment #27
mcaden CreditAttribution: mcaden commentedHmm...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.
Comment #28
mcaden CreditAttribution: mcaden commentedComment #29
dstolI'll take a look at this as soon as I've got some time, although patches are always welcome. :)
Comment #30
mcaden CreditAttribution: mcaden commentedWhoa 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.
Comment #31
dstolWow, 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
Comment #32
mcaden CreditAttribution: mcaden commentedI'll get CVS installed.
CVS as opposed to SVN or Git is pretty much the reason I don't submit more patches ;)
Comment #33
dstolActually, you can clone the repo using git.
http://git.drupalcode.org/project/taxonomy_menu.git
Comment #34
indytechcook CreditAttribution: indytechcook commentedSo "_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:
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)
Cheers,
Neil
Comment #35
indytechcook CreditAttribution: indytechcook commentedBTW, the change was in _taxonomy_menu_item()
Comment #36
mcaden CreditAttribution: mcaden commentedI'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...
Comment #37
mcaden CreditAttribution: mcaden commentedCVS 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
Comment #38
mcaden CreditAttribution: mcaden commentedOk 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.
Comment #39
mcaden CreditAttribution: mcaden commentedHere'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.
Comment #40
mcaden CreditAttribution: mcaden commentedNaming patch appropriately
Comment #41
tadfisher CreditAttribution: tadfisher commentedHere's a shorter version of the function that avoids the unnecessary comparisons, flag variables, and control nesting.
Comment #42
dstol@mcaden, give this one a test would you please?
Comment #43
miguel_angel CreditAttribution: miguel_angel commentedWhat about this?:
Comment #44
hovel CreditAttribution: hovel commentedSince 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:
With:
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
Comment #45
hovel CreditAttribution: hovel commentedDisadvantage 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.
Or the same shorter:
Comment #46
miguel_angel CreditAttribution: miguel_angel commentedHi, 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.
Comment #47
Michael Zetterberg fd. Lopez CreditAttribution: Michael Zetterberg fd. Lopez commentedNone 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).
Comment #48
miguel_angel CreditAttribution: miguel_angel commentedTrying 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...
Comment #49
hovel CreditAttribution: hovel commentedHi,
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:
Regards,
hovel
Comment #50
miguel_angel CreditAttribution: miguel_angel commentedHi, 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.
Comment #51
Michael Zetterberg fd. Lopez CreditAttribution: Michael Zetterberg fd. Lopez commentedhovel,
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.
Comment #52
hovel CreditAttribution: hovel commentedHi 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.
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
Comment #53
hovel CreditAttribution: hovel commentedMichael,
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.
Comment #54
dstolComment #55
maximpodorov CreditAttribution: maximpodorov commentedIt 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.
Comment #56
mckinleymedia CreditAttribution: mckinleymedia commentedThis is awesome! Thank you, thank you, thank you!
Comment #57
nevets CreditAttribution: nevets commentedPlease do not change issue titles.
Comment #58
vrivera82 CreditAttribution: vrivera82 commentedmckinleymedia
What did yo do?
Comment #59
Jenechka CreditAttribution: Jenechka commentedHere is the patch from me, that solves the problem.
Comment #60
Jenechka CreditAttribution: Jenechka commentedComment #61
dstolUpdating status
Comment #63
johnvmore explicite title.
this issue might be a duplicate of #1333246: 'Hide taxonomy terms' hides empty parents of child terms that do have nodes
Comment #64
Jenechka CreditAttribution: Jenechka commentedNext attempt with the patch.
Comment #66
Jenechka CreditAttribution: Jenechka commentedComment #67
Jenechka CreditAttribution: Jenechka commented#64: taxonomy_menu.hide_empty_terms.patch queued for re-testing.
Comment #68
Enemy CreditAttribution: Enemy commented#64: taxonomy_menu.hide_empty_terms.patch queued for re-testing.
Comment #69
Enemy CreditAttribution: Enemy commentedwork! thanks!
Comment #70
skylord CreditAttribution: skylord commentedPatch for latest dev - if anyone is using this module on legacy sites. :-)
Comment #72
dstolThe Drupal 6 version is no longer supported