Discovered while working on #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only.
From #2930028-9: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only's test results:
--- Expected
+++ Actual
@@ @@
-{"errors":[{"title":"Unprocessable Entity","status":422,"detail":"Unprocessable Entity: validation failed.\ntitle: Title: this field cannot hold more than 1 values.\n","links":{"info":"http:\/\/www.w3.org\/Protocols\/rfc2616\/rfc2616-sec10.html"},"code":0}]}
+{"errors":[{"title":"Unprocessable Entity","status":422,"detail":"There was an error un-serializing the data. Message: Deserialization for the format is not supported","links":{"info":"http:\/\/www.w3.org\/Protocols\/rfc2616\/rfc2616-sec10.html"},"code":0}]}
Plus, the following test assertions had to be commented:
// DX: 415 when no Content-Type request header. HTML response because
// missing ?_format query string.
$response = $this->request('POST', $url, $request_options);
$this->assertSame(415, $response->getStatusCode());
$this->assertSame(['text/html; charset=UTF-8'], $response->getHeader('Content-Type'));
$this->assertContains('A client error happened', (string) $response->getBody());
$url->setOption('query', ['_format' => 'api_json']);
// DX: 415 when no Content-Type request header.
$response = $this->request('POST', $url, $request_options);
$this->assertResourceErrorResponse(415, 'Unsupported Media Type', 'No "Content-Type" request header specified', $response);
| Comment | File | Size | Author |
|---|---|---|---|
| #38 | 2934149-38.patch | 1.11 KB | wim leers |
| #36 | 2934149-36.patch | 1023 bytes | gabesullice |
| #35 | 2934149-35.patch | 8.94 KB | gabesullice |
| #32 | 2934149-32.patch | 10.68 KB | wim leers |
| #31 | 2934149-31.patch | 10.51 KB | wim leers |
Comments
Comment #2
wim leersComment #3
wim leersOh FFS, this is a bug in Drupal core.
Comment #4
wim leersNote that this blocks #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only.
Comment #5
wim leersCreated an issue for Drupal core, plus test coverage: #2934152: ContentTypeHeaderMatcher should not run for GET, HEAD, OPTIONS or TRACE requests. Please review+RTBC!
Comment #6
wim leersComment #7
wim leersFunny, so for now the correct bugfix doesn't work due to a core bug 😩
The reason core doesn't run into this bug: because the REST module doesn't have a single route for reading *and* writing.
Comment #8
wim leers#7 comments the existing test, but that's no solution either, because the new coverage being added in #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only also contains HEAD requests, and so will run into the same failure.
:/ :/ :/
Comment #9
wim leersWow, #2934152: ContentTypeHeaderMatcher should not run for GET, HEAD, OPTIONS or TRACE requests already landed, but only in 8.5. I think it should also be able to land in 8.4. But even then JSON API wouldn't be able to commit+release this until a 8.4 patch release is tagged that contains it.
Well, in any case, the necessary bugfix does exist in 8.5, so this will definitely get fixed now :)
Comment #10
e0ipsoShould we merge this one and it will work for people running 8.5? Or what approach do you want to take?
Comment #11
wim leersWe should wait to commit+release this at least until 8.5.0 is released.
If we commit this now,
HEADrequests will fail when you're running Drupal <8.5. To be fair, until #2933515: JSON API does not support HEAD requests, HEAD requests didn't work in JSON API at all. So… maybe you don't mind committing this as-is, but that does imply that you're accepting that HEAD requests will never work in 8.4!That's why I think it's probably better to wait until at least 8.5 is out, and quite possibly we should only commit this when JSON API requires >=8.5.
Comment #12
e0ipsoI wish d.o had a minor release alarm system.
Removing the blocker tag.
Comment #13
wim leers#12: maybe some day in the future :) Every minor release gets better and better at signaling things to contrib! For example, the use of
@trigger_error(…, E_USER_DEPRECATED), and the testing infrastructure around that.Comment #14
wim leersSo #2934152: ContentTypeHeaderMatcher should not run for GET, HEAD, OPTIONS or TRACE requests definitely did not land in 8.4.
Comment #15
wim leersClosed #2863582: Provide a helpful error message when the Content-Type is not provided as a duplicate of this.
Comment #16
wim leersComment #17
wim leersRebased #7.
Comment #18
wim leers#2934152: ContentTypeHeaderMatcher should not run for GET, HEAD, OPTIONS or TRACE requests already landed in 8.5, so we can actually revert the test changes that were necessary to make this pass tests.
Comment #19
wim leersAnd if all is well, then we should now be able to restore commented out test coverage that this patch fixes.
Comment #20
wim leers#19 has one new fail:
The improvements we made to
ContentTypeHeaderMatcherin #2934152: ContentTypeHeaderMatcher should not run for GET, HEAD, OPTIONS or TRACE requests were correct … but apparently we didn't think to also do theDELETEcase.DELETErequests never have a request body. And so it doesn't make sense to require aContent-Typerequest header either.So we'll need to fix that in
ContentTypeHeaderMatchertoo. Created #2971012: ContentTypeHeaderMatcher should not run for DELETE requests for that.Comment #21
wim leers#2971012: ContentTypeHeaderMatcher should not run for DELETE requests now has a patch, with test coverage, that will hopefully land soon.
Comment #22
wim leersMore importantly, all those other failures (which you already see in #17 and #18) look like this:
Those are happening because with this patch, a 415 response will occur whenever no
Content-Typeheader is specified, so some of the pre-existing test coverage needs to be updated: it needs some commented test coverage uncommented.Comment #23
wim leersd.o-- (d.o messed up the interdiff)
Here's the right interdiff.
Comment #24
wim leersHoly shit, #2971012: ContentTypeHeaderMatcher should not run for DELETE requests already got committed! 🙀
Back to Needs Work now…
Comment #25
wim leersActually, no, just re-testing the patch should fix most of the failures! #22 has 12 fails. Reuploading the same patch, now should have fewer fails.
Comment #26
wim leersAnd fixing some minor bugs in some tests should make the patch green!
Comment #27
wim leersBam!
Comment #29
gabesullice+1, I don't think this issue ever technically had a review, it just had a lot of back and forth. It does look good to me though :) nice DX improvement.
Comment #30
wim leersI didn't mind self-RTBC'ing because it's really just a pure bug fix release. This change is necessary to use the D8 request processing/routing system correctly. The test coverage was inherited from core's REST module, which underwent as much scrutiny as one could ask for, and had to be commented due to this oversight/bug.
Still, great to have your confirmation :)
Comment #31
wim leers#2943170: JSON API's RequestHandler causes fatal PHP error when a PATCH or POST request has no body was committed, this needed a significant rebase. Nice consequence: the diff is now much easier to understand :)
Comment #32
wim leersConflicted with #2971562: Refactor/clean-up Routes.php in
Routes.php. Rebased.Comment #35
gabesulliceI didn't realize you had posted a different patch, I simply applied a rerolled version of #31. It's a bit different from yours. Attached the full patch.
Comment #36
gabesulliceThe above patch broke relationship tests in HEAD. This is a case of bad tests not logic at least :) It does prove why this needed to be a 2.x change.
Comment #38
wim leersThere was one bit of commented test coverage that I failed to uncomment.
Comment #40
wim leersComment #41
wim leersCR created: https://www.drupal.org/node/2983180.