Convert cron settings to configuration system

First shot of the patch.

Updated patch, fix the typo (cron_max_threshold <> cron_safe_threshold)

The tests will fail because of #1324618: Implement automated config saving for simple settings forms.
Once this issue will be fixed, the test will be successful.

Added the upgrade path in system.install.
The upgrade path is a simple variable_del('cron_safe_threshold');.

Here's my stab at just that. cron_last is also 'state' not 'config' just like cron_semaphore, so no changes there. Also helped out aggregator and simpletest.

My bad, calling ::save() now and not trying to get the variable names from config()->get(). I guess we hardcode instead?

Holy cow, lets get our files into the diff!

Per beejeebus, we are still trying to infer variable names from the config() active store as they really ought to be present.

@beejeebus: thanks for the help!

Few things in patch #17

+++ b/core/modules/system/system.installundefined
@@ -283,7 +283,8 @@ function system_requirements($phase) {
+  ¶

Extra line

+++ b/core/modules/system/system.installundefined
@@ -515,7 +516,7 @@ function system_install() {
+  config('system.cron')->set('cron_key', $cron_key);

Is there a reason why save() is not called in system_install for setting cron_key

Attached patch fixes these two.

Now rerolled with update hook.

Ok and now with added new file that is missing in my previous patches :/

This is my first look at the config management system.

+++ b/core/modules/system/system.installundefined
@@ -1762,6 +1764,28 @@ function system_update_8005() {
+function system_update_8006() {
+  $var_names = array_keys(config('system.cron')->get());

$var_names will be empty, as there's nothing in the active store at this point. Therefore this update does nothing - it's a circular problem - this update is supposed to populate the active store but can't as it relies on the existence of the store to run. Perhaps get the names from somewhere else or hardcode them?

+++ b/core/modules/system/system.installundefined
@@ -1762,6 +1764,28 @@ function system_update_8005() {
+      ->fields('v')

Rather specify the exact fields you would like to query.

+++ b/core/modules/system/system.installundefined
@@ -1762,6 +1764,28 @@ function system_update_8005() {
+    foreach($var_values as $name => $val) {

Minor code style, put a space after 'foreach'

The update didn't work for me when I tested it, see above.

This patch has the added issue that the variables are first deleted and then saved to new config file. The order needs to be reversed to prevent possible data loss if the write to config fails.

The upgrade path is being covered in #1497132: Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system, setting the dependence here.

Rerolling it using the new update function.

Another try.

Changes in here:

• Added a default for cron_safe_threshold, if we're deleting the constant I think we should provide a default.
• Replaced system_settings_form by a custom submit, temporary change until #1324618: Implement automated config saving for simple settings forms lands.

Not sure what's the plan for 'state' vars such as cron_last.

Zero byte patch in #31.

Oops, not sure what happened. Attaching a new one :)

+++ b/core/modules/system/system.admin.incundefined
@@ -1588,11 +1588,29 @@ function system_cron_settings() {
+    'name' => 'system.cron',
+    'path' => 'cron_safe_threshold',
+  );
+ ¶

Whitespace

-29 days to next Drupal core point release.

I'm trying to work on this today.

Fix a fatal error, use $config when available and do not create a new $config when not necessary (also, for the consistency).

Don't know why some change was included in interdiff but not in the full patch.

Space in filename?

system.cron.xml added.

Sorry, this time it is really.

Updated and back to CNR

Sorry folks this will need a re-roll for the cron.php hunk, since that file no longer exists.

I'm working for a patch.

Here you are.

+++ b/core/modules/system/config/system.cron.xml
@@ -0,0 +1,8 @@
+  <cron_threshold_error>1209600</cron_threshold_error>
+<cron_key>drupal</cron_key>

Seems like the indentation is off.

+++ b/core/modules/system/system.admin.inc
@@ -1589,11 +1589,29 @@ function system_cron_settings() {
+  $form_state['config']['cron_safe_threshold'] = array(
+    'name' => 'system.cron',
+    'path' => 'cron_safe_threshold',
+  );

I think it's weird that we're adding this code, even though system_settings_form() has not been converted yet. Maybe add a little comment about that?

Needs work for the first at least, the second is debatable I guess.

Same patch as #49, solves two comments in #51. I just added a simple @todo, there are a few debatable points, but they could be fixed later. A more important point, which will require an update function, is how the variables are name: here in system.cron.xml they are named with cron_ prefix, but in other (e.g. search.module), the variable prefix was removed. But I think it is bikeshedding, as 51 comments and nobody points it out, so I leave it as is.

Forgot the patch.

Actually, #53 is a valid point. The config keys should not be prefixed with cron.
I guess?!
Current examples in core:
http://drupalcode.org/project/drupal.git/blob/HEAD:/core/modules/system/...

http://drupalcode.org/project/drupal.git/blob/HEAD:/core/modules/block/c... (Hmm... This one uses "block_cache" as key in "block.performance")

http://drupalcode.org/project/drupal.git/blob/HEAD:/core/modules/image/c...
and friends

This no longer applies after the simpletest PSR-0 conversion. Tagging novice for re-roll.

Git detected file move and created the attached patch automatically. It should hopefully work.

Committed/pushed to 8.x, thanks all.

Thanks for your hard work on this.

Re-opening due to a couple of issues though, and to align the new names with #1599554: Tutorial/guidelines for how to convert variables into configuration

+++ b/core/modules/system/config/system.cron.xml
@@ -0,0 +1,8 @@
+  <cron_max_threshold>10800</cron_max_threshold>

This variable does not exist.

+++ b/core/modules/system/system.admin.inc
@@ -1589,11 +1589,30 @@ function system_cron_settings() {
   $form['cron']['cron_safe_threshold'] = array(
     '#type' => 'select',
     '#title' => t('Run cron every'),
-    '#default_value' => variable_get('cron_safe_threshold', DRUPAL_CRON_DEFAULT_THRESHOLD),
+    '#default_value' => config('system.cron')->get('cron_safe_threshold'),
     '#options' => array(0 => t('Never')) + drupal_map_assoc(array(3600, 10800, 21600, 43200, 86400, 604800), 'format_interval'),
   );

Grepping for this name did not yield any other writes than this configuration form.

Further research showed that this is completely broken and the UI exposes an entirely bogus setting currently. The relevant code logic only differs between 0 or >0, and the actual interval is not taken into account at all.

Fixing that properly is left to #1554872: "Run cron every" setting (cron_safe_threshold) is not an interval but a Boolean

For this issue, we just want to have a sane config key.

+++ b/core/modules/system/system.admin.inc
@@ -1589,11 +1589,30 @@ function system_cron_settings() {
+  // @todo This needs to be reviewed when #1324618 gets in.
+  $form_state['config']['cron_safe_threshold'] = array(
+    'name' => 'system.cron',
+    'path' => 'cron_safe_threshold',
+  );

This is not used anywhere.

What is 'web'?

"threshold.poormanscron" would be most appropriate and most understandable for long-time Drupalers ;)

For anyone else + newcomers? I've no idea! How do you call the frequency or time interval for calling a function that is called as a PHP shutdown function in order to run Drupal cron jobs, mainly via web browsers of random site visitors? :)

"threshold.web" in the sense of "the threshold/interval to execute the web-based/-triggered cron crunner" made some sense to me.

Checking the test failures, I'm fairly sure this patch will pass as-is, after #1496542: Convert site information to config system has landed.

+++ b/core/modules/system/system.admin.inc
@@ -1579,26 +1579,24 @@ function system_cron_settings($form, &$form_state) {
-  // @todo This needs to be reviewed when #1324618 gets in.
-  $form_state['config']['cron_safe_threshold'] = array(
-    'name' => 'system.cron',
-    'path' => 'cron_safe_threshold',
-  );

Above it was (sort of) discussed, that we put this there so that #1324618: Implement automated config saving for simple settings forms automatically works with this patch. I wasn't a huge advocate of putting it in, just saying it was put there for a reason.

+++ b/core/modules/system/system.admin.inc
@@ -1579,26 +1579,24 @@ function system_cron_settings($form, &$form_state) {
+  $form = system_settings_form($form);
+  $form['#submit'] = array('system_cron_settings_submit');

Can you explain what the point of this is? I don't get it.

Regarding the key name, maybe "auto_run", as after that threshold cron is run "automatically" (from the viewpoint of the site administrator)?

Sorry, leaving at "needs review" for now.

1. None of these conversion patches should contain any premature code for #1324618: Implement automated config saving for simple settings forms yet. Regardless of what the final solution will be, it will definitely look different than the removed lines here.
2. The most simple way to convert settings forms at this point is to retain usage of system_settings_form() to get a consistent actions container and submit button, but override the #submit handler being set by it, in order to prevent any variables from getting written. We likely want to add that to #1599554: Tutorial/guidelines for how to convert variables into configuration
3. config('system.cron')->get('threshold.autorun') makes sense to me. Wanna re-roll with that?

   That said, depending on its outcome, it's possible that #1554872: "Run cron every" setting (cron_safe_threshold) is not an interval but a Boolean will likely remove the 'threshold.' prefix, but that's none of our business here.

New patches will still fail as long as #1496542: Convert site information to config system did not land.

Re-rolled with adjustments against latest HEAD.

cron_max_threshold is lost?

Regarding cron_max_threshold, see #60 - the variable is entirely obsolete.

Apparently, the Drupal installer does not install system module in the regular/proper procedure. Due to that, the system.cron:key is never actually set/updated after the Drupal installation. Attached patch additionally fixes the Drupal installer to install System module correctly.

#72: drupal8.config-cron.71.patch queued for re-testing.

Re-rolled against HEAD.

great patch, looks like it catches everything. Only spacing issues here that I'm quibbling about. IMO it's RTBC

+++ b/core/modules/system/system.admin.incundefined
@@ -1596,40 +1595,34 @@ function system_cron_settings($form, &$form_state) {
+
   $status = '<p>' . t('Last run: %cron-last ago.', array('%cron-last' => format_interval(REQUEST_TIME - variable_get('cron_last')),)) . '</p>';

spacing here seems to hurt not help readibility

+++ b/core/modules/system/system.admin.incundefined
@@ -1596,40 +1595,34 @@ function system_cron_settings($form, &$form_state) {
+
   $form['cron'] = array(

as well as here

Looks good to me. Committed to 8.x. Thanks!