Optimize automatic cron subscriber by moving automatic cron to a module

I was wondering whether it would be a good idea to no register that subscriber, unless you really need it. This could be for example achieved by moving the automatic cron into its own module.

A module would be interesting, can we still do this? We could also dynamically register this, with the downside that changing the setting needs to rebuild the container. We did move page cache to a module to avoid exactly that (among other reasons).

If it's a new module, should it be enabled in the standard installation profile?

I'd like to avoid dynamically rebuilding the container - I know we do this for languages but I'm still holding out hope that we'll be able to factor that away somehow and restrict it to module install/uninstall.

I agree that we should try to avoid container rebuilds as much as possible. Moving to a module is an elegant way around that in the sense, that people are more aware what they are doing, when they install a new module.

Let's try it out.

interdiff doesn't make sense.

Corrected missing ':'

State key returned back in order to pass tests.

+++ b/core/modules/automated_cron/automated_cron.module
@@ -0,0 +1,7 @@
+function automated_cron_help() {
+}

Let's ensure that we fill this out :)

The only remaining bet in order to fix the test is to enable the new module in \Drupal\system\Tests\System\CronRunTest::testAutomaticCron.

@dawehner Thanks for helping out to fix all tests.

Basic help text (or at least start of it) added.

Cron settings page is now defined in core\modules\system\src\Form\CronForm.php. Should we move autocron related parts to newly created module as well or disable cron settings if autocron module is completely disabled?

