Remove the need for ?_format=api_json: assume the use of the 'api_json' format for routes managed by JSON API

I know that. But it's jsonapi.org and the jsonapi module et cetera. So api_json makes less sense to me than jsonapi.

We are breaking the specification by not negotiating formats

I think that we actually can bring back Accept header-based negotiation for the JSON API module. Because it has separate URLs at which nothing else than Content-Type: application/vnd.api+json is being served. So then we can be completely in line with the spec too.

The https://www.drupal.org/project/services module's D8 branch has code to bring back Accept header negotiation.

I like api_json better because it does not create confusion between the name of the format and the name of the module.

Works for me. Then that should just be documented :)

No, we could make it work just for JSON API routes.

Besides, JSON API routes only support the JSON API format. So I don't see why it's even necessary to specify the format? Why can't we make JSON API routes always return JSON API?

Reviewing!

I'm up for breaking the spec for a good reason, but breaking the spec because a user may want to use a buggy reverse proxy seems like excessive babysitting for me.

This is incorrect.

It even can happen when using a proxy, which the user may not have any choice in at all! e.g. your company or university requires all port 80 (i.e. HTTP) traffic to go through a proxy. So when your site supports JSON API, and allows anonymous users to make requests, and happens to be accessed via one of those proxies… things break horribly.

1. +++ b/src/StackMiddleware/FormatSetter.php
   @@ -0,0 +1,45 @@
   +  public function handle(Request $request, $type = self::MASTER_REQUEST, $catch = TRUE) {
   +    // Check if the accept header is set to the official header.
   +    $accept = $request->headers->get('Accept');
   +    if (strpos($accept, 'application/vnd.api+json') !== FALSE) {
   +      // Manually set the format.
   +      $request->setRequestFormat('api_json');
   +    }
   +
   +    return $this->httpKernel->handle($request, $type, $catch);
   +  }
   

   Right now, #15 is right, because this applies to all routes, unlike what the FormatSetter class' docblock claims. If you would limit this to just JSON API routes, this would be fine.

