taxonomy_get_parents(), taxonomy_get_children(), and taxonomy_get_tree() do not return a full term objects

It seems that the current version of taxonomy_get_parents does not actually cache the results of the query (tids is never assigned a value after the initial $tids = &drupal_static(__FUNCTION__, array());). The version in this patch *does* cache the results.

The test fail seems to be caused by taxonomy_term_delete (called by taxonomy_vocabulary_delete) relying on taxonomy_get_parents to get an accurate count of parents for a term during mass deletion (terms with multiple parents are not deleted). After deleting some terms, taxonomy_get_parents starts to return stale data (parent terms that were deleted still appear in the returned array), causing the taxonomy_term_delete skip deletion of terms that would otherwise be deleted.

Since the current implementation of taxonomy_get_parents() does not actually cache the values, may be we should leave that as it is and fix the cacheing in a seperate issue?

+++ modules/taxonomy/taxonomy.module	1 Aug 2010 21:25:16 -0000
@@ -702,34 +702,21 @@ function taxonomy_vocabulary_get_names()
  * Find all parents of a given term ID.

Since we're tinkering with the documentation anyway, this should become "Finds all parents..."

+++ modules/taxonomy/taxonomy.module	1 Aug 2010 21:25:16 -0000
@@ -702,34 +702,21 @@ function taxonomy_vocabulary_get_names()
+ * @return

Needs to be preceded by a blank line.

Powered by Dreditor.

This query has to use the query builder and add the term_access tag, as entity_load_multiple() doesn't do any access control, it's not its job.

That should not be there.

Logic and code look very nice.

$query->orderBy('t.weight'); gets added twice in successive lines. Probably a copy/paste snafu.

Unrelated to the patch, taxonomy_terms_static_reset() is an abomination. we clear 7 statics when a term is added/edited/deleted? Thats excessive caching folks. Its bound to cause subtle freshness problems.

It is RTBC for me.

Looked at the surrounding code, and this makes sense. Committed to CVS HEAD.

I was testing the taxonomy system and this patch broke the multi-parent support when using taxonomy_get_tree (see term overview with a simple multi-parent vocabulary), it worked in alpha6 before this patch was committed.

further, using taxonomy_term_load_multiple really slows down taxonomy_get_tree (an already slow function). this can be easily reproduced by generating a taxonomy with around 5000 terms, where just the loading of terms with taxonomy_term_load_multiple in taxonomy_get_tree will take 2-3seconds (at least on my notebook). without this call, the fetching of terms from the database is very fast.

The uploaded patch adds a test case, which verifies that multi-parents are working in the term overview (uses taxonomy_get_tree).

Further I reverted the taxonomy_get_tree function, therefore removed the full entity loading. As I pointed out in my previous comment, the taxonomy_get_tree really gets slower when adding the entity loading and when I take a look at the taxonomy module, the taxonomy_get_tree function is usually used to build forms and overviews of terms, tasks where a full entity is not needed. I understand that having the full object can be useful, but in this case it means a lot of overhead and actually breaks multi-parents.

The patch doesn't affect taxonomy_get_parents and taxonomy_get_children, as they normally do not load that many terms.

>Not providing the full object is also broken.

Agreed. We need to fix the multi-parents, but also the performance of taxonomy_get_tree() is critical as it is used in quite some taxonomy functions. Best we deal with that in separate issues.

added a separate issue with the bug report: #899258: Multi parents broken

issue on performance: #556842: taxonomy_get_tree() memory issues

This should never have gone in, and was committed within five days of being opened without either of the taxonomy maintainers being contacted.

It has made #556842: taxonomy_get_tree() memory issues, many, many times worse, and that issue was already major. I just saw a form validation completely blow up trying to validate a vocabulary with 3,500 terms via taxonomy_allowed_values(). 3,500 terms is nothing.

Re-opening this as a critical bug to roll back the taxonomy_get_tree() hunks. If that's not an option then #556842: taxonomy_get_tree() memory issues needs to be promoted to a release blocker, but at the moment even small-ish sites are going to hit the PHP memory limit much, much quicker than they ever did in D6, making this a serious regression from something that was already very bad in the first place.

I'm OK with rollback if thats the desire of the taxo maintainers. It is a small patch, and I doubt much depends on it. Hopefully bot will flag any problems.

Well. The problem is it was in there when we announced beta1, so my guess is contrib is actively using this. (for example, almost assuredly pathauto and token since they're tagged here) So IMO we need to revert, but keep some sort of a replacement. Is that replacement separate API functions, or an extra "return object = TRUE" argument to these functions? Not sure. Let's discuss.

taxonomy_get_parents() is fine. taxonomy_get_children() is fine most of the time, if you had a vocabulary where one term had thousands of children that would also blow up, but it'd be an edge case.

I took another look at #556842: taxonomy_get_tree() memory issues and the patch was a lot more ready than I thought, re-rolled and Moshe has RTBCed, that definitely gets us back to where D7 was in August, the extra optimization may be improvement on D6 too. There are rough times at http://drupal.org/node/556842#comment-3464092 but no memory numbers. For now I think it makes sense to close this one again, and bump that to critical. If it gets bogged down then we'll need to go for the straight revert though.