Automatic Site Maintenance During Update.php

I think this is the correct way to do it. I recommend that we add a PHP comment. With some code comments, this pach is RTBC.

Looks great Rob. Committed to CVS HEAD. Thanks.

Was this even tested?

Looks to me that this code will never be called:

   batch_process($base_url . '/update.php?op=results', $base_url . '/update.php');
+
+  // Now that the update is done, we can disable site maintenance if
+  // it was previously turned off.
+  if ($site_offline == FALSE) {
+    variable_set('site_offline', FALSE);
+  }

because it is just after a batch_process() (which drupal_goto in the general case...).

Also I would like to see this behavior configurable. I'm pretty sure there are use cases (automatic staging etc.) in which you don't want this to happen automatically.

A minor issue only, but:

     '#description' => t('Message to show visitors when the site is in offline mode.')
   );
 
+  $form['site_offline_during_update'] = array(
+    '#type' => 'checkbox',
+    '#title' => t('Turn site offline during update'),
+    '#default_value' => variable_get('site_offline_during_update', FALSE),
+    '#description' => t('When performing a site update, temporarily toggle site maintenance.')
+  );
+
   return system_settings_form($form);
 }

If we refer to "offline mode" in the previous description, the description in the new form field should use the same term rather than "site maintenance", IMO. That would make it something like "When performing a site update, temporarily toggle offline mode."

Also, I'm not certain about the use of the word "Turn" in the title; it strikes me as a use that will be hard to translate directly. Why not something like "Enable offline mode during update", or something similar.

As discussed on IRC, the original logic was sufficient. No extra setting required.

However, added some failsafe check to update_finished(), so that site maintenance is only disabled if everything went really well.

Also, cleaned-up the inline comments.

Committed to CVS HEAD. Thanks.

Not sure if this is the place I should be commenting on it, but right now, when the site is already in maintenance mode prior to going to update it, it incorrectly implies that the site was taken out of maintenance mode. I think the text on the page should be altered if the site was already in maintenance mode prior, but thats just my opinion.

@Ninja Overlord, this is definitely *not* the place to comment on it. Please describe your issue completely and clearly in a new issue. Commenting on a closed issue just means being ignored (because if a tree falls in the forest and nobody hears it, did it really fall?)