Tests for abort of actions firing in a loop + trigger module API is completely broken + fix

So I wrote this test and found out that the trigger module's API was completely broken and made no sense and didn't work. Interesting bugs include user_access($module) which means the pages are not accessible for any user besides user #1, complete breakage if the module name is not the same as the hook name (some weird special casing was going on for hook_cron() I guess), and a db_select() being attempted with no fields selected. So I changed trigger_menu() along with a few other trigger functions to fix this. This probably needs to be at least partially backported to 6.x afterwards - I know it's an API change, but obviously no one is using the API or someone would have discovered it was completely broken by now.

Because user_access() is used as the access callback if none is specified, so using $module in that array had previously caused user_access($module) to be called rather than user_access('administer actions') or trigger_access_check($module).

subscribe

I'm getting a Warning: call_user_func_array() [/www/manual_php/function.call-user-func-array.html]: First argument is expected to be a valid callback, 'trigger_access_check' was given in _menu_check_access() (line 507 of /www/drupalhead/includes/menu.inc)

Line 507 is:

elseif (drupal_function_exists($callback)) {
  $item['access'] = call_user_func_array($callback, $arguments);
}

I'm going in search of the culprit.

Strange, I thought I had cleared the cache. Submitted admin/build/modules for the second time, and the warning is gone.

Also found a stray semicolon in actions_loop_test.info that was getting in the way of the hidden = TRUE, and fixed it.

I just typed a grandiose patch review, and when I hit submit, my hotel wifi died and I lost it *all*!

Bottom line is, #1 works, and works well. Only gripe is that I can't tell if Actions themselves will successfully abort in an infinite recursion of their own that happens outside of this new Simpletest module.

New patch attached that fixes two small things, but no functional code was changed from #1.

Goodnight world.

Followup to my review in #7: After talking with cwgordon this morning, and being less tired, I went back and looked at the actions_do() call to see whether the simpletests were accurately reflecting a real-world situation. I'm happy to report that the actions_do() function in actions.inc defaults to a static $stack count, but if max count is not defined elsewhere, it defaults to '35'.

Thus, all future actions defined by either core or contrib (such as Rules.module) are protected from infinite recursion, and this test mimics both a default limit of 35 and then a random limit of between 10 and 50 tries. $this->setMaxStack(mt_rand(10, 50));

Commit this baby! :)

