Invalid JSON:API responses when maintenance mode is on

Nice catch! Would you mind adding a regression test to \Drupal\Tests\jsonapi\Functional\JsonApiRegressionTest? :)

@ndobromirov, that sounds right to me.

@Wim Leers, do you know if REST handles this at all?

Good question.

\Drupal\Core\EventSubscriber\MaintenanceModeSubscriber was updated by #2653318: While in maintenance mode, REST routes respond with HTML instead of XML/JSON/… to provide a custom error response for non-HTML requests when maintenance mode is on. I bet this is the You get a non-valid jsonapi response and broken clients. result that @ndobromirov is describing.

JSON:API could add an event subscriber that runs earlier and does return a valid JSON:API response, albeit still an error response.

@ndobromirov, I like the retry-after idea!

I think we need to return a JSON:API compliant response for any JSON response. That could looks like this:

{
  "errors": [{
    "status": "503 Service Unavailable",
    "detail": "{{maintenance message}}"
  }]
}

I don't really think a new header is necessary TBH.

Regarding the proposed solution, I think option #2 is best as well. JSON:API is a stable module in Drupal core and Drupal is API-first, so why shouldn't the core event subscriber have a special case for returning a JSON:API compliant response? It seems more maintainable to do that than to fuss with a new event subscriber and service priorities.

Note that the header solution is generic and payload format agnostic. It will work for core's REST, GraphQL as well as for JSON:API transparently, as it's a solution on HTTP level.

What's the practical advantage of it being format agnostic? I think all real world HTTP clients expect one format or another. As for JSON:API, its spec says: A JSON object MUST be at the root of every JSON:API request and response containing data. This object defines a document’s “top level”.

Unless we send no response body at all, we'll still be in clear violation of the spec. IOW, the current text/plain response breaks clients and makes maintenance mode harder to debug, because all you see in the browser console is a JSON parse error.

I'm not really opposed to the header, I like it actually, I just don't think it's a standalone solution.

Thanks! 👍🙏

Unfortunately, this patch is adding awareness of JSON:API to Drupal core. We need to add an event subscriber to JSON:API itself; JSON:API needs to remain self-contained. Let's add a Drupal\jsonapi\EventSubscriber\MaintenanceModeSubscriber instead.

#20: Why can't JSON:API's maintenance mode subscriber run with a higher priority? \Drupal\user\EventSubscriber\MaintenanceModeSubscriber only really makes sense for HTML responses.

#21: We should avoid changing existing priorities if at all possible, otherwise we risk breaking BC. :(

+++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
@@ -0,0 +1,59 @@
+      && $route_match->getRouteObject()->getDefault(Routes::JSON_API_ROUTE_FLAG_KEY)

This line is failing, getRouteObject() is returning NULL. You probably want to look at how we do this elsewhere :)

Tested. It works.

As a comment: if you browse the JSON API URL (e.g. http://localhost/jsonapi/user/user) with the web browser, it returns the plain maintenance message without the JSON formatting. I guess is not a problem since the browser is accepting HTML as response, but just to let you know.

Thanks for pushing this forward! 🥳

1. +++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
   @@ -0,0 +1,59 @@
   + * Class MaintenanceModeSubscriber.
   + *
   + * @package Drupal\jsonapi\EventSubscriber
   

   This needs to be cleaned up.

2. +++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
   @@ -0,0 +1,59 @@
   +    $events[KernelEvents::REQUEST][] = ['onKernelRequestMaintenance', 35];
   +    $events[KernelEvents::EXCEPTION][] = ['onKernelRequestMaintenance', 35];
   

   We must document why we use these particular priorities.

   See \Drupal\big_pipe\EventSubscriber\HtmlResponseBigPipeSubscriber::getSubscribedEvents() for an example.

3. +++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
   @@ -0,0 +1,59 @@
   +   * {@inheritdoc
   

   Missing trailing }.

4. +++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
   @@ -0,0 +1,59 @@
   +    // Ensure that...
   

   Incomplete :)

5. +++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
   @@ -0,0 +1,59 @@
   +    $is_valid_request = TRUE
   

   Ehm … TRUE && $something is the same as $something, so we can remove this :)

6. +++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
   @@ -0,0 +1,59 @@
   +    \Drupal::service('page_cache_kill_switch')->trigger();
   

   This would need to be injected.

   But the existing maintenance mode subscriber in core also doesn't do that. Perhaps for performance reasons? Let's try to find the answer in the last commit that touched it and let's document it properly.

7. +++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
   @@ -0,0 +1,59 @@
   +        "status" => $this->t("503 Service Unavailable"),
   

   This does not need to be translated!

8. +++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
   @@ -0,0 +1,59 @@
   +    $response = new JsonResponse($data, 503, ['Content-Type' => 'application/vnd.api+json']);
   

   @gabesullice could you check whether this should use new ErrorCollection(…)?

#30.8. It should... unless for some reason the `ResourceResponseSubscriber` that JSON:API provides wouldn't get called after this phase of the request.

