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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pcambra’s picture

Assigned: Unassigned » pcambra
pcambra’s picture

Status: Active » Needs review
FileSize
12.65 KB
Lars Toomre’s picture

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?

pcambra’s picture

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.

pcambra’s picture

Status: Needs work » Needs review
pcambra’s picture

pcambra’s picture

gdd’s picture

Status: Postponed » Needs review

Letting the bot take a shot at it.

gdd’s picture

Status: Needs review » Postponed

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

gdd’s picture

Status: Postponed » Needs review

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

gdd’s picture

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

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

pcambra’s picture

This needs an update using the new update function

pcambra’s picture

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.

pcambra’s picture

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

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

swentel’s picture

FileSize
11.76 KB
+++ 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.

swentel’s picture

Status: Needs work » Needs review

Setting to review

Status: Needs review » Needs work

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

swentel’s picture

Status: Needs work » Needs review
FileSize
15.13 KB

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.

swentel’s picture

Status: Needs work » Needs review
FileSize
15.05 KB

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

swentel’s picture

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)

swentel’s picture

FileSize
15.05 KB

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.

swentel’s picture

Status: Needs work » Needs review
FileSize
15.05 KB

Never ever manually update patch :)

swentel’s picture

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

aspilicious’s picture

Looks good. Tempted to mark this rtbc.

catch’s picture

 /**
+ * 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.

swentel’s picture

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:

 /**
   * 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.

marcingy’s picture

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.

swentel’s picture

Status: Needs work » Needs review
FileSize
13.86 KB

Updated

gdd’s picture

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.

marcingy’s picture

Status: Needs work » Needs review
FileSize
13.37 KB

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.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
13.42 KB

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.

ZenDoodles’s picture

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.

kbasarab’s picture

Status: Needs work » Needs review

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

kbasarab’s picture

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.

kbasarab’s picture

Assigned: pcambra » kbasarab
Status: Needs work » Needs review
FileSize
54.3 KB

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.

kbasarab’s picture

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
kbasarab’s picture

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.

gdd’s picture

- 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

n3or’s picture

Status: Needs work » Needs review
FileSize
12.09 KB

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.

sun’s picture

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.

n3or’s picture

Status: Needs work » Needs review
FileSize
2.16 KB
10.14 KB

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

sun’s picture

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 ;)

alexpott’s picture

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.
sun’s picture

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. :)

n3or’s picture

Status: Needs work » Needs review
FileSize
7.51 KB
10.15 KB

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.

sun’s picture

Status: Needs work » Needs review
FileSize
1.77 KB
10.65 KB

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

alexpott’s picture

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();.

sun’s picture

FileSize
1.97 KB
12.62 KB

5920cf3 Fixed Menu\RouterTest.

alexpott’s picture

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?

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

sun’s picture

Status: Fixed » Needs review
FileSize
658 bytes

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

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

If green this is rdy to go!

sun’s picture

no_commit_credit’s picture

FileSize
427 bytes
661 bytes
xjm’s picture

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

Dries’s picture

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.

amontero’s picture

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