node_save() saves a node, then calls insert or update hooks. These hooks may run functions that, in turn, call node_save().

This all works fine except that if a revision is being created, the revision gets serialized twice -- once on the original node_save() call and once on the callback's node_save() call.

The attached two-line patch simply inserts a boolean variable called rev_serialized to keep track of whether the serialization has already been done.

I'd like to see cvs and 4.6 patched.

This patch is necessary so I can release actions and workflow for 4.6.

Background: http://drupal.org/node/24326

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dries’s picture

I don't like this patch. It's a hairy fix. There must be a better way to avoid recursion in first place.

Dries’s picture

Status: Needs review » Needs work
moshe weitzman’s picture

gosh, this a two line change with zero performanhce impact. seems like a useful and minimally invasive fix for our current dilemma. we'll remove it once a better fix comes around. is a shame to hold back workflow/actions for such a minor issue. any others want to comment or suggest a different solution?/

chx’s picture

If there would be a little comment why this is needed, then I'd +1 it heartily.

killes@www.drop.org’s picture

Status: Needs work » Reviewed & tested by the community

Please commit. In case revisions shouldn't make it (for performance reasons ;) we need it fixed for 4.7.

moshe weitzman’s picture

The first user of this will be workflow+actions modules. They listen on nodeapi's insert/update operations. When a node is seen there, arbitrary actions may fire according to the admin's wishes. So for example, a node may be promoted or may become sticky. To do that properly, the actions should call node_save() again with $node->promote=1. Thus, we are recursively calling node_save() which leads to a double serialize problem. This 2 line fixes it.

There may be more nodes which benefit from this.

moshe weitzman’s picture

I meant to say - "there may be more *modules* which benefit from this."

Dries’s picture

Won't commit. If you are calling node_save() recursively, something is really wrong. For one, you can no longer guarantee that the node is in a 'safe state'. Like, what happens if you saved half a node and you recursively try saving that node again? This needs a better solution.

chx’s picture

I have a possible solution, which does not need core change. Keep a stack of necessary actions in SESSION and pop the stack first in _exit then subsequent _init calls 'till the stack is empty.

jvandyk’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.83 KB

OK, here is an alternate approach. We leave node_save() as it is, but add two new nodeapi hooks to node_submit().

For reference, here's the meat from my message to the devel list, as chx wants background:

The nodeapi calls for nodes go like this on node submission:

1. nodeapi validate hook
2. (node is saved)
3. nodeapi insert/update hook
   a... modules respond
   b... modules respond
   c... modules respond
   ...
   workflow modules responds, fires actions, the last of which saves the node (go to 2)
   x... modules respond
   y... modules respond
   z... modules respond

As you can see, we have some problems. Because module_invoke() works alphabetically, any modules with names alphabetically after "workflow" have not yet responded when the workflow hook is invoked. Remember, the workflow hook activity looks like this:

1. nodeapi insert/update is caught by workflow module
2. workflow module fires the 'transition pre' workflow hook to let modules know a transition is about to occur
3. modules have a chance to veto the workflow transition
4. if no module vetoes the transition, the 'transition post' workflow hook is fired so modules can respond to the workflow state change.

So to move ahead, we need a 1a nodeapi hook that says, "validation passed and this node will now be saved" so workflow changes can happen there, and/or a 4 nodeapi hook that says, "the node has been saved, all modules have done their thing, it's safe to do a node_load and get a node that is in a consistent state".

I'd like to see both because it gives maximum flexibility for workflow. For example, it's much more efficient to change the status of the node to published after validation but before saving than it is to use the current method of saving the node again.

So the revised flow looks like this:

1. nodeapi validate hook
1a. node is guaranteed validated; nodeapi save pre hook (workflow fires efficient actions like changing status)
2. (node is saved)
3. nodeapi insert/update hook
   a... modules respond
   b... modules respond
   c... modules respond
   ...
   x... modules respond
   y... modules respond
   z... modules respond
4. node is guaranteed saved; nodeapi save post hook (workflow fires actions like email notification)
jvandyk’s picture

FileSize
1.83 KB

After discussion with chx and moshe, changed names of hooks to be update pre, update post, insert pre, insert post so that listeners have a better context in which to make decisions.

I don't like the _exit hook solution because that will slow down every page.

Todd Grimason linked to this page of Ruby on Rails hooks on drupal-dev.

Although these are 4 hooks, note that only 2 will ever execute at a given time, because we are either doing an insert or an update.

chx’s picture

I like this, but if this gets in, next issue will be to clean up 'validate'...

Kjartan’s picture

From the conversations on IRC here is where I ended up on nodeapi calls.

Nodeapi calls when creating a new node:
default values -> form pre -> form post -> validate -> finalize insert -> insert

Nodeapi calls when updating an existing node:
load -> form pre -> form post -> validate -> finalize update -> update

Not sure I completely agree with splitting finalize, but I am sure those who support it will post nice examples to make it a clear choice :)

This would remove the hack in node_form with validating an empty node, and let modules perform actions just before its saved.

jvandyk’s picture

FileSize
1.78 KB

I have updated this patch to work with HEAD after the recent node changes.

Here's a review of this patch:

Why do we need the 'update pre' hook?

Because we need a nodeapi call where modules can assume that they are working with a node that has passed validation. For example, if actions such as e-mail sending are going to fire, you don't want them to fire and then a module later in the node_invoke_nodeapi() sequence to throw a validation error. You want validation to pass as a separate step, and then be able to fire actions without worrying that a node_save() might not happen.

Why do we need the 'update post' hook?

