Spec Compliance: `_format` is a disallowed query parameter name

Wow. Such a great find! And so painfully obvious now 😳

I propose that in the 2.x branch of JSON API, we simply forbid ?_format=api_json. Using it will result in a 400 response.

That's what http://jsonapi.org/format/#query-parameters requires. And in practice, most people using JSON API probably are already omitting it anyway.

Implementing this will be easy after #2933062: [>=8.5.4] Spec Compliance: Return 400 for unrecognized/unsupported query parameters lands, since #2933062-25: [>=8.5.4] Spec Compliance: Return 400 for unrecognized/unsupported query parameters added a @todo referring to this issue; we'll just need to delete that @todo and the line below it!

👌

Don't know why this was RTBC lol.

LOL! 😅

Re-applied #9's interdiff on top of #7.

Are we forbidding any parameter unmanaged by the spec? Or are we targeting only _format?

There are many reasons to need arbitrary qs params. I want to make sure that is still a possibility.

Sooo, I'm not too pleased with how this interdiff looks. Not sure if we want to fix it like this for now and just have follow up to do it "right" later or if we want to make it easy to do "right" first.

Wasn't sure about this either, but I think I managed to find another solution without spending a lot of time on it. It:

• Changes \Drupal\jsonapi\EventSubscriber\JsonApiRequestValidator::validateQueryParams() to not return a ResourceResponse (this was the only place creating a ResourceResponse with array as its data, probably wrongly so — my bad!), and instead return a HTTP exception.
• HTTP exceptions are already handled by \Drupal\jsonapi\EventSubscriber\DefaultExceptionSubscriber::onException(), so we're basically done! There is only one caveat: that means we lose the errors.links.info = http://jsonapi.org/format/#query-parameters piece of information. But even that can be solved pretty elegantly.

This is even a net code reduction, and frankly is how JsonApiRequestValidator.php should've worked in the first place. It was only added yesterday (in #2933062: [>=8.5.4] Spec Compliance: Return 400 for unrecognized/unsupported query parameters). So it's definitely fine to refine it.

#12: See #2933062: [>=8.5.4] Spec Compliance: Return 400 for unrecognized/unsupported query parameters!

I saw that and realized that it was forbidding all qs params. That's unfortunate. I'd like to review that.

D'oh #13 should have been posted on #2982478: JsonApiRequestValidator creates ResourceResponse object with array instead of JsonApiDocumentTopLevelNormalizerValue 😅 Did that, and got it committed. HEAD is now passing tests again!

I saw that and realized that it was forbidding all qs params. That's unfortunate. I'd like to review that.

It doesn't! It forbids all query string parameters that the JSON API spec reserves. Something like foo=bar is forbidden, but something like foo_foo=bar is allowed. This is just the JSON API spec, nothing more or less. See http://jsonapi.org/format/#query-parameters

According to those rules, _format=api_json is also forbidden. But since likely many clients are using URL?_format=api_json, that parameter was special-cased to be allowed by #2933062: [>=8.5.4] Spec Compliance: Return 400 for unrecognized/unsupported query parameters. This issue aims to change that, for better spec compliance.

Re-testing #7, which should still apply cleanly, and now should no longer be affected by failing HEAD tests.

If we commit this, then accessing /jsonapi/node/article via a browser will result in Dynamic Page Cache entries that are have the html rather than api_json format. This is because of the logic in FormatSetter only automatically setting the api_json format only if the correct Accept header is specified.

I think we want to change that, so that the api_json format is always set for every JSON API route?

This may seem out of scope here, but the change here is effectively forbidding the use of /jsonapi/node/article?_format=api_json, and hence this issue is forcing the need for that change.

I should have been more specific. This is what I meant:

(That's with only those 2 requests in the browsers on the right hand side having occurred.)

If we commit this, then accessing /jsonapi/node/article via a browser will result in Dynamic Page Cache entries that are have the html rather than api_json format.

Actually, this is already the case in HEAD. I should've just opened a separate issue for that. Did that now: #2987205: FormatSetter doesn't set the format to `api_json` when accessing just `/jsonapi`.

Sorry for distracting this issue.

IMHO that leaves one thing left to do: ensure that the upgrade DX is good.

{
  "errors": [
    {
      "title": "Bad Request",
      "status": 400,
      "detail": "The following query parameters violate the JSON API spec: '_format'.",
      "links": {
        "info": "http://jsonapi.org/format/#query-parameters"
      }
    }
  }
}

Is not very helpful for those that were doing it before. Let's be more helpful.

{
  "errors": [
    {
      "title": "Bad Request",
      "status": 400,
      "detail": "JSON API does not need that ugly '_format' query string! 🤘 Use the URL provided in 'links' 🙏",
      "links": {
        "info": "http://d8/jsonapi/taxonomy_term/tags/3fadcc4a-6bb2-45dc-a8fe-87f3f6884ba9/parent"
      }
    }
  }
}

That's nicer, isn't it?

I'm sure some would argue "why even bother restricting this if we can just whitelist this one parameter and not annoy anybody with it?". I understand that.

?_format got so much hate after Drupal 8 was released. So much. I'd need all my fingers and toes multiple times to count the number of times I've had to link to this CR: https://www.drupal.org/node/2501221. Many comments on that CR too, all complaining.

The reasons for having to use ?_format don't affect JSON API, thanks to it being served from its own routes, and hence there not being any need for content negotiation

But if tutorials/documentation keeps referring to ?_format, then that keeps harming us/Drupal.

Correct!

But if tutorials/documentation keeps referring to ?_format, then that keeps harming us/Drupal.

I'm not sure about this point. I think the Drupal hate comes from somewhere else, but I'll concede to

The reasons for having to use ?_format don't affect JSON API, thanks to it being served from its own routes, and hence there not being any need for content negotiation

Merging.

I got merge conflicts. Sending a new re-rolled patch.

#2949807: Spec Compliance: Error responses are missing the `jsonapi` top-level member

🍻