Cron triggers are not executed [#246096]

Working on the 6.x-1.x-dev version on Simplenews I came across the following:
An action defined to run on cron is not executed by the cron trigger ("Send single simplenews newsletter" see code below).
The defined action is listed on the cron triggers page (admin/build/trigger/cron).
When executing cron from the browser url the action is not executed.

function simplenews_action_info() {
  return array(
    'simplenews_send_newsletter_action' => array(
      'description' => t('Send single simplenews newsletter'),
      'type' => 'simplenews',
      'configurable' => TRUE,
      'hooks' => array(
        'cron' => array('run'),
        'user' => array('insert'),
      ),
    ),
...

(code will shortly be available in the latest Simplenews 6.x-1.x-dev version: http://drupal.org/node/234976)

System module defines a 'cron' hook with 'run' operation:

function system_hook_info() {
  return array(
    'system' => array(
      'cron' => array(
        'run' => array(
          'runs when' => t('When cron runs'),
        ),
      ),
    ),
  );
}

however trigger_cron() calls _trigger_get_hook_aids('cron') with an empty operation.

function trigger_cron() {
  $aids = _trigger_get_hook_aids('cron');
  $context = array(
    'hook' => 'cron',
    'op' => '',
  );
...

_trigger_get_hook_aids() now queries {trigger_assignments} for a matching hook/operation pair (cron/ but finds none, since it is defined as cron/run

The attached patch uses operation 'run' to trigger_cron(). It works but I can not see it this is the right approach.

Alternatively defining the cron hook with empty op operation in simplenews_action_info() does not help because it is not recognized as a valid definition.

I have not found any implementation of a cron trigger in D6 core or contrib modules to use as a working example.

Comment	File	Size	Author
#42	trigger-follow.patch	708 bytes	andypost
#42
#38	cron-trigger-D6.patch	760 bytes	andypost
#38
#35	cron-trigger3.patch	3.99 KB	andypost
#35
#30	cron-trigger2.patch	3.9 KB	andypost
#30
#27	cron-trigger.patch	3.93 KB	andypost
#27
#26	246096-cron-trigger_5.patch	3.69 KB	mr.baileys
#26
#20	246096-cron-trigger_4.patch	2.22 KB	WorldFallz
#20
#17	246096-cron-trigger_3.patch	3.7 KB	mr.baileys
#17
#13	246096-cron-trigger.patch	3.87 KB	mr.baileys
#13
#11	246096-cron-trigger.patch	4.16 KB	mr.baileys
#11
#7	trigger-issue-246096.patch	726 bytes	Pedro Lozano
#7
#2	actions_cron.patch	492 bytes	matt@antinomia
#2
	cron.patch	691 bytes	Sutharsan

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Sutharsan CreditAttribution: Sutharsan commented 21 April 2008 at 13:03

Status:

Active

» Needs review

The attached patch needs review.

Comment #2

matt@antinomia CreditAttribution: matt@antinomia commented 14 September 2008 at 20:15

File	Size
actions_cron.patch	492 bytes

I haven't reviewed this patch for 6.x, but in 5.x-2.5 it also appears to be broken. The attached patch fixes the issue in the same fashion as the patch in #1.

Comment #3

14 September 2008 at 20:24

Status:

Needs review

» Needs work

Patch failed to apply. More information can be found at http://testing.drupal.org/node/14360. If you need help with creating patches please look at http://drupal.org/patch/create

Comment #4

patrickharris CreditAttribution: patrickharris commented 23 September 2008 at 08:42

Thanks Sutharsan - your patch fixed this issue for me. How come this has remained broken for almost six months?

Comment #5

Sutharsan CreditAttribution: Sutharsan commented 23 September 2008 at 16:33

I guess no one uses cron triggers.

Comment #6

Pedro Lozano CreditAttribution: Pedro Lozano commented 3 November 2008 at 14:33

Version:	6.2	» 7.x-dev
Status:	Needs work	» Reviewed & tested by the community

Fixed the patch against HEAD.

This is unfixed in 6.x and 7.x.

Comment #7

Pedro Lozano CreditAttribution: Pedro Lozano commented 3 November 2008 at 14:39

File	Size
trigger-issue-246096.patch	726 bytes

I forgot the patch.

Comment #8

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 9 November 2008 at 06:17

Status:

Reviewed & tested by the community

» Needs work

It would be good to make sure we have a test case to verify that this trigger continues working as intended. There is a base to build from in modules/trigger/trigger.test that has tests for content actions currently.

Comment #9

Pedro Lozano CreditAttribution: Pedro Lozano commented 9 November 2008 at 21:55

The problem here is that (on the crontrary to content actions) there is no actions in Drupal core that are assignable to the cron trigger, so the test would be a little more complicated, maybe creating a temporary action.

Comment #10

patrickharris CreditAttribution: patrickharris commented 2 December 2008 at 23:19

It's getting close to a year, and cron actions still aren't working in Drupal 6. Sutharsan's small patch works fine for me.

Comment #11

mr.baileys

Dutch

🇧🇪 (Ghent)

CreditAttribution: mr.baileys commented 23 February 2009 at 10:28

Assigned:	Unassigned	» mr.baileys
Status:	Needs work	» Needs review

File	Size
246096-cron-trigger.patch	4.16 KB

Attached is a patch containing the fix from #7 (thanks Pedro Lozano) as well as a first attempt at a test case for this particular scenario.

The test adds a mock module, which I think is required because there are no actions availabe to be triggered by a cron run. The mock module adds an action called Cron test action. The test then tries to assign "Cron test action" to be run on the cron run trigger, and verifies that it actually did run (or not, in which case the test should fail).

First attempt at writing tests, so all input welcome (that includes you, testbot!).

Comment #12

23 February 2009 at 11:00

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #13

mr.baileys

Dutch

🇧🇪 (Ghent)

CreditAttribution: mr.baileys commented 23 February 2009 at 14:07

Status:

Needs work

» Needs review

File	Size
246096-cron-trigger.patch	3.87 KB

Fixed an error in the test function.

Comment #14

mr.baileys

Dutch

🇧🇪 (Ghent)

CreditAttribution: mr.baileys commented 25 February 2009 at 07:33

Issue tags:

+actions, +cron, +triggers

Added issue tags.

Comment #15

Pedro Lozano CreditAttribution: Pedro Lozano commented 10 March 2009 at 23:50

Status:

Needs review

» Reviewed & tested by the community

Reviewed and tested manually. The patch works and the test is correct.

Comment #16

sun

German

Karlsruhe

CreditAttribution: sun commented 24 March 2009 at 02:37

Status:

Reviewed & tested by the community

» Needs work

+    // Enable the trigger-module, as well as the 'trigger_test' mock module.
+    parent::setUp('trigger', 'trigger_test');

Please remove the comment. The modules are already stated clearly in the next line.

+    // Test 1: Assign an action to a trigger, then pull the trigger, and make sure the actions fire.

I do not see a 2nd test.

+name = "Trigger Test"
+description = "Support module for Trigger tests."

AFAIK, the quotes can be removed here.

+version = VERSION

Please remove this from the .info file.

+ * @file
+ *
+ * Mock module to aid in testing trigger.module.

Please remove the blank PHPDoc line here.

+ * Action to be assigned to the trigger "cron run" in order to
+ * be able to test cron run action triggers.

Can we find a one-line summary here and move the rest into the PHPDoc description?

+  // Set a peristent variable so the test can verify that this action
+  // was triggered.

1) typo in "persistent".
2) Can we remove the obvious parts in that comment and just stick to the important things? How + why would be interesting (instead).

Comment #17

mr.baileys

Dutch

🇧🇪 (Ghent)

CreditAttribution: mr.baileys commented 24 March 2009 at 09:14

Status:

Needs work

» Needs review

File	Size
246096-cron-trigger_3.patch	3.7 KB

Thanks for reviewing this patch: I incorporated all the changes you suggested and re-rolled the patch. I also noticed I'd neglected to add // $Id$ to the trigger_test.module file, which has also been fixed now.

Comment #18

4 April 2009 at 01:00

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #19

andypost

he/him

Russian

CreditAttribution: andypost commented 26 April 2009 at 18:34

Status:

Needs work

» Needs review

Retest and subscribe, as for me applies cleanly

Comment #20

WorldFallz CreditAttribution: WorldFallz commented 26 April 2009 at 18:38

File	Size
246096-cron-trigger_4.patch	2.22 KB

rerolled against current head, applies cleanly. Giving testbot another shot

Comment #21

WorldFallz CreditAttribution: WorldFallz commented 26 April 2009 at 18:46

tested along with the action added in #383220: Cron trigger needs an action -- also required #408434: Actions.inc: fatal error after DBTNG conversion to get past the actions error, works as intended.

Comment #22

26 April 2009 at 18:50

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #23

WorldFallz CreditAttribution: WorldFallz commented 26 April 2009 at 19:09

Status:

Needs work

» Needs review

glancing at the test results, it looks like it's probably failing because of #408434: Actions.inc: fatal error after DBTNG conversion.

Comment #24

WorldFallz CreditAttribution: WorldFallz commented 26 April 2009 at 19:11

Status:

Needs review

» Needs work

not sure how the status got changed, but there's no point in retesting.

Comment #25

Anonymous (not verified) CreditAttribution: Anonymous commented 27 April 2009 at 12:57

Comment #26

mr.baileys

Dutch

🇧🇪 (Ghent)

CreditAttribution: mr.baileys commented 4 May 2009 at 12:19

Status:

Needs work

» Needs review

File	Size
246096-cron-trigger_5.patch	3.69 KB

Looks like somewhere along the line the mock module that was required for the tests got removed from the patch. Re-added it to the re-rolled patch from #20, all tests now pass, back to needs review.

Comment #27

andypost

he/him

Russian

CreditAttribution: andypost commented 15 May 2009 at 02:44

Issue tags:

+Needs backport to D6

File	Size
cron-trigger.patch	3.93 KB

Marked as duplicate #346262: Trigger module will not run actions assigned to cron runs

new patch against current HEAD but trigger_test.module placed to modules/simpletest/tests

For 6.x proposed #383220: Cron trigger needs an action

Comment #28

andypost

he/him

Russian

CreditAttribution: andypost commented 25 May 2009 at 00:50

So where to put trigger_test.module ?
Any thoughts...

Comment #29

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 25 May 2009 at 00:53

modules/trigger/tests/trigger_test.module is the standard we're trying to adhere to, even though currently things are a total mess.

Comment #30

andypost

he/him

Russian

CreditAttribution: andypost commented 25 May 2009 at 01:57

File	Size
cron-trigger2.patch	3.9 KB

So here patch to place tests into trigger folder

Comment #31

25 May 2009 at 02:20

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #32

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 25 May 2009 at 04:46

Status:

Needs work

» Needs review

Comment #33

andypost

he/him

Russian

CreditAttribution: andypost commented 25 May 2009 at 18:29

Tests passed, so maybe someone review this to start ball

Comment #34

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 27 May 2009 at 15:02

Status:

Needs review

» Needs work

Just some minor things...

+class TriggerCronTestCase extends DrupalWebTestCase {

Needs PHPDoc.

+    // Assign an action to a trigger, then pull the trigger, and make sure the actions fire.
+    $test_user = $this->drupalCreateUser(array('administer actions'));
+    $this->drupalLogin($test_user);

That comment doesn't seem to at all match the code below it. Maybe it's more PHPDoc?

+    // Force a cron run

Needs a period.

+    $action_run = variable_get('trigger_test_system_cron_action', FALSE);
+    $this->assertTrue($action_run, t('Check that the cron run triggered the test action.'));

Could we get a one-liner comment here?

Comment #35

andypost

he/him

Russian

CreditAttribution: andypost commented 27 May 2009 at 15:44

File	Size
cron-trigger3.patch	3.99 KB

Comments fixed, waiting for testing bot...

Please review comments, English is not my native

Comment #36

andypost

he/him

Russian

CreditAttribution: andypost commented 27 May 2009 at 15:45

Status:

Needs work

» Needs review

Just a status

Comment #37

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 27 May 2009 at 16:30

Version:	7.x-dev	» 6.x-dev
Status:	Needs review	» Patch (to be ported)

Awesome. Committed to HEAD!

Needs back-port to 6.x.

Comment #38

andypost

he/him

Russian

CreditAttribution: andypost commented 27 May 2009 at 16:57

Issue tags:

+Quick fix

File	Size
cron-trigger-D6.patch	760 bytes

Here the patch for d6, quickFix

Comment #39

andypost

he/him

Russian

CreditAttribution: andypost commented 27 May 2009 at 16:58

Status:

Patch (to be ported)

» Needs review

forget to change status

Comment #40

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 27 May 2009 at 16:58

Status:

Needs review

» Reviewed & tested by the community

Since this is the same fix that was committed to HEAD, marking RTBC.

Comment #41

tobiasb

German

Berlin

CreditAttribution: tobiasb commented 27 May 2009 at 17:40

getinfo isn't static

just add "public static" to function getInfo()

  public static function getInfo() {
....

}

error msg

Strict warning: Non-static method TriggerCronTestCase::getInfo() cannot be called statically in simpletest_categorize_tests() (line 264 of ...\drupal7\modules\simpletest\simpletest.module).