Add note to log when site put into and taken out of Offline mode

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

Created a patch to address this request, please review.

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

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.

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

Submitting a new patch for feature request.

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" :

Hence Moving this to RTBC

Thanks :)

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.

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

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

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

Attached is a D8 implementation and screenshot.

Note: This will only log changes made through the UI, not through Drupal Console / Drush.

Reviewing patch now

I've used the message from the patch in #17 and moved it into the CacheCollector:set() method.

This allows the message to be logged when being set using Drush, although it is logged as being changed by 'Anonymous'.

Example log entries:

I won't create an interdiff as it's essentially a new patch.

Maybe an alternative approach: we could add a method to \Drupal\Core\Site\MaintenanceMode and let this logging happen there?

> Maybe an alternative approach: we could add a method to \Drupal\Core\Site\MaintenanceMode and let this logging happen there?

+1 to this. The cache collector seems an odd place to put logging.

Nitpick:

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.php
@@ -166,6 +166,13 @@ public function set($key, $value) {
+    // Log a message if maintenance mode has been set.

Comment is wrong - it logs when entering and leaving maintenance mode.

  /**
namespace Drupal\Core\Site;

use Drupal\Core\Routing\RouteMatchInterface;
use Drupal\Core\Session\AccountInterface;

/**
 * Defines the interface for the maintenance mode service.
 */
interface MaintenanceModeInterface {

  /**
   * Returns whether the site is in maintenance mode.
   *
   * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
   *   The current route match.
   *
   * @return bool
   *   TRUE if the site is in maintenance mode.
   */
  public function applies(RouteMatchInterface $route_match);

I tried putting logger inside this function with if..else condition but it runs multiple times and print multiple log message for the same event.

Here is the attached screenshot .

In -- > Maintenance Mode Enabled
Out --> Maintenance Mode Disabled

> \Drupal\Core\Site\MaintenanceMode and let this logging happen there?

Ah. That service is about *reporting* whether the site is in maintenance mode. It doesn't allow you to change the status. So that's not the right place to put it.

I am new to D8. So I should declare my own method in MaintenanceModeInterface.php.
I should create the method in MaintenanceMode.php.
How should I access the Enabled /disabled boolean in my new method ?

I don't think it's that simple.

Adding a method to set the status in MaintenanceModeInterface is an API change. I think this one should wait for the maintainer to look at.

Hi
I just crossed the perfect example on why this issue should be taken much seriously: a admin user saw the "outdated message", so thinking doing the right thing he follows the link and follow the update process putting the site in maintenance mode... The update failed but he didn't notice was still in maintenance mode because he was already logged in.
He do it twice over several months...
It take a while to debug it since there is no drupal log, and webserver log didn't show any visit to maintenance page.

Ok, the fault is mainly on the dev team side (update module enable on production site and bad permissions) but still I think this issue shoul get a higher priority (or the first patch should be accepted)

See also: https://github.com/drush-ops/drush/issues/5365

FWIW the last patch still applies cleanly against 9.5.x.

Thanks, I just add it to my `composer.json`.

So the patch number 17 could be marked as RTBTC

The patch still applies cleanly. I don't see any outstanding requests for changes. Switching this to 'needs review.'

The issue summary is pretty bare.

Also could use some kind of test coverage. Even simply coverage.

I am running core version 9.5.11 I tested the patch from #21. I was not able to get anything in logs when setting the maintenance mode through the UI. I tried clearing caches with Drush a few times but it didn't have any effect.

Patch from #17 also applies cleanly and now I got log messages when it was set. But I think the solution in #21 would be better if it worked because we also set maintenance mode with Drush and would be good to catch those also.

As maintenance mode is stored in state, it is good to log message in Drupal\Core\State::set() method. So, moving the fix given in #21 to State.php. This way, there will be a log for both cases - drush and form save. Verified the attached patch with Drupal 10.1.4

@sumit_saini Thanks for the patch - tests are still needed, so moving back to Needs Work.

The last patch is still missing the test(s).

And it would be great to update to update the issue summary (see #42). We should also include the summary of possible solutions (State::set() vs CacheCollector:set() vs SiteMaintenanceModeForm ..), as there are multiple patches with different approaches, so better to have this summarized. Thanks!

Rerolled.

For the issue summary update and tests.

We encountered an error when using this patch in core with Redirect 404 submodule of Redirect.

After upgrading to Drupal 10.2, I get the following error when trying to disable the maintenance mode in en/admin/config/development/maintenance:

The website encountered an unexpected error. Try again later.

TypeError: Drupal\system\Form\SiteMaintenanceModeForm::__construct(): Argument #1 ($logger_factory) must be of type Drupal\Core\Logger\LoggerChannelFactory, Drupal\redirect_404\Render\Redirect404LogSuppressor given, called in /var/www/html/web/core/modules/system/src/Form/SiteMaintenanceModeForm.php on line 74 in Drupal\system\Form\SiteMaintenanceModeForm->__construct() (line 58 of core/modules/system/src/Form/SiteMaintenanceModeForm.php).

Drupal\system\Form\SiteMaintenanceModeForm::create(Object) (Line: 28)
Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition('\Drupal\system\Form\SiteMaintenanceModeForm') (Line: 48)
Drupal\Core\Controller\HtmlFormController->getFormObject(Object, '\Drupal\system\Form\SiteMaintenanceModeForm') (Line: 58)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 627)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 704)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)