Reordering taxonomy terms with drag-and-drop removes taxonomy breadcrumb path

Please try the latest 6.x-1.x-dev version and see if the problem persists. There have been a few changes that may impact this. Let me know if its still affecting the current version and I'll look into it further.

No response in about a month. Apparently not an issue anymore?

Upgraded to 6.x-1.0. Problem persists. Tried it on a much smaller vocab (~10 terms).

Sorry for the delay! There aren't enough hours in the day, days in the week, weeks in the... etc. :)

Please try the 6.x-1.x-dev version. I've tried to replicate the problem that you describe and can't do it with the latest version. When I use the drag and drop interface to reorder terms, the paths stay assigned to the taxonomy terms, as expected.

Once 6.x-1.x-dev is further tested, I'll be able to release 6.x-1.1. So your input on the dev version would really help.

Thanks

I just realized that I was updating HEAD, not the 6.x.1.x-dev branch. I've now brought the dev branch up to date. It might take a day before a new snapshot is released. Until then, you can get the latest code from cvs. In the future, I'll be committing all new code to the dev branches for testing. Thanks

I think I have the same issue and I've found the reason.

Let me give an example single hierarchy vocab with it's weights:

North America (5)
-- USA (1)
-- Canada (2)
Africa (6)
-- Nigeria (3)
-- Cameroon (4)

Nodes should have the breadcrumbs set as in the following examples:

"Africa >> Nigeria"
"North America >> Canada"

With this current weight setup, taxonomy breadcrumb works, because the lightest term is always a child term, and thus the parent gets set properly. Nigeria is lighter than Africa, for example.

However, a different configuration of weights will cause it to stop working (this can easily happen when the drag-n-drop functionality is used). The following is still a valid taxonomy structure:

North America (0)
-- USA (1)
-- Canada (2)
Africa (3)
-- Nigeria (4)
-- Cameroon (5)

Now the children are heavier, and you won't get the correct breadcrumb set.

I propose that the SQL for finding the lightest term in _taxonomy_breadcrumb_node_get_lightest_term gets changed to account for whether a term has a parent or not. So the current SQL which is:

should include the term_hierarchy table and then sort on term_hierarchy.parent as a secondary sort (before term weight and term name, but after vocab weight). All terms that don't have a parent have the value '0' in the term_hierarchy table, thus will be the last ones.

In my limited testing this is working nicely!

Thanks. Patch attached.

Attachment	Size
taxonomy_breadcrumb-parent_terms-372813-6.patch	1.27 KB

New patch... forgot the curly braces!

Attachment	Size
taxonomy_breadcrumb-parent_terms-372813-7.patch	1.33 KB

Thanks Mark. cooperwd, can you test the patch in #8 and let us know if that fixes it for you. I am a little concerned about the extra join, but if it works we can give it a try in the dev version for a while and look for performance problems.

Sure thing. I also am aware of performance problems with extra joins, but I'm not sure if there is an alternative. Caching maybe?

Would it not be better to list by parent first and then weight because you'd usually be more interested in listing deeper taxonomy hierarchies? Or maybe add an option to decide which way to do it?

Here's another patch.

Attachment	Size
taxonomy_breadcrumb-n372813-order_with_parents.patch	1.21 KB

Thanks! I will test within the next week and report back.

Dave

Any feedback on this?

Anything further? Would be nice to see this change. I definitely need it!

Damien > Your proposed query is below...

SELECT t.* FROM {term_node} r
INNER JOIN {term_data} t ON r.tid = t.tid
INNER JOIN {term_hierarchy} h ON t.tid = h.tid
INNER JOIN {vocabulary} v ON t.vid = v.vid
WHERE r.nid = %d
ORDER BY h.parent DESC, v.weight, t.weight, t.name

Basically, you have moved the vocab weight. Is this your intention?

I would prefer the taxonomy breadcrumb to always come from the same vocab (the lightest one). That makes sense to me.

Mark: If you are trying to build a tree then you need to adhere to the parent first, to group child items together, and weight second.

Maybe I'm missing something... but from what I can tell, you're talking about term weight, not vocab weight.

Is that correct? Or are you talking about vocab weight? Because both are factors in the query.

Mark: My mistake. You are correct, I misread the original patch from message 8, which is correct.

No prob ;)

Here's a new patch rerolled against 6-1.

Any chance of a commit?

Attachment	Size
taxonomy_breadcrumb-parent_terms-372813-19.patch	1.45 KB

