Move modal / dialog to query parameters

Everything using the ajax API is already always sending {js: true} in the data (on top of the jQuery-specific header: X-Requested-With).

The patch makes the frontend send an option for the modal properly, it's called data-dialog-type in the HTML attribute and dialogType in the data sent to be backend. We might want to rename it data-drupal-ajax-type and make the api more consistent (we would be getting rid of the magic use-ajax and use-ajax-submit classes). but it's a start.

I have not touched the backend stuff so all the negociation stuff needs cleanup and it needs to use the new stuff.

Damn, but get it

Looks basically fine - might need to check for an existing query string in the JS

in the if/else you don't really need an extra variable, how about jut:

if (ajax.options.url.indexOf('?') === -1) {
  ajax.options.url += '?'
}
else {
  ajax.options.url += '&'
}
ajax.options.url += 'dialogType=' + element_settings.dialogType;

Also - it would probably be a best practice to urlencode element_settings.dialogType if it coule be specified by a module to some random value?

Should we have a whitelist here? Or at least verify it's sane like [a-z0-9_]

+    if ($type = $request->query->get('dialogType')) {
+      return 'drupal_' . $type;

+1 to a white list, container parameter?

Should we have a whitelist here? Or at least verify it's sane like [a-z0-9] ?

+    if ($type = $request->query->get('dialogType')) {
+      return 'drupal_' . $type;

Allowing raw user input to pass into the system here seems like something that could cause security issues later.

Rather then messing with the content negotiation class why don't we just make this its own request subscriber?

+++ b/core/lib/Drupal/Core/FormatMapping.php
@@ -36,12 +36,15 @@ public function getContentType(Request $request) {
+    if ($type = $request->query->get('dialogType')) {

Per the phone summit today, should we use a more generic name than dialogType? wrapper, envelope, etc?

The patch looks fine to me otherwise.

My gut says we should not use _format but I'm not really sure. Naming is hard but easily refactored so we can keep discussing it.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/WrapperFormatSubscriber.php
   @@ -0,0 +1,40 @@
   +  const WRAPPER_FORMAT = '_wrapper_format';
   

   Good call on the constant. :-) Though if we expect people to use it, putting it outside of the subscriber would be a good idea. (Not sure off hand where else we'd put it, though.)

2. +++ b/core/lib/Drupal/Core/EventSubscriber/WrapperFormatSubscriber.php
   @@ -0,0 +1,40 @@
   +    if ($request->query->has(static::WRAPPER_FORMAT)) {
   +      $request->setRequestFormat($request->query->get(static::WRAPPER_FORMAT));
   +    }
   

   This is probably fine for now, but we may have to split it to its own property later. Which of course we're trying to avoid having more of. Carp.

It's been my experience that "using one thing for two things" backfires more often than it works out. SRP is hard but it really is the driving force behind good software.

Can we guarantee that we will never ever ever need to differentiate between the payload format and envelope format? We will always care about only one or the other? I'd rather be cautious and give ourselves an out.

We talked about this being a blocker to #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use

+++ b/core/lib/Drupal/Core/EventSubscriber/WrapperFormatSubscriber.php
@@ -0,0 +1,40 @@
+      $request->setRequestFormat($request->query->get(static::WRAPPER_FORMAT));

Should we have some sort of whitelist here? could be container parameter to allow others to edit/add to it. Raw user input this deep in routing feels wrong for some reason.

This patch looks like it's almost ready!

#31++

+++ b/core/lib/Drupal/Core/EventSubscriber/WrapperFormatSubscriber.php
@@ -0,0 +1,40 @@
+/**
+ * Sets the wrapper format, if specified for the request.
+ */
+class WrapperFormatSubscriber implements EventSubscriberInterface {

Is this now a generic concept? Or does it really only apply to HTML responses? Can we think of use cases of this concept for e.g. JSON?

Then that's fine, I think. But if it were HTML-specific, then we could write clearer docs. That's why I was asking.

NW for #31, then, and then this should be ready!

1. +++ b/core/core.services.yml
   @@ -568,6 +568,10 @@ services:
   +  http_negotiation.wrapper_format_subscriber:
   +    class: Drupal\Core\EventSubscriber\WrapperFormatSubscriber
   +    tags:
   +      - { name: event_subscriber }
   

   Seem this should just be in MainContentViewSubscriber. Its only job is already just to do this sort of envelope resolution.

2. +++ b/core/lib/Drupal/Core/ContentNegotiation.php
   @@ -38,7 +38,7 @@ public function getContentType(Request $request) {
   -    $priority = array('html', 'drupal_ajax', 'drupal_modal', 'drupal_dialog');
   
   +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
   @@ -240,7 +241,7 @@ public static function preRenderAjaxForm($element) {
   +        WrapperFormatSubscriber::WRAPPER_FORMAT => 'ajax',
   

   Manually testing, I can't add a block instance. I assume these not matching is the reason why.

Seem this should just be in MainContentViewSubscriber. Its only job is already just to do this sort of envelope resolution.

No, it only does that for controllers that return a render array. This is why I asked #33: if it's intended to be more generic than only for HTML-like things, then it needs to be separate. If it's only for HTML-like things, then yes, it should be in MainContentViewSubscriber.

1. +++ b/core/lib/Drupal/Core/ContentNegotiation.php
   @@ -38,7 +38,7 @@ public function getContentType(Request $request) {
        // Check all formats, if priority format is found return it.
   ...
   -    $priority = array('html', 'drupal_ajax', 'drupal_modal', 'drupal_dialog');
   +    $priority = array('html');
   ...
          if (in_array($format, $priority, TRUE)) {
   

   Not sure if this is a temporary change, but if it staying, this should just check with ===

2. +++ b/core/modules/block/src/BlockListBuilder.php
   @@ -362,7 +362,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -          'data-accepts' => 'application/vnd.drupal-modal',
   +          'data-dialog-type' => 'modal',
   

   Beautiful!

Took a stab at fixing a couple things. There where a lot of tests that weren't converted. I assume more will fail when I upload this.

I still don't understand why MainContentViewSubscriber isn't the place this is suppose to happen. Moving it though helped me expose a bunch of places that where still relying on content negotiation where it shouldn't so that's in the patch.

There are still a bunch of tests that have our vendor accept headers so we'll need to fix those before this can go in. As I mentioned I expect some of those will break in testbot.

#41: can you please provide an interdiff?

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -1799,11 +1794,8 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
+      $headers[] = 'Accept: application/vnd.drupal-ajax';

Huh, an Accept header again? I'm confused.

Sorry, I have the interdiff I just didn't get it on the upload.

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -1786,11 +1786,6 @@ protected function drupalPostForm($path, $edit, $submit, array $options = array(
   protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_path = NULL, array $options = array(), array $headers = array(), $form_html_id = NULL, $ajax_settings = NULL) {
-    if (isset($options['query'][MainContentViewSubscriber::WRAPPER_ENVELOPE])) {
-      $original_wrapper = $options['query'][MainContentViewSubscriber::WRAPPER_ENVELOPE];
-      unset($options['query'][MainContentViewSubscriber::WRAPPER_ENVELOPE]);
-    }
-

@@ -1799,11 +1794,8 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
-    if (isset($original_wrapper)) {
-      $options['query'][MainContentViewSubscriber::WRAPPER_ENVELOPE] = $original_wrapper;
-    }
-    else {
-      $options['query'][MainContentViewSubscriber::WRAPPER_ENVELOPE] = 'drupal_ajax';
+    if (!preg_grep('/^Accept:/', $headers)) {
+      $headers[] = 'Accept: application/vnd.drupal-ajax';

@@ -1843,9 +1835,8 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
-      $ajax_path = isset($ajax_settings['url']) ? preg_replace('/^' . preg_quote($GLOBALS['base_path'], '/') . '/', '', $ajax_settings['url']) : 'system/ajax';
+      $ajax_path = isset($ajax_settings['url']) ? $ajax_settings['url'] : 'system/ajax';
     }
-    $ajax_path = $this->container->get('url_generator')->generateFromPath($ajax_path, $options);
 

No, these where what I was fixing. These methods are suppose to be used to test our ajax/modal/dialog code and they're still using accept headers. If we don't fix them, we're not testing the code we are changing and the passes are false impressions of the actual state of things. @see earlier review where things where passing but not working.

this really doesn't want to make it onto the issue.

+++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
@@ -80,7 +80,7 @@ public function onViewRenderArray(GetResponseForControllerResultEvent $event) {
+      $wrapper = $request->query->get(static::WRAPPER_ENVELOPE, $request->getRequestFormat());

So I didn't get this but after spending a while with it, it does something very specific and non-obvious. The difference in these lines is _the_ way we provide a 406 for content negotiation. This should be handled in a more specific and clear way I think but this is more the focus of the parent issue so I'm just going to document it as such and leave it so tests pass.

Also fix a failing test because of a string match and move URL encoding fix to only affect settings based "path" stuff which is where the problem was.

API change section needs update. We also need change notice for this change.

+++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
@@ -76,20 +78,21 @@ public function onViewRenderArray(GetResponseForControllerResultEvent $event) {
+      // We default to requestFormat() because content negotiation uses this as
+      // its fallback method when format matching fails to match a route to
+      // display a 406.
+      // TODO - This logic should be replaced in https://www.drupal.org/node/2364011 with a more clear opt in to avoid cache poisoning.

Needs some doc love.

Better?

    // We default to requestFormat() because content negotiation uses this as
    // its fallback method for displaying a 406 when we fail to match a route
    // based on format.
    // TODO - This logic should be replaced in https://www.drupal.org/node/2364011 with a more clear opt in to avoid cache poisoning.

Correct.

    // We default to requestFormat() because content negotiation uses this as
    // its fallback method for displaying a 406 when we fail to match a route
    // based on format.
    // @todo this logic should be replaced in 
    //    https://www.drupal.org/node/2364011 with a more clear opt in to 
    //    avoid cache poisoning.

@todo statements don't parse on multiple lines.

Please see https://www.drupal.org/node/1354#todo

i stand corrected, phpdoc doesn't seem to and phpstorm doesn't but PSR6 allows it, I'll roll a new patch.

Thanks for the fix.

+++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
@@ -50,6 +50,8 @@ class MainContentViewSubscriber implements EventSubscriberInterface {
+  const WRAPPER_ENVELOPE = '_wrapper_format';

+++ b/core/misc/ajax.js
@@ -266,6 +263,18 @@
+      ajax.options.url += '_wrapper_format=drupal_' + element_settings.dialogType;

So we have a constant in PHP. Can we have some kind of key on drupalSettings which we pass from PHP so that we can change it from one single place in future? Just an idea.

Sending the value of a (by definition hardcoded) constant over via drupalSettings means sending it with every request. That's not okay.

I think we can handle renaming that line of code in the JS.

I think we can handle renaming that line of code in the JS.
so something like var WRAPPER_ENVELOPE = '_wrapper_format'; then?

#62: that'd be fine, yes.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
   @@ -50,6 +50,8 @@ class MainContentViewSubscriber implements EventSubscriberInterface {
   +  const WRAPPER_ENVELOPE = '_wrapper_format';
   

   Needs to be documented.

   Also: why the leading underscore?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
   @@ -76,20 +78,22 @@ public function onViewRenderArray(GetResponseForControllerResultEvent $event) {
   +      // We default to requestFormat() because content negotiation uses this as
   

   s/requestFormat()/getRequestFormat()/

3. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
   @@ -76,20 +78,22 @@ public function onViewRenderArray(GetResponseForControllerResultEvent $event) {
   +      // its fallback method for displaying a 406 when we fail to match a route
   

   Note that this comment is inside the is_array($result) if-test, which checks if the controller result is a render array.

   So I don't see how this is "a fallback method".

   It's quite possible this is only confusing because the code flow *elsewhere* is confusing, but at least this class was very clear, very focused.

   This makes it vague, IMO.

4. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
   @@ -76,20 +78,22 @@ public function onViewRenderArray(GetResponseForControllerResultEvent $event) {
   -        $supported_formats = array_keys($this->mainContentRenderers);
   -        $supported_mimetypes = array_map([$request, 'getMimeType'], $supported_formats);
            $event->setResponse(new JsonResponse([
              'message' => 'Not Acceptable.',
   -          'supported_mime_types' => $supported_mimetypes,
   +          'supported_wrapper_envelopes' => array_keys($this->mainContentRenderers),
            ], 406));
   

   This is definitely a bad change.

   This view subscriber is about render a render array in the requested format. Yes, in core there are only html/modal/dialog/ajax, which are all variations on "html". But it's perfectly possible that contrib adds more formats.

   That's why this code is sending a 406 response in case a format is requested that we cannot render the render array into, per the HTTP spec. This code written to carefully follow the HTTP spec. These changes break that.

   If this code is indeed only about wrapper formats (which I'm not at all sure about), then this does not care about Accept headers, and then this should also never send a 406; sending a 406 would be up to the rest of the system then. i.e. the entire else-branch should then be removed.

   This is why I find these changes so confusing & conflicting:
   - on the one hand, we say this only cares about wrapper formats, which can only be set through query parameters
   - yet on the other hand, we send a machine-readable 406, which is actually only applicable to Accept-header content negotiation; it'd make more sense to fall back to HTML in this case
   - and finally, this makes it impossible to do content negotiation to other formats than html/ajax/modal/dialog, i.e. this is implicitly claiming "render arrays only ever need wrapper formats, never need 'proper' formats, because render arrays will always render to HTML"

5. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1785,6 +1787,11 @@ protected function drupalPostForm($path, $edit, $submit, array $options = array(
      protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_path = NULL, array $options = array(), array $headers = array(), $form_html_id = NULL, $ajax_settings = NULL) {
   +    if (isset($options['query'][MainContentViewSubscriber::WRAPPER_ENVELOPE])) {
   +      $original_wrapper = $options['query'][MainContentViewSubscriber::WRAPPER_ENVELOPE];
   +      unset($options['query'][MainContentViewSubscriber::WRAPPER_ENVELOPE]);
   +    }
   
   @@ -1793,8 +1800,11 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
   +    if (isset($original_wrapper)) {
   +      $options['query'][MainContentViewSubscriber::WRAPPER_ENVELOPE] = $original_wrapper;
   +    }
   +    else {
   +      $options['query'][MainContentViewSubscriber::WRAPPER_ENVELOPE] = 'drupal_ajax';
        }
   

   Why do we need to retain the original wrapper?

   If there is a valid reason, then let's do

   $ajax_options = $options;
   $ajax_options['query'][MainContentviewSubscriber::WRAPPER_ENVELOPE] = 'drupal_ajax';
   

   and hence not do this dance?

This is the place here in HEAD which throws the 406 and will continue to do so ...

Note that removing the else-statement would allow us to handle it elsewhere. Not sure what makes most sense.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
   @@ -50,6 +50,11 @@ class MainContentViewSubscriber implements EventSubscriberInterface {
   +   * Url query attribute to indicate the wrapper used to render a request.
   
   +++ b/core/misc/ajax.js
   @@ -17,6 +17,13 @@
   +   * Url query attribute to indicate the wrapper used to render a request.
   

   s/Url/URL/

2. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
   @@ -80,7 +85,7 @@ public function onViewRenderArray(GetResponseForControllerResultEvent $event) {
   -      // We default to requestFormat() because content negotiation uses this as
   +      // We default to getRequestFormat() because content negotiation uses this as
   

   80 cols

3. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
   @@ -51,6 +51,13 @@ class MainContentViewSubscriber implements EventSubscriberInterface {
   +  const WRAPPER_ENVELOPE = '_wrapper_format';
   

   The name indicates genericness. Yet it lives in MainContentViewSubscriber, which only cares about render arrays.

   So it's not actually generic…

   Therefore, +1 to: I prefered the additiona lsubscriber to be honest.

On the question of the fallback method, that is infact what it does. You are correct, the code elsewhere is unclear. I agree it should be a separate subscriber but we're about to replace logic with something in the parent issue so I just added the @todo. I assumed it out of scope as well.

To explain the code flow, Accept header format matching is done by the router. rest.module adds a new route for each format and then lets the router resolve that and then serializes it. That serialization then isn't an array so it skips the code in this subscriber.

If it fails to route based on getRequestFormat() though the default route(our HTML route) gets used and it hits this code. Now, here is where all the assumptions that really make this confusing come in. Because there isn't a response or a string (array check) we know this is a Drupal centric return and we know we're rendering HTML(or maybe one of the special wrapper formats). Now, the request format doesn't match the wrapper list so we know its not html(html is always in the mime types) and so we assume the format was invalid and serve a fallback 406.

If you're still confused, I don't blame you I spent an hour playing with it and trying to figure out why rest.module tests where failing before it clicked.

63.4) mime_types -> wrapper_envelope - This matches terminology and is technically more accurate I think because we are not using mime-types for negotation. The error implies you supplied the wrong mime type.

That said... that's only for this issue. In the fallback it is incorrect and a bad change, I'll see if I can tease apart the two use cases.

#68: But MainContentViewSubscriber only does something for controllers that have returned an array. The REST module's controller is RequestHandler::handle(). And looking at that code, I see only 3 ways to return:

1. An early return in case of an exception during deserialization, which returns a Response object (with HTTP status 400). Therefore, MainContentViewSubscriber cannot possibly be called.
2. An early return in case of an exception during the invocation of the operation on the resource plugin, which returns a Response object. Therefore, MainContentViewSubscriber cannot possibly be called.
3. The third and final case always returns the $response value retrieved during the invocation of the operation on the resource plugin, and before returning it, we always do $data = $response->getResponseData(), which would cause a PHP error if $response were an array.

In other words: I don't see how the code flow could ever allow for a REST module response to be an array. :( Did I make a mistake in this analysis?

Aha! dawehner++

So the problem is that:

1. in the case of REST, the selected route (and thus controller) depends on the format (i.e. N formats => N controllers),
2. in the case of "regular" web pages, the selected route is always the same, and the formatting into the requested format happens *later*, in a KernelEvents::View subscriber (i.e. N formats => 1 controller + VIEW subscriber).

I guess that if we'd be consistent (either always approach 1, or always approach 2 — I'm not judging which is better), then we wouldn't have this problem. But there's no changing any of that right now, obviously.

Alright, so with that better understanding, reviewing the patch again:

1. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
   @@ -76,20 +83,22 @@ public function onViewRenderArray(GetResponseForControllerResultEvent $event) {
   +      $wrapper = $request->query->get(static::WRAPPER_ENVELOPE, $request->getRequestFormat());
   

   Then this definitely does not make sense. If we separate "wrapper envelopes" from "formats", then it's also not okay to fall back to the request format in case no wrapper envelope is specified. I.e. we should always fall back to "html", then.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
   @@ -76,20 +83,22 @@ public function onViewRenderArray(GetResponseForControllerResultEvent $event) {
          else {
   -        $supported_formats = array_keys($this->mainContentRenderers);
   -        $supported_mimetypes = array_map([$request, 'getMimeType'], $supported_formats);
            $event->setResponse(new JsonResponse([
              'message' => 'Not Acceptable.',
   -          'supported_mime_types' => $supported_mimetypes,
   +          'supported_wrapper_envelopes' => array_keys($this->mainContentRenderers),
            ], 406));
          }
   

   And then this does indeed make sense.

3. But point 1 is not possible right now, that's why there's a @todo there, and that's why #44 changed that.
4. So I think we should solve this by changing
   

   if (is_array($result)) {
   

   to

   if (is_array($result) && $request->getRequestFormat() === 'html') {
   

   i.e. to ensure that the main content view subscriber only ever cares about the "HTML" request format. Because that's what we are saying here, that conceptually, this is always about HTML, and it just may be wrapped differently.

   … but that poses two problems:

   1. How are we then going to send a 406 response for the scenario described in #70?
   2. it is not actually true that render arrays can be rendered only to HTML

In conclusion, I can't help but think that the the two distinct ways of handling "send a resource in a certain format" as described at the very beginning of this comment is causing all this trouble: it forces us to invent the concept of "wrapper envelopes", which then causes further problems because "wrapper envelopes" implies that the response of "regular routes" (i.e. "non-REST routes") is always of a single format — otherwise we wouldn't know how to reliably wrap it.

Why did we switch from the separate subscriber approach that we had until #38 to this approach? I think it's because @neclimdul said this in #36:

Seem this should just be in MainContentViewSubscriber. Its only job is already just to do this sort of envelope resolution.

But that's not completely true; it just happens that Drupal core doesn't have anything that turns a render array into something not-at-all-HTML-like.

However, let's assume that it would always be HTML-like (which could indeed be argued). What we've now ended up with is two levels of content negotiation: base types (HTML) and more specific subtypes (html/dialog/modal/ajax), if you will.

Which could be fine, except that we're actively relying on very twisted behavior due to those two distinct ways again (top of comment): one type is missing (HAL-JSON), so the router decided to fall back to HTML, but the only accepted format is actually not supported, so we should send a 406, but we already fell back to HTML, so it shouldn't be MainContentViewSubscriber's job to figure out that we actually didn't request HTML, but HAL-JSON, even though we did fall back to HTML, etc.

If that would be fixed in #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use, then fine, go ahead. But it is extremely confusing. And IMO it is because either Symfony, or we, or both, have failed at making a clear decision on how the same resource should be available in multiple formats (again, see the top of the comment).

I guess that if we'd be consistent (either always approach 1, or always approach 2 — I'm not judging which is better), then we wouldn't have this problem. But there's no changing any of that right now, obviously.

I went back and forth on this a lot, but in the end I think they have to be separate. In some cases you want the same "content" but rendered differently; in others you want different logic to run depending on the format. As long as we have controllers that can execute arbitrary code I don't think we can unify that flow, and not having arbitrary code controllers is a Drupal 9-level change at least. :-)

In hindsight, a truly REST-centric system would not have controllers. It would only have resources. But given that HTML is about 20x as involved as any other format, even that unification would be difficult to do. I've toyed briefly with this area on my own and don't have a good solution given the use cases that Drupal tries to support (most of which are entirely legitimate).

That said:

it is not actually true that render arrays can be rendered only to HTML

We keep saying this, but as long as there is are #markup, #prefix, and #suffix keys it will never be the case. Render arrays are an abstract syntax tree of the output, but that output is NOT, by that point, format agnostic. That's a lovely theory but to my knowledge has never been true. I long ago accepted that the theme and render systems were HTML-speciific, and that has made my life far easier.

In hindsight, a truly REST-centric system would not have controllers. It would only have resources.

Exactly!

But given that HTML is about 20x as involved as any other format, even that unification would be difficult to do.

Indeed.

Render arrays are an abstract syntax tree of the output, but that output is NOT, by that point, format agnostic.

Indeed; it depends on how you look at it. A render array can be turned into a PDF just like HTML can: by taking the rendered HTML and putting that into a PDF.

This is why I said at the bottom (below the last horizontal rule) in the "However" and "Which" paragraphs that 1) I could see us choosing to make it so that "we can only render render arrays as HTML" but 2) that that then means the current patch's changes to MainContentViewSubscriber don't make sense. Could you read those two paragraphs again, and try to formulate what you think would be a sensible solution? :)

Marking this as triaged since it blocks #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use which has already been triaged.

In response to #72/#73 are we just saying that view listeners are for formats that either are, or can contain, HTML? Both PDF and epub fall under that category, and I think we discussed on the other issue that for at least those two formats they could probably be implemented as either REST or via render arrays (i.e. potentially you could have competing contrib modules doing each approach).

I very much see render arrays as only useful for rendering "HTML-like" content. It seems like we're all in agreement on that though. That has definitely driven a lot of my decisions and thinking about this and the parent issue.

I wonder if we're trying to over complicate something here though. Because someone chose to implement that weird clash of concerns in that subscriber shouldn't be leading this discussion. That's just an implementation detail we can sort out. Separating the check aligns with our short term goal of this issue and decouples things so we can continue to sort out the larger problem in the follow ups.

This patch:

1. Moves routing 406 logic to a separate subscriber.
2. Fixes an error in the ajax constant. (that file is a bit weird)
3. Removes ViewSubscriber (I couldn't figure out how this was being used and it was in the way. If there is a compelling argument to keep it or it breaks tests I put it back in some way).

Thoughts?

1. Nits from #65 still apply.
2. #75.3: AFAICT this is the code that was intended to handle 406 responses for non-HTML requests, but due to the router falling back to the generic (render array/HTML) route, it seems like this has been dead code for a long time.

3. +++ b/core/lib/Drupal/Core/EventSubscriber/AcceptNegotiation406.php
   @@ -0,0 +1,47 @@
   + * @todo fix or replace this in https://www.drupal.org/node/2364011
   

   This is the reason why I think this may be acceptable to land now, so that #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use is unblocked.

4. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
   @@ -84,23 +82,13 @@ public function onViewRenderArray(GetResponseForControllerResultEvent $event) {
   +    if (is_array($result) && ($request->query->has(static::WRAPPER_ENVELOPE) || $request->getRequestFormat() == 'html')) {
   +      $wrapper = $request->query->get(static::WRAPPER_ENVELOPE, 'html');
   

   This can be simplified to:

   if (is_array($result) && $request->getRequestFormat() == 'html')) {
     $wrapper = $request->query->get(static::WRAPPER_ENVELOPE, 'html');
   

5. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
   @@ -84,23 +82,13 @@ public function onViewRenderArray(GetResponseForControllerResultEvent $event) {
   +      // Fall back on HTML if we don't have a renderer.
   

   Better comment: Fall back to HTML if the requested wrapper envelope is not available.

too bad, so we can't give any tips at that point anymore?

We were only listing vendor formats for envelopes, not mime types for accept negotiation so it wasn't really relevant.

#77.3 yeah. agreed and Crell +1'd on IRC.
#77.4 No, it sadly can't be simplified. The reason is non-obvious though and I almost posted this with something similar but then I did some manual testing and ajax.js didn't work.

The reason its more complex is because if ?_wrapper_format is set we actually have to ignore the accept header format. This is because $.ajax is being given Datatype:json as an argument(totally valid) and so is adding the application/json Accept header. If we only check getRequestFormat(), when we get to this code we see 'json' != 'html' and rather then using the wrapper, we skip it and fail through to the 406 code.

All that said, we could probably revisit this when we start changing accept based content negotiation because having to explicitly opt in to accept based negotiation might change this requirement.

All that said, we could probably revisit this when we start changing accept based content negotiation because having to explicitly opt in to accept based negotiation might change this requirement.

Exactly.

Alright, let's get this one done, so that the more important critical is unblocked!

Noted documentation fixes.

Found a bug during manual testing.

STR:

1. Install Standard profile with the patch applied.
2. Open the web developer console
3. Either:
   • 1. Go to /node/add/article
     2. Click the "Image" button in CKEditor.
     3. Note that no image dialog appears, and that in the network inspector, you see that the AJAX request got a 406 response.
   • or:
     1. Go to /admin/structure/views/view/content
     2. Note that no preview appears, and that in the network inspector, you see that the AJAX request got a 406 response.

Ouch, yeah 2 separate javascript bugs too.

woops, correct interdiff

1. +++ b/core/lib/Drupal/Core/EventSubscriber/AcceptNegotiation406.php
   @@ -0,0 +1,47 @@
   +    // If this is a render array then we assume that the router went with the
   +    // generic controller and not one with a format. If the format requested is
   +    // not HTML though we can also assume that the requested format is invalid
   +    // so we provide a 406 response.
   +    if (is_array($result) && $request->getRequestFormat() !== 'html') {
   +      $result = new JsonResponse([
   +        'message' => 'Not Acceptable.',
   +      ], 406);
   +      $event->setResponse($result);
   +    }
   

   No need to do this ourselves. Just throw the appropriate exception. I believe that should already get picked up properly.

2. +++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
   @@ -120,7 +120,7 @@ public function dialog() {
   -        'data-accepts' => 'application/vnd.drupal-modal',
   +        'data-accepts' => 'application/vnd.drupal-modal', // TODO replace this.
   

   Why can't we replace it yet?

Confirmed working. NW for #85.

Fixes for #85

Just throw the appropriate exception. I believe that should already get picked up properly.

Didn't do anything here.

Hmm. Looks like we *do* need to do something if removing AcceptNegotiation406.

I'm happy for someone else to take a look at this.

Spoke with @dawehner at the sprint, and it seems I misunderstood what @Crell was asking for in #85.

Re-added the listener, and now throwing the exception.

Forgot to re-add the service definition. Also added a Json exception handler for 406, but still getting a failure for matching the returned message, so I don't think it's getting called.

Don't see failures with this. The exception definitely creates a different output as can be seen in the interdiff.

Interdiff vs #83

Review of #93

1. +++ b/core/lib/Drupal/Core/EventSubscriber/AcceptNegotiation406.php
   @@ -0,0 +1,44 @@
   +  function onViewDetect406(GetResponseForControllerResultEvent $event) {
   

   Needs a docblock

2. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1834,8 +1844,21 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
   +          '/^' . preg_quote($GLOBALS['base_path'], '/') . '/',
   

   Can we avoid the $GLOBAL?

3. +++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
   @@ -117,10 +118,11 @@ public function dialog() {
   +      '#url' => Url::fromRoute('ajax_test.dialog_contents', array(), array(
   +        'query' => array(MainContentViewSubscriber::WRAPPER_ENVELOPE => 'drupal_modal'),
   +      )),
   

   nit: this would be easier to read with [] array notation

1 and 2 of #95 apply to #94 as well.

Even if it's a 5xx, it's actually an uncatched exception or something like that, and never a fatal error. So that's highly confusing in all cases...

Just suggesting that this might be something that could be improved in a follow-up issue, since it's already bad :)

Looks like some code got lost between #93 and #94. Let's try to fix this up.

One little word...

+++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
@@ -65,4 +65,15 @@ public function on405(GetResponseForExceptionEvent $event) {
+  public function on406(GetResponseForExceptionEvent $event) {
+    $response = new JsonResponse(['message' => $event->getException()->getMessage()], Response::HTTP_NOT_ACCEPTABLE);

+++ b/core/modules/rest/src/Tests/ReadTest.php
@@ -92,7 +92,9 @@ public function testRead() {
+    $this->assertEqual($response, Json::encode([
+      'error' => 'A fatal error occurred: Not acceptable',
+    ]));

This doesn't seem relevant since the test isn't failing even though the response would be different.

1. +++ b/core/core.services.yml
   @@ -855,6 +855,10 @@ services:
   +      - { name: event_subscriber, priority: -10 }
   
   +++ b/core/lib/Drupal/Core/EventSubscriber/AcceptNegotiation406.php
   @@ -0,0 +1,51 @@
   +    $events[KernelEvents::VIEW][] = ['onViewDetect406', -10];
   

   Why do we define the priority twice?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
   @@ -65,4 +65,15 @@ public function on405(GetResponseForExceptionEvent $event) {
   +  public function on406(GetResponseForExceptionEvent $event) {
   

   Hrm… why do we special-case JSON? Or must this exist for every format?

   OTOH, this already exists for 403/404/405… so… I guess changing this is out of scope? Not sure.

3. +++ b/core/misc/ajax.js
   @@ -289,6 +296,13 @@
   +   * Url query attribute to indicate the wrapper used to render a request.
   

   s/Url/URL/

4. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1785,6 +1787,11 @@ protected function drupalPostForm($path, $edit, $submit, array $options = array(
   +    if (isset($options['query'][MainContentViewSubscriber::WRAPPER_ENVELOPE])) {
   +      $original_wrapper = $options['query'][MainContentViewSubscriber::WRAPPER_ENVELOPE];
   +      unset($options['query'][MainContentViewSubscriber::WRAPPER_ENVELOPE]);
   +    }
   
   @@ -1793,8 +1800,11 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
   -    if (!preg_grep('/^Accept:/', $headers)) {
   -      $headers[] = 'Accept: application/vnd.drupal-ajax';
   +    if (isset($original_wrapper)) {
   +      $options['query'][MainContentViewSubscriber::WRAPPER_ENVELOPE] = $original_wrapper;
   +    }
   +    else {
   +      $options['query'][MainContentViewSubscriber::WRAPPER_ENVELOPE] = 'drupal_ajax';
        }
   

   See #63.5.

5. +++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
   @@ -120,7 +120,7 @@ public function dialog() {
   +        'data-accepts' => 'application/vnd.drupal-modal', // TODO replace this.
   

   Any reason we can't replace this yet?

bah, I'll know those out.

I already am, stand by while I write a comment. :-)

Never mind #104.4, @neclimdul pointed out in IRC that @dawehner already answered this in #64.

OK, here's the issue. Turns out, core is not catching ANY hal_json errors. They're falling through to the default exception handler.

Also, the Json exception handler and the default handler use *different* properties for the error message. Because Durpal.

I talked briefly with dawehner and Alex Pott. Standardizing the properties is now a spinoff issue: #2486943: Standardize error message property name

Since hal_json is module-provided, not core provided, we put its error handling in its own module service but the behavior is identical. Then we fix the tests at least for now.

Re #104: All per-type, per-error is opt-in, so adding the method is the proper way to do it. And I have no idea why the priority is specified twice, so let's fix that.

I can't speak to Wim's other questions.

1. +++ b/core/core.services.yml
   @@ -858,7 +858,7 @@ services:
   +      - { name: event_subscriber}
   

   Missing space before closing curly brace.

2. +++ b/core/modules/hal/src/EventSubscriber/ExceptionHalJsonSubscriber.php
   @@ -0,0 +1,23 @@
   + * Handle HAL exceptions the same as JSON exceptions.
   

   "HAL" or "HAL JSON"?

3. +++ b/core/modules/hal/src/EventSubscriber/ExceptionHalJsonSubscriber.php
   @@ -0,0 +1,23 @@
   +  }
   +}
   

   Missing newline in between.

Meh.

Took a stab at making #104.4 a little clearer anyways. there wasn't really any docs on what was happening. Also removed "todo" added that needed to go away.

Took a stab at making #104.4 a little clearer anyways. there wasn't really any docs on what was happening.

So much clearer, many thanks!

Final round of remarks. I'd RTBC this if it was just the first nitpick. The third should be fixed, but the second should *definitely* be fixed

1. +++ b/core/modules/hal/src/EventSubscriber/ExceptionHalJsonSubscriber.php
   @@ -0,0 +1,24 @@
   +   * {@inheritDoc}
   

   Super nitty nit: s/Doc/doc/

2. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1834,8 +1843,21 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
   +          '/^' . preg_quote($GLOBALS['base_path'], '/') . '/',
   

   Quoting #95.2:

   Can we avoid the $GLOBAL?

3. +++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
   @@ -120,7 +120,6 @@ public function dialog() {
          '#attributes' => array(
            'class' => array('use-ajax'),
   -        'data-accepts' => 'application/vnd.drupal-modal',
          ),
   

   The reason that this works, is that we can't run JS in our tests. So we have to mimic what the JS would do, in PHP. That's what DialogTest does. But, for correctness, this should actually still set 'data-dialog-type => 'modal'.

Also: I did another round of manual testing, still working perfectly :)

CR is still needed.

1) sure.
2) actually, we sort of use GLOBALS about as often as global $base_path and this terrible code stands out for future improvements/refactor imho.
3) sure.

Let's do this!

Next step: #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use.

Worked with @dawehner to fix the javascript dialogType.

Generally this looks great. Just this made my eyes screw up a bit.

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -1834,8 +1843,21 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
+      if (isset($ajax_settings['url'])) {
+        $parsed_url = UrlHelper::parse($ajax_settings['url']);
+        $options['query'] = $parsed_url['query'] + $options['query'];
+        $options += ['fragment' => $parsed_url['fragment']];
+        $ajax_path = preg_replace(
+          '/^' . preg_quote($GLOBALS['base_path'], '/') . '/',
+          '',
+          $parsed_url['path']
+        );

It's not clear to me why this much work is necessary for something that's explicitly set in ajax_settings. Could use a comment?

@catch Good question, its really confusing but that's sort of par for the course in webtest I guess. When we're given path for the ajax url as an argument we get a site path like "node/1." You can't see it in the diff, but earlier in the method $ajax_settings however can be generated by xpath'ing form information out of a GET request so the url will have been prefixed with the site base_path. We go through all this trouble to remove the path so the rest of the code can work on the assumption that we have a simple site path.

OK that could really use some docs. Otherwise I don't see anything else to hold this up, looks good overall.

CNW for the docs.

1. +++ b/core/modules/simpletest/src/WebTestBase.php
   +++ b/core/modules/simpletest/src/WebTestBase.php
   ++        // In order to allow to set for example the wrapper envelope query
   ++        // parameter we need to get the system path again.
   ...
   ++        // We know that $parsed_url['path'] is already with the base path
   ++        // attached.
   

   These docs do help make sense of this ugliness

2. +++ b/core/modules/simpletest/src/WebTestBase.php
   +++ b/core/modules/simpletest/src/WebTestBase.php
    -          '/^' . preg_quote($GLOBALS['base_path'], '/') . '/',
   ++          '/^' . preg_quote(base_path(), '/') . '/',
   

   Might as well use the function, +1

Thanks for the updated issue summary and CR!

Random failure in Drupal\views\Tests\Plugin\RowRenderCacheTest->doTestRenderedOutput()

This issue addresses a critical task and is allowed per https://www.drupal.org/core/beta-changes. Committed ad87128 and pushed to 8.0.x. Thanks!

Published the change record ...

I adjusted the CR to make the examples consistent (the request and render array examples were demonstrating dialogType 'modal', the #ajax examples was demonstrating dialogType 'ajax' - moved all to 'modal' for symmetry)

Filed follow-up #2725435: Remove outdated @todo pointing to #2364011