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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Active » Needs review
FileSize
2.17 KB

First, let's improve the existing test coverage. It's pretty poor right now.

Wim Leers’s picture

FileSize
2.07 KB
3.28 KB

This is the test I want to pass. That's what I'm proposing in the IS. This should currently fail.

Wim Leers’s picture

FileSize
1.42 KB
4.64 KB
Wim Leers’s picture

Issue tags: +API-First Initiative

The last submitted patch, 3: 2854560-3.patch, failed testing.

boaloysius’s picture

@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

Wim Leers’s picture

Works fine here:

$ git apply -3v 2854560-4.patch
Checking patch core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php...
Checking patch core/tests/Drupal/Tests/Core/Routing/RequestFormatRouteFilterTest.php...
Applied patch core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php cleanly.
Applied patch core/tests/Drupal/Tests/Core/Routing/RequestFormatRouteFilterTest.php cleanly.

What git version do you have?

boaloysius’s picture

git version: 2.11.0

Wim Leers’s picture

Then I suggest redownloading the patch.

$ md5 2854560-4.patch 
MD5 (2854560-4.patch) = 1736ca9b0be9a265c1baa3a5e06c95be
krknth’s picture

@boaloysius : The patch #4 works. Make sure you've latest code cloned in 8.3.x.

boaloysius’s picture

@WimLeers and @krknth. It works. Sorry.

boaloysius’s picture

It 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.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
    @@ -23,7 +23,20 @@ public function applies(Route $route) {
    +    // Determine the request format. By default, use 'html' as the default
    +    // format. But when there's only a single route match, and that route
    +    // specifies a '_format' requirement listing a single format, then use that
    +    // as the default format.
    +    $default_format = 'html';
    +    if ($collection->count() === 1) {
    +      $only_route = $collection->getIterator()->current();
    +      $required_format = $only_route->getRequirement('_format');
    +      if (strpos($required_format, '|') === FALSE) {
    +        $default_format = $required_format;
    +      }
    +    }
    

    I'm wondering whether for better readability this blob of code could be moved into a well named protected method.

  2. +++ b/core/tests/Drupal/Tests/Core/Routing/RequestFormatRouteFilterTest.php
    @@ -35,10 +35,22 @@ public function testAppliesWithFormat() {
    +    if ($request_format) {
    +      $request->setRequestFormat($request_format);
    +    }
    

    If we pass along NULL, we don't really need this IF I guess?

  3. +++ b/core/tests/Drupal/Tests/Core/Routing/RequestFormatRouteFilterTest.php
    @@ -77,4 +92,20 @@ public function testNoRouteFound() {
    +   * @expectedException \Symfony\Component\HttpKernel\Exception\NotAcceptableHttpException
    +   * @expectedExceptionMessage No route found for the specified format html.
    

    Note: We use setExpectedException now, see https://thephp.cc/news/2016/02/questioning-phpunit-best-practices

Wim Leers’s picture

Addressed #14.

boaloysius’s picture

screenshots of #13 still hold.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @Wim Leers!

The static for me is actually quite cool! It just proves that you are not manipulating any state, which is great!

xjm’s picture

Nice, I like how we were able to make this change cleanly and totally BC way.

  • xjm committed 0f5eab1 on 8.4.x
    Issue #2854560 by Wim Leers, boaloysius, dawehner: \Drupal\Core\Routing\...
xjm’s picture

Committed 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.

Wim Leers’s picture

The static for me is actually quite cool! It just proves that you are not manipulating any state, which is great!

Exactly :)

  • xjm committed 7ad78e1 on 8.3.x
    Issue #2854560 by Wim Leers, boaloysius, dawehner: \Drupal\Core\Routing\...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

And, backporting to 8.3.x. Thanks!

Status: Fixed » Closed (fixed)

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

boaloysius’s picture