While in maintenance mode, REST routes respond with HTML instead of XML/JSON/…

Yeah I guess we need to adapt \Drupal\Core\EventSubscriber\MaintenanceModeSubscriber::onKernelRequestMaintenance to support other formats as well.

1. \Drupal\Core\EventSubscriber\MaintenanceModeSubscriber doesn't live in the REST module
2. \Drupal\Core\EventSubscriber\MaintenanceModeSubscriber needs to be have better even when not using the REST module, but just using other formats

Therefore, moving to base system.

Hmm… interesting. That would not be possible, because that subscriber lives in core itself, but the serializer service lives in the serialization module in core. If we'd indeed have to go that way… then the only way to do this would be to override the MaintenanceModeSubscriber in the serialization module, which feels very very inappropriate.

Not sure yet how to solve this. Perhaps @dawehner or others have ideas.

Well, the idea world would be to return a MaintenanceMode domain object and then convert that to its appropriate response, but I think that kind of ship has sailed?
Maybe we want a dedicated listener for HTML and one for JSON?

I think #11 makes a lot of sense. Why do you think that wouldn't work anymore?

#12 is a step forward for core from an end user POV, but it's still going to be broken as soon as sites install modules that add support for more serialization formats. We should solve the generic problem, which is what #11 describes.

Why do you think that wouldn't work anymore?

Well, sadly we kinda dropped using that approach a while ago for Drupal itself, but sure, maybe reintroducing that here makes sense. On the other hand, for maintenance mode, we might want to return as early as possible, so that code would not work.

+++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
@@ -97,16 +100,39 @@ public function __construct(MaintenanceModeInterface $maintenance_mode, ConfigFa
+              $json_encoder = new JsonEncoder();
+              $content = $json_encoder->encode(['message' => $content], 'json');
...
+            case 'xml':
+              $xml_encoder = new XmlEncoder();
+              $content = $xml_encoder->encode(['message' => $content], 'json');
+              break;
+          }

IMHO we don't really need a full serializer object for something we could just use JsonResponse

Moving to rest.module for better visibility.

Mh, it would be cool to override this service from within the serializer module, so it can use whatever format exists out there, by levering the serializer service.

Mh, it would be cool to override this service from within the serializer module, so it can use whatever format exists out there, by levering the serializer service.

Agreed. So, basically: any module that adds another serialization format is responsible for overriding the maintenance mode subscriber to provide maintenance mode support for that particular format.

Here's a patch that does that, mirrored after \Drupal\Core\EventSubscriber\MaintenanceModeSubscriber. Manual testing shows it works. Now this still needs test coverage.

Assigning to damiankloip, because this is actually changing serialization module, which he is the maintainer of.

Just a general question, is is okay to initialize the serializer service now on every request?

Wow, that's a great question!

The answer is of course: probably not. But… how to then fix this? Should we make serializer a lazy service?

An alternative approach as talked with @damiankloip would be the following:

Throw a Maintenance mode exception.
Provide a exception handler which deals with HTML
Provide a exception handler for serializer.

Note that REST services do not have to respond in the format the client requests, see the HTTP spec. The important thing a client must check is the status code: and that will be 500 while the site is in maintenance mode. Go fix your client to check status codes and do not rely on the response body when the status is 500.

But… quoting yourself in #11:

Well, the idea world would be to return a MaintenanceMode domain object and then convert that to its appropriate response, but I think that kind of ship has sailed?

Why would #26 be okay? It'd be breaking BC in some sense. Although perhaps this is not considered an actual/official API.

But… quoting yourself in #11:

There are of course always alternative ways to implement things. For me conceptually, at this moment in time and space, maintenance mode is not the normal flow of things, but I agree we should avoid throwing exceptions if possible.

Why would #26 be okay? It'd be breaking BC in some sense. Although perhaps this is not considered an actual/official API.

Well, event subscribers are certainly not an API, or are they?

Go fix your client to check status codes and do not rely on the response body when the status is 500.

Does this mean this is a won't fix?

This answer sounds eminently sensible to me.

IMHO its still odd that we return only HTML but I agree, clients have been always able to workaround it.

The question is whether clients should show it somehow that the sites is in maintenance mode or just guess based upon the status code.

The question is whether clients should show it somehow that the sites is in maintenance mode or just guess based upon the status code.

Indeed. But … would they be able to parse <response>The site is in maintenance mode…</response> in a sane way in case of XML? They'd be expecting a completely differently structured response anyway.

503 == "service unavailable" == maintenance mode, just without the friendly message. In case of REST, you need to provide your own friendly messages anyway.

So the more I think about it, the more klausi's point makes sense.

#34: I think it's fair to expect a REST API consumer to simply ignore the response body in case of a HTTP 5xx response. I think that if we do this, we don't do what I did in #23, where I put the site maintenance message in the response body. We just send an empty body: the 503 alone is sufficient.

So yeah what about providing a really simple implementation for now: For HTML return the HTML page, for everything else return 'text/plain' with just the message?

#36: I think that sounds eminently sensible, especially when considering #27: Note that REST services do not have to respond in the format the client requests, see the HTTP spec.

#37 Yeah exactly, this was my idea. Sadly "Giraffe" is not valid JSON. This would have been even better.

So, something like this then.

00:00:24.253 PHP Parse error:  syntax error, unexpected ';' in ./core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php on line 103

I suck.

