Problem/Motivation
This core bug blocks this JSON API contrib module issue: #2831137: Remove the need for ?_format=api_json: assume the use of the 'api_json' format for routes managed by JSON API.
Currently \Drupal\Core\Routing\RequestFormatRouteFilter::filter()
determines the request format by doing:
$format = $request->getRequestFormat('html');
First of all, it's pointless to specify that parameter 'html'
, since that's the default.
Second, this is extremely HTML-centric. What if a single route matches the given URL (i.e. based on request path), but it has a different required format ($route->setRequirement('_format', 'json')
)? Why shouldn't we then serve json
even if the _format
request parameter is omitted?
When a route lists multiple formats ($route->setRequirement('_format', 'hal_json|json|xml|foo')
), we should not be picking a format. But what if a route will only ever serve a single format?
Look at the case of the JSON API module. All of its paths use the /jsonapi
path prefix. Therefore no routes other than JSON API routes will ever be matched. Consequently, there's always a single matching route being filtered. And that single matching route is always dismissed by \Drupal\Core\Routing\RequestFormatRouteFilter::filter()
when no _format
request parameter is specified, simply because the assumed, hardcoded default is html
.
The JSON API module's routes all have $route->setRequirement('_format', 'api_json')
. They're always the sole route match. And they're always filtered away by \Drupal\Core\Routing\RequestFormatRouteFilter::filter()
when you don't specify ?_format=api_json
in your URL. This makes no sense. It's HTML-centric.
Proposed resolution
If there's only a single matching route being filtered, and that route also specifies a _format
requirement, let that be the default format instead of 'html'
.
Remaining tasks
TBD
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#15 | 2854560-15.patch | 5.44 KB | Wim Leers |
#4 | 2854560-4.patch | 4.64 KB | Wim Leers |
#13 | 3after_applying_patch_without_format_parameter.jpg | 1.77 MB | boaloysius |
#13 | 2before_applying_patch_without_format_parameter.jpg | 195.47 KB | boaloysius |
#13 | 1before_applying_patch_with_format_parameter.png | 1 MB | boaloysius |
Comments
Comment #2
Wim LeersFirst, let's improve the existing test coverage. It's pretty poor right now.
Comment #3
Wim LeersThis is the test I want to pass. That's what I'm proposing in the IS. This should currently fail.
Comment #4
Wim LeersComment #5
Wim LeersComment #7
boaloysius CreditAttribution: boaloysius as a volunteer commented@WimLeers I am unable to test your patch. I downloaded drupal from https://www.drupal.org/node/3060/git-instructions/8.3.x/nonmaintainer and tried to apply patch. It shows errror
Comment #8
Wim LeersWorks fine here:
What git version do you have?
Comment #9
boaloysius CreditAttribution: boaloysius as a volunteer commentedgit version: 2.11.0
Comment #10
Wim LeersThen I suggest redownloading the patch.
Comment #11
krknth CreditAttribution: krknth as a volunteer and at Valuebound commented@boaloysius : The patch #4 works. Make sure you've latest code cloned in 8.3.x.
Comment #12
boaloysius CreditAttribution: boaloysius as a volunteer commented@WimLeers and @krknth. It works. Sorry.
Comment #13
boaloysius CreditAttribution: boaloysius as a volunteer commentedIt works fine.
For the url
http://d83.dd:8083/jsonapi/node/article/a674e88e-fa4a-4a25-8e4b-525374c9c426
,Earlier:
$format = $request->getRequestFormat($default_format);
was assigning value "html".Now:
applying the patches
1) 2854543-7.patch (#2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't),
2) 2854560-4.patch,
it is assigning "api_json".
Now the above url gives the json output without the need of (_format) parameter.
I am attaching the previous and current screenshots.
Comment #14
dawehnerI'm wondering whether for better readability this blob of code could be moved into a well named protected method.
If we pass along NULL, we don't really need this IF I guess?
Note: We use
setExpectedException
now, see https://thephp.cc/news/2016/02/questioning-phpunit-best-practicesComment #15
Wim LeersAddressed #14.
Comment #16
boaloysius CreditAttribution: boaloysius as a volunteer commentedscreenshots of #13 still hold.
Comment #17
dawehnerThank you @Wim Leers!
The static for me is actually quite cool! It just proves that you are not manipulating any state, which is great!
Comment #18
xjmNice, I like how we were able to make this change cleanly and totally BC way.
Comment #20
xjmCommitted and pushed to 8.4.x. Since this includes an internal API addition, it would normally only be allowed in a minor release, but since it is a contributed project blocker and we are in beta, is almost certainly safe and appropriate to backport. Checking with Wim about it.
Comment #21
Wim LeersExactly :)
Comment #23
xjmAnd, backporting to 8.3.x. Thanks!
Comment #25
boaloysius CreditAttribution: boaloysius as a volunteer and at Google Summer of Code commented