We all love to hate on trigger. But the Flag Trigger implementation needed a bit of an upgrade. We need to build the context with more interesting things so an action can do more interesting things. So this patch updates it. (Like Activity2 integration)

CommentFileSizeAuthor
#1 flag_trigger.patch963 bytessirkitree
trigger.patch962 bytesScott Reynolds

Comments

sirkitree’s picture

sirkitree commented
StatusFileSize
new flag_trigger.patch963 bytes

Ah, very nice. +1

Got a couple problems applying though. Empty line and offset fuzz.

Here it is again applied to latest dev.

quicksketch’s picture

quicksketch
he/him
commented

Would this be fixed by the (much larger) patch in #397464: Action for executing a flag? The trigger implementation was written before I really knew how trigger was supposed to work. As it is currently, it doesn't do much of anything helpful at all.

Scott Reynolds’s picture

Scott Reynolds commented

No as that patch doesn't address the need for more input into $context.

Basically, this patch adds in the 'relevant_action_objects' to the context.

quicksketch’s picture

quicksketch
he/him
commented

I'm really not enthusiastic about trying to follow the squirrely actions/trigger function chain at the moment but I'll be happy to take your word for reasonings. :-)

Before this patch the context looks like this:

array(
  'flag' => 'flag_name',
  'content_type' => 'node',
  'content_id' => 10,
  'content_title' => 'Node title',
  'content_url' => 'http://example.com/node/10',
);

After the patch it looks like this:

array(
  'flag' => $flag,
  'node' => $node,
  'account' => $account,
  'content_type' => 'node',
  'content_id' => 10,
  'content_title' => 'Node title',
  'content_url' => 'http://example.com/node/10',
);

Is it unusual that this $context variable now contains some objects and some strings? Especially considering previously $context['flag'] was a string, now it's an object.

In flag_actions.module, we have a section of code like this:

      $relevant_objects = $flag->get_relevant_action_objects($content_id);

      // Code in the middle here...

      $context['hook'] = 'flag';
      $context['type'] = $action->type;
      $context['account'] = $account;
      $context['flag'] = $flag;
      $context['flag-action'] = $flag_action;
      // We add to the $context all the objects we know about:
      $context = array_merge($relevant_objects, $context);

So we actually set flag-action to a separate item in the context, rather than bunching it into the root level. Would that help with your use-case while not breaking other existing actions? I'm not sure they're being used for anything (by trigger.module) the way they're written currently.

Scott Reynolds’s picture

Scott Reynolds commented

So the reason why i didn't nuke the current context but rather, added stuff is because I didn't want to break current trigger > actions.

But if thats not a concern, the yes the code from flag_actions would work wonderfully.

In the context array , I need objects that can be used in token_replace. So having the account, and the relevant_action_objects() achieves that aim.

Scott Reynolds’s picture

Scott Reynolds commented

missing from the trigger implementation is 'op'

http://drupal.org/node/375833

Scott Reynolds’s picture

Scott Reynolds commented
Status: Needs review » Postponed

..and $aids = _trigger_get_hook_aids($action, $action); only works when you do a flag action not an unflag action

it needs to be $aids = _trigger_get_hook_aids('flag', $action);

... Im moving all this over to the Activity integration, because I don't discover these bugs without that. It makes it a bigger patch but its all for the good. And given that I highly doubt anyone uses flags Trigger implementation anywhere, Activity would really be the first one.

#495526: Activity2 Integration

quicksketch’s picture

quicksketch
he/him
commented
Status: Postponed » Closed (duplicate)

Let's just mark this duplicate if it's being included entirely in #495526: Activity2 Integration. I'm fine with the merge because the new patch is still only 5k.