Posted by geodaniel on March 4, 2008 at 12:27pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | cytefx |
| Status: | needs work |
Issue Summary
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
#1
This would require a override of the submit hook for the form, I think that is the safest way of handling this request.
#2
Created a patch to address this request, please review.
#3
#4
+ * This function logs changes to the site offline statusAdd 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.
#5
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.
#6
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);
}