The maintenance mode settings at admin/config/development/maintenance need to be converted to use the new configuration system.

Tasks

  • Create a default config file and add it to the system module.
  • Convert the admin UI in system.admin.inc to read to/write from the appropriate config.
  • Convert any code that currently accesses the variables used to use the config system.
  • Write an upgrade path from D7 -> D8 to migrate existing data to the new system and drop the appropriate variables.

This would be a good task for someone wanting to get up to speed on how the new config system works. Feel free to ping me on IRC if you need help.

Files: 
CommentFileSizeAuthor
#63 system-1496458-63.patch661 bytesno_commit_credit
PASSED: [[SimpleTest]]: [MySQL] 39,832 pass(es).
[ View ]
#63 interdiff.txt427 bytesno_commit_credit
#60 drupal8.config-system-maintenance.60.patch658 bytessun
PASSED: [[SimpleTest]]: [MySQL] 39,803 pass(es).
[ View ]
#57 config.maintenance.57.patch12.62 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,215 pass(es).
[ View ]
#57 interdiff.txt1.97 KBsun
#55 config.system-maintenance.55.patch10.65 KBsun
FAILED: [[SimpleTest]]: [MySQL] 37,214 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#55 interdiff.txt1.77 KBsun
#53 config-system-maintenance.53.patch10.15 KBn3or
FAILED: [[SimpleTest]]: [MySQL] 36,971 pass(es), 143 fail(s), and 100 exception(s).
[ View ]
#53 interdiff.txt7.51 KBn3or
#49 config-system-maintenance.49.patch10.14 KBn3or
FAILED: [[SimpleTest]]: [MySQL] 36,967 pass(es), 143 fail(s), and 100 exception(s).
[ View ]
#49 interdiff.txt2.16 KBn3or
#47 config-system-maintenance.47.patch12.09 KBn3or
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-system-maintenance.47.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#42 1496458-42.patch54.3 KBkbasarab
FAILED: [[SimpleTest]]: [MySQL] 36,664 pass(es), 15 fail(s), and 1 exception(s).
[ View ]
#36 maintenance_mode-1496458-36.patch13.42 KBkbasarab
FAILED: [[SimpleTest]]: [MySQL] 36,625 pass(es), 15 fail(s), and 1 exception(s).
[ View ]
#34 1496458-34.patch13.37 KBmarcingy
FAILED: [[SimpleTest]]: [MySQL] 36,235 pass(es), 128 fail(s), and 87 exception(s).
[ View ]
#32 1496458-32.patch13.86 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 36,171 pass(es).
[ View ]
#26 1496458-26.patch15.05 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 35,905 pass(es).
[ View ]
#24 1496458-24.patch15.05 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1496458-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#22 1496458-22.patch15.05 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 35,927 pass(es).
[ View ]
#20 1496458-20.patch15.13 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 35,904 pass(es), 9 fail(s), and 1 exception(s).
[ View ]
#17 1496458-17.patch11.76 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 35,786 pass(es), 35 fail(s), and 9 exception(s).
[ View ]
#7 1496458-maintenance_mode_settings-7.patch11.72 KBpcambra
FAILED: [[SimpleTest]]: [MySQL] 35,767 pass(es), 37 fail(s), and 13 exception(s).
[ View ]
#4 1496458-maintenance_mode_settings-4.patch12.65 KBpcambra
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1496458-maintenance_mode_settings-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 1496458-maintenance_mode_settings.patch12.65 KBpcambra
FAILED: [[SimpleTest]]: [MySQL] 35,892 pass(es), 37 fail(s), and 13 exception(s).
[ View ]

Comments

Assigned:Unassigned» pcambra

Status:Active» Needs review
StatusFileSize
new12.65 KB
FAILED: [[SimpleTest]]: [MySQL] 35,892 pass(es), 37 fail(s), and 13 exception(s).
[ View ]

Status:Needs review» Needs work

