[drupal-devel] [bug] Make node_save() recursion-aware

Kjartan drupal-devel at drupal.org
Fri Aug 26 16:18:49 UTC 2005 

Issue status update for 
http://drupal.org/node/27007
Post a follow up: 
http://drupal.org/project/comments/add/27007

 Project:      Drupal
 Version:      cvs
 Component:    node system
 Category:     bug reports
 Priority:     normal
 Assigned to:  jvandyk
 Reported by:  jvandyk
 Updated by:   Kjartan
 Status:       patch (code needs review)

>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.




Kjartan



Previous comments:
------------------------------------------------------------------------

Tue, 19 Jul 2005 02:14:16 +0000 : jvandyk

Attachment: http://drupal.org/files/issues/node_serialized2x.patch (664 bytes)

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




------------------------------------------------------------------------

Tue, 19 Jul 2005 18:36:23 +0000 : Dries

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




------------------------------------------------------------------------

Thu, 18 Aug 2005 01:26:21 +0000 : moshe weitzman

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?/




------------------------------------------------------------------------

Thu, 18 Aug 2005 04:57:11 +0000 : chx

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




------------------------------------------------------------------------

Thu, 18 Aug 2005 11:59:14 +0000 : killes at www.drop.org

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




------------------------------------------------------------------------

Thu, 18 Aug 2005 14:22:15 +0000 : moshe weitzman

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.




------------------------------------------------------------------------

Thu, 18 Aug 2005 14:23:24 +0000 : moshe weitzman

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




------------------------------------------------------------------------

Thu, 18 Aug 2005 21:50:36 +0000 : Dries

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.




------------------------------------------------------------------------

Thu, 18 Aug 2005 23:21:41 +0000 : chx

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.




------------------------------------------------------------------------

Fri, 19 Aug 2005 00:58:11 +0000 : jvandyk

Attachment: http://drupal.org/files/issues/save_hooks.diff (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)


------------------------------------------------------------------------

Fri, 19 Aug 2005 19:00:53 +0000 : jvandyk

Attachment: http://drupal.org/files/issues/save_hooks2.diff (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 [1] 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.
[1] http://api.rubyonrails.com/classes/ActiveRecord/Callbacks.html




------------------------------------------------------------------------

Fri, 19 Aug 2005 19:10:46 +0000 : chx

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

More information about the drupal-devel mailing list