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: jvandyk -Status: patch (ready to be committed) +Status: patch (code needs review) 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) jvandyk 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@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.