2. +++ b/src/RequestCacheabilityDependency.php
   @@ -16,9 +16,11 @@ class RequestCacheabilityDependency implements CacheableDependencyInterface {
   +    array_push($contexts, 'headers:Accept');
   
   +++ b/tests/src/Unit/RequestCacheabilityDependencyTest.php
   @@ -42,6 +42,7 @@ class RequestCacheabilityDependencyTest extends UnitTestCase {
   +      'headers:Accept',
   

   Using this cache context is a big problem: the Accept header may contain any value.

   Use the request_format cache context instead.

3. +++ b/src/StackMiddleware/FormatSetter.php
   @@ -0,0 +1,45 @@
   +    if (strpos($accept, 'application/vnd.api+json') !== FALSE) {
   

   !== FALSE should be === 0, otherwise even Accept: text/html, application/vnd.api+json would match.

#28+#29: agreed!

But I honestly wonder why it can't be done in a much simpler way.

We already have

->setRequirement('_format', 'api_json')

This means the api_json format is the only one supported, and that users must specify this format in their Accept header or ?_format query string.

But if this is the only format that is supported, why don't we just omit that requirement, just like we do for HTML (render array) routes? In other words: we can make JSON API routes have a different default format. This is also what the Symfony FOSRestBundle does: http://stackoverflow.com/a/21805333.

This does not yet pass testing; what's failing here is that \Drupal\jsonapi\EventSubscriber\DefaultExceptionSubscriber is not being used for 4xx responses (e.g. when trying to access an inaccessible entity), because api_json is not the format associated with the route. I think adding something like FOSRestBundle's default_format route option to Drupal core's routing system would make sense, and that would definitely solve it. It'd effectively be a cleaner implementation of exactly the principle that @dawehner proposed in #20: hardcoding a certain format for a certain route.

P.S.: this could even help with #2841027: JSON API format should only be used on JSON API routes, but can currently also be used for REST resources.

Indeed. What I'm suggesting (but wasn't expressed clearly enough, sorry about that!) is that we shouldn't add a new route filter … we should add this default_format capability to Drupal's routing system. The default value for it would be html. And the JSON API module would change that to api_json for its routes.

what if an error occurs before the route has been matched? How would be fetch the default_format?

Well that's the thing: default_format would cause the format to be set very early. How do we determine request format today?

    public function getRequestFormat($default = 'html')
    {
        if (null === $this->format) {
            $this->format = $this->get('_format', $default);
        }

        return $this->format;
    }

Also today, we need $request->set('_format', 'hal_json') to be called very early for HAL+JSON requests to set correct error responses. This would be no different.

This currently happens in \Drupal\Core\StackMiddleware\NegotiationMiddleware::handle().

Perhaps you meant to ask: how can we ensure that the JSONAPI format is used even before the request has been matched to a JSONAPI route? — the answer to that is that we indeed cannot. But such early failures are problematic (and rare) anyway. The vast majority of failures happen after routing, i.e. during the execution of the controller of the route that was matched to the request.

It is really a question at which point in time you add this information.

Indeed.

Do we really want people to avoid specifying the format they want to retrieve?

How are we doing that?

In a pure REST world you would always want to specify the accept header, don't you?

Yes, but the real world is different: Facebook and GitHub APIs for example use example.com/…/blah.json.

The reason I'm a bit resistant is that the Drupal routing system is already utterly complex

Agreed! And this is why I think that adding default_format is simpler than #20, because it wouldn't require the route filter system to be expanded/refined, and it wouldn't require the addition of a new route filter by the JSON API module. There are other modules that would benefit from this, e.g. https://www.drupal.org/project/acquia_contenthub and https://www.drupal.org/project/tag1quo.

and designed for a usecase for its actually not working (see page_manager).

Huh? Can you elaborate?

Actually, why are we not just simply setting the _format key in defaults? Doesn't that do exactly what I'm writing here?

In fact, what I wrote in #37 can work easily. We just need a bugfix in core's negotation middleware. It's FAR too eager to set html as the requested format, even when it wasn't requested.

(This uses the fact that Request::getRequestFormat() returns 'html' by default, unless you specify a different default.)

Simple, is it not?

We discussed this on today's JSON API call.

So we have three options right now:

1. In HEAD: ?_format: works always
2. @dawehner's proposal in #20/#24 with a lazy route filter
3. my proposal in #39

Point 2 does work for this also works for 404s (e.g. /jsonapi/articlesss will result in a JSON API-formatted 404 response.

@dawehner and @e0ipso preferred my proposal in #39, but before we make a decision there, we need to compare the different approaches (and perhaps additional ones). So let's evaluate it based on these 3 cases:

1. 404 because of typo
2. 403 because of route access
3. basic auth credentials wrong

The expectation is that all three of them use the JSON API exception subscriber. If that's not the case with one of the approaches, then that means we should carefully consider whether that approach is then still acceptable.

I worked on getting #39's core patch into core: #2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't.

And while working on evaluation mentioned at the bottom of #40, I encountered yet another bug in core that was getting in the way of the approach shown in #39: #2854560: \Drupal\Core\Routing\RequestFormatRouteFilter::filter() is too HTML-centric.

So, here's the evaluation of those 3 cases listed at the bottom of #39:

1. 404 because requesting http://d8/jsonapi/taxonomy_term/tagss instead of http://d8/jsonapi/taxonomy_term/tags results in this:
   

   The website encountered an unexpected error. Please try again later.
   
   Symfony\Component\HttpKernel\Exception\NotAcceptableHttpException: No route found for the specified format html. in Drupal\Core\Routing\RequestFormatRouteFilter->filter() (line 48 of core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php).
   Drupal\Core\Routing\LazyRouteFilter->filter(Object, Object) (Line: 247)
   Drupal\Core\Routing\Router->applyRouteFilters(Object, Object) (Line: 151)
   Drupal\Core\Routing\Router->matchRequest(Object) (Line: 90)
   Drupal\Core\Routing\AccessAwareRouter->matchRequest(Object) (Line: 203)
   Drupal\system\PathBasedBreadcrumbBuilder->getRequestForPath('/jsonapi', Array) (Line: 145)
   Drupal\system\PathBasedBreadcrumbBuilder->build(Object) (Line: 83)
   Drupal\Core\Breadcrumb\BreadcrumbManager->build(Object) (Line: 72)
   Drupal\system\Plugin\Block\SystemBreadcrumbBlock->build() (Line: 203)
   Drupal\block\BlockViewBuilder::preRender(Array)
   call_user_func('Drupal\block\BlockViewBuilder::preRender', Array) (Line: 376)
   Drupal\Core\Render\Renderer->doRender(Array) (Line: 448)
   Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
   Drupal\Core\Render\Renderer->render(Array) (Line: 490)
   Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1) (Line: 97)
   __TwigTemplate_2f32790813595ecc8efd497cb7ec0a5e027860abaefc98f501b957166bd035f4->doDisplay(Array, Array) (Line: 379)
   Twig_Template->displayWithErrorHandling(Array, Array) (Line: 347)
   Twig_Template->display(Array) (Line: 358)
   Twig_Template->render(Array) (Line: 64)
   twig_render_template('core/themes/bartik/templates/page.html.twig', Array) (Line: 384)
   Drupal\Core\Theme\ThemeManager->render('page', Array) (Line: 435)
   Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
   Drupal\Core\Render\Renderer->render(Array) (Line: 490)
   Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1) (Line: 90)
   __TwigTemplate_2479872e0e873dcbd8e108f98025f0816528f5baa7960814bfd1508980248385->doDisplay(Array, Array) (Line: 379)
   Twig_Template->displayWithErrorHandling(Array, Array) (Line: 347)
   Twig_Template->display(Array) (Line: 358)
   Twig_Template->render(Array) (Line: 64)
   twig_render_template('core/themes/classy/templates/layout/html.html.twig', Array) (Line: 384)
   Drupal\Core\Theme\ThemeManager->render('html', Array) (Line: 435)
   Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
   Drupal\Core\Render\Renderer->render(Array) (Line: 147)
   Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 574)
   Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 148)
   Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
   Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object) (Line: 111)
   Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object) (Line: 144)
   Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 2) (Line: 62)
   Symfony\Component\HttpKernel\HttpKernel->handle(Object, 2, 1) (Line: 57)
   Drupal\Core\StackMiddleware\Session->handle(Object, 2, 1) (Line: 47)
   Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 2, 1) (Line: 99)
   Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 2, 1) (Line: 78)
   Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 2, 1) (Line: 47)
   Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 2, 1) (Line: 52)
   Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 2, 1) (Line: 23)
   Stack\StackedHttpKernel->handle(Object, 2) (Line: 153)
   Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber->makeSubrequest(Object, '/system/404', 404) (Line: 109)
   Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber->on404(Object) (Line: 109)
   Drupal\Core\EventSubscriber\HttpExceptionSubscriberBase->onException(Object, 'kernel.exception', Object) (Line: 111)
   Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.exception', Object) (Line: 216)
   Symfony\Component\HttpKernel\HttpKernel->handleException(Object, Object, 1) (Line: 70)
   Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
   Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
   Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 207)
   Drupal\page_cache\StackMiddleware\PageCache->fetch(Object, 1, 1) (Line: 121)
   Drupal\page_cache\StackMiddleware\PageCache->lookup(Object, 1, 1) (Line: 75)
   Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
   Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
   Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
   Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 656)
   Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
   

   instead of:

   {
     "errors": [
       {
         "title": "Not Found",
         "status": 404,
         "detail": "No route found for \"GET /jsonapi/taxonomy_term/tagss\"",
         "links": {
           "info": "http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.5"
         },
         "code": 0
       }
     ]
   }
   

