As proposed at http://drupal.org/node/394586#comment-1637724

So lets remove unnessesary loop and simplify array

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Issue tags: +Quick fix, +actions, +trigger
FileSize
4.27 KB

Here another fix, but this one require #408434: Actions.inc: fatal error after DBTNG conversion

andypost’s picture

FileSize
4.19 KB

Forget to clean commented code

andypost’s picture

Issue tags: +Needs tests

Taxonomy trigger changed to run without foreach loop so $term as parameter needs to be tested if actions execution change it.

Is chained actions need a fresh copy of object in actions_do()? Suppose needs test because action function can receive arguments by reference.

In old implementation $object was always old not changed.

andypost’s picture

Status: Needs review » Needs work

trigger the bot

andypost’s picture

Status: Needs work » Needs review

so re-test is needed

andypost’s picture

FileSize
4.19 KB

re upload same patch

Berdir’s picture

Patch looks good, seems like a nice simplification. As it was basically my idea, I can't really rtbc it :)

mr.baileys’s picture

Status: Needs review » Needs work
  1. I know the current version also has this, but now that we're changing the code anyway, can we find a more meaninful alias for trigger_assignments, instead of aa?
    SELECT aa.aid, a.type FROM {trigger_assignments} aa
  2. -  foreach ($aids as $aid => $action_info) {
    +  foreach ($aids as $aid => $type) {
         if ($action_info['type'] != 'user') {
    -      if (!isset($objects[$action_info['type']])) {
    

    if ($action_info['type'] != 'user') should use $type instead, as $action_info is not defined.

The rest of the patch looks ok to me (visual inspection).

andypost’s picture

FileSize
4.22 KB
andypost’s picture

Status: Needs work » Needs review

ups, comment was lost on submit

1) {trigger_assignments} alias aa changed to ta
2) $action_info changed to $type

Suppose arg() node_load() should be changed to menu_get_object() maybe file another issue for this?

function _trigger_normalize_user_context($type, $account) {
  switch ($type) {
    // If an action that works on comments is being called in a user context,
    // the action is expecting a comment object. But we have no way of
    // determining the appropriate comment object to pass. So comment
    // actions in a user context are not supported.

    // An action that works with nodes is being called in a user context.
    // If a single node is being viewed, return the node.
    case 'node':
      // If we are viewing an individual node, return the node.
      if ((arg(0) == 'node') && is_numeric(arg(1)) && (arg(2) == NULL)) {
        return node_load(array('nid' => arg(1)));
      }
  }
}
mr.baileys’s picture

Status: Needs review » Reviewed & tested by the community

Looks great, and makes for more readable code... I think this is good to go.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Committed to CVS HEAD. Still needs tests so setting to 'needs work'.

andypost’s picture

As I write in #3 There should be a ideas - is data hookable or not?

andypost’s picture

Status: Fixed » Closed (fixed)
Issue tags: -Quick fix, -Needs tests, -actions, -trigger

Automatically closed -- issue fixed for 2 weeks with no activity.