API page: https://api.drupal.org/api/drupal/core!core.api.php/function/hook_cron/8...

This code in the sample code won't do anything:

  $expires = \Drupal::state()->get('mymodule.cron_last_run', REQUEST_TIME);

as 'mymodule.cron_last_run' won't have been set as a value in state.

This is what works:

  $last_cron_run = \Drupal::state()->get('system.cron_last');

Might be worth also mentioning how to get the last cron run time in the docs?

CommentFileSizeAuthor
#11 update-cron-example-2778461-11.patch1.75 KBapaderno
#6 update-cron-example-2778461-6.patch1.75 KBapaderno
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

Actually, \Drupal::state()->get('mymodule.cron_last_run', REQUEST_TIME) does what it needs to do: It gets the value set with \Drupal::state()->set('mymodule.cron_last_run', $value), if such call was made, or REQUEST_TIME.

There is also a difference between \Drupal::state()->get('mymodule.cron_last_run', REQUEST_TIME) and \Drupal::state()->get('system.cron_last'): The first gets the last time the hook_cron() implementation of that module has been executed, while the second gets the last time cron tasks were executed. Since it could be that not all the hook_cron() implementations are invoked, for example because one of the implementations raised an exception, the two values are not necessary the same.

Probably, 'mymodule.cron_last_run' is not the best ID to use. I would rather use 'mymodule.last_check', which is similar to 'update.last_check' used from update_cron(). At least, it would not give to users the wrong impression they need to use their own state value to know the last time cron tasks were ran.

  $update_config = \Drupal::config('update.settings');
  $frequency = $update_config->get('check.interval_days');
  $interval = 60 * 60 * 24 * $frequency;
  $last_check = \Drupal::state()->get('update.last_check') ? : 0;
  if ((REQUEST_TIME - $last_check) > $interval) {
    // If the configured update interval has elapsed, we want to invalidate
    // the data for all projects, attempt to re-fetch, and trigger any
    // configured notifications about the new status.
    update_refresh();
    update_fetch_data();
  }
joachim’s picture

joachim CreditAttribution: joachim commented

Ah right, that makes sense. Though:

> Since it could be that not all the hook_cron() implementations are invoked, for example because one of the implementations raised an exception, the two values are not necessary the same.

The cron runner catches exceptions from each hook_cron() implementation, though it's true that one implementation could cause a timeout and so prevent execution of others, so as you say, implementation should use their own state value to know for certain when they last ran.

Though the default of REQUEST_TIME seems wrong to me. If I am finding out when my implementation last run, it's to do something with things that have happened since the last run (eg, all entities created since last cron run). If the state value isn't set, then I want all entities since the beginning of time, so it should be 0.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

Yes... And that is why the Aggregator module uses \Drupal::state()->get('update.last_check') ?: 0.

Also, let's not use deprecated functions. The part copied from aggregator_cron() is also old code, since it now uses (Feed::loadMultiple() and \Drupal::entityQuery() copy that code again.

joachim’s picture

joachim CreditAttribution: joachim commented
Title: Use real working code for getting last cron run in hook_cron() sample code » Update hook_cron() sample code

Wouldn't that be cleaner like this:

\Drupal::state()->get('update.last_check', 0);

Updating issue title to expand it to generally cleaning up the sample code here.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
Status: Active » Needs review
FileSize
update-cron-example-2778461-6.patch1.75 KB

This is my patch based on the suggestions.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: update-cron-example-2778461-6.patch, failed testing.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
Status: Needs work » Reviewed & tested by the community

It is strange: It says the patch failed the test, but I see all the tests passing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
Status: Reviewed & tested by the community » Needs review
FileSize
update-cron-example-2778461-11.patch1.75 KB
joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 493eac1 to 8.3.x and 76dbd64 to 8.2.x and 1aee773 to 8.1.x. Thanks!

  • alexpott committed 493eac1 on 8.3.x 
    Issue #2778461 by kiamlaluno, joachim: Update hook_cron() sample code

  • alexpott committed 76dbd64 on 8.2.x 
    Issue #2778461 by kiamlaluno, joachim: Update hook_cron() sample code

(...

  • alexpott committed 1aee773 on 8.1.x 
    Issue #2778461 by kiamlaluno, joachim: Update hook_cron() sample code

(...

Status: Fixed » Closed (fixed)

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