Closed (fixed)
Project:
JSON:API
Version:
8.x-1.x-dev
Component:
Code
Priority:
Critical
Category:
Bug report
Assigned:
Reporter:
Created:
24 May 2017 at 05:46 UTC
Updated:
12 Jun 2017 at 14:49 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
spleshkaComment #3
spleshkaTested with php 7.0 and php 7.1 on Drupal 8.3.x - could not reproduce the issue. Assuming it's something in Drupal 8.4.x.
Comment #4
e0ipsoThanks for reporting this. I think this is our current highest priority. I don't have a PHP7 environment at the moment. Maybe someone else can work on this.
Comment #5
spleshkaI'll get home in several hours, will take a look
Comment #6
spleshkaJust a note while I'm still searching for the issue: was able to reproduce the issue on both php7.0 and php7.1 with latest Drupal core 8.4.x.
Comment #7
spleshkaGot it: here's the issue which broke our tests: #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available. If I run tests with this commit in core - they fail. If I go 1 commit before - tests pass. Now going to think about the proper fix.
Comment #8
spleshkaThe issue is related to the new route added in the patch:
So instead of json response, Drupal returns html with rendered 4xx page. Trying to find a good solution (all ideas are welcome)
Comment #9
spleshkaSo now
DefaultExceptionHtmlSubscriberimplements methodon4xx(), which eventually returns the html page:I think that JSON API needs its own ExceptionSubscriber which provides the expected error handing.
Comment #10
spleshkaOh, actually
DefaultExceptionSubscriberexists in JSON API. Wondering then how the error handing appears inDefaultExceptionHtmlSubscriber?Comment #11
spleshkaOkay, I think I know the answer to #10. We're trying to test the case when there is no
Content-Typeheader. The request is treated as html request, andDefaultExceptionHtmlSubscriberis used instead of the one from JSON API. Since there was introduced a new methodon4xx(), it simply returns its value (obviously, in html representation).Comment #12
spleshkaThe fix for this issue turns out to be simple: instead of
Content-Typeheader we should sendAcceptheader to indicate that we want to get response in json format.As a follow-up, we should create an issue to highlight in documentation how important headers when it comes to errors handing.
Comment #13
spleshkaFixed the same issue in several other places.
Comment #14
spleshkaNow all tests are passing, yay! Awaiting for manual review.
Comment #15
e0ipsoThis looks good. I'll be merging soon.
Comment #17
e0ipsoComment #18
spleshkaSweeeet, thanks Mateu.
Comment #19
wim leersThe follow-up mentioned in #12 was created: #2882124: Enhance documentation to include information about "Accept" header.
Comment #20
wim leers#7 pinpointed #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available as the root cause.
Comment #21
wim leersQuoting #2882124-5: Enhance documentation to include information about "Accept" header:
Comment #22
wim leersI was also reminded of
\Drupal\rest\EventSubscriber\ResourceResponseSubscriber::getResponseFormat()by the changes here, but I don't think updating\Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriberwould have fixed it.On the one hand, I feel that this didn't actually fix the root cause. On the other hand, it's totally reasonable to require that a format is actually requested, i.e. that you must specify the
Acceptrequest header. It's just a continuation of what was already decided in #2831137: Remove the need for ?_format=api_json: assume the use of the 'api_json' format for routes managed by JSON API.So: +1!