+++ modules/simpletest/tests/actions.test	16 Aug 2009 06:12:57 -0000
@@ -61,3 +61,68 @@
+class ActionLoopTestCase extends DrupalWebTestCase {
...
+  function testActionLoop() {
...
+  protected function setMaxStack($limit) {
...
+  protected function triggerActions() {

Please add PHPDoc to all functions and class definitions. (except setUp and getInfo)

+++ modules/simpletest/tests/actions.test	16 Aug 2009 06:12:57 -0000
@@ -61,3 +61,68 @@
+  protected $limit = 35;

Let's document what this variable does, since I don't see Senpai's comment in the source code anywhere.

+++ modules/simpletest/tests/actions.test	16 Aug 2009 06:12:57 -0000
@@ -61,3 +61,68 @@
+    $result = db_query("SELECT * FROM {watchdog} WHERE type='actions_loop_test' OR type='actions' ORDER BY timestamp");

spaces before/after the =

+++ modules/simpletest/tests/actions.test	16 Aug 2009 06:12:57 -0000
@@ -61,3 +61,68 @@
+    while ($row = db_fetch_object($result)) {
+      // Ignore everything until the first message from actions_loop_test.
+      if ($row->type != 'actions_loop_test' && !$loop_started) {
+        continue;
+      }
+      elseif (!$loop_started) {
+        $loop_started = TRUE;
+      }
+      $expected_message = array_shift($expected);
+      $this->assertEqual($row->message, $expected_message, t('Expected message %expected, got %message.', array('%expected' => $expected_message, '%message' => $row->message)));
+    }

Should this be a do ... while loop? That first condition is very confusing.

+++ modules/simpletest/tests/actions.test	16 Aug 2009 06:12:57 -0000
@@ -61,3 +61,68 @@
+  protected function eraseWatchdogEntries() {
+    db_delete('watchdog')->execute();
+  }

This seems like an unnecessary bit of abstraction that just makes the test harder to follow. Let's just call db_delete() directly in the test.

+++ modules/trigger/trigger.module	16 Aug 2009 06:12:57 -0000
@@ -88,11 +56,12 @@
+    // We've already done system.module..

Two periods.

+++ modules/simpletest/tests/actions_loop_test.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,94 @@
+  $aids = _trigger_get_hook_aids('watchdog', 'run');
+  $context = array(
+    'hook' => 'watchdog',
+    'op' => 'run',
+  );
+  actions_do(array_keys($aids), $log_entry, $context);

Let's add a quick comment here. This test is our best shot at documenting how the actions/trigger system works.

+++ modules/simpletest/tests/actions_loop_test.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,94 @@
+/**
+ * Implement hook_help().
+ */
+function actions_loop_test_help() {
+  if (!empty($_GET['trigger_actions_on_watchdog'])) {
+    watchdog_skip_semaphore('actions_loop_test', 'Triggering action loop');
+  }
+}

Woah, what? Why is this code in hook_help()? Shouldn't this be in hook_init() or similar?

+++ modules/simpletest/tests/actions_loop_test.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,94 @@
+/**
+ * Implement hook_action_info().
+ */
+function actions_loop_test_action_info() {
+  return array(
+    'actions_loop_test_log' => array(
+      'description' => t('Write a message to the log.'),
+      'type' => 'system',
+      'configurable' => FALSE,
+      'hooks' => array(
+        'any' => TRUE,
+      )
+    ),
+  );
+}

Actually. Why don't we add a "real" watchdog action in system_action_info() as part of this test? Seems like it would be useful outside of this test.

This review is powered by Dreditor.

+++ modules/simpletest/tests/actions.test	16 Aug 2009 06:12:57 -0000
@@ -61,3 +61,68 @@
+class ActionLoopTestCase extends DrupalWebTestCase {
...
+  function testActionLoop() {
...
+  protected function setMaxStack($limit) {
...
+  protected function triggerActions() {

Please add PHPDoc to all functions and class definitions. (except setUp and getInfo)

Done.

+++ modules/simpletest/tests/actions.test	16 Aug 2009 06:12:57 -0000
@@ -61,3 +61,68 @@
+  protected $limit = 35;

Let's document what this variable does, since I don't see Senpai's comment in the source code anywhere.

Done.

+++ modules/simpletest/tests/actions.test	16 Aug 2009 06:12:57 -0000
@@ -61,3 +61,68 @@
+    $result = db_query("SELECT * FROM {watchdog} WHERE type='actions_loop_test' OR type='actions' ORDER BY timestamp");

spaces before/after the =

Done.

+++ modules/simpletest/tests/actions.test	16 Aug 2009 06:12:57 -0000
@@ -61,3 +61,68 @@
+    while ($row = db_fetch_object($result)) {
+      // Ignore everything until the first message from actions_loop_test.
+      if ($row->type != 'actions_loop_test' && !$loop_started) {
+        continue;
+      }
+      elseif (!$loop_started) {
+        $loop_started = TRUE;
+      }
+      $expected_message = array_shift($expected);
+      $this->assertEqual($row->message, $expected_message, t('Expected message %expected, got %message.', array('%expected' => $expected_message, '%message' => $row->message)));
+    }

Should this be a do ... while loop? That first condition is very confusing.

Refactored the code to make the logic unnecessary and was able to use the word "plethora" in the process.

+++ modules/simpletest/tests/actions.test	16 Aug 2009 06:12:57 -0000
@@ -61,3 +61,68 @@
+  protected function eraseWatchdogEntries() {
+    db_delete('watchdog')->execute();
+  }

This seems like an unnecessary bit of abstraction that just makes the test harder to follow. Let's just call db_delete() directly in the test.

Done.

+++ modules/trigger/trigger.module	16 Aug 2009 06:12:57 -0000
@@ -88,11 +56,12 @@
+    // We've already done system.module..

Two periods.

Fixed.

+++ modules/simpletest/tests/actions_loop_test.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,94 @@
+  $aids = _trigger_get_hook_aids('watchdog', 'run');
+  $context = array(
+    'hook' => 'watchdog',
+    'op' => 'run',
+  );
+  actions_do(array_keys($aids), $log_entry, $context);

Let's add a quick comment here. This test is our best shot at documenting how the actions/trigger system works.

I disagree, it's not my responsibility here to cover for the inadequacies of the actions/triggers API. I did it anyway though to eliminate excuses for marking back to code needs work.

+++ modules/simpletest/tests/actions_loop_test.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,94 @@
+/**
+ * Implement hook_help().
+ */
+function actions_loop_test_help() {
+  if (!empty($_GET['trigger_actions_on_watchdog'])) {
+    watchdog_skip_semaphore('actions_loop_test', 'Triggering action loop');
+  }
+}

Woah, what? Why is this code in hook_help()? Shouldn't this be in hook_init() or similar?

Fixed.

+++ modules/simpletest/tests/actions_loop_test.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,94 @@
+/**
+ * Implement hook_action_info().
+ */
+function actions_loop_test_action_info() {
+  return array(
+    'actions_loop_test_log' => array(
+      'description' => t('Write a message to the log.'),
+      'type' => 'system',
+      'configurable' => FALSE,
+      'hooks' => array(
+        'any' => TRUE,
+      )
+    ),
+  );
+}

Actually. Why don't we add a "real" watchdog action in system_action_info() as part of this test? Seems like it would be useful outside of this test.

No, I really think that's out of scope for this patch, which has taken quite a lot of scope on already by not only adding tests but also completely fixing trigger.module. Please see this article. ;)

+++ modules/simpletest/tests/actions.test	16 Aug 2009 06:12:57 -0000
@@ -61,3 +61,68 @@
+class ActionLoopTestCase extends DrupalWebTestCase {
...
+  function testActionLoop() {
...
+  protected function setMaxStack($limit) {
...
+  protected function triggerActions() {
...
+  protected function eraseWatchdogEntries() {

Please add PHPDoc to all functions and class definitions. (except setUp and getInfo)

I addressed this above.

+++ modules/simpletest/tests/actions.test	16 Aug 2009 06:12:57 -0000
@@ -61,3 +61,68 @@
+  protected $limit = 35;

Let's document what this variable does, since I don't see Senpai's comment in the source code anywhere.

I addressed this above.

Patch Review of #10
All of the tests now have proper comments. Good.
Congrats on removing the protected $limit = 35;. It's cleaner this way.
Shouldn't actions_loop_test_hook_info() be actions_loop_test_info()? Oh, wait, worst function name evah. I get it now. ;)
actions_loop_test_help() was successfully changed to actions_loop_test_init().
Another congrats on using the word 'plethora' in a PHPdoc comment. I highly approve of this refactoring. Plethora++

Webchick sez: "Why don't we add a "real" watchdog action in system_action_info() as part of this test?"
Cwgordon repliz: "Think of the tiny furries!"
Senpai agrees with Cwgordon.

Summary
No functional code was changed or modified in the attached patch. This patch still applies to head, and no animals were harmed in it's creation or editing. In fact, a small, furry kitteh was saved from certain disaster and one porpoise from the eastern ocean was so ecstatic it was spotted doing the backstroke.

BTW Charlie, congrats on promoting worldwide $aids awareness with the invention of this patch. :)

RTBC.

Great! Committed to HEAD. :)

I *think* we need upgrade documentation here to explain to other modules how to expose their stuff as triggers.

More importantly, this needs to be back ported to Drupal 6? It's a valid bug with the module, we can clearly tell how the API was meant to work, and it currently does not work in Drupal 6. Might be too big of a change for the stable release, though.

Is there any interest in this issue? Has this been fixed in the latest D6?

OK, that is fine. Just checking for interest.

Here is a patch incorporating the changes for trigger.admin.inc. Is it safe to assume that the changes to trigger.module are an API change and should not be incorporated?