Problem/Motivation

The MaintenanceModeSubscriber uses a request attribute to specify whether the site is in maintenance mode for the currently logged in user. Also the subscriber is split up into two methods, one running early and one running later in the request. The early method determines whether the site is in maintenance mode, while the later renders the maintenance page if necessary.

The purpose of this split is to allow customization of the maintenance mode detection logic by additional request-subscribers, which are required to run within a specified range. The comment in \Drupal\Core\EventSubscribers\MaintenanceMode::getSubscribedEvents() states:

    // In order to change the maintenance status an event subscriber with a
    // priority between 30 and 40 should be added.

For #2286971: Remove dependency of current_user on request and authentication manager it is necessary to straighten a bit the maintenance mode logic, such that the MaintenanceModeSubscriber can be moved entirely after routing / authentication.

Proposed resolution

  1. Introduce \Drupal\Core\Site\MaintenanceMode (service: maintenance_mode with the following public methods:
    • applies($route_match) Returns TRUE if the site is in maintenance mode for the given route match.
    • exempt($account) Determines whether a user has access to the site in maintenance mode.
  2. On routes with the _maintenance_access option set to TRUE, maintenance mode will never be active. Use this option on user.login, user.pass and user.reset and remove the code responsible for excluding those routes from maintenance mode from the user maintenance mode subscriber.

This allows us to remove the MaintenanceModeSubscriber::onKernelRequestDetermineSiteStatus() and the _maintenance request attribute without scarifying customization options for contrib and site builders.

Remaining tasks

Review.

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol’s picture

Status: Active » Needs review
FileSize
21.3 KB

Status: Needs review » Needs work

The last submitted patch, 1: 2288665-remove-maintenance-request-attribute.diff, failed testing.

znerol’s picture

Oops, remove the editor autosave file.

znerol’s picture

The last submitted patch, 3: 2288665-remove-maintenance-request-attribute-3.diff, failed testing.

znerol’s picture

damiankloip’s picture

Nice, overall I think this patch is a really good idea.

  1. +++ b/core/lib/Drupal/Core/Site/StatusNegotiator.php
    @@ -0,0 +1,62 @@
    +   * The maintenance status.
    

    @var

  2. +++ b/core/lib/Drupal/Core/Site/StatusNegotiator.php
    @@ -0,0 +1,62 @@
    +    if (isset($account) && $account->hasPermission('access site in maintenance mode')) {
    +      return StatusNegotiatorInterface::SITE_ONLINE;
    +    }
    

    IMO we shouldn't have an account dependency or this logic in this service. Maintenance mode just uses the site's status. I think that should be in the MaintenanceModeSubscriber.

  3. +++ b/core/lib/Drupal/Core/Site/StatusNegotiator.php
    @@ -0,0 +1,62 @@
    +  public function overrideStatus($status = NULL) {
    

    Do you think we should allow a null status to be set?

  4. +++ b/core/lib/Drupal/Core/Site/StatusNegotiatorInterface.php
    @@ -0,0 +1,59 @@
    +  public function getStatus(AccountInterface $account = NULL);
    

    Same as last comment.

  5. +++ b/core/modules/user/src/EventSubscriber/MaintenanceModeSubscriber.php
    @@ -66,7 +93,7 @@ public function onKernelRequestMaintenance(GetResponseEvent $event) {
    +        $event->setResponse(new RedirectResponse(url('user/' . $this->account->id() . '/edit', array('absolute' => TRUE))));
    

    We should consider changing this to use the urlGenerator instead here.

znerol’s picture

Working on another issue I realized that probably it would be a good idea to introduce a chain of responsibility (like suggested by @Crell before in #2124749-5: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API. E.g. the user module excludes some routes from the maintenance mode (login and password reset). I think that the risk of performance regressions mentioned on the comment in the other issue is minor. Especially if we only allow exclusion of pages/users from maintenance mode (i.e. if maintenance mode is on, give contrib the chance to exclude some pages/users from it). This way production mode would be not affected at all.

znerol’s picture

FileSize
13.9 KB

This implements a simple chain-of-responsibility. Note that in this case it is not necessary to implement priorities (like e.g. in the breadcrumb implementation). This is because the response from MaintenanceExemption is boolean. I'm not too sure about the naming of the new classes / methods. Any opinions?

Note that this implementation also removes the requirement that the user maintenance mode subscriber is run before the core-one. In fact it looks like it may become possible to remove the user maintenance mode subscriber entirely in the long run, if we manage to implement exemption from the maintenance mode in a sane way.

znerol’s picture

znerol’s picture

Use the RouteMatch class in MaintenanceExemption.

znerol’s picture

Restore test-case in MenuRouterTest using maintenance exemption instead of a request subscriber.

dawehner’s picture

  1. +++ b/core/core.services.yml
    @@ -595,9 +595,14 @@ services:
    +    tags:
    +      - { name: service_collector, tag: maintenance_mode_exemption, call: addExemption }
    

    Not really sure wether this is a good name. I had to look it up, but I never saw that name somewhere else yet.

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
    @@ -101,70 +82,40 @@ public function __construct(StateInterface $state, ConfigFactoryInterface $confi
    +        $content = DefaultHtmlPageRenderer::renderPage($content, t('Site under maintenance'));
    

    Can we please use $this->t() which should be available ... I know the original code did not had that either.

  3. +++ b/core/lib/Drupal/Core/Site/MaintenanceExemptionInterface.php
    @@ -0,0 +1,30 @@
    +  public function exempt(RouteMatchInterface $route_match, AccountInterface $account);
    

    I really love the abstraction coming from the route match now. I wonder whether we really need getStatus and exempt(). getStatus could depend on route_match as well.

Crell’s picture

I really like the direction this is going!

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
    @@ -101,70 +82,40 @@ public function __construct(StateInterface $state, ConfigFactoryInterface $confi
    +      $route_match = RouteMatch::createFromRequest($event->getRequest());
    +      if (!$this->statusNegotiator->exempt($route_match, $this->account)) {
    +        // Deliver the 503 page if the site is in maintenance mode and the
    +        // logged in user is not allowed to bypass it.
    +        drupal_maintenance_theme();
    +        $content = Xss::filterAdmin(String::format($this->config->get('system.maintenance')->get('message'), array(
    +          '@site' => $this->config->get('system.site')->get('name'),
    +        )));
    +        $content = DefaultHtmlPageRenderer::renderPage($content, t('Site under maintenance'));
    +        $response = new Response('Service unavailable', 503);
    +        $response->setContent($content);
    +        $event->setResponse($response);
    

    Could we even go a step farther and turn this into a MaintenanceModeException, which then gets processed by the normal exception handling routines (and thus we don't need to do this duplicate weirdness and can get closer to eliminating drupal_maintenance_theme() and DefaultHtmlPageRenderer::renderPage())?

  2. +++ b/core/lib/Drupal/Core/Site/StatusNegotiator.php
    @@ -0,0 +1,74 @@
    +    $this->exemptionHandlers = array();
    

    This line is unnecessary. Just set the property to = []; above.

  3. +++ b/core/lib/Drupal/Core/Site/StatusNegotiator.php
    @@ -0,0 +1,74 @@
    +  /**
    +   * Add an exemption handler.
    +   *
    +   * @param \Drupal\Core\Site\MaintenanceExemptionInterface $handler
    +   *   A maintenance exemption handler.
    +   */
    +  public function addExemption(MaintenanceExemptionInterface $handler) {
    

    This needs to be part of the interface.

  4. +++ b/core/modules/user/src/EventSubscriber/MaintenanceModeSubscriber.php
    @@ -19,46 +21,59 @@
    +        $event->setResponse(new RedirectResponse(url('user/login', array('absolute' => TRUE))));
    

    Should get the generator injected and use a route name here.

  5. +++ b/core/modules/user/src/Site/MaintenanceExemption.php
    @@ -0,0 +1,29 @@
    +    if ($route_name == 'user.login' || $route_name == 'user.pass' || $route_name == 'user.reset') {
    

    in_array() is a bit cleaner here. I don't know if it's any faster (probably not) but it's shorter and can be returned directly. Ie:

    return in_array($route_name, ['user.login', 'user.pass', 'user.reset']);
    
znerol’s picture

Adressed #10.1 and #10.5 and #9.2.

Note that it is necessary to restrict the maintenance-mode check to the master request because the symfony exception listener spawns a subrequest to pass the exception into the ExceptionController. Throwing the ServiceUnavailableHttpException when attempting to respond to the ServiceUnavailableHttpException leads to recursion.

Also note that the call to drupal_maintenance_theme() is gone. I do not understand how to use $this->htmlPageRenderer such that it renders the maintenance theme. Therefore I simply copied over the relevant code from on500Html() to on503Html() which uses DefaultHtmlPageRenderer instead of the injected instance.

Regarding #9.3:

I wonder whether we really need getStatus and exempt()

I think we should remove getStatus() entirely and use $this->state->get('system.maintenance_mode') (like HEAD). Otherwise we would also have to introduce a setStatus() method and use it in the SiteMaintenanceModeForm. Will address #10.2, #10.3 when exploring that direction.

#10.4 is addressed in #2288911: Use route name instead of system path in user maintenance mode subscriber. However due to the overlaps I may just merge that one into this issue.

znerol’s picture

Replace the StatusNegotiator with ChainMaintenanceExemption and add appropriate interfaces. Addresses #9.3, #10.2, #10.3.

The last submitted patch, 15: 2288665-remove-maintenance-request-attribute-15.diff, failed testing.

Status: Needs review » Needs work

The last submitted patch, 16: 2288665-remove-maintenance-request-attribute-16.diff, failed testing.

znerol’s picture

Manual testing also reveals that we now have the Operating in maintenance mode message when the login page is accessed by an anonymous user.

This means that we cannot use exempt() for detecting whether a route should be exempted from maintenance mode as well as whether the logged in user has the permission to bypass it. I think it might be possible to simplify the whole thing and just extract maintenance-detection logic from the event subscriber into a dedicated service. Also we could add an attribute to the login and password reset route such that the status negotiation service is able to detect whether a page should be exempted from maintenance mode without having to implement a chain of responsibility for that part.

znerol’s picture

Tried #19, interdiff is against #15.

znerol’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 20: 2288665-site-maintenance-status-negotiator-20.diff, failed testing.

znerol’s picture

Turns out that symfony ExceptionListener::logException() invokes error_log() if the status-code of a HTTP exception >=500. Because of this and because of the recursion pointed out in #15 let's not throw an exception here.

znerol’s picture

Status: Needs work » Needs review
znerol’s picture

Polished the interface:

  • Removed SITE_OFFLINE and SITE_ONLINE constants.
  • Renamed StatusNegotiatorInterface to MaintenanceModeInterface,also StatusNegotiatorInterface::getStatus() to MaintenanceModeInterface::applies()
  • Renamed site_status_negotiator service into maintenance_mode service
znerol’s picture

Title: Remove _maintenance request attribute and replace it with a site status negotiator » Remove _maintenance request attribute and replace it with a maintenance mode service
Issue summary: View changes
znerol’s picture

Issue summary: View changes
Crell’s picture

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
    @@ -101,70 +80,40 @@ public function __construct(StateInterface $state, ConfigFactoryInterface $confi
    +        // Deliver the 503 page if the site is in maintenance mode and the
    +        // logged in user is not allowed to bypass it.
    +        drupal_maintenance_theme();
    +        $content = Xss::filterAdmin(String::format($this->config->get('system.maintenance')->get('message'), array(
    +          '@site' => $this->config->get('system.site')->get('name'),
    +        )));
    +        $content = DefaultHtmlPageRenderer::renderPage($content, $this->t('Site under maintenance'));
    

    Can we at least leave a todo on here, since DefaultHtmlPageRenderer::renderPage() is a crime against humanity that is slated for execution as soon as possible?

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
    @@ -101,70 +80,40 @@ public function __construct(StateInterface $state, ConfigFactoryInterface $confi
    +        // setting spage.
    

    Space in the wrong place.

znerol’s picture

xjm’s picture

Change record that will need an update for this issue:
https://www.drupal.org/node/2020005

I've added a reference to this issue so that it appears listed here.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

I think we're good here, then. Thanks, znerol!

We update the existing change notice AFTER this gets committed, right?

xjm’s picture

@Crell, yep, if it's only a minor change that is fine.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed baf2bba and pushed to 8.x. Thanks!

diff --git a/core/lib/Drupal/Core/Site/MaintenanceModeInterface.php b/core/lib/Drupal/Core/Site/MaintenanceModeInterface.php
index 064885c..45c4457 100644
--- a/core/lib/Drupal/Core/Site/MaintenanceModeInterface.php
+++ b/core/lib/Drupal/Core/Site/MaintenanceModeInterface.php
@@ -9,7 +9,6 @@

 use Drupal\Core\Routing\RouteMatchInterface;
 use Drupal\Core\Session\AccountInterface;
-use Drupal\Core\State\StateInterface;

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

Fixed on commit

  • alexpott committed baf2bba on 8.x
    Issue #2288665 by znerol: Remove _maintenance request attribute and...
xjm’s picture

The change record updates are still needed here. Let's update them and then the tag can be removed. Otherwise I will haunt you. :D

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.