2. 403 because of route access, e.g. http://d8/jsonapi/taxonomy_vocabulary/taxonomy_vocabulary:
   

   {"data":[],"meta":{"errors":[{"title":"Forbidden","status":403,"detail":"The current user is not allowed to GET the selected resource. The \u0027administer taxonomy\u0027 permission is required.","links":{"info":"http:\/\/www.w3.org\/Protocols\/rfc2616\/rfc2616-sec10.html#sec10.4.4"},"code":0,"id":"\/taxonomy_vocabulary--taxonomy_vocabulary\/389a43cd-898a-42be-a8c5-1dee7fa8718b","source":{"pointer":"\/data"}}]},"links":{"self":"http:\/\/d8\/jsonapi\/taxonomy_vocabulary\/taxonomy_vocabulary"}}
   

3. incorrect basic auth credentials: that doesn't even work correctly with the current JSON API module… :( Try accessing http://d8/jsonapi/taxonomy_vocabulary/taxonomy_vocabulary?_format=api_json with incorrect basic auth, this is the response you get:
   

   {"data":[],"meta":{"errors":[{"title":"Forbidden","status":403,"detail":"The current user is not allowed to GET the selected resource. The \u0027administer taxonomy\u0027 permission is required.","links":{"info":"http:\/\/www.w3.org\/Protocols\/rfc2616\/rfc2616-sec10.html#sec10.4.4"},"code":0,"id":"\/taxonomy_vocabulary--taxonomy_vocabulary\/389a43cd-898a-42be-a8c5-1dee7fa8718b","source":{"pointer":"\/data"}}]},"links":{"self":"http:\/\/d8\/jsonapi\/taxonomy_vocabulary\/taxonomy_vocabulary"}}
   

   No 401 response as one would expect.