+++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
@@ -90,10 +91,21 @@ public function __construct(MaintenanceModeInterface $maintenance_mode, ConfigFa
+        $message = SafeMarkup::format($this->config->get('system.maintenance')->get('message'), array(
+          '@site' => $this->config->get('system.site')->get('name'),
+        ));

Could we move

        $content = Xss::filterAdmin(SafeMarkup::format($this->config->get('system.maintenance')->get('message'), array(
          '@site' => $this->config->get('system.site')->get('name'),
        )));

into its own method?

-        $content = Xss::filterAdmin(SafeMarkup::format($this->config->get('system.maintenance')->get('message'), array(
-          '@site' => $this->config->get('system.site')->get('name'),
-        )));
-        $response = $this->bareHtmlPageRenderer->renderBarePage(['#markup' => $content], $this->t('Site under maintenance'), 'maintenance_page');
+        $response = $this->bareHtmlPageRenderer->renderBarePage(['#markup' => $this->getSiteMaintenanceMessage()], $this->t('Site under maintenance'), 'maintenance_page');

I removed Xss::filterAdmin() … because #markup gets that done automatically :) (See \Drupal\Core\Render\Renderer::ensureMarkupIsSafe().)

This looks like a great solution for the problem. Now some simple non HTML tests would be nice.

Done. Test-only patch = interdiff.

I should've uploaded the FAIL patch first. My bad.

Cool

#50: that would be a separate problem. Please create a nee issue for that.

+++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
@@ -90,18 +91,23 @@ public function __construct(MaintenanceModeInterface $maintenance_mode, ConfigFa
+      if ($request->getRequestFormat() !== 'html') {
+        $response = new Response($this->getSiteMaintenanceMessage(), 503, array('Content-Type' => 'text/plain'));
+        $event->setResponse($response);
+        return;
+      }
+
       if (!$this->maintenanceMode->exempt($this->account)) {

Why's this outside the exempt check? I think it should be in it.

Eh… you're absolutely right.

It's confusing that two separate comments are now joined together.

aneek: I'm saying the changes you made in #55 are correct, but there should have been a blank line in between the comments.

@aneek addressed the sole remark alexpott had in #53 over comments #54-#66. Restoring @dawehner's RTBC status from #49.

Looks exactly like my patch :) And yes, I posted to the wrong issue ...

+++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
@@ -180,6 +180,13 @@ public function testSerializerResponses() {
+
+    // Set the site to maintenance mode.
+    $this->container->get('state')->set('system.maintenance_mode', TRUE);
+    \Drupal::service('page_cache_kill_switch')->trigger();
+    $this->drupalGetWithFormat('test/serialize/entity', 'json');
+    // Verify that the endpoint is unavailable for anonymous users.
+    $this->assertResponse(503);

This is failing and I think, but I'm not sure, might be part of another patch.

True, see #50 + #52. #58 brought the fix to this issue. It feels like it's within scope, because it's again about REST responses + maintenance mode. However, if you really want, we could split that off to #2732309: REST views ignore the maintenance mode setting.

And it definitely used to pass :( Queued for re-testing.

Failed again. I wonder what changed in REST views code or elsewhere that is causing this to now fail?

This particular subset of the test never worked, see #55 and following.

What happens is the following:

    // PageCache miss
    $expected = $serializer->serialize($entities, 'json');
    $actual_json = $this->drupalGetWithFormat('test/serialize/entity', 'json');
    $this->assertIdentical($actual_json, $expected, 'The expected JSON output was found.');
    $expected = $serializer->serialize($entities, 'xml');
    // PageCache is set.

    // PageCache hit
    $this->drupalGetWithFormat('test/serialize/entity', 'json');

Questions:

1. Is it okay to invalidate the caches in the test?
2. Why are we not invalidated any cache in \Drupal\system\Form\SiteMaintenanceModeForm::submitForm?
3. Why does the page cache not have a dedicated cache tag, so it can be cleared for itself?

Why are we not invalidated any cache in \Drupal\system\Form\SiteMaintenanceModeForm::submitForm?

#2453351: Maintenance mode message ends up in page cache, served endlessly ensured that nothing ends up in page cache while maintenance mode is on.

But it looks like maintenance mode is designed to NOT prevent page cache from working completely; it seems it's designed to still let cached pages for anonymous users (either in Page Cache or in Varnish). That actually makes sense: if anonymous responses are cached in Page Cache/Varnish… which means that end users can continue to see sensible output… then why would we force them to see a maintenance message?

Why does the page cache not have a dedicated cache tag, so it can be cleared for itself?

Because it must be treated the same way as any other reverse proxy, such as Varnish.

So, as far as I'm concerned, the problem is that the additions to StyleSerializerTest are happening after PageCache has been filled. It should test PageCache behavior before PageCache has been filled, because once filled, it's intentional that users get to see the cached responses.

+1 to #75 - yes we should show users cached pages if we have them.

So then #74 is wrong. Discussed with @dawehner: let's move that test into a separate test method, so that it runs in a unique environment, i.e. with a cold page cache.

Adding a new method.

Committed f692cd0 and pushed to 8.1.x and 8.2.x. Thanks!

I don't think MaintenanceModeSubscriber should be considered API therefore adding a protected method to solve a bug is fine in a patch release.

diff --git a/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
index 37d7575..57d43ec 100644
--- a/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
+++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
@@ -3,7 +3,6 @@
 namespace Drupal\Core\EventSubscriber;
 
 use Drupal\Component\Utility\SafeMarkup;
-use Drupal\Component\Utility\Xss;
 use Drupal\Core\Config\ConfigFactoryInterface;
 use Drupal\Core\Render\BareHtmlPageRendererInterface;
 use Drupal\Core\Routing\RouteMatch;

Remove unused use on commit.