The taxonomy nodeapi hook calls taxonomy_node_save to save the terms. The problem is that the hook does not update the $node object to reflect the changes in free tagging terms that may be performed in taxonomy_node_save. Further compounding the problem is that taxonomy_node_get_terms uses a static variable as a cache for node terms, and this cache is not invalidated when the node is updated, so a node_load (even one that forces a reset for node_load) that occurs later in the same page request is not able to see the updated terms. The attached patch allows that cache to be invalidated when terms are updated and also updates the $node object in the nodeapi hook to reflect the changes so that later hooks can see the updates.

Files: 
CommentFileSizeAuthor
#12 767104_taxonomy_cache-D6.patch1.47 KBmkalkbrenner
#9 drupal_taxonomy_cache_fix.patch1.79 KBEugenMayer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_taxonomy_cache_fix.patch.
[ View ]
#4 taxonomy-updated_terms_visibility-767104-3.patch1.74 KBmgriego
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy-updated_terms_visibility-767104-3.patch.
[ View ]
#2 taxonomy-updated_terms_visibility-767104-2.patch1.83 KBmgriego
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy-updated_terms_visibility-767104-2.patch.
[ View ]
taxonomy-updated_terms_visibility.patch1.83 KBmgriego
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy-updated_terms_visibility.patch.
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, taxonomy-updated_terms_visibility.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.83 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy-updated_terms_visibility-767104-2.patch.
[ View ]

Fix the patch format for the qa bot.

Status:Needs review» Needs work

The last submitted patch, taxonomy-updated_terms_visibility-767104-2.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch taxonomy-updated_terms_visibility-767104-3.patch.
[ View ]

Trying again.

Status:Needs review» Active

OK, this is my first core patch attempt. I now assume that the automated testing is failing since I'm not patching against 7.x, which would make sense since that's the current HEAD.

Status:Active» Reviewed & tested by the community

At first glance, this issue does not appear to affect 7.x, because of its completely rewritten taxonomy module that uses the entity system and the core drupal_static mechanism, so rolling a patch against 7.x doesn't make any sense. The patches above do apply cleanly to 6.16 and the current 6.x-dev, and we are currently using them as is in production.

Status:Reviewed & tested by the community» Needs review

While the patch looks good, we do not consider it a good practice to mark your own patch reviewed by the *community*. It would be great to get some independent reviews so we can ensure this works fine. I agree that the non-resettable static caching is an issue and is generally exterminated in D7 thankfully :)

I'd love to have some other people review it. I wasn't prepared to mark it critical, though. I wish there was an intermediate priority between normal and critical. In any case, as I said, we've been using this for a number of weeks in a sizable production site. It has allowed us to have nodeapi hooks that execute later in the stack actually be able to see the changes after they were processed by the taxonomy module.

Priority:Normal» Major
StatusFileSize
new1.79 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_taxonomy_cache_fix.patch.
[ View ]

Well run into the same issue. I also have written a patch, it works slightly different. It resets deep in the heart, so rather in taxonomy_node_save then in the nodeapi. That way its rensured that the cache is cleared when a node get saved.

In addition iam not clearing the whole cache but rather only the cache for the updated vid to save performance. For this, i changed the static var to be a global to be accessable (renamed the var for namespace conflict avoidence).

This bug is pretty ugly and esp when you think about solr integrations, this is painful to debug and lets you end up with a inconsistent index. This should probably even be an issue for drupal.org.

Status:Needs review» Needs work

The last submitted patch, drupal_taxonomy_cache_fix.patch, failed testing.

I guess the patch from @1 looks more "drupal like" and it also supports only clearing the cache for a vid.

The patch looks good and works. I would rather tend move the cache-reset from the nodeapi methods to the taxonomy_node_save method though.

Version:6.16» 6.x-dev
Status:Needs work» Needs review
StatusFileSize
new1.47 KB

The original patch looks good to me. But I simplified it according to EugenMayer's suggestion from #11.

Thanks Markus

Status:Needs review» Reviewed & tested by the community

i have used this patch for 5 months now, works perfect.

Status:Reviewed & tested by the community» Fixed

All right, committed, pushed. Thanks!

There's a possible related follow up to this at #1427848: Node taxonomy array key change on node_save.

Status:Fixed» Closed (fixed)

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