Node revisions corrupted when actions triggered [#24326]

In a particular situation, triggering certain actions will result in a node's revisions being corrupted. I encountered this bug when using the workflow module in the following situation:

1. User is in the node editing screen, about to submit an update to an existing node.
2. The 'create new revision' option is ticked (or, if the user doesn't have the "administer nodes" privilege, the default option for that node type is 'create new revision').
3. The user has selected a transition (a change from one workflow state to another) that will result in the triggering of actions.
4. One or more of the triggered actions will perform an operation on the node (e.g. "publish node" action, "make node sticky" action, etc).

The problem is, that when these actions perform an operation on the node's data, and then pass that data to node_save(), they are passing $node->revision in serialized form. However, node_save() automatically serializes $node->revision before running the database query. So anytime that an action saves a revision using node_save(), the revision is getting serialized TWICE.

When this happens, the already-serialized string of data is stored in the database as one big string. When node.module comes along (at some later point) to try and fetch the node's old revisions, it expects to find an array of revisions, but instead gets one long, useless, serialized string (and it then dies ungracefully).

Attached is a patch that gets around this problem, by creating a new copy of the node data, and by unserializing $node->revision before passing the data to node_save(). Since the data is passed by reference to the actions, a copy needs to be made - serializing the by-reference revisions doesn't work, I tried it.

By the way, I've got both workflow.module and actions.module working fine with current cvs Drupal (i.e. 4.6.x), except for this bug.

Comment	File	Size	Author
#6	actions.inc.patch	1.97 KB	yched
#5	actions.inc	13.18 KB	yched
	actions.inc.revision_fix.patch	4.74 KB	Jaza

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jvandyk CreditAttribution: jvandyk commented 16 June 2005 at 03:08

I don't think this is a problem with actions. Color me naive, but this is a problem with node_save(). node_save() should be alert to the fact that modules might call node_save() on the insert callback, effectively creating a recursive node_save() call.

The answer is not to manually unserialize revisions in every action, as the proposed patch does. The answer is for the node to keep track of the serialization status of node->revisions, and to not serialize it if it is already serialized.

Comment #2

Jaza CreditAttribution: Jaza commented 16 June 2005 at 06:16

Is it possible for node_save() to make this check? There's no is_serialized() function in PHP, although I guess that if is_array() returns true, it's pretty clear that the data's NOT serialized. If it's not possible to check by analysing the variable, then we'll have to add an is_revisions_serialized flag to node_save's parameters.

But personally, I think it's better design for node_save to accept the data in one form, and for other functions to convert the data to the correct form if need be. Allowing serialized and un-serialized revision data to be passed might just create more problems than it solves.

Comment #3

merlinofchaos CreditAttribution: merlinofchaos commented 15 September 2005 at 21:21

I realize it's easy to say that node_save() should fix it, thus putting the problem on the Drupal core, but that doesn't change the fact that without some kind of workaround, things don't work. Now, if the Core developers want to fix node_save() in 4.7 that's great, but it'll never be fixed in 4.6; which means for a 4.6 release (and the 4.5 release?) it should be fixed in workflow and that fix can be removed if/when node_save() is corrected.

As it is, I had to put in this patch (for which I am quite thankful, Jaza!)

Comment #4

yched CreditAttribution: yched commented 22 March 2006 at 23:39

There is still a problem with 4.7beta6 new handling of revisions.

Revisions are not loaded with the node anymore, they are stored in their own table.

Yet when you have an action like node_moderate triggerted by a workflow state change,
if the node that is being saved happens to be a new revision, TWO revisions are in fact created :
1 - the new revision is created by the 'regular' node_save
2 - ANOTHER one is created when re-saving the node in the action, since the 'revision' flag is still set...

The included patch corrects this (simply unsets the revision flag before re-saving the node....