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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

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

mgriego’s picture

Status: Needs work » Needs review
FileSize
1.83 KB

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.

mgriego’s picture

Status: Needs work » Needs review
FileSize
1.74 KB

Trying again.

mgriego’s picture

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.

mgriego’s picture

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.

Gábor Hojtsy’s picture

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 :)

mgriego’s picture

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.

EugenMayer’s picture

Priority: Normal » Major
FileSize
1.79 KB

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.

EugenMayer’s picture

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.

mkalkbrenner’s picture

Version: 6.16 » 6.x-dev
Status: Needs work » Needs review
FileSize
1.47 KB

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

EugenMayer’s picture

Thanks Markus

jiv_e_old’s picture

EugenMayer’s picture

Status: Needs review » Reviewed & tested by the community

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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

All right, committed, pushed. Thanks!

xtfer’s picture

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.