Actions only trigger one action per node page load

I think this is a bug in the trigger.module. It keeps a static flag to avoid recursion when calling actions from trigger_nodeapi but it fails to unset the flag at the end of the function avoiding the actions from been executed again for a different node.

Probably we should add an index to the recursion array so recursion is prevented only for the same node. So recursive invocations of the hook for other nodes will also trigger the actions.

This version of the patch also implements this.

This bug is also present in 7.x

Rerrolled against HEAD.

Doesn't break the trigger module tests. Maybe the tests should be modified to test the case where a nodeapi op is invoked more than one time (non recursively) for two different nodes.

See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476

Pedro,

Good catch, but your patch is still wrong since there are two return statements after the recursive[...] = TRUE statement.

Of course, a much better way of doing this would be to use RAII, but I guess that does not exist in Drupal. So I'll keep quiet.

/**
 * Implementation of hook_nodeapi().
 */
function trigger_nodeapi(&$node, $op, $a3, $a4) {
  // Keep objects for reuse so that changes actions make to objects can persist.
  static $objects;
  // Prevent recursion by tracking which operations have already been called.
  static $recursion;
  // Support a subset of operations.
  if (!in_array($op, array('view', 'update', 'presave', 'insert', 'delete')) || isset($recursion[$op])) {
    return;
  }
  $recursion[$op] = TRUE;

  $aids = _trigger_get_hook_aids('nodeapi', $op);
  if (!$aids) {

// NEED FIX HERE

    return;
  }
  $context = array(
    'hook' => 'nodeapi',
    'op' => $op,
  );

  // We need to get the expected object if the action's type is not 'node'.
  // We keep the object in $objects so we can reuse it if we have multiple actions
  // that make changes to an object.
  foreach ($aids as $aid => $action_info) {
    if ($action_info['type'] != 'node') {
      if (!isset($objects[$action_info['type']])) {
        $objects[$action_info['type']] = _trigger_normalize_node_context($action_info['type'], $node);
      }
      // Since we know about the node, we pass that info along to the action.
      $context['node'] = $node;
      $result = actions_do($aid, $objects[$action_info['type']], $context, $a4, $a4);
    }
    else {
      actions_do($aid, $node, $context, $a3, $a4);
    }
  }

// AND NEED FIX THERE
}

Subscribing. I'm running into this issue in 6.9.

Just a reroll, needs work... and a test!

subscribing. it's freaking comical that 6 was release with this bug. i'd already thought the whole triggers/actions thing was half-baked and not core worthy but this just confirms it. i'll see what i can do about some tests.

I actually think that keying by node id defeats the purpose of this check. It seems that you'd first and foremost be trying to stop the case where a node is inserted and a trigger fires and action that inserts another node causing a cascade.

I think the attached patch is sufficient to make trigger useful for the majority of cases where you'd be making bulk changes to nodes. It reorders the _trigger_get_hook_aids() call so that it is checked before the recursion flag is set and unsets the recursion flag at the only remaining exit point.

Still could use specific tests but I want the test bot's feedback.

Here's a D6 version of the same.

just realized that trigger_user() has the same problem.

uh... i'll quickly pull my foot out of my mouth. it's not a problem.

   static $recursion;
-  if (isset($recursion[$op])) {
-    return;
-  }
-  $recursion[$op] = TRUE;
 
   $aids = _trigger_get_hook_aids('node', $op);
   if (!$aids) {
     return;
   }
+
+  if (isset($recursion[$op])) {
+    return;
+  }
+  $recursion[$op] = TRUE;

Sorry, but I don't see a difference in here. I'm not sure which edge-case this patch makes work, but it can surely only be an edge-case, where $op is different for another node. Not really sure whether that wasn't already covered by the existing code.

If I'm mistaken, then please take into account that I did not read this issue, only the latest patch. If I'm mistaken, many other developers will be mistaken, and it clearly means that documentation is missing.

Got to agree that if $aids is NULL the first time, it will always be NULL and thus there is no need to unset() the $recursion. Then next time you can exit the function real quickly since $recursion is TRUE and we already know that $aids is NULL.