Given the recent addition of the jsonapi.resource_list route that uses the /jsonapi path (introduced in #2790395: Patch file included in commit), I'm somewhat surprised that point 1 is failing like it does. But apparently the D8 Symfony-based router does consider /jsonapi, but then dismisses it because it has number_parts = 1, but the query for /jsonapi/taxonomy_term/tagss does something like WHERE pattern_outline IN (…, '/jsonapi') AND number_parts >= 3.

So we could add the artificial /jsonapi/%, /jsonapi/%/%, /jsonapi/%/%/% and /jsonapi/%/%/%/% routes just for catching typos, and hence serving JSON API-formatted error responses. but that seems to be missing the point too.

1. As far as I can tell, this means that my proposed approach in #39 does not seem adequate.
2. Sadly, dawehner's proposal in #20 has the exact same problems :(
3. This is because both of our approaches use route filters. And route filters only run after successful routing: a route is found (no 404), and access is granted (no 403). For this to work, we also need to ensure 403 and 404s behave as expected, and that will AFAICT require going back to an approach like @e0ipso's in #11: using a middleware.

Didn't mean to get testbot to test those patches in #39. Meant to set to NR for #43's analysis.

Clarifying title.

We discussed this on today's JSON API call.

We agreed on the following steps forward:

1. Set the format using a route filter, as in the patches that @dawehner and I wrote. Probably my approach makes more sense, especially now that #2854560: \Drupal\Core\Routing\RequestFormatRouteFilter::filter() is too HTML-centric is already committed and #2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't is RTBC. Either way, the route filter approach means that 4xx/500 responses before routing to a JSON API route happened, which includes 404s, but excludes e.g. 403s due to insufficient permissions will result in less-than-great responses.
2. Support the Accept request header on JSON API routes, which will then ensure that all 4xx/500 responses work fine.

I see now that I already proposed this in #6. And @e0ipso already implemented it in #11. We had a discussion about this in #15, #16, #17 and #18.

We'll need to dig in deeper whether the steps we agreed on actually make sense after all…

1. +++ b/src/EventSubscriber/DefaultExceptionSubscriber.php
   @@ -32,8 +33,8 @@ class DefaultExceptionSubscriber extends SerializationDefaultExceptionSubscriber
   -    $format = $event->getRequest()->getRequestFormat();
   -    if ($format !== 'api_json') {
   +    $route = $event->getRequest()->get(RouteObjectInterface::ROUTE_OBJECT);
   +    if (!$route || !$route->getOption('_is_jsonapi')) {
   
   @@ -50,12 +51,13 @@ class DefaultExceptionSubscriber extends SerializationDefaultExceptionSubscriber
   -    $format = $event->getRequest()->getRequestFormat();
   -    if ($format !== 'api_json') {
   +    $route = $event->getRequest()->get(RouteObjectInterface::ROUTE_OBJECT);
   +    if (!$route || !$route->getOption('_is_jsonapi')) {
   

   I don't understand why we're making these changes.

   Can you explain why they are necessary? Which tests do they make pass?

2. +++ b/src/Routing/Routes.php
   @@ -120,7 +119,6 @@ class Routes implements ContainerInjectionInterface {
   -        ->setRequirement('_format', 'api_json')
   

   Actually, why are we removing these?

   I've been working on core patches to unblock this issue through fixes in core, and they rely on this. See #2854560: \Drupal\Core\Routing\RequestFormatRouteFilter::filter() is too HTML-centric and #2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't.

3. +++ b/tests/src/Functional/RestJsonApiUnsupported.php
   @@ -82,20 +82,7 @@ class RestJsonApiUnsupported extends ResourceTestBase {
   -    $expected_body = Json::encode([
   -      'errors' => [
   -        [
   -          'title' => 'Not Acceptable',
   -          'status' => 406,
   -          'detail' => 'Not acceptable format: api_json',
   -          'links' => [
   -            'info' => 'http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.7',
   -          ],
   -          'code' => 0,
   -        ],
   -      ],
   -    ]);
   -    $this->assertResourceResponse(406, $expected_body, $response);
   +    $this->assertResourceErrorResponse(406, FALSE, $response);
   

   Why is this changing?

1+2: but once #2857793: [PP-1] Add Accept header negotiation for JSON API is in this will change again. Why are we doing #2857793 separately and not as part of this issue? I think it makes it very confusing to understand what the intended end state is.

3: ok.

I think it'll be clearer to have both in this issue, yes.

Yay, glad this is in! And hurray, we now have a beta-1 release! :D

I have lots of nits though.

1. +++ b/jsonapi.services.yml
   @@ -99,6 +99,12 @@ services:
   +    # Set priority to 201 so it happens right before the page cache middleware
   +    # has the opportunity to respond. http_middleware.page_cache is 200.
   

   Indentation is wrong.

2. +++ b/jsonapi.services.yml
   @@ -99,6 +99,12 @@ services:
   +      - { name: http_middleware, priority: 201, responder: true }
   

   I don't think this needs responder: true.

   Quoting \Drupal\Core\DependencyInjection\Compiler\StackedKernelPass:

    * The 'http_middleware' service tag additionally accepts a 'responder'
    * parameter. It should be set to TRUE if many or most requests will be handled
    * directly by the middleware.
   

3. +++ b/src/Controller/RequestHandler.php
   @@ -11,6 +11,7 @@ use Symfony\Component\DependencyInjection\ContainerAwareTrait;
   +use Symfony\Component\HttpKernel\Exception\HttpException;
   

   Sole change in this file, hence unnecessary.

4. +++ b/src/EventSubscriber/DefaultExceptionSubscriber.php
   @@ -50,13 +51,30 @@ class DefaultExceptionSubscriber extends SerializationDefaultExceptionSubscriber
   +    $response->headers->set('content-type', 'application/vnd.api+json');
   

   Nit: Content-Type.

5. +++ b/src/EventSubscriber/DefaultExceptionSubscriber.php
   @@ -50,13 +51,30 @@ class DefaultExceptionSubscriber extends SerializationDefaultExceptionSubscriber
   +    return $format == 'api_json' || ($route && $route->getOption('_is_jsonapi'));
   

   ===

6. +++ b/src/StackMiddleware/FormatSetter.php
   @@ -0,0 +1,47 @@
   + * If the request belongs to a JSON API managed route, then sets the api_json
   + * format manually.
   

   Violates 80 cols rule. More importantly: it's completely wrong. That's not what this does. This only inspects the Accept header. It doesn't check that this is a JSON API route. And AFAICT that is very dangerous. Because it means we've re-introduced Accept header-based negotiation. Only a limited one. But it means you can force a JSON API response on any route. Fortunately it looks like this can't cause much damage, since requesting e.g. a HTML-only route with Accept: application/vnd.api+json results in a 406 error.
   But still, why not force this to be limited to requests that at least also contain /jsonapi in their request path? That may be imperfect, but reduces the problem surface area until we can find a more precise solution.

7. +++ b/src/StackMiddleware/FormatSetter.php
   @@ -0,0 +1,47 @@
   +   * Constructs a PageCache object.
   

   C/P remnant.

8. +++ b/src/StackMiddleware/FormatSetter.php
   @@ -0,0 +1,47 @@
   +    // Check if the accept header is set to the official header.
   

   s/accept/'Accept'/
   s/official header/JSON API MIME type/

9. +++ b/src/StackMiddleware/FormatSetter.php
   @@ -0,0 +1,47 @@
   +      return strpos($accept, 'application/vnd.api+json') !== FALSE;
   

   Wouldn't === 0 be more logical?

10. +++ b/src/StackMiddleware/FormatSetter.php
    @@ -0,0 +1,47 @@
    +      // Manually set the format.
    

    Pointless comment.

11. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
    @@ -244,6 +244,21 @@ class JsonApiFunctionalTest extends JsonApiFunctionalTestBase {
    +    // 19. Test non-existing route without negotiation header.
    ...
    +    // 20. Test non-existing route with negotiation header.
    

    s/negotiation/'Accept'/

12. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
    @@ -244,6 +244,21 @@ class JsonApiFunctionalTest extends JsonApiFunctionalTestBase {
    +    // Without the Accept header we cannot know we want the 404 error formatted
    ...
    +    // With the Accept header we can know we want the 404 error formatted as
    

    s/Accept/'Accept'/

Patch attached. I'm happy to move this to a new issue if you prefer that.

Riiiiight, good call!

#85.2: haha I thought about exactly that, I first used is, then switched to has. Honestly, it doesn't matter, because this is protected: it's not an API. So, I'd say: @e0ipso, you can choose :)

#89: Ah, yes, of course. That'd be much nicer. Done.

#87: I ignored your patch… because you didn't provide an interdiff, nor a comment, so I have no idea what changed. Plus, my patch in #84 passed, yours did not.

#92: they do, if you click the "4 fail" red link: https://www.drupal.org/pift-ci-job/620758.