#296322 by cwgordon7 andSenpai: Fix horribly broken trigger module and add tests so we don't do it anymore.

modules/simpletest/tests/actions.test

@@ -6,7 +6,7 @@ class ActionsConfigurationTestCase extends DrupalWebTestCase {
     return array(
       'name' => 'Actions configuration',
       'description' => 'Tests complex actions configuration by adding, editing, and deleting a complex action.',
-      'group' => 'System',
+      'group' => 'Actions',
     );
   }
 
@@ -61,3 +61,65 @@ class ActionsConfigurationTestCase extends DrupalWebTestCase {
     $this->assertFalse($exists, t('Make sure the action is gone from the database after being deleted.'));
   }
 }
+
+/**
+ * Test actions executing in a potential loop, and make sure they abort properly.
+ */
+class ActionLoopTestCase extends DrupalWebTestCase {
+  public static function getInfo() {
+    return array(
+      'name' => 'Actions executing in a potentially infinite loop',
+      'description' => 'Tests actions executing in a loop, and makes sure they abort properly.',
+      'group' => 'Actions',
+    );
+  }
+
+  function setUp() {
+    parent::setUp('dblog', 'trigger', 'actions_loop_test');
+  }
+
+  /**
+   * Set up a loop with 10-50 recursions, and see if it aborts properly.
+   */
+  function testActionLoop() {
+    $user = $this->drupalCreateUser(array('administer actions'));
+    $this->drupalLogin($user);
+
+    $hash = md5('actions_loop_test_log');
+    $edit = array('aid' => $hash);
+    $this->drupalPost('admin/structure/trigger/actions_loop_test', $edit, t('Assign'));
+
+    // Delete any existing watchdog messages to clear the plethora of
+    // "Action added" messages from when Drupal was installed.
+    db_delete('watchdog')->execute();
+    $this->triggerActions();
+
+    // Clear the log again for another test, this time with a random maximum.
+    db_delete('watchdog')->execute();
+    variable_set('actions_max_stack', mt_rand(10, 50));
+    $this->triggerActions();
+  }
+
+  /**
+   * Create an infinite loop by causing a watchdog message to be set,
+   * which causes the actions to be triggered again, up to default of 35 times.
+   */
+  protected function triggerActions() {
+    $this->drupalGet('<front>', array('query' => array('trigger_actions_on_watchdog' => TRUE)));
+    $expected = array();
+    $expected[] = 'Triggering action loop';
+    for ($i = 1; $i <= variable_get('actions_max_stack', 35); $i++) {
+      $expected[] = "Test log #$i";
+    }
+    $expected[] = 'Stack overflow: too many calls to actions_do(). Aborting to prevent infinite recursion.';
+
+    $result = db_query("SELECT * FROM {watchdog} WHERE type = 'actions_loop_test' OR type = 'actions' ORDER BY timestamp");
+    $loop_started = FALSE;
+    while ($row = db_fetch_object($result)) {
+      
+      $expected_message = array_shift($expected);
+      $this->assertEqual($row->message, $expected_message, t('Expected message %expected, got %message.', array('%expected' => $expected_message, '%message' => $row->message)));
+    }
+    $this->assertTrue(empty($expected), t('All expected messages found.'));
+  }
+}

modules/trigger/trigger.admin.inc

@@ -23,11 +23,13 @@ function trigger_assign($type = NULL) {
 
   $build = array();
   $hooks = module_invoke_all('hook_info');
-  foreach ($hooks as $module => $hook) {
+  foreach ($hooks as $module => $module_hooks) {
-    if (isset($hook[$type])) {
+    if ($module == $type) {
-      foreach ($hook[$type] as $op => $description) {
+      foreach ($module_hooks as $hook => $data) {
-        $form_id = 'trigger_' . $type . '_' . $op . '_assign_form';
+        foreach ($data as $op => $description) {
-        $build[$form_id] = drupal_get_form($form_id, $type, $op, $description['runs when']);
+          $form_id = 'trigger_' . $hook . '_' . $op . '_assign_form';
+          $build[$form_id] = drupal_get_form($form_id, $hook, $op, $description['runs when']);
+        }
       }
     }
   }

modules/trigger/trigger.module

@@ -39,47 +39,15 @@ function trigger_menu() {
     'title' => 'Triggers',
     'description' => 'Tell Drupal when to execute actions.',
     'page callback' => 'trigger_assign',
-    'access callback' => 'trigger_access_check',
+    'access arguments' => array('administer actions'),
-    'access arguments' => array('node'),
-  );
-  // We don't use a menu wildcard here because these are tabs,
-  // not invisible items.
-  $items['admin/structure/trigger/node'] = array(
-    'title' => 'Content',
-    'page callback' => 'trigger_assign',
-    'page arguments' => array('node'),
-    'access callback' => 'trigger_access_check',
-    'access arguments' => array('node'),
-    'type' => MENU_LOCAL_TASK,
-  );
-  $items['admin/structure/trigger/user'] = array(
-    'title' => 'Users',
-    'page callback' => 'trigger_assign',
-    'page arguments' => array('user'),
-    'access callback' => 'trigger_access_check',
-    'access arguments' => array('user'),
-    'type' => MENU_LOCAL_TASK,
-  );
-  $items['admin/structure/trigger/comment'] = array(
-    'title' => 'Comments',
-    'page callback' => 'trigger_assign',
-    'page arguments' => array('comment'),
-    'access callback' => 'trigger_access_check',
-    'access arguments' => array('comment'),
-    'type' => MENU_LOCAL_TASK,
-  );
-  $items['admin/structure/trigger/taxonomy'] = array(
-    'title' => 'Taxonomy',
-    'page callback' => 'trigger_assign',
-    'page arguments' => array('taxonomy'),
-    'access callback' => 'trigger_access_check',
-    'access arguments' => array('taxonomy'),
-    'type' => MENU_LOCAL_TASK,
   );
+
+  // Explicitly define the system menu item so we can label it "cron" rather
+  // than "system".
   $items['admin/structure/trigger/cron'] = array(
     'title' => 'Cron',
     'page callback' => 'trigger_assign',
-    'page arguments' => array('cron'),
+    'page arguments' => array('system'),
     'access arguments' => array('administer actions'),
     'type' => MENU_LOCAL_TASK,
   );
@@ -88,11 +56,12 @@ function trigger_menu() {
   // their hooks and have actions assignable to them.
   $hooks = module_invoke_all('hook_info');
   foreach ($hooks as $module => $hook) {
-    // We've already done these.
+    // We've already done system.module.
-    if (in_array($module, array('node', 'comment', 'user', 'system', 'taxonomy'))) {
+    if ($module == 'system') {
       continue;
     }
     $info = db_select('system')
+      ->fields('system', array('info'))
       ->condition('name', $module)
       ->execute()
       ->fetchField();
@@ -102,7 +71,7 @@ function trigger_menu() {
       'title' => $nice_name,
       'page callback' => 'trigger_assign',
       'page arguments' => array($module),
-      'access arguments' => array($module),
+      'access arguments' => array('administer actions'),
       'type' => MENU_LOCAL_TASK,
     );
   }
@@ -118,13 +87,6 @@ function trigger_menu() {
   return $items;
 }
 
-/**
- * Access callback for menu system.
- */
-function trigger_access_check($module) {
-  return (module_exists($module) && user_access('administer actions'));
-}
-
 /**
  * Get the aids of actions to be executed for a hook-op combination.
  *