A comment would help, however, so we explain why we don't need to unset the $recursion flag in that case!

Thank you.
Alexis

sun, it might not make much difference at all but from the perspective of making the code look saner—you only set the value if it'll be unset on return. i'm not sure the change really needs a comment but i'm happy to put one in if that's the consensus.

sun / AlexisWilke: given it more thought and i think it's a bad idea to reuse the recursion flag just to avoid a call to _trigger_get_hook_aids(). we don't do that in _trigger_comment() or _trigger_user(). if we want it from a performance standpoint we should add a separate static value to each of those functions.

i just want to fix the bug in a way that can be backported to D6 so i'm going to leave the fix in a way that's correct and as simple as possible.

drewish,

As I mentioned in an earlier comment, a better way is to use an RAII approach. In that case, you create an object and when the object is deleted, the flag is reset to FALSE. This is the cleanest way to write this sort of a thing.

More info: http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization

In this case, the resource is what you are protecting. The static would still be the boolean flag.

In other words, the most correct approach (to my point of view) would be even slower than the existing solution which resets a flag in two places.

Thank you.
Alexis Wilke

Looks to me (although I did not look in details) that a test generates an exception and needs to be updated to take the change in account...

Please tell me this did not miss code freeze.

Let's see if the last patch works.

#12: trigger_241013.patch queued for re-testing.

Rerolled since #525540: trigger.module and includes/actions.inc need overhaul changed the name of the recursion flag variable. Added a basic test case to verify that actions are triggered for each node individually.

Some hunks of this patch are already in issue #601398: Simpletest does not allow assigning actions to triggers

Correct, the

-        'runs when' => t('A test trigger is fired'),
+        'label' => t('When a test trigger is fired'),

... is required to avoid notices during testing, and appears in a couple of patches currently in the queue. For example, #306540: Orphaned assigned actions still triggered and cannot be removed has this hunk too. It might be worthwile to create a new "quick fix" issue to fix this separately if none of the other patches get committed soon.

As far as I can see, that's the only hunk that's shared with #601398: Simpletest does not allow assigning actions to triggers, right?

Tagged

@mr.baileys yes this hunk.

Also I think that drupal_set_message() is bad approach because there's no ability to identify "how many times it was really run"

So I revert this to checking exactly to a number

Bot #33 sometimes goes crazy...

re-roll after #601398: Simpletest does not allow assigning actions to triggers

Fix is small, comes with a test, RTBC.

Committed to CVS HEAD. Thanks.

This commit holds a hunk not from this issue http://drupal.org/cvs?commit=346814
system.module should not be changed so here's a rollback for this hunk!

EDIT: patch to roll back has size 1.96 KB

I rolled back that patch. Sorry.

Already fixed :(

There's a patch in #13 but it should be re-worked

Test should be fixed after #601398: Simpletest does not allow assigning actions to triggers

Critical bump.

Committed to CVS HEAD. Thanks.

Dries,

Are we not supposed to have a back port too or was that done in D6 too?

Thank you.
Alexis

Still needs to be ported to D6.

Same patch as #13, stright backport

Somewhat similar problem and this is the closest issue I can find, I try to trigger two actions (email to Admin and send user to a page saying what to do next) when a new user registers. At the same time User Settings tries to send email to the user with their one-time login.

However when there are two trigger actions the User Settings action doesn't happen. When I to cut back to one triggered action (email to Admin) the User Settings action also works.

Is there a limit to the number of actions one trigger can do? The emails are critical but I would like to advise the user of what to do next after they register, instead of just reverting to the homepage.

@ldekay: your problem is most likely caused by the fact that the redirect action prevents further actions from firing. There are numerous issues about this, for example #682368: 'Redirect to URL' action stops execution chain.

This issue is solely about actions not firing for anything but the first node when acting on multiple nodes.

Reverting title.

ack... errr... sorry!

+1

I have an action which automatically creates translations when a new node is created. Nodes are created from a CSV import using a batch process. Trigger was only triggering on the first nodeapi insert in a given page load. With this patch it seems to be working correctly.

+1 :-)

Edit: FYI I installed rules to trigger the action instead which works perfectly.

This was actually fixed for D7.