It's useful to know when a site went into and came back out of maintenance mode, so it would be nice if this information was added to the admin log when it happened.

Comments

Race.it’s picture

Assigned: Unassigned » Race.it

This would require a override of the submit hook for the form, I think that is the safest way of handling this request.

Race.it’s picture

FileSize
2.04 KB
Invalid PHP syntax in system.module. View

Created a patch to address this request, please review.

Race.it’s picture

Status: Active » Needs review
cburschka’s picture

Status: Needs review » Needs work
+ * This function logs changes to the site offline status

Add a period there.

site_maintenance_... is an unnecessary name quirk. This function should start with system_ to reflect the module it is in.

Also, if you use system_site_maintenance_mode_submit (mode rather than admin_settings) you get consistency with the form's name as well as shortening the monster a bit.

---

I need to read how submit handlers and auto-discovery are handled on system settings forms. There are several possibilities here, including a) that you need to add the submit handler after calling system_settings_form() or b) that if you pick the standard FORM_submit() name (in this case the one I suggested) the submit handler will be automatically discovered without having to be registered. I'm just speculating though.

cburschka’s picture

Addendum:

+  if ( variable_get('site_offline') != $site_offline_status) {
+    if ($site_offline_status == 0) {
+      $value = t('Site status') . ' : ' . t('online');
+    }
+    else {
+      $value = t('Site status') . ' : ' . t('offline');
+    }
+  }

There's a space inside the left parenthesis on the first line, which should go.

Also, if you compare a variable that can only be 1 or 0, it is usually more readable to check it as a boolean - and also to put the truth (1) branch before the falsehood (0). In this case, the condition determines the state of a single variable and therefore should probably be a trinary.

Finally, splitting translateable strings when it's not necessary costs an extra string lookup and makes life difficult for the translators. It may look as though the strings are more "normalized", but be aware that by hard-coding this split, the separating colon and the strings' order, linguistic flexibility is lost. A language with more inflections than English can translate "offline" differently whether its stand-alone or part of a sentence.

In summary, a good choice here might be

$value = $site_offline_status ? t('Site status: offline') : t('Site status: online');

Although I would favor a clearer sentence personally, such as "Site was taken offline" or "Site was put online".

(Naturally, all of this also applies to the "reset to defaults" string).

---

Also, it seems I was wrong on both of my earlier speculations:

a) system_settings_form() adds a #submit handler, but doesn't strip the ones that are there (good, because that'd be pretty silly)
b) drupal_prepare_form() adds FORM_ID_submit() only if no #submit handler has been explicitly defined, which system_settings_form() has done by then. So regardless of the name, it has to be explicitly added. It still makes sense to use that name though.

pfahlr’s picture

http://drupal.org/sandbox/pfahlr/1425554

function maintenance_mode_watchdog_form_alter(&$form, &$form_state, $form_id)
{
  if($form_id == 'system_site_maintenance_mode')
  {
    $form['#submit'][] = '_maintenance_mode_watchdog_system_site_maintenance_mode_submit';
  }
}

function _maintenance_mode_watchdog_system_site_maintenance_mode_submit(&$form, &$form_state)
{
  global $user;
  $on_off = ($form_state['values']['maintenance_mode'])?'ON':'OFF';
  watchdog('maintenance_mode', '%username turned maintenance mode %on_off', array('%username'=>$user->name, '%on_off'=>$on_off), WATCHDOG_ALERT);
}
mmedrano’s picture

Submitting a new patch for feature request.

mmedrano’s picture

Status: Needs work » Needs review
panshulk’s picture

Assigned: Race.it » panshulk
Status: Needs review » Reviewed & tested by the community
FileSize
32.7 KB

Hi @mmedrano

Thanks for working on this.

I tested the latest 229778-log-changes-to-offline-mode.patch, the patch works as expected.
Following is the screenshot of the result in "Recent log messages" :

Recent_log_screenshot

Hence Moving this to RTBC

Thanks :)

panshulk’s picture

Assigned: panshulk » Unassigned
David_Rothstein’s picture

Version: 7.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +needs backport to D7

This seems like a good idea but needs to go in Drupal 8 first.

Also a few issues:

  1. +  if ($form['maintenance_mode']['#value'] == TRUE) {
    

    Why not check $form_state['values']['maintenance_mode'] instead? That is usually the standard way to look at submitted form values.

  2. This code looks like it will record the message every time the form is saved with the checkbox checked (e.g., if the checkbox wasn't changed but only the maintenance mode message was edited). Instead it should only record the message when the checkbox goes from unchecked to checked.
  3. +    watchdog('system', 'Maintenance Mode enabled by User - "%user"', array('%user' => $user->name), WATCHDOG_INFO);
    

    It should not be necessary to include the username here - as the above screenshot shows, watchdog already records it automatically. So I think just something like "Site was put in maintenance mode." would be fine here.

  4. Since we're recording when the site was put in maintenance mode, I think it would be very useful to record when the site was taken out of maintenance mode too.
David_Rothstein’s picture

Since we're recording when the site was put in maintenance mode, I think it would be very useful to record when the site was taken out of maintenance mode too.

Eh, especially because that's what the issue title says this issue is about :) (And what the earlier patch in this issue did...)

cburschka’s picture

Note that in D8, maintenance mode can be triggered non-interactively, such as with drupal-console. Right now, these all simply use \Drupal::service('state')->set('system.maintenance_mode', true) just like the form submit function. That's okay as long as the submit function doesn't do anything special.

If we want to do more, the whole code needs to be moved into a separate function (perhaps in the system.manager service) to allow it to be called from modules.

(Unfortunately, as maintenance mode is now a state variable and not a configuration variable, this message cannot be added by listening to a config change event.)

David_Rothstein’s picture

This is true in Drupal 7 also - you can do drush vset maintenance_mode 1. However, I think it's not that big of a deal to only log it when maintenance mode is turned on via the admin UI. (Unless we're setting an expectation for people that it will always be logged unconditionally or something?)

xjm’s picture

Version: 8.3.x-dev » 8.4.x-dev