Implement query parameter based content negotiation as alternative to extensions

+++ b/core/lib/Drupal/Core/Routing/RequestFormatMatcher.php
@@ -0,0 +1,40 @@
+   */
+  public function applies(Route $route) {
+    return $route->hasRequirement('_format');
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function filter(RouteCollection $collection, Request $request) {
+    if ($format = $request->getRequestFormat('html')) {

Isn't the conditional at the start of filter() redundant with the applies() call? (Minor nitpick, I know.)

Fixed a few nits. Sorry for jumping in the issue :)

fix constants, and put back one header change that we'll fix in novice issues.

missed one

comment cleanup and rm class

rm old test also

I know most of these will probably fail, but i want to make sure that tests are getting thru.

re-roll

To enable header-based format, you'd need to change the container to use a different middleware that acts like \Drupal\Core\StackMiddleware\NegotationMiddleware and inspects the headers instead or in addition to the query string

Nice work @dawehner

1. +++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
   @@ -0,0 +1,57 @@
   +      // If its not a match, we move the route to the end, so we effectively
   

   "its not" is not correct either "it's not" or "it is not" :)

2. +++ b/core/misc/progress.js
   @@ -83,11 +83,18 @@
   +        if (uri === -1) {
   +          uri += '?';
   +        }
   +        else {
   +          uri += '&';
   +        }
   

   This is an ugly check can we at least make it trinity condition.

3. +++ b/core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php
   @@ -0,0 +1,169 @@
   +      // Alias with extension pointing to dynamic extension/linked content-type.
   +      // @todo Path aliases with query strings aren't supported at the moment.
   +      // ['alias.json', '', 'application/json', FALSE],
   +      // ['alias.json', '*/*', 'application/json', FALSE],
   +      // ['alias.json', 'application/xml', 'application/json', FALSE],
   +      // ['alias.json', 'application/json', 'application/json', FALSE],
   ...
   +      // Alias with extension pointing to dynamic extension/linked content-type.
   +      // @todo how to test aliasing? Can we just assume aliasing does its thing?
   +      // this might not even work :(
   

   Still @todo?

4. +++ b/core/tests/Drupal/Tests/Core/Routing/RequestFormatRouteFilterTest.php
   @@ -0,0 +1,67 @@
   +  public function testFilter() {
   

   Can we add a quick test method for exception as well?

since this is a major task, can we get a beta evaluation... to make sure time spent on this is time well spent?

This is probably going to end up being the main patch on #2481453: Implement query parameter based content negotiation as alternative to extensions but it depends on a final decision between query parameters vs. extensions where we end up doing this. The beta evaluation is the same as the parent issue though.

The advantage of query parameters is IMHO that we can do:

- Remove all Accept headers (optional, contrib can override somehow)
- Take query argument
- Put into Accept header
- Done

Works like before.

RTBC +1, looks great to me.

But this needs maintainer sign-off.

Also tagging for framework manager review.

I really like the simplicity of the approach used here.

I'll try to review this later tonight. However, I still want to be able to compare it against the extension-based approach as planned since this is a rather large decision. (Reviews from others in the mean time are most welcome!)

I can't really tell if they're captured by the pros/cons in the current issue summary, but the two biggest advantages of query parameters I remember were:

- Query parameters don't cause problems when you actually meant "/sitemap.xml" and not "/sitemap" in the format of "xml"
- Query parameters can safely be ignored if they don't apply, so if ?_format=hal_json would be nonsense for e.g. node/1/edit, you can just fallback to HTML without breaking any spec.

Query parameters have the following advantage over extensions / accept headers:

They are not negotiated during routing, but whatever you set is taken as input value and that is also what the cache varies on.

Yes, we currently use a middleware to set the format from accept and we tried to do the same for extensions, but catch always pushed back on that as it was not exact and had to be negotiated during routing really ...

Yes on critical we need to make a decision on this or extensions to go much further forward with either, and this hasn't had much in the way of reviews.

The patch here looks pretty encouraging to me.

I found a few nitpicks when reading through the code, also the patch doesn't apply, so it needs to be rerolled.

1. +++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
   @@ -0,0 +1,57 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function applies(Route $route) {
   +    return $route->hasRequirement('_format');
   +  }
   

   It seems that documetation for this is missing in the RouteFilterInterface

2. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1895,6 +1895,34 @@ protected function drupalPost($path, $accept, array $post, $options = array()) {
   +   * Performs a POST HTTTP request with a specific format.
   

   Typo, remove one T in HTTTP.

3. +++ b/core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php
   @@ -0,0 +1,169 @@
   +    // \Drupal\simpletest\KernelTestBase::containerBuild removes the alias path
   

   Class methods should have () included.

Is there any compelling reason to go with extensions? DX is listed as a point in favour, but honestly either extensions or query string paramaters are way better DX than the current "manage to type an obscure header properly by hand in cURL" way we have now. And we did a survey of major REST APIs in the issue summary of #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use. Of those, only Twitter's API uses extensions (though granted, none of them used query string params to specify format). But it seems like basically no matter what, as a developer, you need to learn the unique quirks of whatever system you're trying to interact with... there doesn't seem to be any real consistency out there in the wild.

Adding tag, now that we saw the patch failed to apply.

1. +++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
   @@ -0,0 +1,57 @@
   +      // If it is not a match, we move the route to the end, so we effectively
   +      // move the matching route to the front.
   +      if ($supported_formats = array_filter(explode('|', $route->getRequirement('_format')))) {
   +        if (!in_array($format, $supported_formats)) {
   +          $collection->remove($name);
   +        }
   +      }
   +      else {
   +        $collection->add($name, $route);
   +      }
   

   The comment is subtly wrong. If the route has no _format specification, we move it to the end. If it does, then no match means the route is removed entirely.

2. +++ b/core/modules/hal/src/HalServiceProvider.php
   @@ -19,7 +19,7 @@ class HalServiceProvider implements ServiceModifierInterface {
      public function alter(ContainerBuilder $container) {
   -    if ($container->has('http_middleware.negotiation')) {
   +    if ($container->has('http_middleware.negotiation') && is_a($container->getDefinition('http_middleware.negotiation')->getClass(), '\Drupal\Core\StackMiddleware\NegotiationMiddleware', TRUE)) {
          $container->getDefinition('http_middleware.negotiation')->addMethodCall('registerFormat', ['hal_json', ['application/hal+json']]);
        }
      }
   

   I'm unclear on why this change is needed.

3. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -195,14 +196,19 @@ public function denormalize($data, $class, $format = NULL, array $context = arra
   +  protected function getEntityUri(EntityInterface $entity) {
   +    // Some entity types don't provide a canonical link template, at least call
   +    // out to ->url().
   +    if ($entity->isNew() || !$entity->hasLinkTemplate('canonical')) {
   +      return $entity->url('canonical', []);
   +    }
   +    $url = $entity->urlInfo('canonical', ['absolute' => TRUE]);
   +    return $url->setRouteParameter('_format', 'hal_json')->toString();
   

   The comment doesn't make sense. An entity that doesn't have a canonical link template... shouldn't be linked to, because there is nothing to link to. Also, the grammar doesn't work, and I'm not even sure what it's saying. :-)

   The code, too, confuses me. If it has a canonical link, use it; else... use the canonical link?

4. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -266,7 +266,10 @@ public function collectRoutes(RouteCollection $collection) {
          // Format as a string using pipes as a delimiter.
   -      $requirements['_format'] = implode('|', $style_plugin->getFormats());
   +      /** @var \Drupal\rest\Plugin\views\style\Serializer $style_plugin */
   +      if ($formats = $style_plugin->getFormats()) {
   +        $requirements['_format'] = implode('|', $formats + ['html']);
   +      }
   

   Isn't this an unrelated functionality change? This adds html as a valid format for all Views REST Export routes.

   Note: I'm not saying that's a bad thing to do; thinking about it it makes quite a bit of sense, although that may render the point of setting _format irrelevant anyway. But it seems off topic here.

5. +++ b/core/modules/rest/src/Tests/ReadTest.php
   @@ -79,18 +80,18 @@ public function testRead() {
   +    $response = $this->httpRequest($account->urlInfo()->setRouteParameter('_format', $this->defaultFormat), 'GET');
   +    // RequestFormatRouteFilter considers the canonical, non-REST route a match,
   +    // but a lower quality one: no format restrictions means there's always a
   +    // match and hence when there is no matching REST route, the non-REST route
   +    // is used, but can't render into application/hal+json, so it returns a 406.
   

   Is that accurate? From the code above it doesn't feel like it. If _format is set on a route, a non-specified format is removed entirely. It's not considered, even if it's HTML.

   (This comment is repeated multiple times; if changed, remember to change it in the other places, too.)

6. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
   @@ -76,7 +76,7 @@ public function testSerializerResponses() {
   -    $actual_json = $this->drupalGet('test/serialize/field', array(), array('Accept: application/json'));
   +    $actual_json = $this->drupalGet('test/serialize/field', array('query' => ['_format' => 'json']));
   

   If you're going to change one array() -> [], change all of them on the same line. :-)

   (This comment applies to most of this class.)

7. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1896,6 +1896,34 @@ protected function drupalPost($path, $accept, array $post, $options = array()) {
   +  protected function drupalPostWithFormat($path, $format, array $post, $options = array()) {
   

   Whichever approach we end up taking, I like this method! Should there be a GET version, too?

8. +++ b/core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php
   @@ -0,0 +1,169 @@
   +      // Extension is part of the route path. Constant Content-type.
   +      ['conneg/simple.json', '', 'application/json', FALSE],
   +      ['conneg/simple.json', 'application/xml', 'application/json', FALSE],
   +      ['conneg/simple.json', 'application/json', 'application/json', FALSE],
   +      // No extension. Constant Content-type.
   +      ['conneg/html', '', 'text/html', FALSE],
   +      ['conneg/html', '*/*', 'text/html', FALSE],
   

   In these comments, can you specify in human words what the expected behavior is? This looks like it's testing that extensions work, but this is the query parameter patch I thought... (The FALSE on the end tells me nothing.)

9. +++ b/core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php
   @@ -0,0 +1,169 @@
   +      if ($test[1]) {
   +        $request->headers->set('Accept', $test[1]);
   +      }
   

   If we can't use PHPUnit's lovely data providers here because it's KernelTestBase (*cries!*), at least we can name the array parameters to avoid positional data.

10. +++ b/core/modules/system/tests/modules/accept_header_routing_test/src/AcceptHeaderMiddleware.php
    @@ -0,0 +1,45 @@
    +class AcceptHeaderMiddleware implements HttpKernelInterface {
    

    <3

11. +++ b/core/modules/system/tests/modules/conneg_test/src/Controller/Test.php
    @@ -0,0 +1,45 @@
    +class Test {
    

    TestController? To follow convention...

I have some documentation things that need work.

1. +++ b/core/modules/hal/src/Tests/NormalizeTest.php
   @@ -169,14 +170,20 @@ public function testNormalize() {
   +   * @param \Drupal\Core\Entity\EntityInterface
   

   Missing the variable name. This error appears twice, but missed the second when I pasted the code in.

2. +++ b/core/modules/rest/src/Tests/ReadTest.php
   @@ -79,18 +80,18 @@ public function testRead() {
   +    // RequestFormatRouteFilter considers the canonical, non-REST route a match,
   
   +++ b/core/modules/rest/src/Tests/ResourceTest.php
   @@ -55,11 +60,11 @@ public function testFormats() {
   +    // RequestFormatRouteFilter considers the canonical, non-REST route a match,
   
   @@ -84,11 +89,11 @@ public function testAuthentication() {
   +    // RequestFormatRouteFilter considers the canonical, non-REST route a match,
   

   RequestFormatRouteFilter class should have the full namespace, when referenced in comments.

3. +++ b/core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php
   @@ -0,0 +1,165 @@
   +  function testContentRouting() {
   

   The method has no documentation.

4. +++ b/core/modules/system/tests/modules/conneg_test/src/Controller/TestController.php
   @@ -0,0 +1,45 @@
   +class TestController {
   +
   +  public function simple() {
   +    return new JsonResponse(['some' => 'data']);
   +  }
   +
   +  public function html() {
   +    return [
   +      '#markup' => 'here',
   +    ];
   +  }
   +
   +  public function format(Request $request) {
   +    switch ($request->getRequestFormat()) {
   +      case 'json':
   +        return new JsonResponse(['some' => 'data']);
   +
   +      case 'xml':
   +        return new Response('<xml></xml>', Response::HTTP_OK, ['Content-Type' => 'application/xml']);
   +
   +      default:
   +        return new Response($request->getRequestFormat());
   +    }
   +  }
   +
   +  public function variable($plugin_id) {
   +    return [
   +      '#markup' => $plugin_id,
   +    ];
   +  }
   

   No class or method documentation at all.

Well, there are tests with entity types without canonical links. If we don't do that here, we end up with failing tests.

Then those entity types are wrong. A linkable entity MUST have a canonical link. This is why. :-)

Whichever approach we end up taking, I like this method! Should there be a GET version, too?

We could, this is for sure. What about moving that to a follow up?

Works for me. Please file.

This was discussed on the recent EU criticals call - both @catch and I agree that we should proceed with this solution since it requires the least amount of change and leaves the option of exploring accept headers in contrib.

1. +++ b/core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php
   @@ -0,0 +1,168 @@
   +      // @todo how to test aliasing? Can we just assume aliasing does its thing?
   

   Pretty sure that won't work. See #118072: Allow query strings in URL aliases. However also don't think that matters. You can't alias an explicit format with accept header negotiation now either.

2. +++ b/core/modules/system/tests/modules/accept_header_routing_test/src/AcceptHeaderMiddleware.php
   @@ -0,0 +1,47 @@
   +
   +/**
   + * Example implementation of accept header based content negotation.
   + */
   +class AcceptHeaderMiddleware implements HttpKernelInterface {
   +
   +  /**
   

   Nice to see test coverage proving this is possible.

I have mixed feelings on throwing the 406 when the query parameter doesn't match, but they are just mixed feelings - not sure any other alternative would be better and that's something we could discuss further in a follow-up.

Overall this is very encouraging.

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -266,7 +266,11 @@ public function collectRoutes(RouteCollection $collection) {
       // Format as a string using pipes as a delimiter.
-      $requirements['_format'] = implode('|', $style_plugin->getFormats());
+      if ($formats = $style_plugin->getFormats()) {
+        // Always allow to at least access the page via HTML. This will return
+        // The first non HTML format which is configured/available.
+        $requirements['_format'] = implode('|', $formats + ['html']);
+      }

This needs a $requirements = []; before it now, this failed for me. Therefore tests too?

Some mostly minor commentary. Nothing fatal, I think. It sounds like we're all on board with this approach so this is just polish.

1. +++ b/core/modules/hal/src/Tests/NormalizeTest.php
   @@ -169,14 +170,20 @@ public function testNormalize() {
   +  protected function getEntityUri(EntityInterface $entity) {
   +    // Some entity types don't provide a canonical link template, at least call
   +    // out to ->url().
   +    if (!$entity->hasLinkTemplate('canonical')) {
   +      return $entity->url('canonical', []);
   +    }
   +    $url = $entity->urlInfo('canonical', ['absolute' => TRUE]);
   +    return $url->setRouteParameter('_format', 'hal_json')->toString();
   

   I still think this is working around a bug rather than fixing it. If fixing it is off topic here, at least let's add a @todo to fix those entities? If an entity can be linked to, it MUST provide a canonical link. That's the point of a canonical link.

2. +++ b/core/modules/page_cache/src/Tests/PageCacheTest.php
   @@ -105,43 +106,43 @@ function testAcceptHeaderRequests() {
   -    $this->drupalGet($node_uri);
   +    $this->drupalGet($node_url);
   

   Why the variable rename? Seems unnecessary...

3. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -266,10 +266,11 @@ public function collectRoutes(RouteCollection $collection) {
   +      if ($formats = $style_plugin->getFormats()) {
   +        // Always allow to at least access the page via HTML. This will return
   +        // The first non HTML format which is configured/available.
   +        $route->setRequirement('_format', implode('|', $formats + ['html']));
   +      }
   

   dawehner explained this block to me in IRC as being mostly for debug support; ie, if you define a REST View in the UI and then click on it to see if you didn't screw it up, you want the browser to show the output. That's not clear from the comment, which leaves me going "WTF? Why would I want to always return HTML?"

   Suggested alternative comment:

   "Allow a REST Export View to be returned with an HTML-only accept format. That allows browsers or other non-compliant systems to access the view, as it is unlikely to have a conflicting HTML representation anyway."

4. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1401,8 +1401,27 @@ protected function drupalGet($path, array $options = array(), array $headers = a
   +  protected function drupalGetWithFormat($path, $format, array $options = array(), array $headers = array()) {
   +    $options += ['query' => ['_format' => $format]];
   +    return $this->drupalGet($path, $options, $headers);
      }
   

   Nitpick: The defaults in the method signature should use [], since the method body does so.

5. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1897,6 +1916,34 @@ protected function drupalPost($path, $accept, array $post, $options = array()) {
   +  protected function drupalPostWithFormat($path, $format, array $post, $options = array()) {
   +    $options['query']['_format'] = $format;
   +    return $this->drupalPost($path, '', $post, $options);
   +  }
   

   Same as previous.

6. +++ b/core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php
   @@ -0,0 +1,168 @@
   +      // ['path', 'accept', 'content-type', 'vary'],
   

   OK, that works I guess for documentation. :-)

I think that covers it on the code front. Thanks, dawehner!

This technically needs a change record or record update or something. I'm not sure which. But RTBCing anyway to pressure dawehner into writing it before committers notice. :-)

Overall this is great, some minor things in test coverage but also one @todo asks quite a big question that I think we need at least a follow-up to figure out and document.

1. +++ b/core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php
   @@ -0,0 +1,168 @@
   +
   +      // Alias with extension pointing to dynamic extension/linked content-type.
   +      // @todo Path aliases with query strings aren't supported at the moment.
   +      // ['alias.json', '', 'application/json', FALSE],
   +      // ['alias.json', '*/*', 'application/json', FALSE],
   +      // ['alias.json', 'application/xml', 'application/json', FALSE],
   +      // ['alias.json', 'application/json', 'application/json', FALSE],
   

   Can we remove this and add it as an interdiff to the path alias query string issue?

2. +++ b/core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php
   @@ -0,0 +1,168 @@
   +
   +      // Alias with extension pointing to dynamic extension/linked content-type.
   +      // @todo how to test aliasing? Can we just assume aliasing does its thing?
   +      // this might not even work :(
   

   This could do with a follow-up @todo, or we should decide in this issue what's going to happen with aliases. But this comment is a bit of a shrug as it is.

3. +++ b/core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php
   @@ -0,0 +1,168 @@
   +    foreach ($tests as $test) {
   

   $test[3] (vary) is never used? Is it cruft or should we be asserting the header?

4. +++ b/core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php
   @@ -0,0 +1,168 @@
   +    $tests = [
   +      // ['path', 'accept', 'content-type'],
   +
   

   This doesn't have vary this time (and nor do the actual test arrays).

Also general note this conflicts with #2381277: Make Views use render caching and remove Views' own "output caching" but it's a minimal conflict in either direction.

Well, for query parameters this actually works, doesn't it?

Yeah if you have /about and do /about?_format=foo then that should work.

What you can't do at the moment is create about-json and point it to node/1?_format=foo but that's a long-standing thing and has a feature request #118072: Allow query strings in URL aliases. Just removing the @todo is good, I don't think anything is broken there by this patch.

@dawehner, not really, I'm pretty happy with this.

Moving back to RTBC. I'll either give it another once over then commit tomorrow unless I spot something, or if someone else beats me to the commit that's fine too.

Gave this another look over and nothing more to add. We should open a follow-up for the entity canonical route issue Crell brought up, and we still have #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use open for any additional follow-up to be tracked on.

I added neclimdul and znerol to commit credit due to patches on the other issue.

Committed/pushed to 8.0.x, thanks!

Here's my review - I guess this can all be handled in followups - no biggie :)

1. -    // Check all formats, if priority format is found return it.
   -    $first_found_format = FALSE;
   -    foreach ($request->getAcceptableContentTypes() as $mime_type) {
   -      $format = $request->getFormat($mime_type);
   -      if ($format === 'html') {
   -        return $format;
   -      }
   -      if (!is_null($format) && !$first_found_format) {
   -        $first_found_format = $format;
   -      }
   -    }
   -
   -    // No HTML found, return first found.
   -    if ($first_found_format) {
   -      return $first_found_format;
   +    if ($request->query->has('_format')) {
   +      return $request->query->get('_format');
        }
   

   Nice - this looks a lot simpler :) And it's great that we're able to move AcceptHeaderMatcher to a test to ensure we maintain the ability to do it in contrib.

2. +++ b/core/core.services.yml
   @@ -560,10 +560,6 @@ services:
   -      - [registerFormat, ['drupal_ajax', ['application/vnd.drupal-ajax']]]
   -      - [registerFormat, ['drupal_dialog', ['application/vnd.drupal-dialog']]]
   -      - [registerFormat, ['drupal_modal', ['application/vnd.drupal-modal']]]
   

   Can we get a followup to remove the last remnants of drupal-ajax (used in tests alot) and it's drupal- friends. Or maybe it should be cleanup here? We still have things like:

   $response = $this->drupalPost('editor/' . 'node/1/body/en/full', 'application/vnd.drupal-ajax', array());
   

   and

       // We set up a request so it looks like an request in the live preview.
       $request = new Request();
       $request->setFormat('drupal_ajax', 'application/vnd.drupal-ajax');
       $request->headers->set('Accept', 'application/vnd.drupal-ajax');
   

3. +++ b/core/core.services.yml
   @@ -758,8 +754,8 @@ services:
   +  request_format_route_filter:
   +    class: Drupal\Core\Routing\RequestFormatRouteFilter
   

   Not the fault of this patch but at some point we should have a service name standard. Note: this is just @alexpott grumbling.

4. +++ b/core/modules/quickedit/src/Tests/QuickEditLoadingTest.php
   @@ -106,7 +107,7 @@ public function testUserWithoutPermission() {
   +    $response = $this->drupalPostWithFormat(Url::fromRoute('quickedit.metadata'), 'json', $post);
   
   +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1891,6 +1910,34 @@ protected function drupalPost($path, $accept, array $post, $options = array()) {
   +   * @param string $path
   +   *   Drupal path where the request should be POSTed to. Will be transformed
   +   *   into an absolute path automatically.
   ...
   +  protected function drupalPostWithFormat($path, $format, array $post, $options = []) {
   

   Looks like this accepts a Url object too.

5. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1395,8 +1395,27 @@ protected function drupalGet($path, array $options = array(), array $headers = a
   +   * @param string $path
   +   *   Path to request AJAX from.
   

   I think this should be:

      * @param \Drupal\Core\Url|string $path
      *   Drupal path or URL to load into internal browser
   

Just posted a follow-up in #2503643: Follow-up: #2481453 (content negotiation) causes Simpletest to fail on Windows.

Wow, congrats!

Gentle nudge to @dawehner for the Needs change record updates tag.

Another followup. This one is about plugable type negotiation in contrib. It seems like it would make more since to have the type negotiation override in the ContentNegotiation class rather than having to replace the NegotiationMiddleware but there is no interface for ContentNegotiation and NegotiationMiddleware is expecting the ContentNegotiation class during instantiation.

#2506533: Remove ContentNegotiation and embed functionality in the middleware

Sorry not to have seen this one earlier : while moving from extension to query is as good a move as described, and switching from Accept header to query as a default makes /some/ sense, actively removing the ability to use that header in content negotiation within core, relegating its use to contrib strikes me as a significant loss, especially when REST best practice is a D8 sellling point.

And, yes, I know Fielding says content negotiation needn't be used constantly, but this is mostly in the context of published content to be consumed by browsers, not API access by smart clients like servers and apps.

Edit: quoting http://www.newmediacampaigns.com/blog/browser-rest-http-accept-headers :

Bottom line: If you're building APIs for other developers to consume, consider using Accept-based content-negotiation. If you're building consumer facing web apps: ignore the Accept header until WebKit and IE get their acts together.

Unfortunately it turns out this was incomplete: #2825347: 'Accept' header still is used for determining response format of error responses, ?_format= has no effect AND serialization module's exception subscriber does not support status code 500 error responses.

This issue has moved unit tests to a location where they never get executed, opened #2859538: system/tests/modules/accept_header_routing_test/tests are in the wrong place, never get executed.