Problem/Motivation

First reported at #2977600-22: Spec Compliance: `_format` is a disallowed query parameter name.

See #22, #23, #26 there.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#18 2987205-18.patch5.31 KBwim leers
#14 2018-08-13 09-23-33.png170.68 KBe0ipso
#11 2987205-11.patch12.03 KBwim leers
#11 interdiff.txt3.51 KBwim leers
#8 2987205-8.patch8.88 KBwim leers
#8 interdiff.txt827 byteswim leers
#7 2987205-6.patch8.81 KBwim leers
#7 interdiff.txt2.19 KBwim leers
#4 2987205-4.patch6.87 KBwim leers

Comments

Wim Leers created an issue. See original summary.

gabesullice’s picture

gabesullice
Primary language English
commented

Whoa. When/how did this ever work? Of course the $request object doesn't have that attribute

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't

I just thought of https://www.drupal.org/node/2954953 … because that involved landing #2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't, which was originally created for JSON API: for #2831137: Remove the need for ?_format=api_json: assume the use of the 'api_json' format for routes managed by JSON API, the issue that made the patch above possible at all!

Now that #2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't is in, I think much of #2831137: Remove the need for ?_format=api_json: assume the use of the 'api_json' format for routes managed by JSON API is now obsolete? Needs investigating.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
Issue tags: +DX (Developer Experience)
StatusFileSize
new 2987205-4.patch6.87 KB

Yep … thanks to #2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't having landed, we can just set the _format route requirement on all JSON API routes to api_json, and \Drupal\Core\Routing\RequestFormatRouteFilter will automatically select it!

#2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't was a critical bugfix in https://www.drupal.org/project/drupal/releases/8.5.4. The 2.x branch already requires Drupal 8.5. I think it's reasonable to bump that to 8.5.4.

So much complexity … gone! 💀

Also, this makes JSON API more consistent: 404s previously would not have resulted in a JSON API-formatted responses, now they are. The Accept: application/vnd.api+json request header is now optional too! Better DX!

I fixed a bug by mostly deleting code:

 6 files changed, 8 insertions(+), 96 deletions(-)

😀

Status: Needs review » Needs work

The last submitted patch, 4: 2987205-4.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » wim leers
Status: Needs work » Needs review

Let's make this green!

This failed on the 415 response assertions. Because \Drupal\Core\Routing\ContentTypeHeaderMatcher::filter() throws a 415 and the format hasn't been set. I think a 415 itself is a strong enough signal. A response with this status code is all that the JSON API spec requires anyway.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt2.19 KB
new 2987205-6.patch8.81 KB

d.o is drunk 🤡

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt827 bytes
new 2987205-8.patch8.88 KB

#2949807: Spec Compliance: Error responses are missing the `jsonapi` top-level member and #2991389: Test coverage: relationship response cacheabiliity made changes that requires a change here too.

The last submitted patch, 7: 2987205-6.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 8: 2987205-8.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt3.51 KB
new 2987205-11.patch12.03 KB

Status: Needs review » Needs work

The last submitted patch, 11: 2987205-11.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt632 bytes
new 2987205-13.patch11.73 KB

D'oh.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Assigned: Unassigned » wim leers
StatusFileSize
new 2018-08-13 09-23-33.png170.68 KB

This is great 🎉🔥!

