In some cases, an action under "Content is going to be saved" will trigger rules to save the content after all actions are done, but before the content is actually "saved" from the event which raised the trigger.
This causes recusion to occure though the trigger; In my tests it always ran the same trigger twice before catching on.

The fix will probably be to ignore requests to save content if that content is in a "Content going to be saved" event run.

Also somewhat related, the save was being triggered by "Set CCK field". This is CCK specific, but it raises the question: Should actions trigger a node save when relevant, or should that be left to the rules user to make a "Save content" call? User-friendlyness points to the former, but that would directly cause the case of this bug unless the above fix is implemented (which of course will not be an issue if it is).

Comments

RoboPhred’s picture

Why this is important: My specific setup doesn't care much. However, if you had a one-time action (rather than field changes as I do), it would run repeatedly:
Sending e-mails
Creating *
Duplicate messages (drupal_set_message / error).

fago’s picture

>The fix will probably be to ignore requests to save content if that content is in a "Content going to be saved" event run.

It should already work that way. I'm going to give it a test.

RoboPhred’s picture

I tested it on my own with a new content type clearing a cck value as my broken rule does. It worked as expected... However, my actual production content types still have this issue.

Here is the relevant trace output from my bug report content type.
In this trace, I changed its status from "fixed" to "open". This should:
1. Clear the "target" cck field, becasue we were changed from "fixed".
2. Enable comments and post a comment, because we were changed to "open".

To help you make sense of this, I have two other rule sets being called from here. Both act like functions, in that they have a single rule with the same name and no conditions. They both have a single action of custom php code.
They are "Comments: Set enabled" which sets $node->comment without saving it, and "Comments: Create RPDNet comment" which creates a comment using the comment_edit function. Both of them are passed the content type as an argument, and the "create RPDNet comment" also has strings for subject and comment text.

0 ms Content is going to be saved has been invoked. 
18.133 ms Included MODULE.rules.inc files. 
19.496 ms Executing the rule Bug report submitted on rule set Content is going to be saved 
19.735 ms Condition Saved content is Bug report evaluated to TRUE. 
20.156 ms Loaded variable unchanged content 
20.235 ms Action execution: Bug report submitted 
>>
21.489 ms Bug report submitted has been invoked. 
21.752 ms Executing the rule Status changed from Open but not to Resolved on rule set Bug report submitted 
22.175 ms Condition Status has changed evaluated to TRUE. 
22.45 ms Condition Old Status was Fixed evaluated to TRUE. 
22.63 ms Condition Status is Resolved evaluated to FALSE. 
22.813 ms Condition Status is Fixed evaluated to FALSE. 
22.959 ms Action execution: Clear target 
...rules that didn't execute due to conditions
25.22 ms Executing the rule Status changed to Open on rule set Bug report submitted 
25.425 ms Condition Status is Changed evaluated to TRUE. 
25.6 ms Condition Status is Open evaluated to TRUE. 
25.803 ms Action execution: Enable comments 
>>
26.272 ms Comments: Set enabled has been invoked. 
26.536 ms Executing the rule Comments: Set enabled on rule set Comments: Set enabled 
26.709 ms Action execution: Execute custom PHP code  //This does NOT save the content.  It directly sets the $node->comment variable.
26.986 ms Evaluation of Comments: Set enabled has been finished. 
<<
27.194 ms Action execution: Comments: Create RPDNet comment 
>>
27.56 ms Comments: Create RPDNet comment has been invoked. 
27.938 ms Executing the rule Comments: Create RPDNet comment on rule set Comments: Create RPDNet comment 
28.089 ms Action execution: Execute custom PHP code //This does NOT save the content.
32.878 ms Evaluation of Comments: Create RPDNet comment has been finished. //This does NOT save the content.  It creates a new comment via php call.
...rules that didn't execute due to conditions
>> //We go one deeper here, yet nothing triggered us to!
34.448 ms Content is going to be saved has been invoked. 
34.711 ms Not executing the rule Bug report submitted on rule set Content is going to be saved to prevent recursion. 
...rules that didn't execute due to conditions
<<
>>
72.835 ms After updating existing content has been invoked. //Why?  What triggered this?  We are still inside "Content is going to be updated"!
++Rules that executed as they should when "updating existing content" called. //We shouldn't have been called here, though!
<<
191.818 ms Saved variable Content of type node. //A save call was ran while inside "Bug report submitted" which is inside "Content is going to be saved"
191.901 ms Evaluation of Bug report submitted has been finished. 
<<
0 ms Evaluation of Content is going to be saved has been finished. //Yep, we were inside "going to be saved", and we saved inside of it...
0 ms After updating existing content has been invoked. 
++Rule sets for "after updating" run properly, but they were already ran improperly above!

Perhapse passing the content as an argument to a rule set has something to do with it?

I re-tested this with the current -dev (April 16) with no change.

RoboPhred’s picture

More testing:

If I disable the rule "Executing the rule Status changed from Open but not to Resolved on rule set Bug report submitted" (which is misnamed, should be "...from Fixed or Resolved but not to either"), the issue does not occure. The only action this rule contains is setting the cck field to nothing (the field is a non-required noderef field). It also goes away if I leave the rule enabled, but remove its only action.

Giving a noderef field to the Test content type, and adding the same clearing action to it does not reproduce the problem...

RoboPhred’s picture

I tried throwing debug messages in the code, all I came up with is that somewhere between

node.rules.inc

function node_rules_event_info() {
  // Specify that on presave the node is saved anyway.
  $items['node_presave']['arguments']['node']['saved'] = TRUE;

and rules.variables.inc

  function save_changes() {
    //if the variable is not saved automatically, save it
    if ($this->_changed && !$this->info['saved'] && $this->data->is_savable()) {

$this->info['saved'] is being explicitly set to false.

A quick look in cck's content.rules.inc shows no actual changes to it.

Without knowing what actually happens, it might be possible that the node argument definition in content.rules.inc...

function content_rules_action_info() {
  $info = array();
  $info['content_rules_action_populate_field'] = array(
    'label' => t('Populate a field'),
    'arguments' => array(
      'node' => array(
        'type' => 'node',
        'label' => t('Content'),
      ),
    ),
    'eval input' => array('code'),
    'help' => t('You should make sure that the used field exists in the given content type.'),
    'module' => 'CCK',
  );
  return $info;
}

...is somehow overriding the settings of the existing node argument that is passed in to it and clearing the 'saved' value. However, this should be occurring every time then, and does not explain why it only happens with a "populate field" on the bug report set of rules and not on a separate content type...

fago’s picture

Status: Active » Closed (duplicate)

Oh, I think I know what's the issue here. It's probably a duplicate of #373268: Using rule set invokes node_save() when not needed.

The rule set doesn't know about the variable info, it just knows that there is a node, and when it does changes, it saves them itself. That need to be improved, yes.

fago’s picture

As you see in the other issue, it's fixed now. Please test and report whether it works now. But let's keep the discussion in the other issue.

mitchell’s picture

Component: Rules Engine » Rules Core

Updated component.