NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't

1. +++ b/core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php
   @@ -71,7 +71,7 @@ public function testFormatViaQueryParameter() {
      public function testUnknowContentTypeReturnsHtmlByDefault() {
   

   Should we rename the method now?

2. +++ b/core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php
   @@ -71,7 +71,7 @@ public function testFormatViaQueryParameter() {
   -    $this->assertSame('html', $this->contentNegotiation->getContentType($request));
   +    $this->assertSame(NULL, $this->contentNegotiation->getContentType($request));
   
   @@ -83,7 +83,7 @@ public function testUnknowContentTypeButAjaxRequest() {
   -    $this->assertSame('html', $this->contentNegotiation->getContentType($request));
   +    $this->assertSame(NULL, $this->contentNegotiation->getContentType($request));
   

   What about using assertNull though.

Thank you.

I'm not sure its true that the test suite passes since we changed it. I'm pretty sure from what I remember of that original issue that was explicitly the desired behavior. We didn't want anything to fall back on Content-type because we didn't trust reverse proxies to correctly handle the Varies header.

Shouldn't we add a test with code similar to what the contrib module is trying to do. So that we can be sure it works? At the moment the only tests changed here are unit tests which seem pretty tied to our assumptions about the way the code works.

Also I think @neclimdul's comment needs addressing. However it's going to be hard to reason about because this behaviour from the routing merge...

git log -S "// Do HTML last so that it always wins."
commit f29f1e212967ce4aa5f879a30d828565bb44089c
Author: Alex Pott <alex.a.pott@googlemail.com>
Date:   Thu Aug 27 00:27:14 2015 +0100

    Issue #2506533 by Jaesin, minnur, neclimdul, sdstyles, dawehner, Crell: Remove ContentNegotiation and embed functionality in the middleware

commit 223049a02c9e8b07e3755f05c6bcdab2094acf39
Author: Katherine Bailey <katherine@katbailey.net>
Date:   Wed Apr 18 22:06:17 2012 -0700

    Use the getFormat() method on the request object for content type negotiation, and fallback to html in the case of no Accept header.

Shouldn't we add a test with code similar to what the contrib module is trying to do.

Don't we have that already? \Drupal\KernelTests\Core\Routing\ContentNegotiationRoutingTest::testFullNegotiation does that already

Here is a way to solve this issue. We override the request object and let it behave like we want it to behave, aka. not storing the default value, but still have the same fallback. I actually believe we could fix that upstream, but here is a patch to prove the point.

Let's see what the testbot says

1. +++ b/core/lib/Drupal/Component/HttpFoundation/Request.php
   @@ -0,0 +1,20 @@
   +      $this->format = $this->get('_format');
   

   Never directly use ->get() on the request object, since that method behaves differently depending on Symfony major version. Use ->query->get() instead.

2. +++ b/index.php
   @@ -15,7 +15,9 @@
   +Request::setFactory('\Drupal\Core\DrupalKernel::createRequest');
    $request = Request::createFromGlobals();
   +
   

   Interesting. So everywhere Request::createFromGlobals() is called we need to make sure the factory is set.

   It looks like we can't put this in a more central place like DrupalKernel. We could put it into the constructor, not sure if that is a good idea.

   Or maybe we should introduce a \Drupal::createRequestFromGlobals() method that makes sure the factory is set. Then we could replace all our Request::createFromGlobals() calls.

Never directly use ->get() on the request object, since that method behaves differently depending on Symfony major version. Use ->query->get() instead.

LOL, well, that is a copy from upstream code.

Note: Here is the corresponding PR: https://github.com/symfony/symfony/pull/21805

It looks like we can't put this in a more central place like DrupalKernel. We could put it into the constructor, not sure if that is a good idea.

yeah i thought about that for a second, and wasn't sure this would be a good idea at all.

Or maybe we should introduce a \Drupal::createRequestFromGlobals() method that makes sure the factory is set. Then we could replace all our Request::createFromGlobals() calls.

Good idea. Well, let's see whether we can just get the symfony PR in, as this solves the problem as well.

Ok, this should backout the changes now that https://github.com/symfony/symfony/pull/21805 is in.

DefaultFormatTest and NegotiationMiddlewareTest pass locally. Let's do a full run to see what else happens.

Nice! A few questions and a nit :)

1. +++ b/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php
   @@ -88,8 +90,8 @@ protected function getContentType(Request $request) {
   -    // Do HTML last so that it always wins.
   -    return 'html';
   +    // No format was specified in the request.
   +    return NULL;
   

   This is the significant change that will fix the root cause of this issue.

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -156,12 +156,22 @@
   +  protected function provisionEntityResource($single_format = FALSE) {
   

   Small suggestion: why not $formats = NULL then $format = is_null($format) ? [static::$format, 'foobar'] : $formats;? Then for 'no format' just pass an empty list [].

3. +++ b/core/modules/system/tests/modules/default_format_test/default_format_test.routing.yml
   @@ -0,0 +1,17 @@
   +    # Same controller + method!
   +    _controller: '\Drupal\default_format_test\DefaultFormatTestController::content'
   

   Good note. Am I missing how a single, non-html format is tested?

4. +++ b/core/modules/system/tests/modules/default_format_test/default_format_test.routing.yml
   @@ -0,0 +1,17 @@
   \ No newline at end of file
   

   Nit: missing newline

Full disclosure: that's not what I noticed. I did indeed overlook that the routes were different even though the controller was the same, so they indeed had only one format :)

+++ b/core/tests/Drupal/FunctionalTests/Routing/DefaultFormatTest.php
@@ -20,7 +20,7 @@ public function testFoo() {
-    $this->assertSame('format:html', $this->getSession()->getPage()->getContent());
+    $this->assertSame('format:json', $this->getSession()->getPage()->getContent());

Should we keep the incorrect format call and assert 406 here too?

There was no incorrect ?_format=… query string?

Never mind me, I was being dense.

This looks good to me now :)

I applied the patch in #51. I updated my View -> Rest export display to have "json" be the only "Accepted request formats" in Format -> Serializer -> Settings. Now I no longer need to have the _format=json query argument.

Thanks!

I haven't worked out why yet but this patch isn't working for the Relaxed module which should return JSON based on CouchDB protocol.

/relaxed - Works
/relaxed/live - Works
/relaxed/live/* (/relaxed/live/_changes) - Client Error

All are configured the same with only json accepted so not sure why this is happening. Trying to work through it now.

I have been trying to get patch #51 working with Relaxed as well. One issue is that RequestFormatRouteFilter->getDefaultFormat() always returns 'html' if there are multiple routes in the collection.

I have created a new patch which checks if all the routes require the same format. This works for the RemoteCheck plugins in Relaxed - check for Relaxed Remote errors on the status report page. Deploy still fails because rest.relaxed.doc.GET accepts json and mixed formats.

Sorry, please try this patch not #56.

I have updated my patch (based on #51) so that RequestFormatRouteFilter->filter() keeps routes if they support only one format and the request does not specify the format.

1. +++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
   @@ -17,7 +17,13 @@ class RequestFormatRouteFilter implements FilterInterface {
   -    $format = $request->getRequestFormat($default_format);
   +    if (is_null($request->getRequestFormat(NULL))) {
   +      $format = $default_format;
   +      $request->setRequestFormat($default_format);
   +    }
   +    else {
   +      $format = $request->getRequestFormat($default_format);
   +    }
   

   It would be nice to document in line, why we need this additional logic.

2. +++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
   @@ -70,14 +78,25 @@ public function filter(RouteCollection $collection, Request $request) {
   +      $iterator->next();
   +      for ($iterator; $iterator->valid(); $iterator->next()) {
   +        $route = $iterator->current();
   

   Is there a reason we don't just use foreach which doesn't require you to use the iteration directly?

3. +++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
   @@ -70,14 +78,25 @@ public function filter(RouteCollection $collection, Request $request) {
   +        if ($required_format != $route->getRequirement('_format')) {
   

   Let's use a strict comparison here :)

Thanks for the feedback. I'll try to provide more details later and to look at the large set of test failures for patch #58.

Also jeqq is working on a fix for Relaxed - #2955317-9: The module needs fixes after changes in Drupal 8.5.x core

Thank you for your work.

I tested #58 successfully.
Same remark as in #54. I needed to configure Json as the only "Accepted request formats" in Format -> Serializer -> Settings
Now I no longer need to have the _format=json query argument.

Some interesting observations while comparing what happens in 8.4.x and 8.5.x in relaxed:

• Relaxed overrides \Drupal\multiversion\RouteProvider, but it turns out
  that doesn't matter
• Relaxed overrides \Drupal\relaxed\Plugin\rest\resource\ResourceBase::routes,
  which means that assumptions might no longer be true
• Finally \Drupal\rest\Routing\ResourceRoutes::getRoutesForResourceConfig sets the format,
  which was changed in b4a02309bf739ae16ceedda3ff3a181d9a3a9b82. Basically one problem I see is
  ::routes used to be overridable, with b4a02309bf739ae16ceedda3ff3a181d9a3a9b82 more
  logic is living outside.

Summary: I think we should move the relaxed usecase into its own issue, because it is actually a rest module problem,
not a generic routing problem.

I have updated patch #57 to remove the iterator and to add some comments.

Hi all, sorry to bump the thread - I'm doing some follow up work on the Drupal 8 REST documentation, according to my current 8.6.x-dev it seems to me that the "_format=" argument is still ALWAYS required - can someone vouch if that is the case?

As it stands, both the config (config/sync/rest.resource.foobarinfo.yml, "default_format: json") and the comment block in my resource handler dont' make any use of default_format

/**
 * Represents foobar test resource.
 *
 * @RestResource(
 *   id = "foobarinfo",
 *   label = @Translation("Foobar Information"),
 *   default_format = "json",
 *   uri_paths = {
 *     "canonical" = "/api/foobar/{foobar}",
 *   },
 * )
 */
class FoobarInfoResource extends ResourceBase {
  public function get($foobar = 0) {
    $record = ['id' => $foobar];
    return new ResourceResponse($record);
  }
}

curl -H "Accept: application/json" http://172.17.0.1/api/foobar/666 will give me a 406
however
curl -H "Accept: application/json" http://172.17.0.1/api/foobar/666?_format=json" will work.

According to the restui configuration, I have only the "JSON" format enabled.

Again, sorry to muddy the waters on this thread.

note: patch from #58 seemed to drop the requirement for _format=, however when more than 1 format are enabled it does not let me choose the response via the "Accept: application/(json|xml|hal_json)" header

#68, there are several related issues to _format being required now at the top of the page. Those related issues may turn up a few more. Unfortunately, this is a complex situation. The related issues tackes various aspects: this one is about inbound routing, there is another about paths for view displays, etc.

The background behind all of this is probably best summarized in #2481453: Implement query parameter based content negotiation as alternative to extensions and #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use, along with the CR in [#2501221]. The TL;DR is that the Accept header is horribly broken across the interwebs and not a reliable way to do content negotiation in a generic manner in core.

@mpdonadio ok great, yeah I'm not too worried about the the actual state of the issue, just to know if this is worth updating the documentation that _format=json type argument should always be added, so that sounds correct to me then, thanks for the reply!

Patch from comment #51 worked for me.

This looks like a nice and reasonable improvement over the original RTBC patch. I agree that the more specific REST issues need to be addressed in a separate issue Re: REST module.

This then looks like it can be RTBC again (assuming tests pass).

I started to review and test the patch with Relaxed, it's still not working with it, will add details tomorrow when I finish the testing as today I was limited in time.

Thank you @Wim Leers for the work on this!

Tested patch from #72 (..test_plus_67..) on a custom POST Resource I created and I was able to post without putting _format=json. Thanks!

After testing with Relaxed - some of our routes work without _format, but most of them not.

Are working routes with one required format and only if there is one route in the collection. Also it will work with more than one route in the collection only if they all have one and the same required format.

Which are not working: routes with multiple format requirements e.g. json|mixed and multiple routes in the collection with different format requirements (this is how RequestFormatRouteFilter::getDefaultFormat() works).

One idea might be to use a default format for the routes: in the route defaults to have _format that can be only one format (not multiple like the _format from route requirements). In this case we can check in RequestFormatRouteFilter::getDefaultFormat(), after we check for the required format in the route, for the default format of the route. Having this we have more chances to return other format that html when there is not _format specified in the query and not end with a 406.

This still won't fix all the issues we have with Relaxed and the CouchDB replicator, that because the replicator is relying on the Accept header when getting the response in a specific format, one example would be the rest.relaxed.doc.GET route that has json|mixed format requirement. The CouchDB replicator expects to get the format it needs by sending the specific format in the Accept header ('multipart/mixed' or 'application/json').

@Wim Leers I remember you said that we can't rely on using the Content-Type header, but how about Accept? Can we use it to determine the format and return the response it depending on this header (when it's set)?

@jeqq, I think it was agreed in #66, #72 & #73 that the Relaxed issues need to be addressed in a separate follow-up issue.

Relaxed appears to have very specific requirements which will prevent this fix from landing and "un-breaking" the more standard sites that were broken by this bug fix.

I think the separate problem that needs to be addressed for Relaxed is: "how to do media type negotiation without the _format query parameter." That is beyond the scope of this issue, however.

I agree with #79.

We have an issue for exactly that: #2852587: Allow REST resource config entities to specify a default format

Wll take a look.

I never said that; I actually said you can't rely on the Accept header, and that's exactly the thing that D8 has never supported. I also provided the relevant link: https://www.drupal.org/node/2501221

I wanted to check that but couldn't find the link. :)

Will also check the solutions Wim proposed. Thanks!

Couple of minor observations

1. +++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
   @@ -70,14 +79,22 @@ public function filter(RouteCollection $collection, Request $request) {
   +    $routes = array_values($collection->all());
   +
   +    $required_format = $routes[0]->getRequirement('_format');
   

   is there a chance that RouteCollection could be empty here? i.e should we be checking that [0] exists before we call methods on it? (defensive programming)

2. +++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
   @@ -70,14 +79,22 @@ public function filter(RouteCollection $collection, Request $request) {
   +    if (strpos($required_format, '|') !== FALSE) {
   

   should me make an explicit protected method for this?

   
   self::supportsMultipleFormats($requiredFormat)
   
   

   Would be more obvious what is going on? Could be public - I'm guessing we have the same logic in multiple places

3. +++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
   @@ -70,14 +79,22 @@ public function filter(RouteCollection $collection, Request $request) {
   +    for ($i = 1; $i < count($routes); $i++) {
   +      if ($required_format != $routes[$i]->getRequirement('_format')) {
   

   any reason why we didn't use foreach here? Would aid readability.

Bumping back to CNR for those points.

For points 2 + 3, the easiest solution was to almost completely refactor @polynya's code, but retain the same fundamental logic. An array_reduce() to get a list of all formats of all routes in the collection, then turn it into a set, if the set contains only one format, then we can use that as the default. Sounds simple enough hopefully? :) Much smaller too.

The new code looks quite a bit like you started to learn functional programming :)

+++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
@@ -69,15 +79,16 @@ public function filter(RouteCollection $collection, Request $request) {
+    $formats = array_unique(array_filter($all_formats));
...
+      ? $formats[0]

I think you better use reset() here. array_filter doesn't reset keys, see

 print_r(array_unique(array_filter(['', 4, 2, 3, 2, 3])));
Array
(
    [1] => 4
    [2] => 2
    [3] => 3
)

Did I mention I very much miss a Set data structure in PHP? :(

I'm sitting in a corner crying everyday that the JS Set doesn't implement union and diffs, but yeah at least it implements the Set semantics :)

Is this meant as "yay FP" or "you're obviously getting started with FP"? I'm fine with either/both, just curious now :D I've been doing this style of logic for this kind of problem for a while now.

I made the experience that many problems solved in functional style has less ugs than solved in imperative way. You can decide for hay or nay :)

Edit: Nevermind. I assumed latest patch 88 contained all previous code.

Patch only worked on the preview for me. I preview with contextual filters, go to the path provided in the preview. Page is a 406, but shows json in the preview.

Applying patch #88 with composer is generating bad files.

--- /dev/null
+++ b/core/modules/system/tests/modules/default_format_test/default_format_test.info.yml

creates the directory b/core in the .../core directory.

@alphex that is a reported issue in the latest version of composer-patches

Adding review credits

Fixed on commit

diff --git a/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
index 7448e6f1ef..8d14dd52cf 100644
--- a/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
+++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
@@ -85,7 +85,7 @@ protected static function getDefaultFormat(RouteCollection $collection) {
       $route_formats = !$route->hasRequirement('_format')
         ? ['html']
         : explode('|', $route->getRequirement('_format'));
-      return array_merge($carry,$route_formats);
+      return array_merge($carry, $route_formats);
     }, []);
     $formats = array_unique(array_filter($all_formats));

Committed as 41dfad3 and pushed to 8.6.x.
Cherry-picked as 3f955a3 and pushed to 8.5.x.

Not working for a pure headless implementation (8.5.5)

I had to put 'json' as the default format

// The default format is 'html' unless ALL routes require the same format.
return count($formats) === 1
? reset($formats)
: 'html';

return count($formats) === 1
 ? reset($formats)
: 'json';