+++ b/tests/src/Functional/InternalEntitiesTest.php
@@ -154,6 +152,7 @@ class InternalEntitiesTest extends BrowserTestBase {
+    var_dump($path);

I see you're still iterating on tests. The code itself looks fantastic and is approved.

Let's make these tests pass and we can RTBC.

I am compelled to point out that the spec says MUST in here:

This is a great example where we say, the spec is there to make our life easier not harder. If it doesn't make sense, we should go against it (it was written by flawed humans, like us).

I will probably quote this example in other issues 😜

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Assigned: wim leers » Unassigned

Sorry, a race condition made the assignment change.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs review » Needs work

I think I'm missing something WRT tests. I am not sure why assertions are changing so much for this issue.

  1. +++ b/tests/src/Functional/InternalEntitiesTest.php
@@ -100,16 +100,14 @@ class InternalEntitiesTest extends BrowserTestBase {
-        $document['errors'][0]['status'],
...
+      $this->assertSame(404, $this->getSession()->getStatusCode());

    These are not asserting the same thing. I'm assuming this was a cosmetic change? 

  2. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
@@ -283,9 +283,8 @@ class JsonApiFunctionalTest extends JsonApiFunctionalTestBase {
+    // Even without the 'Accept' header the 404 error is formatted as JSON API.
+    $this->assertSession()->responseHeaderEquals('Content-Type', 'application/vnd.api+json');

    👏🏽 

  3. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
@@ -592,11 +591,6 @@ class JsonApiFunctionalTest extends JsonApiFunctionalTestBase {
     $this->assertEquals(415, $response->getStatusCode());
-    $this->assertNotEmpty($created_response['errors']);
-    $this->assertEquals(
-      'Unsupported Media Type',
-      $created_response['errors'][0]['title']
-    );

    I'm unsure why we are losing these assertions and not the one on line 594. 

  4. +++ b/tests/src/Functional/MessageTest.php
@@ -172,7 +172,8 @@ class MessageTest extends ResourceTestBase {
-    $this->assertResourceErrorResponse(405, 'No route found for "GET /jsonapi/contact_message/camelids": Method Not Allowed (Allow: POST)', $response, FALSE, ['4xx-response', 'http_response'], [''], FALSE, 'UNCACHEABLE');
+    $this->assertSame(405, $response->getStatusCode());

    We seem to be losing some additional assertions.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review

#14:

This is great 🎉🔥! […] The code itself looks fantastic and is approved. […] Let's make these tests pass and we can RTBC.

😀 Hurray!

I will probably quote this example in other issues 😜

Hah! 😀 I'm not sure which "MUST" you were referring to in the screenshot?

#16:

I am not sure why assertions are changing so much for this issue.

I explained that in #6.

  1. Both the old and new assertion are asserting that the response is a 404. The new assert is no longer asserting the message, because the message is no longer there. For a reason similar to that in #6: we do still end up with a 404, but the format is not being set to api_json, since … no route was found, and hence the format never got set to api_json.
    And hence the 404 response does not contain JSON, but HTML (the default).
  2. :) Note that the reason this 404 does use the api_json format, is that it's a non-existing UUID on an actual JSON API route. The assertions in point 1 are 404s on a non-existing route. That's the difference!
  3. See #6 again.
  4. For the same underlying reason as #6, but this time in MethodFilter.

I share your concerns. The only way to make this work then though is to still keep FormatSetter…

As explained in #3 and #4, thanks to #2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't plus setting requirements: [format: api_json] allows us to remove a lot of custom code. But there's one downside: 4xx exceptions thrown during routing do not result in api_json responses.

So we have a choice to make here:

  1. Either we accept that 4xx responses caused during routing do have the correct status code but not the correct format; this allows the JSON API module to be simpler
  2. Or we do not accept this, and keep FormatSetter … but make it a little bit simpler
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 2987205-18.patch5.31 KB
  1. Either we accept that 4xx responses caused during routing do have the correct status code but not the correct format; this allows the JSON API module to be simpler
  2. Or we do not accept this, and keep FormatSetter … but make it a little bit simpler

If we choose 1, #13 is the patch.

If we choose 2, this is the patch (hopefully green on first try 🤞). No code removal, but best spec compliance, and still code simplification!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I actually prefer option 2 too. Fewer changes, more robust, stronger spec compliance.

I would have loved to do option 1, but alas, it comes with too many downsides.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

I prefer option 2 as well. However, we explored that option years ago and run into dead ends:

  1. We cannot rely on /jsonapi being in the URL (it can be overridden by JSON API Extras). We'll need to check the container for the param and it may not be ready that early.
  2. We cannot rely on free detection of the JSON API prefix, because one may blog about JSON API in their /blog/jsonapi and expect HTML format.
  3. We cannot expect the prefix to be at the beginning of the path because of language negotiation and any other unanticipated URL based negotiation realms. The APIs for that are there and we should not ignore them.

With all that in mind we resolved that the only acceptable way to know if we are dealing with a JSON API request is to check the matched route's metadata. That implies that errors that happen before matching routes will miss the format setter (404, route level 403).

What about requiring the Accept header? Did we reopen that debate?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
  1. That was fixed a a long time ago, in #2971745: Don't hardcode `/jsonapi/` in FormatSetter, read JSON API base path from container parameter instead.
  2. That is fixed in #18, because it specifically checks that the request path starts with /jsonapi/!
  3. That is the only thing that is currently not guaranteed to work. But translation support is not yet official, so we can deal with that edge case in #2794431: [META] Formalize translations support. Furthermore, #18 specifically adds test coverage that verifies that only the "404/405/415 triggered during routing" results in a HTML response. The existing \Drupal\Tests\jsonapi\Functional\JsonApiFunctionalMultilingualTest::testReadMultilingual() test coverage proves that multilingual "normal" JSON API responses do in fact work.

So I think #18 actually already addresses 99% of your concerns?! :)

What about requiring the Accept header? Did we reopen that debate?

We haven't yet. But that would violate one of your primary principles AFAIK … because that'd mean going to /jsonapi in your browser would NOT return JSON, but a HTML 4xx response…

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs review » Reviewed & tested by the community

But translation support is not yet official

Hmm, I don't agree with this. Many, many people use JSON API in multilingual decoupled apps. @dawehner and I worked on this in the past to make sure GETs were supported. Oddly enough no one ever asked about more than that when it comes to translations.

I don't think we should break what we have unless it's completely necessary and unavoidable. We could however (if we really wanted to) transition to a new content translation paradigm (force language negotiation based on Accept-Language headers), and then revisit this. However, even then I don't think we should proceed because the realm negotiation APIs are still in effect for anyone to make use of them. By inspecting the URL like that we get a very small win compared with the damage it causes.

Again, we considered this years ago and decided against, I think all of the reasons from back then are still valid today.

My opinion is let's go with #13 if we want to move this forward.

RTBCing #13 here. If you want to push for #18 we can set back to Needs Review and I'm happy to discuss further.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

RTBCing #13 here.

I'm … confused. I actually would like to do #13 too (because less code, less complexity), but I thought you actually disliked that more, based on your review in #16? And it comes with certain downsides that I explained in #17. So … yeah … I'm confused 🙃 😀

Oddly enough no one ever asked about more than that when it comes to translations.

Moved this to #2794431-53: [META] Formalize translations support! 🙏

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

LOL, all in agreement for #13. Sorry for the confusion, my review in #16 was meant to be positive.

In any case, feel free to commit #13.

  • Wim Leers committed cc3c6f0 on 8.x-2.x 
    Issue #2987205 by Wim Leers, e0ipso: FormatSetter doesn't set the format...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

Heh, no problem 😁

That was a bit confusing indeed, which is why I did all that extra work to absolutely ensure that you agreed, and had the full set of choices available to you 🙏

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.