Actions only triggers one action per node page load
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | trigger.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
I discovered this bug while using Actions 5.x-2.0, but am posting here so that it can be fixed in 6x first and then backported. It will need to be confirmed in 6.x. Here's my original post on dev list:
I'm using Actions 5.x-2.0 and Image 5.x-2.x-dev and I wrote a configurable action that automatically adds nodes to a nodequeue on node insert (in this case, images).
It works fine when adding one image, but when I use image_import it only runs for the first image. Image import uses image_create_node_from() which in turn uses node_submit() and node_save() just like node module, so I'm having trouble understanding why it's only running once.
pwolanin responded:
From playing with actions/triggers in 6.x I noticed that seemed
impossible to get it to work on more than one node per page load due
(I think) to internal caching. I'm guessing the same may be true in
5.x.
Not being able to run more than one action per node per page load is problematic for modules that use bulk editing/import features.
#1
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.
#2
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.
#3
This bug is also present in 7.x
#4
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.
#5
The last submitted patch failed testing.
#6
See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
#7
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.
<?php
/**
* 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
}
?>
#8
The last submitted patch failed testing.
#9
Subscribing. I'm running into this issue in 6.9.
#10
Just a reroll, needs work... and a test!
#11
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.
#12
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.
#13
Here's a D6 version of the same.
#14
just realized that trigger_user() has the same problem.
#15
uh... i'll quickly pull my foot out of my mouth. it's not a problem.
#16
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.
#17
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
#18
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.
#19
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.
#20
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
#21
The last submitted patch failed testing.
#22
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...
#23
Please tell me this did not miss code freeze.
#24
Let's see if the last patch works.