#30.6 For the kill switch, I don't know if there is any reason it wasn't injected per #2453351: Maintenance mode message ends up in page cache, served endlessly.
#30.8 \Drupal\jsonapi\JsonApiResource\ErrorCollection receives an array of \Symfony\Component\HttpKernel\Exception\HttpExceptionInterface. We could construct an HttpException with 503 and pass it to error collection. But the question in #31 remains if ResourceResponseSubscriber would be called (which is why we need tests.)

I did notice the returned error object did not contain an array of errors, per https://jsonapi.org/examples/#error-objects.

It still needs a test.

Going through old issues, this is a nice addition and does seem close to done.

#30.6 @wimleers I think this was an oversight, the bug was critical. I remember pretty well when we also notices that bug :x The commit introducing this only contains that line and a test. I think a task issue to move the call to injection is in place.

The current patch doesn't work when I test it, seems the getRequestFormat alsways returns html. Digging a little deeper is seems routes are only set later, priority needs to be 31 max. I think this might have something to do with: KernelEvents::REQUEST => [['onKernelRequest', 32]], in the \Symfony\Component\HttpKernel\EventListener\RouterListener since the metadata only gets added to the request after that event, which in turn allows us to find out the request wants to receive api_json.

So, lets get the review in ^^

1. So i think the following is needed to fix the RouteMatch issue:

   +++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
   	-    $events[KernelEvents::REQUEST][] = ['onKernelRequestMaintenance', 35];
   	+    $events[KernelEvents::REQUEST][] = ['onKernelRequestMaintenance', 32];
   

2. Please use the proper classes here. As mentioned earlier, you could do the following:

   $httpException = new HttpException(503, strip_tags($this->getSiteMaintenanceMessage()));
   $document = new JsonApiDocumentTopLevel(new ErrorCollection([$httpException]), new NullIncludedData(), new LinkCollection([]));
   $response = new ResourceResponse($document, $httpException->getStatusCode(), ['Content-Type' => 'application/vnd.api+json']);
   $event->setResponse($response);
   

3. And ofcourse, we do need a test ;) Although the test in the main implementation should help a lot to get that working.

I just found this issue because I have the same problem with my decoupled site. But I'm not using JSON:API as the requirements are kinda complex and very difficult to implement with JSON:API without producing tons of overhead on both server and client side. Instead I'm using custom controller actions. In other scenarios where you might have just a few simple REST endpoints (like providing a health check endpoint with some structured data – i. e. including if Drupal can connect to the database – consumable by uptime monitoring services) using JSON:API might be overkill.

To summarize, the same handling as for JSON:API request should work for "normal" JSON requests as well, that means when the request Content-Type header has the value application/json and not only when it has application/vnd.api+json. This should return

new JsonResponse($data, 503, ['Content-Type' => 'application/json']);

The same problem exists when the global exception handler kicks in and returns an error response (mainly HTTP 4xx, HTTP 500, HTTP 501 and HTTP 502 as they can occur before your controller action gets executed, which of course should implement error handling and return proper responses), both for JSON and JSON:API requests.

Here's a shot at test coverage for jsonapi in maintenance mode.

The patch in #34 doesn't cause this to pass, by the way. That's because this condition is not met:

+++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
@@ -0,0 +1,63 @@
+      $request->getRequestFormat() === 'api_json'

Seems to me *any* request to a jsonapi should return json, but this is pretty far out of my comfort zone.

UPDATE: Since I originally posted, I read #38 more carefully and confirmed that changing 35 to 32 causes the new test case to pass.

Ok, here's a patch that updates #34 with the suggestions from #38. The updated comments may be inaccurate, so review them carefully! This also has the test from #40. That test still needs review too.

In my manual testing, the maintenance mode message looks nice!

{
  "jsonapi": {
    "version": "1.0",
    "meta": {
      "links": {
        "self": {
          "href": "http://jsonapi.org/format/1.0/"
        }
      }
    }
  },
  "errors": [
    {
      "title": "Service Unavailable",
      "status": "503",
      "detail": "Drush Site-Install is currently under maintenance. We should be back shortly. Thank you for your patience.",
      "links": {
        "via": {
          "href": "https://core93.ddev.site/jsonapi/taxonomy_term/tags"
        },
        "info": {
          "href": "http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.5.4"
        }
      }
    }
  ]
}

Thanks for the help. I've gone through the code and have a few comments.

1. +++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
   @@ -0,0 +1,62 @@
   +    $httpException = new HttpException(503, strip_tags($this->getSiteMaintenanceMessage()));
   

   I don't think we need striptags here? If i check for the text/plain response in core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php line 117 the response is not stripped.

   $response = new Response($this->getSiteMaintenanceMessage(), 503, ['Content-Type' => 'text/plain']);

   Why should we do it here then?

2. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -43,6 +43,12 @@ public function testRead() {
   +    // 0.1 Request in maintenance mode returns valid JSON.
   

   I dont really like 0.1 here, perhaps just put it in the bottom of this list of tests.

   Also, when i look at the tests that look for example at the 404 (line 319) is also checks for $this->assertSession()->responseHeaderContains('Content-Type', 'application/vnd.api+json');. I think that makes sense here also as a check.

Thanks for the review, @bbrala.

I've made the requested adjustments, which I think are both good ideas. I'm happy to adjust the placement of the new test case again if you don't like where it ended up.

Regarding the strip_tags ("Why should we do it here then?"), that was a copy/paste job from comments in #38. It seems like we're probably fine matching the behavior of core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber

Thanks for the changes, I think this is all fine like this. Did some extra manual testing and seems to work like a charm.

Went through the issue and added the correct credits.

🥳 way to go everyone, #44 looks great

That was a random test fail related to this issue: #3208791: [random test failure] Random fail in LayoutBuilderDisableInteractionsTest

Setting back to RTBC. Thanks all!

Sorry, I have a few remarks 😅

1. +++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
   @@ -0,0 +1,62 @@
   +/**
   + * Maintenance mode subscriber for JSON:API requests.
   + */
   +class JsonapiMaintenanceModeSubscriber extends MaintenanceModeSubscriber {
   

   This needs

    * @internal JSON:API maintains no PHP API. The API is the HTTP API. This class
    *   may change at any time and could break any dependencies on it.
   

   added to the docblock, just like all other JSON:API event subscribers.

2. +++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
   @@ -0,0 +1,62 @@
   +    $httpException = new HttpException(503, $this->getSiteMaintenanceMessage());
   

   1. Shouldn't we suggest a Retry-After?

      That'd allow API clients to retry automatically after N seconds.

      What if we ask it to retry in 60 seconds? 🤓

   2. Drupal core coding standards require to only use camelCase for method names and class properties. This is a local variable and should hence be named $http_exception 🙈
   3. Nice reuse of a parent method! 🤩

3. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -513,6 +513,14 @@ public function testRead() {
   +    $this->assertNotEmpty(Json::decode($response));
   

   Shouldn't we assert the message?

1. Good point.
2. Retry after is neat, we could do that if you like. 60 seconds feels like an eternity though, but I don't really want to start a discussion on how long it should be hehe.
3. Coding standards, you are right. I wanted to say; not if is consequent, but it isn't. Nice one.
4. Yes that is more precise.

RE: Retry-After: I just said 1 minute because maintenance mode tends to be on relatively long. But +1 to make it as short as possible. How about 5 + rand(0, 5) (i.e. between 5 and 10 seconds, to reduce the thundering herd effect).

That seems like it's reasonable. Then clients might actually not even notice that much:)

@Wim Leers and @bbrala, all good ideas. I felt tester's remorse for not asserting the message, so I'm glad that got called out actually.

Here's an update that I think checks all the boxes. Let me know if I missed or misunderstood anything.

It's that dern LayoutBuilderDisableInteractionsTest again. Setting back to Needs Review.

Reviewed, all feedback has been addressed, thanks!

+++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
@@ -0,0 +1,71 @@
+    // This must run before MaintenanceModeSubscriber (30) so that this one
+    // takes priority, but after
+    // \Symfony\Component\HttpKernel\EventListener\RouterListener (32) because
+    // that one adds metadata related to the request format that we need.
+    $events[KernelEvents::REQUEST][] = ['onKernelRequestMaintenance', 32];

Reading this I keep on thinking well why is this not 31 - the answer is because \Drupal\user\EventSubscriber\MaintenanceModeSubscriber is on 31 and this needs to come before that. Which begs a question about logging out. The user module subscriber will log out accounts that aren't exempt but with this change now jsonapi will not. Are we sure we want this behaviour change? This behaviour was added in #363580: OpenID login fails when in maintenance mode. I've skimmed the issue and it's not 100% obvious as to why but one reason that makes some sense if you hit the maintenance mode with a user that is not exempt then logging out gives you a chance to log in as a user that is exempt if you have access to such an account.

+++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
@@ -0,0 +1,71 @@
+    // Retry-After will be 5 to 10 seconds. The goals are to keep it short and
+    // to reduce the thundering herd problem.
+    $retry_after_time = 5 + rand(0, 5);

It feels as though this should be configurable in some way. People often know how long a site will be out for for maintenance and 5 seconds might be woefully short.

A couple of thoughts: we could make it a setting that people can configure in settings.php, we could add it to the jsonapi.settings configuration or we could add it as class property so at least if a site wants to decorate and extend this event listener it wouldn't have to change much.

I think adding it to the jsonapi.settings is probably the best place to do it. I think we don't need to add a UI here.

#58
You have a point about the logging out. But the fact is that it is not possible to login using jsonapi so any logging in would go though another channel.

Thinking about it, I think in this situation where you would be consuming this API though an application you might rather not be logged out. Combined with the retry-after this would mean that the whole process has a little impact as possible since the clients would reconnect after a while.

The only issue might be if there are changes deployed while in maintenance mode which need a relogin to work as they should.

What would you recon, should we move the listnener to make sure people are logged out? I really think this might be a bad experience in decoupled situations.

#59:
That makes sense. Adding a config variable for this is probably best. I added the "Needs change record" tag since we would need a change record for this change and the new setting i think.

I think the whole logging out if you hit the site and it is maintenance mode is a little odd. That's why I was trying to find out why it was added. I'm okay with not logging out but I think we need to document this difference in the code and explain the priority of 32 a little better in the code. The priorities feel very very fragile here fwiw. I certainly don't think we should move the listener to after the user one. I do wonder if we should make the user one html only since it is doing a redirect. And then the priority of the new listener could be 31 as well.

The only issue might be if there are changes deployed while in maintenance mode which need a relogin to work as they should.

If this is the case then it is up to the update to take an action that would result in all users being logged out.

So that would mean the following tasks:

1. Add $request->getRequestFormat() == 'html') to Drupal\user\EventSubscriber\MaintenanceModeSubscriber.
2. Change the priority of JsonapiMaintenanceModeSubscriber to 31 and expand the documentation in the code to describe why we won't logout as done in the Drupal\user\EventSubscriber\MaintenanceModeSubscriber.
3. Add a setting to jsonapi (without an interface element) named: jsonapi.settings.maintenance_header_retry_after.min and jsonapi.settings.maintenance_header_retry_after.max to allow control over this setting by implementations.

   Change the code to read something like:

   $retry_after_time = rand($this->config->get('jsonapi.settings.maintenance_header_retry_after.min'), $this->config->get('jsonapi.settings.maintenance_header_retry_after.max'));
   

4. We also need a hook_update_N to migrate the new configuration value. Default: min 5, max 10.

Here's a patch that I think includes all of the requested changes.

I'm not sure what the N for the update_N is supposed to be. I made my best guess. I included a new test for the update path. I copied off of the one in the action module.

I added fields for the new min/max values to the settings form with a little validation. Do we need to add tests for the Form at this point? I couldn't find tests for the existing form, which was pretty darn simple.

Thanks for the patch!

N would be the update hook number. I can't tell you right now what that should be.

In the task list I mentioned not adding something to the interface, this is an advanced setting that doesn't really need a way to adjust in the admin interface. That also skips the need for some other core gates I think so I would prefer it without the formelements.

I've not looked at the code, I'm on mobile ATM. :)

In the task list I mentioned not adding something to the interface

You did indeed! (#62.3) I missed that apparently. No big deal. I'll undo the changes to the form in the next patch. I'm sure there will be other changes needed to the one in #63. That's a big interdiff to have everything else be perfect!

Here's a new patch without changes to the form. I'm posting an interdiff between this one and #54. Let's forget that #63 ever existed.

I've gone through the code. Also i've tested this locally and it works before the update although there is no config, and after the update the config is correct. When i change the values and import the config, the values are also updates on the header. Yay.

1. +++ b/core/modules/jsonapi/jsonapi.install
   @@ -82,3 +82,15 @@ function jsonapi_requirements($phase) {
   + * Set values for maintenance_header_retry_after min and max.
   

   I'd like a @see here to this issue for clarification.

2. +++ b/core/modules/jsonapi/jsonapi.install
   @@ -82,3 +82,15 @@ function jsonapi_requirements($phase) {
   +function jsonapi_update_9400() {
   

   I would name this 9401 as that seems to be the consensus when i look through git history of earlier update hooks.

3. +++ b/core/modules/jsonapi/tests/fixtures/update/jsonapi.php
   @@ -0,0 +1,54 @@
   +<?php
   

   Cool to see this, haven't come across this yet before :)

4. +++ b/core/modules/jsonapi/tests/src/Functional/Update/JsonApiUpdatePathTest.php
   @@ -0,0 +1,50 @@
   + * @group legacy
   

   Legacy? Why would this be legacy? If a look at a lot of the updatepatchtests it seems there is no real consensus on how they are tagged unless i miss something.

   I would say, remove the legacy tag.

5. +++ b/core/modules/jsonapi/tests/src/Functional/Update/JsonApiUpdatePathTest.php
   @@ -0,0 +1,50 @@
   +  public function testUpdate9400() {
   

   Dont forget to change the ID here also.

Only thing I was not 100% sure about is if this should be a hook_update or hook_post_update, but it seems config changes are fine in hook_update from what I can tell.

Legacy? Why would this be legacy?

In the UpdatePathTestBase class the docblock reads Ensure that the test is in the legacy group..

But searching the codebase I see what you see: hugely inconsistent tagging. I would say Update is the most common tag to have.

That docblock comment was added in this issue, though it doesn't give any context. That issue spun off of another one that features this comment which is the best I can find.

Is it ok if I leave it?

Ok, keep legacy and jsonapi then, we'll see when we get to RTBC what is preferred. :)

Updates attached! Just changing 9400 to 9401 and adding the @see.

I've written a change record.

Thanks, @bbrala! I think it looks good. I added a couple notes and fixed a typo or two.

As tests also pass, code looks good, handing of as RTBC again :) Thanks for the work @danflanagan8!

+++ b/core/modules/jsonapi/tests/src/Functional/Update/JsonApiUpdatePathTest.php
@@ -0,0 +1,50 @@
+ * @group legacy

The reason this is marked legacy is that update tests are prone to triggering deprecated code paths (by their very nature) and this prevents the test from failing for the reason because it is okay if they do trigger deprecated code.

There is a update path test that is not marked legacy to ensure that our update process is not triggering legacy in and of itself.

1. +++ b/core/modules/jsonapi/config/install/jsonapi.settings.yml
   @@ -1,2 +1,5 @@
   +maintenance_header_retry_after:
   +  min: 5
   +  max: 10
   

   Since we have no UI I think it would be good if the config key name had seconds in it so people know the unit.

   How about: maintenance_header_retry_seconds

2. +++ b/core/modules/jsonapi/jsonapi.install
   @@ -82,3 +82,17 @@ function jsonapi_requirements($phase) {
   +  $config->save();
   

   This needs to be $config->save(TRUE) because don't want to recalculation all config schema at this point and we can guarantee that these types are correct.

Here are those few changes, @alexpott. I'll also update the change record to reflect the updated name for the config key.

This needs to be $config->save(TRUE) because don't want to recalculation all config schema at this point and we can guarantee that these types are correct.

You actually mentioned that, oops forgot it in the task list.

Tests are green, changes seem to have addressed all feedback.

The reason this is marked legacy is that update tests are prone to triggering deprecated code paths (by their very nature) and this prevents the test from failing for the reason because it is okay if they do trigger deprecated code.

Good to know. But it seems there is a lot of update tests without that tag :)

+++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
@@ -0,0 +1,76 @@
+    $retry_after_time = rand($header_settings['min'] ?? 5, $header_settings['max'] ?? 10);

Let's not add the default values here. Once the update has run these values will never be null. And a site is allowed to be "broken" before all of the hook_update_N() have been run.

Once the update has run these values will never be null.

Unless someone runs a config import before running a config export. I know that's developer error but it's easy enough to make that mistake.(I'm pretty sure I've done it!) I tested what happens if a developer were to make this mistake. The code>Retry-After value ends up being zero, and there's a php warning in the logs:

Warning: Trying to access array offset on value of type null in Drupal\jsonapi\EventSubscriber\JsonapiMaintenanceModeSubscriber->onKernelRequestMaintenance() (line 66 of /var/www/html/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php)

If we remove the default values from the code, I think we should add an error to jsonapi_requirements if the new config values aren't set. Seeing as the jsonapi settings form does not expose these settings, this might all be especially confusing without a message.

I'm also wondering if the @see in jsonapi_update_9401 would be improved by pointing to the change record instead of this issue. Thoughts anyone?

I'll wait to make changes until I get a response. I'm setting the status to NR to indicate that.

would be improved by pointing to the change record instead of this issue. Thoughts anyone?

I think pointing to the change record is the norm, and makes it easier to understand why the upgrade is running.

if the new config values aren't set. Seeing as the jsonapi settings form does not expose these settings, this might all be especially confusing without a message.

Drupal core doesn't do this right now. So I think it's fine allowing a warning to be present if the upgrade wasn't run / or config was lost.

Thanks, @mglaman. Can I get some clarification on this comment:

Drupal core doesn't do this right now. So I think it's fine allowing a warning to be present

Are you in favor of adding to jsonapi_requirements? Or are you opposed to adding a new check to jsonapi_requirements?

I'm not in favor of adding to jsonapi_requirements. If we did, we'd have to bikeshed what the statement should be and how to best communicate. I see this issue holding up at least another month over that. And we can fallback on the fact Drupal core (for better or worse) hasn't handled this before as "we don't need to." And it is considered, in Drupal, the site is allowed to be partially broken between code deployment and updates finished.

Thanks again, @mglaman!

Here's an updated patch that:

1. Removes the default values from code (see #74)
2. Changes the @see to point at the change record instead of this issue.

I also touched up the IS so that the alternative proposed resolution has been removed.

@danflanagan8 there's way way more that can be broken if you don't manage your config through an update properly. This is one of the things the drush deploy command tries to help with.

The @see should either be removed or point to the CR. It's doesn't need to point to the issue.

Hmm, @alexpott

The change record was already added, see linked change record. The patch @danflanagan8 provided removed the default value as requested and updates the @see to the changerecord?

Not sure why you've hidden those in comment #85

The changes I made to the IS in #84 got reverted too. I think there was some kind of accident. :)

As feedback was fixed, setting rtbc

Hmm, think you might be right there @danflanagan8

Yep I had the issue open too long. Sorry and thanks.

How to grab the drupal maintance text with graphQL to be displayed ?
and there must be a rule to only use this page id Drupal is actually in maintance?

#42 guide is all hard coded :-(

How to grab the drupal maintance text with graphQL to be displayed ?
and there must be a rule to only use this page id Drupal is actually in maintance?

This seems like a problem the GraphQL contrib would also need to solve on its own.

But GraphQL is just using json Data that Drupal hasto supply, for building react frontend

I was about to commit this when I realised that this issue contains quite a big behaviour change that we shouldn't do.

+++ b/core/modules/user/src/EventSubscriber/MaintenanceModeSubscriber.php
@@ -51,15 +51,20 @@ public function __construct(MaintenanceModeInterface $maintenance_mode, AccountI
+    // A user will only be logged out if this is an html request. For other
+    // request formats such as 'api_json', logging out the use may not be an
+    // appropriate behavior.
+    if ($request->getRequestFormat() === 'html') {

Doing some more thinking about this bit. I'm not sure we should make this change like this.

I think that we should consider making the current behaviour the default behaviour but making it overridable. I.e this event listener needs to trigger another event that the json api module listens for and handles. And the user module also needs to listen to and do the usual logout. The json api one would call stopPropagation() on the event it listens to if the request is has api_json as the format.

This would mean that we don't break anyone that is currently relying on this behaviour but also that we can fix this cleanly for json api in a way that the whole 30 - 32 event priority is not so important.

The pseudo code for this would look like this...

public function onKernelRequestMaintenance(RequestEvent $event) {
  $request = $event->getRequest();
  if ($this->maintenanceMode->applies($route_match)) {
    $this->eventDispatcher->dispatch($event, System::MAINTENANCE_MODE_REQUEST);
  }
}

Also this particular event listener would not be in the user module - it should be in system or core. FYI I'm not sure about SystemEvents - but it needs to be something like that and the system module does provide the maintenance mode state stuff. But OTOH core provides the maintenance mode config.

Also adding a new event will make it much simpler for modules like GraphQL to do their own thing.

I *think* I understand what @alexpott is saying in #97. Here's a patch that tries to check all those boxes.

The big change here is that there is a new event called MaintenanceModeEvents::MAINTENANCE_MODE_REQUEST that is dispatched by Drupal\Core\EventSubscriber\MaintenanceModeSubscriber when the site is in maintenance mode and the user is not exempt. That new event can get listened to by any module. In the patch here, json_api listens first, then user. If the event is still propagating then Drupal\Core\EventSubscriber\MaintenanceModeSubscriber will handle it.

It's a huge patch so there's a lot to look at, but I think in the end it simplifies everything. For example, that mysterious call to \Drupal::service('page_cache_kill_switch')->trigger(); is only in one place now.

Note that I added params to a few services. I did it in a BC way but didn't take the time to trigger deprecations at this point because I was lazy. Might as well get feedback first.

cs violation fix

1. +++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
   @@ -92,6 +102,10 @@ public function __construct(MaintenanceModeInterface $maintenance_mode, ConfigFa
   +    if (!$event_dispatcher) {
   

   Since we are adding a new argument, this should normally also include a deprecated message. See for example Drupal\jsonapi\Normalizer\LinkCollectionNormalizer::__construct.

2. +++ b/core/lib/Drupal/Core/Site/MaintenanceMode.php
   @@ -18,14 +20,27 @@ class MaintenanceMode implements MaintenanceModeInterface {
   +    if (!$config_factory) {
   

   Another extra argument, that should probably also include the deprecation message.

3. +++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeSubscriber.php
   @@ -0,0 +1,89 @@
   +    $event->setResponse($response);
   

   Should this also $event->stopPropagation(); as mentioned by @alexpott. This would stop the event going through and then rendering html.

+++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
@@ -157,6 +166,10 @@ protected function getSiteMaintenanceMessage() {
+      20,

I think this should be a really low priority.

And yes if a maintenance mode subscriber sets a response it should stop propagation.

After discussion on slack with alexpott, removed the 4th point in my review about seperating the logic into another place.

Slack thread:

Björn Brala (bbrala)  3 hours ago
@alexpott You had some feedback on the linked issue. I think i understood you correctly and reviewed the patch @danflanagan8 submitted. Could you just check if my review (bullet 4 in my review) is correct regarding your feedback? https://www.drupal.org/project/drupal/issues/3049048#comment-14334989

alexpott  2 hours ago
Commented.

Björn Brala (bbrala)  2 hours ago
Thanks. I asked about point 4. (the suggestion to core/modules/system/src/EventSubscriber/SystemMaintenanceModeSubscriber  which was the one i was most unsure of. Assume because you don't mention that in your comment you agree? ;)

alexpott  2 hours ago
I think it is okay for the same class to subscribe to two events - especially when they are so linked.

Björn Brala (bbrala)  1 hour ago
In my opinion it would make a lot of sense separating that event from the kernel handling. We are seperating logic into seperate events so we can be more precise regarding maintenance mode.
If we seperate the handling of the the maineance mode rendering to system this could also mean we don't need bare_html_renderer in the MaintenanceModeSubscriber.
Also consider, if you want to handle default mainenance mode differrently you can do this more easily if the firing of the event is in a different subscriber than the one handling it.
One reason not to do it might be the fact that it does change the signature a bit more, since it would remove the need for BareHtmlPageRendererInterface $bare_html_page_renderer and this is an signature change that is a little more annoying to handle.
Perhaps is actually makes sense to do that in a possible followup?
New

alexpott  8 minutes ago
I think it is totally fine for an event subscriber class to trigger an new event and then subscribe to same event to provide the default behaviour.

alexpott  7 minutes ago
Also consider, if you want to handle default mainenance mode differrently you can do this more easily if the firing of the event is in a different subscriber than the one handling it.
Then add an event subscriber and stop propagation. Exactly like what the JSON API module is doing. (edited) 

Björn Brala (bbrala)  1 minute ago
Ok, fine, thanks. :slightly_smiling_face:

if a maintenance mode subscriber sets a response it should stop propagation

The setResponse function automatically calls stopPropagation. I've added comments to make that clear.

The rest of the changes in this patch (and there are many) are related to deprecations. As pointed out in 101.1 and 101.2 I needed to add some notices for new arguments.

But then I started thinking about how user\EventSubscriber\MaintenanceModeSubscriber was kind of a mess. First, it no longer used the maintenance_mode service that was being injected. Second, I had renamed the public function since it was subscribing to a new event, which is kinda a BC no-no. It seemed like maybe the better way to go would be to deprecate user\EventSubscriber\MaintenanceModeSubscriber and the corresponding service, and add a new class/service that does the right thing.

So that's what I did. Hopefully it's not a mess.

The only thing left that I don't like is that the core maintenance_mode_subscriber is being injected with config.factory, which is no longer needs. I don't know how big a deal that is.

The interdiff might be kind of noisy.

To be honest, i these changes are looking good. There are a few questions in regard to how we should go about deprecations and such. But rather have a framework manager have a look how we should proceed.

So i'm setting it to rtbc and adding `Needs framework manager review` tag.

1. +++ b/core/modules/jsonapi/src/EventSubscriber/JsonapiMaintenanceModeRequestSubscriber.php
   @@ -0,0 +1,90 @@
   +    // Calling RequestEvent::setResponse() also stops propagation of event.
   +    $event->setResponse($response);
   

   Didn't know that setting the response stops propagation... that makes sense and makes this change look really neat. Nice.

2. +++ b/core/modules/user/user.services.yml
   @@ -29,6 +29,12 @@ services:
      user_maintenance_mode_subscriber:
        class: Drupal\user\EventSubscriber\MaintenanceModeSubscriber
        arguments: ['@maintenance_mode', '@current_user']
   +    deprecated: The "%service_id%" service is deprecated in drupal:9.4.0 and is removed from drupal:10.0.0. Use the user_maintenance_mode_request_subscriber service instead. See https://www.drupal.org/node/3247453
   +    tags:
   +      - { name: event_subscriber }
   +  user_maintenance_mode_request_subscriber:
   +    class: Drupal\user\EventSubscriber\MaintenanceModeRequestSubscriber
   +    arguments: [ '@current_user' ]
        tags:
          - { name: event_subscriber }
   

   I think we should change the existing maintenance mode subscriber rather then introducing a new one. Event subscribers are not part of the BC promise. We could deprecate the old kernel request method and introduce a new method but I think emptying getSubscribersEvents() and having a deprecated service is more disruptive than just making thing work the way we want them too. Other than this I really like what we've achieved here.

3. Re the config factory not being needed - let's open a follow-up to sort that out.

Getting close!

Here's an updated patch with a fairly noisy interdiff. Here are the essentials.

1. I un-deprecated user_maintenance_mode_subscriber / user\MaintenanceModeSubscriber, removed the new user_maintenance_mode_request_subscriber / user\MaintenanceModeRequestSubscriber, and moved its functionality into user_maintenance_mode_subscriber. I deprecated the MaintenanceModeSubscriber::onKernelRequestMaintenance function and removed it from the subscribed events. I removed the deprecation test because it seems like overkill for a deprecation that is not part of the BC promise.
2. I changed the name of JsonapiMaintenanceModeRequestSubscriber back to JsonapiMaintenanceModeSubscriber since I had only added the word Request to match what was going on in the user module.
3. I also added test coverage related to logging out users because I was feeling like we were getting ourselves into some test debt. It makes me feel much more confident about our changes. FWIW I cannot find existing coverage for logging out users in maintenance mode anywhere. So this is a nice win I think.

Regarding followups, we'll want to remove the maintenance_mode service from user_maintenance_mode_subscriber once the deprecated function is removed. I'll tag for followups.

We'll also need to update the change record to note at least the following:

1. MaintenanceModeInterface has a new function getSiteMaintenanceMessage
2. There's a new event to listen to for maintenance mode behavior: MaintenanceModeEvents::MAINTENANCE_MODE_REQUEST

I'll add the tag for that. Do we need more than one change record for this stuff?

Looking good.

+++ b/core/modules/user/src/EventSubscriber/MaintenanceModeSubscriber.php
@@ -64,11 +70,31 @@ public function onKernelRequestMaintenance(RequestEvent $event) {
+      30,

I think this priority doesn't make much sense. Imo it should be -1000 or something - like normally we want any other events to have a go first.

Thanks, @alexpott!

The way things are in the patch, there are three subscribers to MaintenanceModeEvents::MAINTENANCE_MODE_REQUEST.

1. \Drupal\jsonapi\EventSubscriber\JsonapiMaintenanceModeSubscriber with priority 40
2. \Drupal\user\EventSubscriber\MaintenanceModeSubscriber with priority 30
3. \Drupal\Core\EventSubscriber\MaintenanceModeSubscriber with priority -256

First, is that the correct order?
Second, what would you set the priorities to?

Thanks!

Order doesn't matter much except the user one needs to be very late. -1000 as @alexpott suggest would be fine there. The jsonapi one (and user one for that matter) will only be triggered after the Drupal\Core\EventSubscriber\MaintenanceModeSubscriber trows the event.

So, i think, just change the user one to -1000 and hopefully call it a day.

Thanks, @bbrala

But now I'm confused. Are we suggesting this?

1. \Drupal\jsonapi\EventSubscriber\JsonapiMaintenanceModeSubscriber with priority 40
2. \Drupal\Core\EventSubscriber\MaintenanceModeSubscriber with priority -256
3. \Drupal\user\EventSubscriber\MaintenanceModeSubscriber with priority -1000

In this case I don't think the user one would ever be triggered and the user would never be logged out.

Order should be:

1. \Drupal\jsonapi\EventSubscriber\JsonapiMaintenanceModeSubscriber
2. \Drupal\user\EventSubscriber\MaintenanceModeSubscriber
3. \Drupal\Core\EventSubscriber\MaintenanceModeSubscriber

If the jsonapi wants to do its thing it should. Then the user one needs to work its magic if needed. Then if nothing happens we do through to the default.

Since the default priority for an event subscriber is 0. I think we should prefer these to run after that. So we need a negative priority. I think the following should be fine, I think only really matters between subscribers subscribed to the same event.

1. \Drupal\jsonapi\EventSubscriber\JsonapiMaintenanceModeSubscriber - -800
2. \Drupal\user\EventSubscriber\MaintenanceModeSubscriber - -900
3. \Drupal\Core\EventSubscriber\MaintenanceModeSubscriber - -1000

If we do the following, should a contrib module subscribe without giving a priority it will at least trigger before the core ones, which i think is a good idea.

Attached is a patch with updated priorities suggested by @bbrala in #112. Thanks!

+1 to the logic that's arrived at #113 nice work @danflanagan8 and @bbrala

Thanks :)

You probably need an rtbc, setting to rtbc.

Hmm, first let's fix the change record. The whole event subscriber think might need a separate one from the jsonapi change.

• I've checked the initial CR and added an extra one for the new Event, removing the changerecord tags.
• Since Alex reviewed this in regards to the deprecations i'll remove the needs framework manager review tag.
• I also a created a related issue #3255800: [PP-1] Remove ConfigFactory from Drupal\Core\EventSubscriber\MaintenanceModeSubscriber for the follow up regarding the config factory. Removing the Needs followup tag.

Setting RTBC

Thanks @bbrala! I added a third CR related to MaintenanceModeInterface.

Here's an updated patch where all the CR references are correct (I think!)

I guess I have to set it back to NR, but it should be an easy review.

Cheers, checked the url's. RTBC

DrupalCI is hitting grief with Composer 2.2, moving status back.

Unfortunately we need a 10.0.x version of the patch as well. I think there are some changes to the Symfony event system that we'll need to account for - so we'll need separate patches.

Here's an attempt at a re-roll for 10.0.x. Let's see what fails!

That does seem like what is supposed to change. But now we wait ^^

I noticed something I messed up resolving one of the few merge conflicts. I added the return type back in here.

Well that green was surprising... :)

I guess it's ready for review again!

Looking good thanks.

Committed aa3434c and pushed to 10.0.x. Thanks!
Committed b7b2a8b and pushed to 9.4.x. Thanks!

@bbrala @danflanagan8 can you write a release note in the issue summary? The current text can be improved :)

Fixed small typo in release note snippet

patch #118 no longer applies for D9.4.0

@Rar9, this is fixed in D9.4.0, so there's no need to apply a patch to 9.4.0.

FTR, Opened #3347601: Add Retry-After header to HTML maintenance mode responses as a follow up because a Retry-After header for HTML responses could be useful as well.