Revisions and taxonomy don't play well

Revisions aren't designed to keep track all data related to a node. Only the teaser and the body. I think it's questionable as to whether you would want the taxonomy to automatically revert back to a previous state anyway. So I wouldn't call the first half of your report a bug.

But categorization shouldn't be lost when changing revisions. I consider that to be a bug.

Steve is quite right: the term_node table is not revisioned. But you shouldn't use the tern_node data when reverting.

Heh, I wasn't fully awake: "should not lose" is what I meant.

I'd be ok with revisioning taxonomy data as well. Noboy wanted that so far, so I didn't implment it. I think we can do that in 4.8 if desired.

I tried your patch, Frodo, but it didn't work for me. When I click 'revert' for a node revision (the 'set active' link has been renamed to 'revert' recently, btw), the taxonomy mappings still get lost. I encountered this issue many months ago - see http://drupal.org/node/24336 - but since the forms API came in, I thought it was well and truly gone. Apparently I was wrong. :-(

This new patch fixes the problem for me. It seems that each item in the array of terms getting passed to taxonomy_node_save() is an object, when reverting a revision. This patch adds a condition that checks for an object field in each array element. It's not an ideal solution - really there should be only one format for the array of terms passed to taxonomy_node_save(), rather than FOUR (!?!?), and this patch is just feeding the devil - but it will do for now.

Also, I give a -1 to the idea of revisioning taxonomy-to-node mappings. I don't think this data is suitable for being tied to versions of nodes - it feels right versioning actual content (e.g. body text, event fields, flexinode fields, etc), but I don't think it feel right versioning relationships between entities.

I consciously decided (for the reason explained above) not to implement versioning in the category module, and I don't think it should be implemented in taxonomy either.

Both patches modify the taxonomy module. i think we should modify node.module instead to include the taxonomy info.

The real problem is of course that taxonomy data is not loaded by default.

I had the opposite experience to you, Frodo. I just downloaded latest CVS, and the problem is still there just as it was. Steps to reproducing the problem:

1. Create a new node (e.g. story), with one or more categories assigned to it, and its 'create new revision' box checked.
2. Once the story is created, you'll see the categories assigned to it (listed somewhere on the node's page or preview).
3. Go to the 'revisions' tab, and click 'revert' on the first revision. You may see an error looking something like "duplicate key '1-x in table term_node'.
4. Go back to the node's page. The taxonomy mappings are lost.

My last patch still applies against HEAD, and still fixes the problem. The problem is not that node.module is not including the taxonomy info, the problem is that taxonomy_node_save() currently can't read the info in the format that node.module is providing it (i.e. as an array of objects). My patch makes taxonomy_node_save() able to read it.

there should not be any diff between HEAD and 4.7 still

BTW, we do now load taxo terms during node_load(). not sure why that matters but killes seemed to think it was important.

i doubt that this 'feed the devil' patch is going to be accepted. CVS reviewers will want to fix the root problem.

Note, revisions and forum topics also don't play well, since forums are based on taxonomy terms.

http://drupal.org/node/45810#comment-68683

The term_node.tid always reverts to "1" when a node is reverted. So when reverted, a forum topic may disappear from the forums altogether (e.g. if tid "1" is a container).

Considering all this, it is maybe a better idea to remove this versioning feature. I wanted it to be an example for using revisions in auxiliary tables, but we already got the book module for this.

@killes: how would we remove this versioning feature exactly?

Oops, I apparently commented on the wrong issue. I meant to put this comment on http://drupal.org/node/45810

Ok, I can reproduce the "reverts to tid = 1" problem.

The problem seem to be that node_save passes taxonomy info as an object which taxonomy_node_save which is called from nodeapi does not handle. This maybe got introduced when taxonomy data was made to load in node_load (thanks Moshe!). Anyway, Jaza's patch fixes this for me. It might not be the nicest patch, but the function is already pretty ugly and the taxonomy module needs an overhaul anyway, which should be postponed to 4.8.

I'm thinking we need a clean fix for this. As Moshe points out, we'll want to fix the root of this problem.

Attached patch just loads the taxonomy terms for the node before calling node_save().

Please review.

Thanks
-K

Ah, I didn't notice Moshe's comment about the taxonomy being loaded in node_load.. My bad.

-K

Same patch but uses the existing taxonomy property. I've also tested this with free tags and hierarchical taxonomies..

Thanks
-K

I still think we should try to fix the underlying problem of having an array of arrays on the one method and an array of objects in the other. It would be easy enough to fix the loading of taxonomy data to return arrays, but I am not sure where this will break stuff.

I hope this is the right place to ask. Please let me know if I need to move this...

I'm running 4.6.5 and found that I could not delete revisions.

I applied Frodo's fix ( http://drupal.org/files/issues/drupal-4.7.0beta3-set-active-taxonomy_0.diff ) to taxonomy.module v 1.192.2.7 by finding taxonomy_nodeapi.

The fix worked great!

However, if the revision deleted is the last one - meaning there are now 0 revisions - I think there is still a problem - for me - coming from node.module v 1.485.2.14 when function node_revision_delete invokes node_save. I don't know how to approach this without risking some other problem creeping in...

I tried a change such that node_save would only be invoked if one or more revisions still exist - which cleared up the original error messages I was seeing - but that change ends up leaving the last revision attached to the node... Too bad for me! (And I probably should not be surprised.)

What should I be looking for here, in terms of dealing with the last existing revision being deleted? Any ideas?

*bump* So what do we do about this? I don't think messing with the taxonomy module is really an option at this point..

-K

the most recent patch is exactly 1 line long. seems like a harmless fix considering our current near release state. we can do a big plumbing project later. i tested the patch and it works as advertised.

rerolled to remove fuzz

I agree, committed.