Allow REST routes to use different request formats and content type formats

I think we might want to add a couple more cases that use the new parameter for the data provider and helper method that creates route requirements. Maybe at least one that switches JSON and XML at least.

+++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
@@ -308,6 +310,17 @@ public function providerTestResponseFormat() {
+      'unsafe methods with response (POST, PATCH): client requested format other than request body format when only XML is allowed as a content type format' => [
+        ['POST', 'PATCH'],
+        ['xml'],
+        'json',
+        ['Content-Type' => 'text/xml'],
+        $xml_encoded,
+        'json',
+        'application/json',
+        $json_encoded,
+        ['json'],
+      ],

You're saying you'd also like to test the inverse of this, right? Sounds good.

+++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
@@ -308,6 +310,17 @@ public function providerTestResponseFormat() {
+      'unsafe methods with response (POST, PATCH): client requested format other than request body format when only XML is allowed as a content type format' => [
+        ['POST', 'PATCH'],
+        ['xml'],
+        'json',
+        ['Content-Type' => 'text/xml'],
+        $xml_encoded,
+        'json',
+        'application/json',
+        $json_encoded,
+        ['json'],
+      ],

@@ -368,4 +381,40 @@ protected function getFunctioningResourceResponseSubscriber(RouteMatchInterface
+  protected function generateRouteRequirements(Request $request, array $supported_formats, array $supported_request_formats = []) {

So the second parameter here (['xml']) and the last one (['json']) are the ones that are relevant here. It'd be more logical if they'd be passed after each other. But you chose not to do that to minimize changes most likely.

That new parameter that you added to the end is the that's optional for the new generateRouteRequirements() method.

I think it'd be better to always require a $supported_request_formats and $supported_response_formats parameter. If NULL would be passed to either one, then either _format (for $supported_response_formats) would not be set, or _content_type_format (for $supported_request_formats) would not be set.

I think that'd make things much clearer, because explicit.

+++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
@@ -104,7 +104,7 @@ public function getResponseFormat(RouteMatchInterface $route_match, Request $req
     // If an acceptable format is requested, then use that. Otherwise, including
     // and particularly when the client forgot to specify a format, then use
     // heuristics to select the format that is most likely expected.
-    if (in_array($requested_format, $acceptable_formats)) {
+    if (in_array($requested_format, $acceptable_request_formats)) {

This is confusing, because $acceptable_request_formats is a misnomer (my bad!).

i.e. this code has confusing variable names (which I introduced, so totally my fault!):

    $acceptable_request_formats = $route->hasRequirement('_format') ? explode('|', $route->getRequirement('_format')) : [];
    $acceptable_content_type_formats = $route->hasRequirement('_content_type_format') ? explode('|', $route->getRequirement('_content_type_format')) : [];

I think

    $acceptable_response_formats = $route->hasRequirement('_format') ? explode('|', $route->getRequirement('_format')) : [];
    $acceptable_request_formats = $route->hasRequirement('_content_type_format') ? explode('|', $route->getRequirement('_content_type_format')) : [];

would be 100x clearer.

Fixing all that.

First fixing #9. This is effectively fixing the confusing bits I introduced in #2807501: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty. It's in-scope to clean it up here, since we're touching these very lines, and the confusing names (and comments) are making the solution more confusing than it is!

Implemented #8.

And finally, implemented #7. And also fixed some strings that were incorrect and therefore misleading.

I'm hereby RTBC'ing the changes in #2. Which means @damiankloip can now RTBC the changes I made on top of his.

This looks good - couple of minor observations.

Also I think it would be good to get someone who maintains routing system to sign off on this too - e.g. dawehner

1. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -94,23 +94,24 @@ public function onResponse(FilterResponseEvent $event) {
   +    // If an acceptable response format is requested, then use that. Otherwise,
   +    // including  and particularly when the client forgot to specify a response
   +    // format, then use heuristics to select the format that is most likely
   +    // expected.
   

   looks like something was missed here? Otherwise including and particularly when the client seems like something skipped?

2. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -94,23 +94,24 @@ public function onResponse(FilterResponseEvent $event) {
   +    if (in_array($requested_format, $acceptable_response_formats)) {
   

   nit: should we have third argument TRUE here?

3. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -94,23 +94,24 @@ public function onResponse(FilterResponseEvent $event) {
   -    elseif (!empty($request->getContent()) && in_array($content_type_format, $acceptable_content_type_formats)) {
   +    elseif (!empty($request->getContent()) && in_array($content_type_format, $acceptable_request_formats)) {
   

   The previous if returned, and this hunk returns too. While we're here should we make this just a normal if instead of an elseif for easier readability? - also, should use third argument to in_array

1. No, nothing was forgotten — this is the same as before. The comment is identical, just clarified: s/acceptable format/acceptable response format/. I do see the double space there though, that was not intentional.
2. Good nit :) Slightly out of scope, but if a committer wants it done, let's do it!
3. Hm interesting. I guess this depends on preference. It's still "if this, else if this" logically speaking. You're right we could remove the "else"
   bit though because it returns directly. I personally think the current code more legible. But I don't feel strongly. Changed per your request. Note that this made the patch more invasive!

Also I think it would be good to get someone who maintains routing system to sign off on this too - e.g. dawehner

Am I? I thought I wasn't

+++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
@@ -308,12 +318,35 @@ public function providerTestResponseFormat() {
-      'unsafe methods with response bodies (DELETE): client requested no format, response should have no format' => [
+      'unsafe methods without response bodies (DELETE): client requested no format, response should have no format' => [
         ['DELETE'],

I love this improvement!

The previous if returned, and this hunk returns too. While we're here should we make this just a normal if instead of an elseif for easier readability? - also, should use third argument to in_array

I'd have argued that we should ensure that the patch doesn't change stuff unrelated with this issue. To be fair though by renaming variables we already opened up the can of worms...

I wrote a draft change notice for this - please review https://www.drupal.org/node/2910338

Refined change notice.

Committed as 647b586 and pushed to 8.5.x

Published change record

Yay, one less blocker for #1927648: Allow creation of file entities from binary data via REST requests!