Consider the following scenario

  1. Navigate to cron settings (/admin/config/system/cron)
  2. Change "Run cron every" to "Never"
  3. Click "Save configuration"
  4. Cron runs!

I can see why this would be. it's reasonable to assume that someone altering cron settings might want to immediately run it. But it's counterintuitive for cron to run because you told it not to. What if I have a very good reason that I want to prevent cron from running?

Solutions?

  1. Leave as-is.
    Changing this would cause more harm than good because most people want cron to run in this case. I disagree, but I'm willing to consider it

  2. Don't run cron automatically after updating cron configuration
    That's what the separate "Run cron" button is for!

  3. Code an exception for when the user selects never run cron
    This would probably be even more unexpected than the current situation, but I included it for completeness.

  4. Add a "Run cron after saving" checkbox
    Clunky name aside, this is far more predictable than #3, but probably overkill.
CommentFileSizeAuthor
#7 interdiff.txt1.02 KBamateescu
#7 2903183-7.patch2.91 KBamateescu
#3 2903183.patch2.94 KBamateescu
#3 2903183-test-only.patch1.27 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jkovell created an issue. See original summary.

jwkovell’s picture

jwkovell CreditAttribution: jwkovell commented
Issue summary: View changes
amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Version: 8.3.0 » 8.4.x-dev
Status: Active » Needs review
FileSize
2903183-test-only.patch1.27 KB
2903183.patch2.94 KB

I think that the fact that cron runs when saving the configuration form is totally unexpected and it's simply a byproduct of the fact that the cron confguration form has only one submit handler, even though it has two separate submit buttons.

So option 2. is definitely the one to choose :) Here's a patch with tests as well.

The last submitted patch, 3: 2903183-test-only.patch, failed testing. View results

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Great patch!

+++ b/core/modules/system/src/Tests/System/CronRunTest.php
@@ -105,9 +105,21 @@ public function testCronUI() {
+    sleep(1);
...
+    sleep(1);

Aren't the sleep statements in the original test just there due to the automated cron, but we don't care about that here? I guess there is a good explanation for those sleep calls

jwkovell’s picture

jwkovell CreditAttribution: jwkovell commented

The patch worked for me, amateescu. Thank you so much!

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
FileSize
2903183-7.patch2.91 KB
interdiff.txt1.02 KB

@dawehner, I guess I'm just used to add sleep() calls whenever I deal with cron, but you're right, they're not really necessary.

@jkovell, I'm glad I could help :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

@amateescu
Don't get me wrong, I love taking a nap :)

  • catch committed 0846b23 on 8.5.x 
    Issue #2903183 by amateescu, jkovell, dawehner: Don't run cron after...

catch credited catch.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Status: Fixed » Closed (fixed)

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