+++ b/core/modules/automated_cron/src/EventSubscriber/AutomaticCron.php
@@ -59,20 +57,15 @@ public function __construct(CronInterface $cron, ConfigFactoryInterface $config_
+    $threshold = $this->config->get('threshold.autorun');

Yeah we should move that into a configuration on this module itself ... so we need to migrate the data via hook_update_N() in system and install the module there

Here is first attempt where automatic cron configuration included in the module (with use of hook_form_FORM_ID_alter() and custom submit callback).

System update hook included.

+++ b/core/modules/system/system.install
@@ -1085,3 +1085,11 @@ function system_schema() {
+/**
+ * Enable automatic cron module.
+ */
+function system_update_8000() {
+  $module_list = array('automated_cron');
+  \Drupal::service('module_installer')->install($module_list, FALSE);
+}
 

Note: you need to also write a test for it.

Initial test added.

1. +++ b/core/modules/automated_cron/automated_cron.module
   @@ -0,0 +1,70 @@
   +  $cron_config = \Drupal::config('system.cron');
   ...
   +    '#options' => array(0 => t('Never')) + array_map(array(Drupal::service('date.formatter'), 'formatInterval'), array_combine($options, $options)),
   

   If we moved the functionality to a module that can be disabled, let's drop the "never" option. Disabling the module is the new "Never".

2. +++ b/core/modules/automated_cron/automated_cron.module
   @@ -0,0 +1,70 @@
   +function automated_cron_settings_submit($form, FormStateInterface $form_state) {
   

   Maybe array $form?

3. +++ b/core/modules/automated_cron/src/EventSubscriber/AutomaticCron.php
   @@ -34,10 +34,10 @@ class AutomaticCron implements EventSubscriberInterface {
   +  * Drupal\Core\State\StateInterface;
   

   @var \Drupal\Core\State\StateInterface?

4. +++ b/core/modules/system/src/Tests/System/UpdateHookTest.php
   @@ -0,0 +1,69 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static $modules = ['system'];
   

   The system module is always enabled regardless of installation profile.

5. +++ b/core/modules/system/system.install
   @@ -1085,3 +1085,11 @@ function system_schema() {
   +/**
   + * Enable automatic cron module.
   + */
   +function system_update_8001() {
   +  $module_list = array('automated_cron');
   +  \Drupal::service('module_installer')->install($module_list, FALSE);
   +}
   

   I don't think we want to enable the module in any circumstance. I would test the config cron_safe_threshold and I would enable the module only if cron_safe_threshold > 0.

+++ b/core/modules/automated_cron/automated_cron.module
@@ -0,0 +1,70 @@
+  $form['cron'] = array(
...
+  );
+  $options = array(3600, 10800, 21600, 43200, 86400, 604800);
+  $form['cron']['cron_safe_threshold'] = array(
...
+  );
...
+  $form['actions']['submit'] = array(
...
+  );

+++ b/core/modules/system/system.install
@@ -1085,3 +1085,11 @@ function system_schema() {
+  $module_list = array('automated_cron');

Because this is a new module, let's use the short syntax for arrays. [] instead of array().

I think a Never option is still useful when you want to disable it temporarily or on some environment. That's where #6 is wrong. We don't save anything for sites that have the module enabled as we still need to check how often we need to do it. Checking for never there is no overhead.

I agree with berdir that being able to temporary disable it is not a bad idea.

Added the new module to core/composer.json.

Smaller patch file with git diff -M.

+++ b/core/modules/automated_cron/automated_cron.info.yml
--- /dev/null
+++ b/core/modules/automated_cron/automated_cron.module

+++ b/core/modules/automated_cron/automated_cron.module
@@ -0,0 +1,70 @@
+  $cron_config = \Drupal::config('system.cron');

I think we should the related config from system module to automated_cron.

1. +++ b/core/modules/automated_cron/src/EventSubscriber/AutomaticCron.php
   @@ -34,10 +34,10 @@ class AutomaticCron implements EventSubscriberInterface {
   +  * Drupal\Core\State\StateInterface;
   

   "@var \" missing

2. +++ b/core/modules/automated_cron/src/EventSubscriber/AutomaticCron.php
   @@ -47,8 +47,6 @@ class AutomaticCron implements EventSubscriberInterface {
   -   * @param \Drupal\Core\State\StateInterface $state
   -   *   The state key value store.
   ...
      public function __construct(CronInterface $cron, ConfigFactoryInterface $config_factory, StateInterface $state) {
   

   state is passed and used

3. +++ b/core/modules/automated_cron/src/EventSubscriber/AutomaticCron.php
   @@ -59,20 +57,15 @@ public function __construct(CronInterface $cron, ConfigFactoryInterface $config_
   +    if ($threshold > 0) {
   

   how could it be lees then 0?

4. +++ b/core/modules/system/system.install
   @@ -1203,3 +1203,10 @@ function system_update_8001(&$sandbox = NULL) {
   +  $module_list = ['automated_cron'];
   +  \Drupal::service('module_installer')->install($module_list, FALSE);
   

   useless variable

Addressed points:

• Changed the human-readable name of the module to Automated cron to be consistent with the machine name (automated_cron).
• #33: Moved the threshold.autorun from system.cron to the new automated_cron config.
• Cron Settings form is now a extending only FormBase because without automated_cron module enabled, no more handles a config vars.
• When automated_cron module is disabled, the Cron Settings form displays a help message that tells admin he can enable the module.
• #34.1, #34.2, #34.4.

@andypost, #34.3: Se #29 — the autorun can take the 0 value.

Sorry, patches from #35 were incomplete. Consider the comment from #35together with these patches.

And the interdiff :)

1. I think we should delete the system.cron config object in the upgrade path.

2. Tagging "Needs documentation" so we can see whether the hook_help() implementation passes @jhodgdon's scrutiny. I thought there was a "Needs documentation review" tag but apparently there isn't, so not sure if that's the right one.

@tstoeckler

I think we should delete the system.cron config object in the upgrade path.

We only split a key from system.cron and move it to automated_cron.cron. We need both.

@claudiu.cristea Right, sorry about that. But we should clear the obsolete key in system.cron, no?

Nitpick.

+++ b/core/modules/system/system.install
@@ -1203,3 +1203,22 @@ function system_update_8001(&$sandbox = NULL) {
+  // Remove the 'autorun' key in system module config.
+  $threshold = $system_cron->get('threshold');
+  unset($threshold['autorun']);
+  $system_cron->set('threshold', $threshold)->save();

config has a clear() method to delete stuff.

also $system_config seems to be using \Drupal::config(), which is immutable?

Both this and the one above should pass TRUE to save() to avoid config casting/validation.

1. +++ b/core/modules/automated_cron/automated_cron.module
   @@ -0,0 +1,71 @@
   +  drupal_set_message(t('The configuration options have been saved.'));
   

   Do we really need this additional message for the altered in value? I don't see that we remove any usage of drupal_set_message() in this patch so we are effectively adding one. This feels wrong :)

2. +++ b/core/modules/system/src/Form/CronForm.php
   @@ -110,38 +111,19 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    if (!$this->moduleHandler->moduleExists('automated_cron')) {
   +      $form['cron'] = array(
   +        '#markup' => $this->t('Enable the <em>Automated cron</em> module to allow cron execution at the end of a request.'),
   +      );
   +    }
    
   

   This sounds like a good compromise for site builder experience vs. separation of concerns.

Do we really need this additional message for the altered in value? I don't see that we remove any usage of drupal_set_message() in this patch so we are effectively adding one. This feels wrong :)

In fact we are removing that message by downgrading the form from ConfigFormBase to FormBase and the user will see no feedback on his action.

Addressed #47. Responded in #49 on #48 about message.

Awesome work, I have one more minor nitpick:

downgrading the form from ConfigFormBase to FormBase

Good point, that means we should also bring over the:

    // By default, render the form using theme_system_config_form().
    $form['#theme'] = 'system_config_form';

from ConfigFormBase to allow it to be styled consistently with other config forms.

Ok with #51.

But now, alternatively, I feel that there's a more cleaner way to alter the form, without using hook_form_FORM_ID_alter(). I would:

1. Alter the route system.cron_settings ('/admin/config/system/cron') to point to a new form callback owned by the new module.
2. The new form class is extending \Drupal\Core\Form\ConfigFormBase
3. The new form class will have the old form class (\Drupal\system\Form\CronForm) injected just to grab all needed elements (e.g. ::buildForm() result).

This would make unnecessary all the redundancies added from \Drupal\Core\Form\ConfigFormBase into our FormBase form.

Right now I think I like this approach more than #52. Seems more clean to me.

@catch, #52 is like that.

But the CronForm becomes a simple FormBase (is losing the config functionality). This is forcing us to add some redundancies to the form. IMO #53 is much more cleaner because is using what ConfigFormBase already provides.

Hiding solution from #53. #52 needs review.

Rerolled #52.

Patch doesn't apply to 8.0.x head. Adding needs reroll tag

Uploading the re-rolled patch here.

Looking into the test failures.

There we go.

1. +++ b/core/modules/automated_cron/automated_cron.module
   @@ -0,0 +1,74 @@
   +      $output .= '<dt>' . t('Disabling the automatic cron') . '</dt>';
   +      $output .= '<dd>' . t('If you want to disable the automated cron, change the "Run cron every" value to "Never".')  . '</dd>';
   

   Worth mentioning that you should disable the module if you have a separate cron job running so that you actually benefit from the improved performance?

2. +++ b/core/modules/automated_cron/automated_cron.services.yml
   --- /dev/null
   +++ b/core/modules/automated_cron/config/install/automated_cron.cron.yml
   

   normally we use ..settings.yml if there's just a single file, not sure? Especially since the module name already contains cron.

This is not it yet.

Finally ...

RTBC - but there are test fails remaining.

Seems reasonable to me, and is inline with what we did with Page Cache. Just so long as it's enabled by default in Standard (it is) and has hook_help() (it does), works for me.

Just a simple reroll

Increase MOAR

Reviewing, and addressing most of my nits immediately.

1. +1 to @Berdir's remarks in #74. Fixed those.

2. +++ b/core/modules/automated_cron/automated_cron.info.yml
   @@ -0,0 +1,6 @@
   +description: 'Provides an automated cron by executing it at the end of a request.'
   
   +++ b/core/modules/automated_cron/automated_cron.module
   @@ -0,0 +1,74 @@
   + * Provides an automated cron by executing it at the end of a request.
   ...
   +      $output .= '<p>' . t('The automated cron system runs the Drupal cron operation using normal browser/page requests instead of having to set up a separate cron job. Automatic cron module checks at the end of each Drupal request when cron operation was last ran and, if it has been too long since last run, it executes the cron tasks as part of that request. For more information, see the <a href="!docs">cron tutorial on drupal.org</a>.', array('!docs' => 'https://www.drupal.org/cron'))  . '</p>';
   

   by executing it after a response is sent would be clearer I think. Thoughts?

3. +++ b/core/modules/automated_cron/automated_cron.module
   @@ -0,0 +1,74 @@
   +      $output .= '<dd>' . t('If you want to disable the automated cron, change the "Run cron every" value to "Never".')  . '</dd>';
   

   … wouldn't you actually want to uninstall this module instead?

   Fixed.

4. +++ b/core/modules/automated_cron/automated_cron.module
   @@ -0,0 +1,74 @@
   +  $form['cron'] = [
   ...
   +  $form['cron']['autorun'] = [
   

   Wouldn't it be better if this were keyed under 'automated_cron' instead?

   Fixed.

5. +++ b/core/modules/automated_cron/automated_cron.module
   @@ -0,0 +1,74 @@
   +    '#options' => [0 => t('Never')] + array_map([Drupal::service('date.formatter'), 'formatInterval'], array_combine($options, $options)),
   

   Missing leading backslash.

   Fixed.

6. +++ b/core/modules/automated_cron/automated_cron.services.yml
   @@ -0,0 +1,6 @@
   +    class: Drupal\automated_cron\EventSubscriber\AutomaticCron
   

   Everywhere it's called "automated" but here "automatic".

   Fixed.

7. +++ b/core/modules/automated_cron/config/install/automated_cron.cron.yml
   @@ -0,0 +1,2 @@
   +threshold:
   +  autorun: 0
   

   Wouldn't it be better to only have a single value here, with the name interval or something like that?

   Fixed.

8. +++ b/core/modules/automated_cron/config/schema/automated_cron.schema.yml
   @@ -0,0 +1,13 @@
   +      label: 'Thresholds'
   

   Multiple? There can only ever be one?

   If multiple, then the key should also match this?

   Fixed by doing the above.

9. +++ b/core/modules/config/tests/config_override_test/config/install/system.cron.yml
   @@ -1,5 +1,4 @@
    threshold:
   -  autorun: 0
      requirements_warning: 172800
      requirements_error: 1209600
   

   This explains the config structure and naming scheme in the automated_cron module, but it's not a valid justification IMO.

10. +++ b/core/modules/system/src/Tests/System/CronRunTest.php
    --- /dev/null
    +++ b/core/modules/system/src/Tests/Update/AutomatedCronUpdateWithAutomaticCronTest.php
    
    +++ b/core/modules/system/src/Tests/Update/AutomatedCronUpdateWithAutomaticCronTest.php
    @@ -0,0 +1,36 @@
    +   * Ensures that automatic cron module isn installed and the config migrated.
    ...
    +    $this->assertTrue(isset($module_data['automated_cron']), 'The automatic cron module was installed.');
    
    +++ b/core/modules/system/src/Tests/Update/AutomatedCronUpdateWithoutAutomaticCronTest.php
    @@ -0,0 +1,36 @@
    + * Ensures that the automatic cron module is not installed on update.
    ...
    +   * Ensures that automatic cron module isn't installed and the config migrated.
    ...
    +    $this->assertFalse(isset($module_data['automated_cron']), 'The automatic cron module was not installed.');
    
    +++ b/core/modules/system/system.install
    @@ -1808,5 +1808,27 @@ function system_update_8011() {
    + * Enable automatic cron module and move the config into it.
    

    s/Automatic/Automated/

    Fixed.

Wouldn't it be better if this were keyed under 'automated_cron' instead?

I don't care

by executing it after a response is sent would be clearer I think. Thoughts?

Good suggestion.

Oopsie in schema.

Addressed #93.2 / #96.2.

Sigh, I uploaded the right interdiff but the wrong patch in #98. Here's what I actually intended to upload.

I have no idea how to fix that last failure, that line 85 in UpdatePathTestBaseFilledTest.

dawehner and I suspected it's related to running cron triggering some translation stuff. But it's specifically testing the translation of the site slogan. The site slogan lives in the "site branding" block (since September 10, #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo)). But the tests in HEAD are still passing. And cron doesn't seem involved in any way in config translation. And the other blocks (menu block, user login block) are translated just fine.

Furthermore, no slogan at all shows up.

The translation string that we check for (drupal Spanish) lives in the DB dump. I have no idea at all how this patch could possibly break that. Any ideas?

Wim asked me to review documentation. Looks mostly good! A few small items to address:

1. +++ b/core/modules/automated_cron/automated_cron.info.yml
   @@ -0,0 +1,6 @@
   +name: Automated cron
   

   The capitalization should be:

   Automated Cron

   for a module name. Also I think we normally put multiple-word module names in ' ' quotes in yml files?

2. +++ b/core/modules/automated_cron/automated_cron.info.yml
   @@ -0,0 +1,6 @@
   +description: 'Provides an automated cron by executing it after a response is sent.'
   

   This description is used on the Extend (admin/modules) page. It needs to be understandable to non-programmers. I think this probably needs a better description.

   Also grammar: "it" does not have a clear antecedent...

   So my suggestion would be:

   Provides an automated way to run cron jobs, by executing them at the end of a page request.

3. +++ b/core/modules/automated_cron/automated_cron.module
   @@ -0,0 +1,74 @@
   +      $output .= '<p>' . t('The automated cron system runs the Drupal cron operation using normal browser/page requests instead of having to set up a separate cron job. Automated cron module checks at the end of each Drupal response when cron operation was last ran and, if it has been too long since last run, it executes the cron tasks after sending a response. For more information, see the <a href="!docs">cron tutorial on drupal.org</a>.', array('!docs' => 'https://www.drupal.org/cron'))  . '</p>';
   

   This needs to start with:

   The Automated Cron module ...

   And anytime you refer to the module by name, it should say:

   ... the Automated Cron module ...

   (note capitalization and always use the actual module name from the .info.yml file)

4. Same line:
   Also see note on the yml file. Do not use programming or overly technical terms like "response".
5. Same line:
   Also we don't normally want to use the word "Drupal" in help or UI text. Refer to "your site" or "the site" or something like that instead.
6. Same line:
   The end of the About should always be something like:

   For more information, see the [a href=https://www.drupal.org/documentation/modules/SHORT_NAME]on-line documentation for the Foo Bar module[/a] as well. This page will need to be created. If this link text is not present, a test will fail once #2488032: Integrate help test into module uninstall test goes in.

7. +++ b/core/modules/automated_cron/automated_cron.module
   @@ -0,0 +1,74 @@
   +      $output .= '<dd>' . t('On the <a href="!cron-settings">Cron page</a>,  you can manage the automated cron settings. The default frequency is every three hours.', array('!cron-settings' => \Drupal::url('system.cron_settings')))  . '</dd>';
   

   nitpick: there are two spaces after the comma in the first sentence.

   Also I don't think you need to put in the help what the default setting is... but maybe just say "... you can set the frequency for running cron jobs", since I think that is the only setting?

   Also maybe it should document what the "frequency" means, because it might not be clear.

8. +++ b/core/modules/automated_cron/automated_cron.module
   @@ -0,0 +1,74 @@
   +      $output .= '<dd>' . t('If you want to disable the automated cron, it is recommended to uninstall the Automated cron module (when you have a cron job running on your server already, the module is pointless, and uninstalling it improves performance). Alternatively, change the "Run cron every" value to "Never", for example when you need to temporarily disable it.')  . '</dd>';
   

   Um... so this is clear enough but it's a bit conversational (e.g., "pointless").

   How about a slight rewording:

   To disable automated cron, the recommended method is to uninstall the module, to reduce site overhead. If you only want to disable it temporarily, you can set the frequency to Never on the Cron page, and then change the frequency back when you want to start it up again.

9. +++ b/core/modules/automated_cron/automated_cron.module
   @@ -0,0 +1,74 @@
   + * Implements hook_form_FORM_ID_alter().
   

   I think normally when we document an implementation of this hook, we'd add

   for the system_cron_settings() form.

   at the end of the sentence?

10. +++ b/core/modules/automated_cron/src/EventSubscriber/AutomatedCron.php
    @@ -36,43 +36,38 @@ class AutomaticCron implements EventSubscriberInterface {
    +   * Construct a new automated cron runner.
    

    Construct => Constructs

11. +++ b/core/modules/search/src/Tests/SearchMultilingualEntityTest.php
    @@ -41,8 +41,12 @@ protected function setUp() {
    +    // Make sure that Automated cron is disabled.
    +    /** @var \Drupal\Core\Extension\ModuleHandlerInterface $module_handler */
    +    $module_handler = $this->container->get('module_handler');
    +    if ($module_handler->moduleExists('automated_cron')) {
    +      $this->config('automated_cron.settings')->set('interval', 0)->save();
    

    Is this module check needed? It seems like the test would know whether the automated cron module is enabled or not in its testing profile.

12. +++ b/core/modules/system/src/Form/CronForm.php
    @@ -111,38 +112,19 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +        '#markup' => $this->t('Enable the <em>Automated cron</em> module to allow cron execution at the end of a request.'),
    

    Module name should be

    Automated Cron
    [note caps]

13. I believe that system_help() documents automated cron already. This will need to be removed from that help text, and this patch doesn't currently do that.

The module also probably needs an entry in MAINTAINERS.txt?

#103:

Many, many thanks for your review! :)

1. Fixed. (Good point about the quotes!)
2. Fixed.
3. Fixed.
4. Your proposal includes "page request". I think it is conceptually much clearer to say "this thing runs after the server has responded" than saying "at the end of a page request", which is also actually wrong. But I went with the wording you proposed in #2 for now.
5. Fixed.
6. Fixed.
7. Created that page and linked to it.
8. Fixed the spaces. Applied your suggestion.
9. Applied your suggestion.
10. Fixed.
11. Great point, removed.
12. Fixed.

#105: Done!

+++ b/core/modules/quickedit/js/util.js
@@ -125,7 +125,7 @@
-          Drupal.quickedit.util.networkErrorModal(Drupal.t('Sorry!'), message);
+          Drupal.quickedit.util.networkErModal(Drupal.t('Sorry!'), message);

Ugh this snuck in from another issue, should be removed. Will not cause test failures though, and this patch isn't green yet anyway. The next reroll should remove it.

The patch was green till #53 (August 5). After that something was committed in core that breaks the update. IMO there's a hidden bug somewhere. Very hard to debug. I tried with no chance.

#115: indeed, but it must somehow be related to cron, or it'd have failed a long time ago.

Regarding the wording for the module description, calling it "server response" is fine with me. I just wasn't sure if "response" (it didn't have "server" in there) was clear. The proposed text I reviewed was:

Provides an automated cron by executing it after a response is sent.

So maybe:

Provides an automated way to run cron jobs, by executing them after a server request response...
hm....
Provides an automated way to run cron jobs, by executing them after a server response.

maybe that? I don't want to be inaccurate just so that we don't use technical jargon.

#117: Thanks! :) I'll ensure that happens in one of the next rerolls, as soon as we figure out the root cause of this test failure.

Been trying to dig into the fail as well, but no luck so far either - some remarks though on the current code.

1. +++ b/core/modules/automated_cron/automated_cron.module
   @@ -0,0 +1,74 @@
   +  // Theme this form as a config form.
   +  $form['#theme'] = 'system_config_form';
   

   Probably not needed either? ConfigFormBase takes care of that

2. +++ b/core/modules/automated_cron/automated_cron.module
   @@ -0,0 +1,74 @@
   +  drupal_set_message(t('The configuration options have been saved.'));
   

   Maybe this isn't really needed, this is set in ConfigFormBase::submitForm as well.

3. +++ b/core/modules/quickedit/js/util.js
   @@ -125,7 +125,7 @@
   -          Drupal.quickedit.util.networkErrorModal(Drupal.t('Network problem!'), message);
   +          Drupal.quickedit.util.networkErModal(Drupal.t('Network problem!'), message);
   

   Unintentional change ? networkErModal doesn't exist

Ok, nevermind point 1 and 2 - was looking at the wrong form - kind of weird to see CronForm still in system though, should we just move it ?

@dawehner asked on IRC if we even know how is this supposed to be translated. That one is pretty easy. If you look at drupal-8.filled.standard.php.gz it has this snippet:

->values(array(
  'collection' => 'language.es',
  'name' => 'system.site',
  'data' => 'a:1:{s:6:"slogan";s:14:"drupal Spanish";}',
))

So the system.site config has a variant in the language.es collection that has this slogan. The test in UpdatePathTestBaseFilledTest::testUpdatedSite() checks if there is output of that on the page. So it is a pretty integrated test so to say of whether the config override was still there, the block was still there, the block's config still said the slogan should be printed and the block was printed in the right language after all.

My suspicion is that this patch introduces a module install in the update path and that clears caches that expose failures with the block update path that should have been there before, but this is just a hunch. There does not seem to be caching data in the dump... However I base that on @wimleers observation that no slogan was displayed at all. Grepping for use_site_slogan, it does not seem like that there is an update path to migrate that setting. Looking at system_update_8006(), it does not set that setting for the newly created block, so I assume old sites did not get the slogan in that block.

So the problem is with locale_modules_installed() and in particular this hunk in locale_system_update():

    // Construct a batch to update configuration for all components. Installing
    // this component may have installed configuration from any number of other
    // components. Do this even if import is not enabled because parsing new
    // configuration may expose new source strings.
    \Drupal::moduleHandler()->loadInclude('locale', 'bulk.inc');
    if ($batch = locale_config_batch_update_components([])) {
      batch_set($batch);
    }

@alexpott++

So locale_system_update() just updates config (translations) based on data in locale. The database dump for the update tests does not have the slogan translation in the locale tables (locales_source, locales_target). Looking at the dump however, the source slogan is empty. If you look at LocaleConfigManager::processTranslatableData(), the override would be filtered out from config anyway, because the source value is empty and there is no translation for it in locale.

IMHO to fix this test:

1. Set a slogan in the original system.site in the dump.
2. Add that slogan in the locales_source table in the dump.
3. Add the same "drupal Spanish" slogan translation in the locales_target table in the dump.

That way, when the config translation is regenerated from the locale data, it will match what's in config and will not be removed.

Unassigning given Alex Pott said he is going for dinner ATM.

This is pretty easy to reproduce:

1. Install standard
2. Install config_transalation
3. Add a language
4. Add a site slogan
5. Translate it to the new language
6. Apply the patch
7. Run update.php
8. Lose translation

I think this part of this issue is critical and a different issue. And in fact it does not matter whether the install runs as part of the update.php or just regular installation through the module form.

Just created #2580575: Installing a module can delete config translations since the problem this issue has unearthed is nothing to do with it. Unfortunately it does need to be postponed on it as there is no way to enable a module in the update path without it.

This bug was introduced at some point after #53, 2 months ago.

@claudiu.cristea: I'm fairly sure 2 month ago is when we added the test that is now showing that bug.. very likely that existed for a longer time.

Amazing detective work, @alexpott and @Gábor Hojtsy!

Reopened.

Straight reroll; conflicted with #2571647: Create DateFormatterInterface.

Fixing #107 and #117, which were the only remaining points to address, everything else was about figuring out that mysterious fatal that we've since fixed.

IMO this is now ready.

+++ b/core/MAINTAINERS.txt
@@ -256,6 +256,9 @@ Action module
+Automated Cron module
+- ?
+

Except this :)

#142: we can look for maintainers in a follow-up issue. Plus, many modules don't have active maintainers.

system.install hell

#2575421: Add a Stable base theme to core and make it the default if a base theme is not specified just got committed and already added system_update_8012(). Incrementing.

(Patch is smaller because this time I created it using git diff -M10% again, to force the recording of moves.)

EDIT: @claudiu.cristea beat me to it :) Removed my files.

LOL, c/p remnant: Dynamic Page Cache -> Automated Cron. Oopsie.

Green.

Feel free to use that patch

RTBC, but now someone needs to ping Dries to get approval on the maintainers change, so maybe #152 is better to commit.

made #2581949: Move the cron form to a status report item, then link to the form if automated cron is enabled for discussing the concern in #158