Because we need to guarantee that we are working with a "clean" node. If you use the 'insert' or 'update' hooks, a module later in the node_invoke_nodeapi() sequence may make changes to database tables that will affect the node. Modules typically use the 'insert' and 'update' hooks to modify their own private tables. To ensure consistency and avoid corruption, you want to make certain that all modules have completed their own private database updates before moving on. Why do you want to make certain of this? Because you do not know what actions are going to do. Actions (as in actions.module) are by definition arbitrary. It is much better to have all tables in a consistent state before firing actions. You want to make sure that if an action does a node_load it will get a clean node, including all the updates that listening modules do. Steven, please chime in on this as this was your concern.

One other point on the 'update post' hook. It is conceivable that actions fired may run a long time or run into problems. Of course actions need to be written as cleanly as possible. 'update post' gives actions a chance to decide that they will do things that may have problems (xmlrpc posts that may timeout, for example) and breathe easy knowing that even if there is a timeout or other error, the node has already been saved.

Do we need 'insert pre' and 'update pre'? Why not just use 'save pre'?

I think that since we already know the context, it is nice to pass that along to modules so they do not have to test for the existence of $node->nid to waste a cycle figuring out something we already know.

What about naming?

Kjartan has suggested changing the name 'update pre' to 'finalize update' and 'update insert' to 'finalize insert'. I have left the names as is because I think both hooks are important and I think the current names are easiest to understand. 'finalize' does not give a clue as to whether it is before or after the actual node_save(). But I am open to better naming if it is easy to understand for the developer.

What's the downside?

I do not see a downside to this patch. It adds two nodeapi calls on insert, or two on update. It is not a performance hit. It adds only 4 lines of code. It does not require anyone to rewrite anything. And it is the best solution we have to the problem.

decafdennis’s picture

Priority: Normal » Critical

I am creating a module that creates an additional node in hook_nodeapi('insert', ...). The additional node was created OK, but the actual node (where hook_nodeapi was invoked for in the first place) not: information like taxonomy was lost. I couldn't find anything to fix this bug, untill I came accross this post.

The problem appearently was that I was using node_save recursive, and my module name starts with an A and the taxonomy module with a T (duh). Well, the patch above (save_hooks3.diff) certainly fixed this problem for me by creating the node in the 'insert post' hook instead of the 'insert' hook. Very nice!

I agree with jvandyk, there's no downside to this patch, there are some additional calls but at least module_implements 'caches' the list of modules that implement a certain hook, hook_nodeapi in this case. I myself cannot find a practical implementation of the 'insert pre' hook (or 'update pre'), though. jvandyk says something about sending an e-mail, but this can be done just as wel in either 'insert' or 'insert post'.

By the way, another thing that worked for me was changing in node_invoke_nodeapi:

foreach (module_implements('nodeapi') as $name) {

to

$implementations = module_implements('nodeapi');
foreach ($implementations as $name) {

I don't know why this worked, but it probably has something todo with the static variable in module_implements.

I think this patch is very usefull and this bug should be fixed in 4.7.

Thank you.

jvandyk’s picture

I1uv4t4r says I myself cannot find a practical implementation of the 'insert pre' hook (or 'update pre')

For one thing, it's for performance. A quick example: it is faster to set the node status to "published" during the 'insert-pre' hook than to do an entire node_load and node_save again later to accomplish this change.

chx’s picture

Recently, I was given tasks that were solved easiest by setting up various node relations. When you do that, this patch becomes a godsend.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

The approach of this most recent patch has seen a lot of review and comment here. I don't see any outstanding objections, so I set to 'ready for commit'

matt westgate’s picture

+1 for this 4-line patch.

Currently, bizarre module names (so they are the last to execute) and extraneous node_save calls are the only way around this problem.

Since Drupal is built around the concept of a node it makes sense that core provides a sophisticated node workflow as offerred here.

killes@www.drop.org’s picture

I'd also like this patch to be committed. It will enable us to do cool and exciting new stuff with Drupal.

decafdennis’s picture

This is a very nice addition to the nodeapi hook, and I'm using it already for one of my modules. +1 from me.

Steven’s picture

Status: Reviewed & tested by the community » Needs work

This patch does not apply anymore and probably needs to be double checked now that we have form API.

moshe weitzman’s picture

the 'pre' part of this patch is no longer needed now that we have the execute operation of nodeapi. we still don't have a 'post' operation unless Ive missed something.

chx’s picture

execute is now called submit, but yes.

puregin’s picture

FileSize
3.44 KB

For what it's worth, here's a new patch against HEAD with the 'update pre' calls removed, and a few cleanups suggested by the code_style script.

tangent’s picture

I haven't tested the latest patch but it seems to have some errors.

-      node_save($node);
+      // Give modules a chance to modify node before saving

That node_save() call is not removed in the previous patch.

+      // Give modules a chance to modify node before saving
+      node_invoke_nodeapi($node, 'insert pre');
       node_save(&$node);
moshe weitzman’s picture

Category: bug » feature
Priority: Critical » Normal
Jaza’s picture

Title: Make node_save() recursion-aware » Add 'insert/update post' op to hook_nodeapi
Version: x.y.z » 6.x-dev

I think this is still needed, so moving to 6.x-dev queue. This should now go in node_form_submit().

mlncn’s picture

+1 Please!

colan’s picture

Definitely a +1 from me. Currently, I've got to do a node_save() manually during 'insert'. I can't use 'submit' because what I need to do should only happen on creation.

Should we move this to 7.x-dev? I don't think this is high priority enough to make it into 6 at this point. Especially since we don't have a working patch. ;)

jvandyk’s picture

Version: 6.x-dev » 7.x-dev
Assigned: jvandyk » Unassigned
Jaza’s picture

Version: 7.x-dev » 8.x-dev

AFAIK this is still needed (except that hook_nodeapi is now dead, so we would need a new hook, perhaps two, called something like hook_node_insert_post and hook_node_update_post). Bumping.

moshe weitzman’s picture