+++ b/core/modules/system/system.installundefined
@@ -1762,6 +1762,37 @@ function system_update_8005() {
+function system_update_8006() {
+  $config = config('system.maintenance');
+  // Get all setting names defined in system.rss-publishing.xml.
+  $var_names = array_keys($config->get());
+  if (!empty($var_names)) {
+    //Get any variables currently defined that match the new setting names in
+    //the config file.
+    $query = db_select('variable', 'v')
+      ->fields('v')
+      ->condition('name', $var_names, 'IN');
+    $var_values = $query->execute()->fetchAllKeyed(0);
+
+    // Update the config system settings to use the values previously stored in
+    // the variable table.
+    if (!empty($var_names)) {
+      foreach($var_values as $name => $val) {
+        $config->set($name, $val);
+      }
+      $config->save();
+      // Delete the old variables. The config system will throw an exception if
+      // a value cannot be saved, so this code will not run if there is a
+      // problem running the update.
+      $del = db_delete('variable')->condition('name', $var_names, 'IN');
+      $del->execute();
+    }
+  }

A couple of things about this function...

a) // Get all setting names defined in system.rss-publishing.xml is the wrong config file... needs correction.
b) //Get any variables currently defined that match the new setting names in
+ //the config file. ... needs a space between // and the first word of both lines
c) + // Update the config system settings to use the values previously stored in
+ // the variable table.
+ if (!empty($var_names)) {

I am unclear why this is again checking if $var_names is empty. Perhaps this should be a check for not empty $var_values?

StatusFileSize
new12.65 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1496458-maintenance_mode_settings-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I am unclear why this is again checking if $var_names is empty. Perhaps this should be a check for not empty $var_values?

Yup, that was my intention.

Fixed all comments, patch attached.

Status:Needs work» Needs review

StatusFileSize
new11.72 KB
FAILED: [[SimpleTest]]: [MySQL] 35,767 pass(es), 37 fail(s), and 13 exception(s).
[ View ]

Status:Postponed» Needs review

Letting the bot take a shot at it.

Status:Needs review» Postponed

Oh duh its just going to fail since the other function isn't committed yet. Nevermind.

Status:Postponed» Needs review

OK the upgrade function got committed so we can get back to work on this.

Status:Needs review» Needs work
Issue tags:+Configuration system, +Config novice

The last submitted patch, 1496458-maintenance_mode_settings-7.patch, failed testing.

This needs an update using the new update function

Status:Needs work» Needs review

Hum, nope, the update function is there with the correct update_N number, the tests are complaining about config table not existing (?). Let's do another try for the testbot.

Status:Needs review» Needs work
Issue tags:+Configuration system, +Config novice

The last submitted patch, 1496458-maintenance_mode_settings-7.patch, failed testing.

StatusFileSize
new11.76 KB
FAILED: [[SimpleTest]]: [MySQL] 35,786 pass(es), 35 fail(s), and 9 exception(s).
[ View ]

+++ b/core/includes/ajax.incundefined
@@ -515,8 +515,7 @@ function ajax_prepare_response($page_callback_result) {
+        $commands[] = ajax_command_alert(filter_xss_admin(t($config->get('maintenance_mode_message'), array('@site' => variable_get('site_name', 'Drupal')))));
+++ b/core/includes/common.incundefined
@@ -2592,8 +2592,7 @@ function drupal_deliver_html_page($page_callback_result) {
+        print theme('maintenance_page', array('content' => filter_xss_admin(t($config->get('maintenance_mode_message'), array('@site' => variable_get('site_name', 'Drupal'))))));

$config is unknown in both places.

Trying new patch.

-29 days to next Drupal core point release.

Status:Needs work» Needs review

Setting to review

Status:Needs review» Needs work

The last submitted patch, 1496458-17.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new15.13 KB
FAILED: [[SimpleTest]]: [MySQL] 35,904 pass(es), 9 fail(s), and 1 exception(s).
[ View ]

Ok, the problem with this maintenance patch is that update.php is now trying to read the maintenance status from a table that doesn't exist yet.
This is a bit like the same problem we ran in another issue with D8 language updates/upgrades.

Attached a patch which makes all tests pass already, except for the update functionality. Trying to grasp there what goes wrong.

What changed:
- added update_prepare_d8_configuration() to update.php
- added a file_exists to config_get_config_directory() - now *THIS* was a fun one to debug, but it makes sense.
- removed the updates in system.install, it really needs to happen earlier - especially for upgrade tests.
- changed the submit button value on the maintenance page to those tests will pass well.

Let's see what the bot thinks now.

Status:Needs review» Needs work

The last submitted patch, 1496458-20.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new15.05 KB
PASSED: [[SimpleTest]]: [MySQL] 35,927 pass(es).
[ View ]

This one should pass, it was actually easier than I thought. (wrong conversion in update_finished()).

Note: the fix added in config.inc is also used in #1496510: Convert search settings to configuration system. Once we agree this is the right way to do (or another fix is proposed), the patch there needs to be updated. It probably affects others as well (like #1496534: Convert account settings to configuration system)

StatusFileSize
new15.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1496458-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Patch uses file_prepare_directory() instead of mkdir() and file_exists().

Status:Needs review» Needs work

The last submitted patch, 1496458-24.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new15.05 KB
PASSED: [[SimpleTest]]: [MySQL] 35,905 pass(es).
[ View ]

Never ever manually update patch :)

Also, one thing I've noticed is that CMI now generates a lot of CMI directories in the simpletest directory which aren't cleaned up after test are run, is that a known issue or should we open one for this ?

- Edit - Created an issue for it: #1512676: Simpletest doesn't clean up the config directories generated by the configuration

Looks good. Tempted to mark this rtbc.

/**
+ * Prepare Drupal 8 for configuration system.
+ */
+function update_prepare_d8_configuration() {
+  if (!db_table_exists('config')) {
+    module_load_include('install', 'system');
+    $system_schema = system_schema();
+    db_create_table('config', $system_schema['config']);
+    update_variables_to_config('system.maintenance');
+  }
+}

This shouldn't use system_schema(), this has to work upgrading to any version of Drupal 8, and it's conceivable that we'll change the config schema after beta or rc. So it should use the literal schema definition as it is now, then updates can be run on it previously.

However having typed that, I'm less sure now, because the update system depends on the config system, so the schema has to be absolutely up-to-date. Either way this is going to complicate any schema changes to the config system later down the line - at least once we support 8.x-8.x updates.

Good point, I think having the latest schema is a must though. As you point out, during 8.x upgrades, this could potententialy be split into something like this:

<?php
/**
   * Prepare Drupal 8 for configuration system.
   */
function update_prepare_d8_configuration() {
  if (!
db_table_exists('config')) {
   
module_load_include('install', 'system');
   
$system_schema = system_schema();
   
db_create_table('config', $system_schema['config']);
   
update_variables_to_config('system.maintenance');
  }
  elseif (
drupal_get_installed_schema_version('system') < 8003) {
   
// Do something here because this change would even affect failing update.php
 
}
}
?>

Wow, now, that looks really ugly isn't it ? :) On the other hand, I think if we'd introduce a really dramatic change in the config schema or even storage, we're into a big problem anyway. Not sure how to handle this further, let's get some more opinions here from say heyrocker, beejeebus, sun and others.

Status:Needs review» Needs work

#1517138: SimpleTest does not create configuration folder causing test failures has now been committed to get round the folder issue this likely needs a re-roll.

Status:Needs work» Needs review
StatusFileSize
new13.86 KB
PASSED: [[SimpleTest]]: [MySQL] 36,171 pass(es).
[ View ]

Updated

Status:Needs review» Needs work

Unfortunately this needs a reroll to HEAD because the system update function numbers have changed. Otherwise, I think its looking pretty good.

Status:Needs work» Needs review
StatusFileSize
new13.37 KB
FAILED: [[SimpleTest]]: [MySQL] 36,235 pass(es), 128 fail(s), and 87 exception(s).
[ View ]

This should be a straight reroll aflso takes into account tests moving. This might fail on upgrades due to the refractoring of tests going on at the moment.

Status:Needs review» Needs work

The last submitted patch, 1496458-34.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new13.42 KB
FAILED: [[SimpleTest]]: [MySQL] 36,625 pass(es), 15 fail(s), and 1 exception(s).
[ View ]

This should be just a straight reroll of #34. Only difference should be moving:

diff --git a/core/cron.php b/core/cron.php
index fa9aa14..126d991 100644
--- a/core/cron.php
+++ b/core/cron.php
@@ -20,7 +20,7 @@ if (!isset($_GET['cron_key']) || variable_get('cron_key', 'drupal') != $_GET['cr
   watchdog('cron', 'Cron could not run because an invalid key was used.', array(), WATCHDOG_NOTICE);
   drupal_access_denied();
}
-elseif (variable_get('maintenance_mode', 0)) {
+elseif (config('system.maintenance')->get('maintenance_mode')) {
   watchdog('cron', 'Cron could not run because the site is in maintenance mode.', array(), WATCHDOG_NOTICE);
   drupal_access_denied();
}

Over to:

diff --git a/core/modules/system/system.module b/core/modules/system/system.module
index e685c16..232d24b 100644
--- a/core/modules/system/system.module
+++ b/core/modules/system/system.module
@@ -1134,7 +1134,7 @@ function system_cron_access($key) {
     watchdog('cron', 'Cron could not run because an invalid key was used.', array(), WATCHDOG_NOTICE);
     return FALSE;
   }
-  elseif (variable_get('maintenance_mode', 0)) {
+  elseif (config('system.maintenance')->get('maintenance_mode')) {
     watchdog('cron', 'Cron could not run because the site is in maintenance mode.', array(), WATCHDOG_NOTICE);
     return FALSE;
   }

SimpleTest may still not pass though with all the other testing issues happening.

Status:Needs review» Needs work

The last submitted patch, maintenance_mode-1496458-36.patch, failed testing.

Issue tags:-Config novice

With upgrade path failures this does not look like a novice issue anymore. Be sure to add "config novice" back to tags if it's suitable when the next action is clarified.

Status:Needs work» Needs review

Rerunning test based on: http://qa.drupal.org/node/158

Issue tags:-Configuration system

#36: maintenance_mode-1496458-36.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Configuration system

The last submitted patch, maintenance_mode-1496458-36.patch, failed testing.

Assigned:pcambra» kbasarab
Status:Needs work» Needs review
StatusFileSize
new54.3 KB
FAILED: [[SimpleTest]]: [MySQL] 36,664 pass(es), 15 fail(s), and 1 exception(s).
[ View ]

Rerolled against current 8.x core. This passes upgrade tests locally as well. Didn't run a full test locally though. No interdiff due to core 8.x changes since last roll of this. Made interdiff 3+ mb.

Status:Needs review» Needs work

The last submitted patch, 1496458-42.patch, failed testing.

Looks like some of the settings aren't reading. In system.admin.inc I did some var_dump inside function system_site_maintenance_mode() { and got this:

function system_site_maintenance_mode() {
  $config = config('system.maintenance');
  var_dump($config);
var_dump($config->get('maintenace_mode_message'));

object(Drupal\Core\Config\DrupalConfig)#12 (2) { ["storage":protected]=> object(Drupal\Core\Config\DatabaseStorage)#16 (2) { ["name":protected]=> string(18) "system.maintenance" ["fileStorage":protected]=> NULL } ["data":protected]=> array(2) { ["maintenance_mode"]=> string(1) "0" ["maintenance_mode_message"]=> string(0) "" } } NULL

Wondering here if http://drupal.org/node/1447686#comment-6062768 might have something to do with this here. Seems like All the calls are correct for defining the maintenance setting but its not being read. Its correct in the XML inside system/config/system.maintenance.xml but if you grep the config files you get:

kb-mc:d8git.kb kbasarab [1496458/45] $ grep -r 'maintenance' ./sites/default/files/config*
./sites/default/files/config_6U5QirXCR3TqLkKHOYWE1jsNhwf2gVrcKfxmHy_Yt7o/system.maintenance.yml:maintenance_mode: '0'
./sites/default/files/config_6U5QirXCR3TqLkKHOYWE1jsNhwf2gVrcKfxmHy_Yt7o/system.maintenance.yml:maintenance_mode_message: ''
./sites/default/files/config_VUYcyMVo6kkHidiYYJT0EpoO-tJbsLnVZC2FlgMKtfo/system.maintenance.xml:  <maintenance_mode>0</maintenance_mode>
./sites/default/files/config_VUYcyMVo6kkHidiYYJT0EpoO-tJbsLnVZC2FlgMKtfo/system.maintenance.xml:  <maintenance_mode_message/>

So the messages aren't saving as I expected in #44. Not sure how to force a refresh though of file system config files. Answer might be in issue I cited though.

- The last couple patches don't actually include the system.maintenance.xml file
- The file needs to be updated to YAML instead of XML
- The naming needs to be adjusted to match the conventions defined at http://drupal.org/node/1667896

Status:Needs work» Needs review
StatusFileSize
new12.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-system-maintenance.47.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I created a new patch based on #42. In this patch the rejects of #42 are fixed and the YAML config file is added. Additionally, the naming is adjusted to match the new conventions.
I tried to create a interdiff, but my attempts unfortunately failed.

Status:Needs review» Needs work

Thanks for working on this, @n3or! :)

This looks almost ready. I'm missing the new .yml file in the patch though?

+++ b/core/modules/system/system.admin.inc
@@ -2231,19 +2231,36 @@ function system_date_time_lookup() {
  * @see system_settings_form()
  */
function system_site_maintenance_mode() {
...
-  return system_settings_form($form);
+  $form['submit'] = array('#type' => 'submit', '#value' => t('Save configuration'));
+  return $form;
...
+ * @see system_settings_form()
...
+  drupal_set_message(t('The configuration has been saved.'));

Have a look at the changes to system.admin.inc in the interdiff on #1493108-72: Convert logging and error settings to configuration system to see how the new system_config_form() is supposed to be used.

+++ b/core/modules/system/system.admin.inc
@@ -2231,19 +2231,36 @@ function system_date_time_lookup() {
-  $form['maintenance_mode'] = array(
...
+
+ $form['maintenance_mode'] = array(

Let's revert this :)

+++ b/core/modules/system/system.admin.inc
@@ -2231,19 +2231,36 @@ function system_date_time_lookup() {
+ $config = config('system.maintenance');

Leading TAB (should be two spaces instead).

+++ b/core/modules/system/system.admin.inc
@@ -2231,19 +2231,36 @@ function system_date_time_lookup() {
+ $config = config('system.maintenance');
+  $config->set('mode', $form_state['values']['maintenance_mode']);
+  $config->set('message', $form_state['values']['maintenance_mode_message']);
+  $config->save();

These calls can be chained; i.e.:

config('system.maintenance')
  ->set('foo', 'bar')
  ->set('...
  ->save();

+++ b/core/modules/system/system.module
@@ -1182,7 +1182,7 @@ function system_cron_access($key) {
+  elseif (config('system.maintenance')->get('mode')) {  ¶

Trailing white-space.

Status:Needs work» Needs review
StatusFileSize
new2.16 KB
new10.14 KB
FAILED: [[SimpleTest]]: [MySQL] 36,967 pass(es), 143 fail(s), and 100 exception(s).
[ View ]

Problems with current D8 core and problems mentioned in #48 fixed.

Status:Needs review» Needs work

+++ b/core/modules/system/config/system.maintenance.yml
@@ -0,0 +1,2 @@
+mode: '0'

Didn't we want to rename "mode" into "enabled" ?

+++ b/core/modules/system/system.admin.inc
@@ -2222,22 +2222,36 @@ function system_date_time_lookup() {
+ * Form builder submit handler. Handle submission for site maintenance settings.
...
+ * @see system_settings_form()

Let's change the phpDoc summary into:

"Form submission handler for system_site_maintenance_mode()."

...and then remove the @see.

+++ b/core/modules/system/system.admin.inc
@@ -2222,22 +2222,36 @@ function system_date_time_lookup() {
+ config('system.maintenance')

One TAB left ;)

Issue tags:+Config novice

We missing an upgrade path for the maintenance_mode_message variable. So we need a system_update_N function in system.install to do this.

  update_variables_to_config('system.maintenance', array('maintenance_mode_message => 'message'));

Also, the config keys should be sorted alphabetically

+++ b/core/modules/system/config/system.maintenance.ymlundefined
@@ -0,0 +1,2 @@
+mode: '0'
+message: @site is currently under maintenance. We should be back shortly. Thank you for your patience.

Assigned:kbasarab» Unassigned
Issue tags:+Novice, +API change

Adding the other missing tags, and unassigning @kbasarab, since @n3or seems to be actively working on this patch. :)

Status:Needs work» Needs review
StatusFileSize
new7.51 KB
new10.15 KB
FAILED: [[SimpleTest]]: [MySQL] 36,971 pass(es), 143 fail(s), and 100 exception(s).
[ View ]

Fixed problems of #50 and #51. Thank you guys!

Status:Needs review» Needs work

The last submitted patch, config-system-maintenance.53.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.77 KB
new10.65 KB
FAILED: [[SimpleTest]]: [MySQL] 37,214 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

a99b185 Attempt to fix update.php's temporary global override of maintenance mode.

core/modules/system/lib/Drupal/system/Tests/Menu/RouterTest.php has couple of variable_set('maintenance_mode', TRUE); that need converting to config('system.maintenance')->set('enabled', TRUE)->save();.

StatusFileSize
new1.97 KB
new12.62 KB
PASSED: [[SimpleTest]]: [MySQL] 37,215 pass(es).
[ View ]

5920cf3 Fixed Menu\RouterTest.

Status:Needs review» Reviewed & tested by the community

Patch apply and works as expected. Thanks for all the work!

I wonder if we should mention in the maintenance message field description that it now supports replacing @site with the site name in custom messages?

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Status:Fixed» Needs review
StatusFileSize
new658 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,803 pass(es).
[ View ]

For some reason, we forgot to add a update here.

Status:Needs review» Reviewed & tested by the community

If green this is rdy to go!

StatusFileSize
new427 bytes
new661 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,832 pass(es).
[ View ]

Minor docblock cleanup attached above. Hook implementations get the imperative for their summaries, because they are user-facing:
http://drupal.org/node/1354#hookimpl

Status:Reviewed & tested by the community» Fixed

Committed the missing update function.

Status:Fixed» Closed (fixed)

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

Linking to overlapping functionality issue: #97650: Token support for site maintenance mode message
Feedback is appreciated.