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.
Comment | File | Size | Author |
---|---|---|---|
#12 | 767104_taxonomy_cache-D6.patch | 1.47 KB | mkalkbrenner |
#9 | drupal_taxonomy_cache_fix.patch | 1.79 KB | EugenMayer |
#4 | taxonomy-updated_terms_visibility-767104-3.patch | 1.74 KB | mgriego |
#2 | taxonomy-updated_terms_visibility-767104-2.patch | 1.83 KB | mgriego |
taxonomy-updated_terms_visibility.patch | 1.83 KB | mgriego | |
Comments
Comment #2
mgriego CreditAttribution: mgriego commentedFix the patch format for the qa bot.
Comment #4
mgriego CreditAttribution: mgriego commentedTrying again.
Comment #5
mgriego CreditAttribution: mgriego commentedOK, 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.
Comment #6
mgriego CreditAttribution: mgriego commentedAt 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.
Comment #7
Gábor HojtsyWhile 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 :)
Comment #8
mgriego CreditAttribution: mgriego commentedI'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.
Comment #9
EugenMayer CreditAttribution: EugenMayer commentedWell 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.
Comment #11
EugenMayer CreditAttribution: EugenMayer commentedI 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.
Comment #12
mkalkbrennerThe original patch looks good to me. But I simplified it according to EugenMayer's suggestion from #11.
Comment #13
EugenMayer CreditAttribution: EugenMayer commentedThanks Markus
Comment #14
jiv_e_old CreditAttribution: jiv_e_old commentedRelated issues:
http://drupal.org/node/605182
http://drupal.org/node/594004
http://drupal.org/node/471074
Comment #15
EugenMayer CreditAttribution: EugenMayer commentedi have used this patch for 5 months now, works perfect.
Comment #16
Gábor HojtsyAll right, committed, pushed. Thanks!
Comment #17
xtfer CreditAttribution: xtfer commentedThere's a possible related follow up to this at #1427848: Node taxonomy array key change on node_save.