I've tested this and found no problems with it. I haven't had a case where the path was dropped. I've also benchmarked it in a performance testing environment and saw no significant difference in the request time.

I think its safe to commit to the dev branch for further testing.

@Mark Theunissen and DamienMcKenna, thanks for doing this!
Committed: http://drupal.org/cvs?commit=219018

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

This query still needs some work I think. I appears to deal with vocabulary and term weights (the original problem prompting the need for this fix), but it doesn't take into account the most recent revision of the assigned vocabulary terms in the term_node table.

Unless I am mistaken (which could be, I'm a Drupal Noob), page updates will insert new entries in the term_node table with an incremented vid. The current fix for this issue returns all the terms but in a random order with respects to vid, so the fisrt term in the query isn't always the most recent.

I had a page node classified with two terms from two vocabularies. The query correctly returned the vocabulary based on weight, but it returned the actual terms from a previous edit of the page. Perhaps something like this:

And I just came up with another problem with the another issue with the ordering.

If you have a vocabulary with a structure like

and a node has terms trusluthopu and baprush, then baprush is selected instead of trusluthopu, because of the ordering h.parent DESC.

Removing the DESC seems to fix the problem.

I haven't checked the revision issue yet, but will look into that next.

For a node with multiple terms, some with children, I would expect the "lightest term" would be the deepest child of the lightest parent. To carry on the example above....

North America (0)
-- USA (1)
----New York(4)
-- Canada (3)
----Alberta(2)
Africa (5)
-- Nigeria (6)
---- Zamfara(7)
-- Cameroon (8)

If a node had terms New York and Alberta (don't ask me why), Then I would want New York to be the lightest term because USA is lighter than Canada. But the current (in 6.x-1.x-dev, from #19) query could return either, depending on the tid of the parent.

Instead of ordering by the tid of the parent, we should be ordering by the weight(s) of the parent(s). I couldn't figure out a way of doing this in a single query, so I rewrote the function using taxonomy api functions. This has an advantage of using proven code (automatically fixing the node revision problem in 23, for example), but its not nearly as compact, and I suspect it may be less efficient (but core taxonomy caches, and the current tax_bc code does not, so its not clear and would need to be tested). I like this approach because, it handles these edge cases more faithfully.

The patch is against the curent 6.x-1.x-dev. Let me know what you think of the idea, and testing would be appreciated.

Attachment	Size
372813_tax_bc_admin_patch.diff	2.64 KB
372813_tax_bc_mod_patch.diff	903 bytes

@david.r.benham is right about the query not handling node revisions properly. Here is proposed change to _taxonomy_breadcrumb_node_get_lightest_term($nid):

This query modifies the query from 6.x-1.x-dev to add another join on the node table to look up the current revision of the node (node.vid).

Moreover, ideally when we are looking at old (non-current) revisions of any node, we would show the breadcrumbs that were chosen for that revision. To do so we would need 2 changes:

• Change taxonomy_breadcrumb_nodeapi() to pass $node->vid to _taxonomy_breadcrumb_node_get_lightest_term().
• Change _taxonomy_breadcrumb_node_get_lightest_term() to accept the vid parameter and use it in the query above (instead of joining to the node table we could just add vid to the where clause).

Does this sound right? I would be glad to open a separate issue and roll a patch if people feel that a fix to this would get committed.

[UPDATE: Opened a separate issue #607448: Show breadcrumb for the selected revision.]

@brandonojc, have you tested the patches in #25? That is what needs to be done next. I need feedback on those patches before I can consider alternative approaches. Thanks for testing!

@MGN, the patches in #25 work for me and solve the revision problem. It works right because the core function taxonomy_node_get_terms() takes revisions into account.

I verified both pieces of the problem: it now displays the taxonomy breadcrumb for the current revision of the node; and when viewing older node revisions it correctly displays the breadcrumbs for each revision. Thanks!

I reviewed the patches in #25 and believe the module now functions as expected with respect to this issue. The key seems to be the test on depth (which reflects the hierarchy) when the weights are not telling.

I would suggest renaming the function _taxonomy_breadcrumb_node_get_lightest_term to something like _taxonomy_breadcrumb_node_get_deepest_term and changing the comment to read something like "Return the deepest term with the lightest parent for a node".

Thanks for the feedback. I didn't rename the functions or tweak the comments yet, but the code has now been committed to 6.x-1.x-dev.