I think we need a new test for getting taxonomy terms via the REST API.

When you try to get a taxonomy term with an "Accept" header of application/json. you get the following error:

{"message":"Not Acceptable.","supported_mime_types":["text\/html","application\/vnd.drupal-ajax","application\/vnd.drupal-dialog","application\/vnd.drupal-modal"]}

This has been tested on beta 7. Of course rest has been enabled for taxonomy terms and access has been granted. One must clear the cache after enabling rest for entities. taxonomy/term/{tid} produces the error but node/{nid} works with application/json.

Suggested commit message:

Issue #2449143 by damiankloip, Wim Leers, dawehner, Tybor: REST views specify HTML as a possible request format, so if there is a "regular" HTML view on the same path, it will serve JSON

CommentFileSizeAuthor
#158 interdiff-2449143-158.txt2.47 KBdamiankloip
#158 2449143-158.patch21.83 KBdamiankloip
#156 interdiff-2449143-156.txt1.25 KBdamiankloip
#156 2449143-156.patch20.98 KBdamiankloip
#154 interdiff-rebase-2449143-154.txt6.9 KBdamiankloip
#154 2449143-153.patch20.93 KBdamiankloip
#147 interdiff-2449143-147.txt1.24 KBdamiankloip
#147 2449143-147.patch21.72 KBdamiankloip
#143 2449143-143.patch21.35 KBdamiankloip
#143 2449143-143-tests-only-FAIL.patch3.41 KBdamiankloip
#136 interdiff-2449143-136.txt722 bytesdamiankloip
#136 2449143-136.patch18.22 KBdamiankloip
#134 2449143-129.patch18.21 KBdawehner
#126 interdiff-2449143-126.txt4.75 KBdamiankloip
#126 2449143-126.patch19.11 KBdamiankloip
#123 2449143-123.patch16.93 KBWim Leers
#123 interdiff.txt1.74 KBWim Leers
#117 2449143-117.patch16.93 KBWim Leers
#117 interdiff.txt954 bytesWim Leers
#113 2449143-113.patch16.95 KBdamiankloip
#97 interdiff-2449143-97.txt1.58 KBdamiankloip
#97 2449143-97.patch16.83 KBdamiankloip
#95 interdiff-2449143-95.txt2.56 KBdamiankloip
#95 2449143-95.patch16.01 KBdamiankloip
#90 interdifff.txt882 bytesPavan B S
#90 2449143-90.patch15.08 KBPavan B S
#87 interdiff-2449143-86.txt1.86 KBdamiankloip
#87 2449143-86.patch15.21 KBdamiankloip
#85 2449143-85.patch14.44 KBdamiankloip
#84 2449143-84.patch14.63 KBdamiankloip
#80 interdiff-2449143-80.txt864 bytesdamiankloip
#80 2449143-80.patch14.31 KBdamiankloip
#77 interdiff-2449143-77.txt2.26 KBdamiankloip
#77 2449143-77.patch14.11 KBdamiankloip
#70 Screen Shot 2017-02-19 at 6.29.24 PM.png29.91 KBTybor
#70 Screen Shot 2017-02-19 at 6.20.46 PM.png150.49 KBTybor
#65 2449143-65.patch13.13 KBWim Leers
#65 interdiff.txt1.42 KBWim Leers
#58 interdiff-2449143-58.txt1.48 KBdamiankloip
#58 2449143-58.patch13.68 KBdamiankloip
#55 interdiff-2449143-55.txt2.64 KBdamiankloip
#55 2449143-55.patch13.66 KBdamiankloip
#54 interdiff-2449143-54.txt5.67 KBdamiankloip
#54 2449143-54.patch12.91 KBdamiankloip
#50 interdiff-2449143-50.txt5.36 KBdamiankloip
#50 2449143-50.patch9.24 KBdamiankloip
#46 2449143-46.patch4.74 KBdamiankloip
#29 2449143-29.patch2.89 KBdawehner
#20 2449143-20.patch1.15 KBWim Leers
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Jaesin’s picture

Version: 8.0.0-beta6 » 8.0.0-beta7
larowlan’s picture

Try application/hal+json

Jaesin’s picture

With Hal + appropriate permissions enabled on beta6 and beta7:

Status Code: 406 Not Acceptable

    {
       "message": "Not Acceptable.",
       "supported_mime_types":
       [
           "text/html",
           "application/vnd.drupal-ajax",
           "application/vnd.drupal-dialog",
           "application/vnd.drupal-modal"
       ]
    }
brstdev5’s picture

I am also getting same error

Status Code: 406 Not Acceptable

Resposne:-
{"message":"Not Acceptable.","supported_mime_types":["text\/html","application\/vnd.drupal-ajax",null
,"application\/vnd.drupal-dialog","application\/vnd.drupal-modal"]}

Any solutions will be helpful

Best regards,
dev

Jaesin’s picture

Status: Active » Postponed

Content Negotiation is still in flux as per #2364011: [meta] External caches mix up response formats on URLs where content negotiation is in use. Using the header for content negotiation will likely be eliminated in core in favor of link generation (url) for specific content output formats.

mgifford’s picture

rcaracaus’s picture

Still getting

{
message: "Not acceptable"
}

for

http://dev-mysite.pantheon.io/taxonomy/term/32?_format=hal_json

ra.’s picture

I've resolved this issue when I've disabled default taxonomy term views and recreate similar views page with REST export

jfrederick’s picture

Version: 8.0.0-beta7 » 8.1.x-dev
Status: Postponed » Active

I am still seeing this problem in 8.0.1.

When I expose taxonomy terms via the REST API, visiting http://dev-mysite.pantheon.io/taxonomy/term/32?_format=json results in Message: Not Acceptable.
If I access the URL via curl, I get 403 Access Denied.

curl http://dev-mysite.pantheon.io/taxonomy/term/32?_format=json --include --user admin:password

HTTP/1.1 403 Forbidden
Date: Tue, 29 Dec 2015 19:58:14 GMT
Server: Apache/2.4.16 (Ubuntu)
Cache-Control: must-revalidate, no-cache, post-check=0, pre-check=0, private
X-UA-Compatible: IE=edge
Content-language: en
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
Expires: Sun, 19 Nov 1978 05:00:00 GMT
X-Generator: Drupal 8 (https://www.drupal.org)
Transfer-Encoding: chunked
Content-Type: application/json
jfrederick’s picture

Component: taxonomy.module » rest.module

Similar to #8, I worked around this problem by disabling the default View taxonomy_term. That View was taking precedence when I visited the path I listed above in #9. Thankfully I don't need that View, so I just disabled it, and the REST response took its place as the prioritized route.

In a clean core installation, if I enabled taxonomy terms as a REST Resource, it wouldn't work without first disabling the default View, which is not obvious at all. So I think this is a bug.

mistermoper’s picture

I had the same problem. I debbuged this and i found the problem is, because the route is the same as taxonomy term view, there are a conflict and taxonomy term view reacts first than rest module.This doesn't happens in all cases, like node.

I created another issue because it affects all entity types: Entity resource urls shouldn't be the same as entity view urls.

swentel’s picture

Version: 8.1.x-dev » 8.0.x-dev

Can be fixed in 8.0.x (hopefully)

dawehner’s picture

So the actual underlying issue is "interesting". Its caused that overrides in views works on the path level without taking into account formats.

Wim Leers’s picture

Issue tags: -#rest, -#taxonomy +VDC

Should we move this to routing system?

Wim Leers’s picture

Wim Leers’s picture

Title: "Not Acceptable" for Taxonomy Terms via REST » REST views: roubing problem: "Not Acceptable" for Taxonomy Terms, because Views overrides default route without considering request format
Issue tags: +DX (Developer Experience), +API-First Initiative
Related issues: +#2302615: Show path dialog when adding a path based display
Wim Leers’s picture

Title: REST views: roubing problem: "Not Acceptable" for Taxonomy Terms, because Views overrides default route without considering request format » REST views: routing problem: frontpage can return JSON, Taxonomy Term route can say "Not Acceptable", because Views overrides default route without considering request format

Marked #2605906: Frontpage shows json instead of HTML after adding a rest export display as a duplicate of this because it AFAICT has the same underlying root cause wrt Views' routing.

Quoting @dawehner in #2605906-7: Frontpage shows json instead of HTML after adding a rest export display there:

The problem is the following line of code:

        // Allow a REST Export View to be returned with an HTML-only accept
        // format. That allows browsers or other non-compliant systems to access
        // the view, as it is unlikely to have a conflicting HTML representation
        // anyway.
        $route->setRequirement('_format', implode('|', $formats + ['html']));

As you see, HTML is always added to the allowed formats, as we thought at some point its a good idea to have those sites be accessible via the browser.
Maybe we should better decide that this is a bad idea in the first place ...

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1.15 KB

Per #18.

Wim Leers’s picture

Title: REST views: routing problem: frontpage can return JSON, Taxonomy Term route can say "Not Acceptable", because Views overrides default route without considering request format » REST views specify HTML as a possible request format, so if there is a "regular" HTML view on the same path, it will serve JSON

I think this is a clearer title.

dawehner’s picture

At this point this basically reverts #1969870: REST export view should default to JSON, so yeah we have to ask whether we want that.

Wim Leers’s picture

Nice, thanks for the link.

And, as is often the case, @effulgentsia predicted exactly this problem:

  1. #1969870-34: REST export view should default to JSON: Doesn't this violate HTTP spec? If the agent asked for html, why should we return json?
  2. #1969870-40: REST export view should default to JSON: […] most browsers include */* in their Accept header, so they're capable of displaying JSON, and it's convenient for debugging to take advantage of that. But for agents that specifically ask for 'text/html' only, we need to honor that by returning a 415.

I guess an alternative solution is to make our route matcher smarter: if a particular route only supports HTML, prefer that over others. \Symfony\Component\Routing\RouteCollection is a list, not a set, so this is possible AFAICT. In fact, \Drupal\accept_header_routing_test\Routing\AcceptHeaderMatcher::filter() does exactly that.

Status: Needs review » Needs work

The last submitted patch, 20: 2449143-20.patch, failed testing.

dawehner’s picture

I guess an alternative solution is to make our route matcher smarter: if a particular route only supports HTML, prefer that over others. \Symfony\Component\Routing\RouteCollection is a list, not a set, so this is possible AFAICT. In fact, \Drupal\accept_header_routing_test\Routing\AcceptHeaderMatcher::filter() does exactly that.

Right, accept header negotation is tricky and we are aware of that. The problem is that we basically need to introduce again accept header based routing, if we want to distinct the two routes ... Maybe an alternative idea, in the views UI render the link "view page" with ?_format=json?

Wim Leers’s picture

Maybe an alternative idea, in the views UI render the link "view page" with ?_format=json?

+1

The problem is that we basically need to introduce again accept header based routing, if we want to distinct the two routes ...

Well, no, it would just be smarter matching?

_format: 'html' vs. _format: 'json|xml|hal_json|html' would result in the first one being picked.

Wim Leers’s picture

Note: the failing test coverage was introduced at #1891202: REST Export Views format should be configurable with @todos specifically for this problem. So, that's another related issue.

dawehner’s picture

So yeah the logic in \Drupal\Core\Routing\RequestFormatRouteFilter::filter is a bit different. When there is no format provided we move it to the end. I think you basically propose to special case HTML here, or assume no format === HTML ?

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.89 KB

Status: Needs review » Needs work

The last submitted patch, 29: 2449143-29.patch, failed testing.

dawehner’s picture

I think this was just a random failure.

Wim Leers’s picture

  1. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
    @@ -238,7 +238,7 @@ public function alterRoutes(RouteCollection $collection) {
           // Ensure that we don't override a route which is already controlled by
           // views.
    

    The comment needs to be updated.

  2. +++ b/core/modules/views/tests/src/Unit/Plugin/display/PathPluginBaseTest.php
    @@ -261,6 +261,44 @@ public function testAlterRoute() {
    +   * Tests the altering of a REST route.
    ...
    +    $display['display_plugin'] = 'page';
    

    This should be testing the rest_export display plugin?

  3. +++ b/core/modules/views/tests/src/Unit/Plugin/display/PathPluginBaseTest.php
    @@ -261,6 +261,44 @@ public function testAlterRoute() {
    +    $route = new Route('test_route', ['_controller' => 'Drupal\Tests\Core\Controller\TestController::content']);
    +    $route->setMethods(['POST']);
    

    Usually, routes don't have a method specified. Therefore this test shouldn't specify one either, otherwise we're testing an edge case instead of the common case.

I think the real problem is this code in RestExport:

        // Allow a REST Export View to be returned with an HTML-only accept
        // format. That allows browsers or other non-compliant systems to access
        // the view, as it is unlikely to have a conflicting HTML representation
        // anyway.
        $route->setRequirement('_format', implode('|', $formats + ['html']));

The reason this was added in #1969870: REST export view should default to JSON: "I want to get JSON and not have to specify the necessary ?_format=json or Accept:json because that's easier if I'm a dumb user". This does not make sense. Developers using REST surely know how to specify the format they want. So, let's omit 'html' there.

dawehner’s picture

This should be testing the rest_export display plugin?

Good question ... but well, actually the case layed out in the issue summary is a page display overriding a REST route, not the other way round.

Usually, routes don't have a method specified. Therefore this test shouldn't specify one either, otherwise we're testing an edge case instead of the common case.

The reason this was added in #1969870: REST export view should default to JSON: "I want to get JSON and not have to specify the necessary ?_format=json or Accept:json because that's easier if I'm a dumb user". This does not make sense. Developers using REST surely know how to specify the format they want. So, let's omit 'html' there.

Well, there are two problems we have here.

a) the rest display takes over the HTML route as well.
b) the views HTML route replaces the REST resource route.

Wim Leers’s picture

Right, so:

  • a) can be fixed by not listing html as a format on REST Export displays
  • b) can be fixed by explicitly listing html as a format on path displays

AFAIK we need both?

dawehner’s picture

a) can be fixed by not listing html as a format on REST Export displays

I would be in favour of doing that, but we would first have to ask webchick, whether its okay to drop that feature again. I don't feel to be in any position to disagree with other people's opinion.

b) can be fixed by explicitly listing html as a format on path displays

Well, its not just that. What happens on the /node POST issue is that both the /node POST route and the /node GET route contains the view at the end. Feel free to debug it again if you don't believe me :)
We could not also take into account the METHOD but also use the _content_type_format requirement see:
$route->addRequirements(array('_content_type_format' => implode('|', $this->serializerFormats)));

webchick’s picture

The original issue summary at #1969870: REST export view should default to JSON was just a "feature" I just accidentally blundered into, so it's possible to revert it. I would say that it is super convenient though to be able to view JSON in the browser (as opposed to having to use Postman or cURL), so I wonder if it's possible to retain that functionality, but only with adding ?_format=html add-on or similar, and having the "View page" link in Views automatically set that, as you described. Is that doable at all?

Wim Leers’s picture

so I wonder if it's possible to retain that functionality, but only with adding ?_format=html add-on or similar, and having the "View page" link in Views automatically set that, as you described. Is that doable at all?

Yes, that's what I said at the bottom of #32.

#35/@dawehner: so we now have a +1 from webchick.

dawehner’s picture

@wim
So I think we have multiple issues here, so we should split up the issue into one which includes the current patch and one for the 'html' route problem.

Wim Leers’s picture

Works for me.

dawehner’s picture

Wim Leers’s picture

Wim Leers’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

The title and the summary are inconsisten here, so it would be good to update the summary with the latest direction for the issue.

damiankloip’s picture

Version: 8.2.x-dev » 8.4.x-dev
Status: Needs work » Needs review
FileSize
4.74 KB

So I think we ideally want to do something like this?

dawehner’s picture

+1 for me

Status: Needs review » Needs work

The last submitted patch, 46: 2449143-46.patch, failed testing.

Wim Leers’s picture

  1. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
    @@ -140,20 +140,8 @@ public function initDisplay(ViewExecutable $view, array &$display, array &$optio
    +    $this->setContentType($request_content_type);
    

    This method should really be renamed to setFormat(), but that's probably out of scope.

  2. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
    @@ -327,16 +315,12 @@ public function collectRoutes(RouteCollection $collection) {
    -        $route->setRequirement('_format', implode('|', $formats + ['html']));
    +        $route->setRequirement('_format', implode('|', $formats));
    
    +++ b/core/modules/views/src/Plugin/views/display/Page.php
    @@ -84,6 +84,18 @@ public static function create(ContainerInterface $container, array $configuratio
    +    // Explicitly set HTML as the format for Page displays.
    +    $route->setRequirement('_format', 'html');
    

    YES!

    This will also still work for ?_wrapper_format=drupal_dialog et cetera, because those are wrapper formats for the html format.

    This is also exactly what I proposed in #32, in May 2016, about 8 months ago.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
9.24 KB
5.36 KB

Let's see if this improves things at all.

dawehner’s picture

This looks super good for me.

  1. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -364,14 +367,14 @@ public function testResponseFormatConfiguration() {
         // Should return a 200. Emulates a sample Firefox header.
         $this->drupalGet('test/serialize/field', array(), array('Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8'));
         $this->assertHeader('content-type', 'application/json');
    -    $this->assertResponse(200, 'A 200 response was returned when a browser accept header was requested.');
    +    $this->assertResponse(406, 'A 200 response was returned when a browser accept header was requested.');
    

    <3

  2. +++ b/core/modules/views_ui/src/ViewListBuilder.php
    @@ -255,9 +256,17 @@ protected function getDisplaysList(EntityInterface $view) {
    +            // Wrap this in a try/catch as tryng to generate links to some
    +            // routes may throw a NotAcceptableHttpException if they do not
    +            // respond to HTML, such as RESTExports.
    ...
    +            catch (NotAcceptableHttpException $e) {
    +              $rendered_path = '/' . $path;
    +            }
    

    Mh, that just shows how well the routing system is designed ;) It makes it hard to link to it.

Status: Needs review » Needs work

The last submitted patch, 50: 2449143-50.patch, failed testing.

damiankloip’s picture

Yeah, exactly, Not sure how else we can handle this without special casing the rest export display explicitly, which I don't think we want to do :/

damiankloip’s picture

Status: Needs work » Needs review
FileSize
12.91 KB
5.67 KB

Let's try this. I think this should solve most of those problems.

damiankloip’s picture

Sorry, that one is not good!

The last submitted patch, 54: 2449143-54.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 55: 2449143-55.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
13.68 KB
1.48 KB
damiankloip’s picture

A couple of notes about the test coverage removal:

  1. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -385,14 +393,8 @@ public function testResponseFormatConfiguration() {
    -    // Should return a 406.
    -    $this->drupalGetWithFormat('test/serialize/field', 'html');
    -    // We want to show the first format by default, see
    -    // \Drupal\rest\Plugin\views\style\Serializer::render.
    -    $this->assertHeader('content-type', 'application/json');
    -    $this->assertResponse(200, 'A 200 response was returned when HTML was requested.');
    

    This comment is incorrect for test assertions below. However, the actual assertions are no longer relevant. We don't expect JSOn to be returned anymore and the HTML cases are already tests for 406 responses above.

  2. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -403,12 +405,6 @@ public function testResponseFormatConfiguration() {
    -    // Should return a 200.
    -    $this->drupalGetWithFormat('test/serialize/field', 'html');
    -    // We want to show the first format by default, see
    -    // \Drupal\rest\Plugin\views\style\Serializer::render.
    -    $this->assertHeader('content-type', 'application/json');
    -    $this->assertResponse(200, 'A 200 response was returned when HTML was requested.');
    

    Same with this hunk, it's no longer relevant. As we don't support the same fallback type functionality with this patch that we do now, and all other permutations are already tested in this method I think.

Wim Leers’s picture

Curious what @dawehner has to say about this patch :)

I will look into the root cause of needing this:

+++ b/core/modules/views_ui/src/ViewListBuilder.php
@@ -255,9 +256,17 @@ protected function getDisplaysList(EntityInterface $view) {
+            // Wrap this in a try/catch as tryng to generate links to some
+            // routes may throw a NotAcceptableHttpException if they do not
+            // respond to HTML, such as RESTExports.
+            try {
+              // @todo Views should expect and store a leading /. See:
+              //   https://www.drupal.org/node/2423913
+              $rendered_path = \Drupal::l('/' . $path, Url::fromUserInput('/' . $path));
+            }
+            catch (NotAcceptableHttpException $e) {
+              $rendered_path = '/' . $path;
+            }

I already have suspicions, but I want to be certain. @damiankloip and I already talked about one possible solution: let display plugins implement an optional interface where they can provide this information themselves — the RestExport display plugin would then implement that interface, hence avoiding this exception. But I'd rather not need such an additional interface of course.

dawehner’s picture

Why this happens is quite easy. Route filters are invoked when matching a path in \Drupal\Core\Path\PathValidator::getPathAttributes.

One solution could be to skip route filters by using the route provider directly and then apply access checking on top of it.

dawehner’s picture

IMHO we should add a pointer to #2852107: Allow the PathValidator to match non-HTML/non-GET routes and call it a day.

damiankloip’s picture

So we could remove it/fix it when that issue is done?

dawehner’s picture

@damiankloip
I would say we keep the workaround what we have now, and maybe add a pointer to the other issue.

Wim Leers’s picture

Why not simply do this instead?

dawehner’s picture

Nice idea!

dawehner’s picture

+++ b/core/modules/views/src/Plugin/views/display/Page.php
@@ -85,6 +85,18 @@ public static function create(ContainerInterface $container, array $configuratio
+
+    // Explicitly set HTML as the format for Page displays.
+    $route->setRequirement('_format', 'html');
+

I just fear that this could be a BC problem with potential plugins extending this one. Yes they might do it wrong, but is this change really required?

Ky1eT’s picture

Issue tags: +FLDC17
Tybor’s picture

Confirmed not working in Drupal 8.2.5, as well as 8.4.x

I used Kalabox to recreate a Pantheon site in 8.2.5 and simplytest.me to test in 8.4.x.

Steps to recreate in Drupal 8:

0. Create a "drupal" project from https://simplytest.me. After you've chosen your project type, set the version to 8.4.x (at the bottom of the list).

Once you have your fresh site created and are on the home page:

1. Go to Structure >> Taxonomy >> Tags (List Terms) >> Add Term (Can be anything) and create a term.
2. Click to view the term and you will be taken to the view page of the term. It will be https://{{Your Site Here}}/taxonomy/term/{{Term #}}
3. Add "?_format=json" to the end as a query string and you will see a JSON- returned string with the error message: "message": "Not acceptable format: json"

Summary is not up to date, however tag has already been applied.

dawehner’s picture

I just fear that this could be a BC problem with potential plugins extending this one. Yes they might do it wrong, but is this change really required?

On the other hand. This is what you got from extending classes. You want to inherit its behaviour, if you want a different one, specify it.

Wim Leers’s picture

#71 So this means you're saying we should disregard what you said in #67?

What do you think is still necessary here?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I'd say go with it now. We fix a way bigger problem with this issue than we maybe introduce.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

It'd be great to get an issue summary update and also clarify the BC implications of making this change. It's seems younger @dawehner is more cautious than older @dawehner.

+++ b/core/modules/views_ui/src/ViewListBuilder.php
@@ -9,6 +9,7 @@
+use Symfony\Component\HttpKernel\Exception\NotAcceptableHttpException;

Unused use.

damiankloip’s picture

Also, the trouble with the approach from #65 is that we still link to the rest display path from the browser/views listing which will not work anyway. The previous approach didn't link in that case, just displayed the path. Otherwise, I totally like that idea.

EDIT: sorry I think I misread the patch. The result of fromRoute() will only return if the format matches? So would the behaviour of rest export displays not being linked still remain?

damiankloip’s picture

I tested the latest patch and we do get linked to a 406 response, if you follow it from the browser (which is kind of the point of link tags.).

Also thinking about this a little more, I think when no formats are selected we need to implicitly add all serializer formats that would be available to the route options. Otherwise, you we will will hit the same problem as we have now, that the REST route will response to HTML/anything.

damiankloip’s picture

Like this... Use the format options from the serializer style plugin if the configured format options are empty. This then honours our behaviour without accepting HTML.

This patch doesn't solve my observation from #75/#76 (first part) yet.

dawehner’s picture

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -321,10 +323,17 @@ public function collectRoutes(RouteCollection $collection) {
+
+      // Format as a string using pipes as a delimiter.
+      $route->setRequirement('_format', implode('|', $formats));

I wonder whether its worth to test the case you have no formats selected. To be honest I think this is kind of an edge case

damiankloip’s picture

Well, in the UI we tell users that no formats selected will be equivalent to all formats. So seems fair to actually do that explicitly.

I think a test for this case would be good. I think we would just need to modify the existing test coverage. I think we still have some coverage for the none selected case...

damiankloip’s picture

We only really need to add one additional test for the none selected case that HTML requested results in a 406.

damiankloip’s picture

Status: Needs work » Needs review

The last submitted patch, 77: 2449143-77.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 80: 2449143-80.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
14.63 KB

Let's start with a reroll.

damiankloip’s picture

Correctly rerolled.

The last submitted patch, 84: 2449143-84.patch, failed testing.

damiankloip’s picture

This should fix all of the test failures (Might leave one...) I think. This has uncovered a good bug in \Drupal\simpletest\WebTestBase::drupalGetWithFormat when a query string is used. As it was just using the += operator before, it was not deep merging the 'query' key, so _format was not being added to those requests.

Hopefully now this just leaves the question about how we want to handle the links in the UI. I think we have a couple of options:

1. Revert back to my original method of catching the MethodNotAllowedException and display the path (I think this is my favoured option for now and maybe a follow up to discuss anything else).
2. Add new methods to display plugins to generate UI links, maybe 'getUiLink' or something? This would have a default implementation in PathPluginBase, which RESTExport could then override to basically add a '_format' to the URL for the display. Similar to the getUrl method we already have, but not that. The only trouble with that is do we just choose the first available format if there is more than one configured?

The last submitted patch, 85: 2449143-85.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 87: 2449143-86.patch, failed testing.

Pavan B S’s picture

+++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
@@ -364,14 +367,19 @@ public function testResponseFormatConfiguration() {
+    $this->drupalGet('test/serialize/field', array(), array('Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8'));

Short Array syntax should be used.
Applying the patch, please review

Pavan B S’s picture

Status: Needs work » Needs review
damiankloip’s picture

Maybe let's concentrate on the actual problems in this issue first. That is just a nitpick that can be picked up in a final review. If you want a well earned commit credit please do a worthwhile review or something.

Surprisingly that patch failed with the same 2 fails as before.

Status: Needs review » Needs work

The last submitted patch, 90: 2449143-90.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review

OK, now let's get back to it. This should fix the remaining failures.

Now we just need to do something about my comment from #87 I think:

Hopefully now this just leaves the question about how we want to handle the links in the UI. I think we have a couple of options:

1. Revert back to my original method of catching the MethodNotAllowedException and display the path (I think this is my favoured option for now and maybe a follow up to discuss anything else).
2. Add new methods to display plugins to generate UI links, maybe 'getUiLink' or something? This would have a default implementation in PathPluginBase, which RESTExport could then override to basically add a '_format' to the URL for the display. Similar to the getUrl method we already have, but not that. The only trouble with that is do we just choose the first available format if there is more than one configured?

damiankloip’s picture

Annnd, need the patches too..

Wim Leers’s picture

+++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
@@ -407,7 +407,7 @@ public function testResponseFormatConfiguration() {
-    $this->assertHeader('content-type', 'text/html; charset=UTF-8');
+    $this->assertHeader('content-type', 'text/plain; charset=UTF-8');

This is due to #2825347: 'Accept' header still is used for determining response format of error responses, ?_format= has no effect AND serialization module's exception subscriber does not support status code 500 error responses, which landed only recently. This change makes sense :)


  1. Revert back to my original method of catching the MethodNotAllowedException and display the path (I think this is my favoured option for now and maybe a follow up to discuss anything else).

+1 — indeed a follow-up to discuss anything else seems appropriate. We should fix this major bug first, and then we can still make the solution more pluggable/more nicely architected. Because that's something that A) needs more discussion, B) is kind of out of scope for this issue.

So, let's do that, then this is RTBC again.

damiankloip’s picture

Sounds good, +1 for consensus! Here is a new patch with the try/catching again.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 97: 2449143-97.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Drupal CI had a bad moment.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 97: 2449143-97.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Drupal CI had another bad moment:

00:00:00.339 Warning: failed to get default registry endpoint from daemon (Cannot connect to the Docker daemon. Is the docker daemon running on this host?). Using system default: https://index.docker.io/v1/
00:00:00.341 Cannot connect to the Docker daemon. Is the docker daemon running on this host?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 97: 2449143-97.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Again.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 97: 2449143-97.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Again.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 97: 2449143-97.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Again.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 97: 2449143-97.patch, failed testing.

damiankloip’s picture

Status: Needs work » Reviewed & tested by the community

And back once more. Hopefully for the last time?!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 97: 2449143-97.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
16.95 KB

Rerolled - git resolved all conflicts.

Status: Needs review » Needs work

The last submitted patch, 113: 2449143-113.patch, failed testing.

dawehner’s picture

git++, just saying

Wim Leers’s picture

Fail reproduced. On it.

Wim Leers’s picture

Root cause: #2863267: Convert web tests of views switched ViewTestBase from WebTestBase to BrowserTestBase, which does not have drupalGetJSON().

Simple fix.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
damiankloip’s picture

Looks ok to me! Thanks, Wim.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 117: 2449143-117.patch, failed testing.

damiankloip’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 117: 2449143-117.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.74 KB
16.93 KB

Trivial fix. Caused by changes introduced in #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available. Specifically, that issue/patch/commit introduced \Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber::on4xx(), which causes 4xx errors other than a 403 or 404 when the requested format is html to also be handled by that exception subscriber, which means we don't fall back to \Drupal\Core\EventSubscriber\FinalExceptionSubscriber (which always sends text/plain responses) for those anymore.

You can see several text/plain to text/html changes in #2293697's patch too.

In other words: 406, 415 etc error responses can be HTML responses too now. That's all!

Wim Leers’s picture

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
    @@ -140,20 +140,13 @@ public function initDisplay(ViewExecutable $view, array &$display, array &$optio
    +    // This allows still falling back to the default for things like views
    +    // preview.
    

    We're removing some important comments from this function, and I think this replacement is insufficient. This issue is still tagged as "Needs issue summary update", so perhaps that should be done first, and then a more informative code comment could be figured out here?

  2. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
    @@ -140,20 +140,13 @@ public function initDisplay(ViewExecutable $view, array &$display, array &$optio
    -    // If the requested content type is 'html' and the default 'json' is not
    -    // selected as a format option in the view display, fallback to the first
    -    // format in the array.
    -    elseif (!empty($options['style']['options']['formats']) && !isset($options['style']['options']['formats'][$this->getContentType()])) {
    -      $this->setContentType(reset($options['style']['options']['formats']));
    -    }
    

    Are we sure it's safe to remove this? Do we have test coverage for what this says it was doing?

  3. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -364,14 +367,19 @@ public function testResponseFormatConfiguration() {
    -    $this->drupalGet('test/serialize/field');
    +    $this->drupalGetWithFormat('test/serialize/field', 'json');
         $this->assertHeader('content-type', 'application/json');
         $this->assertResponse(200, 'A 200 response was returned when any format was requested.');
    

    This change is inconsistent with the message provided in the assertResponse() call. Also, we already have a call with an explicitly passed in 'json' format a few lines down already. But I don't think we want to remove the original test of a GET request without an explicit format. Shouldn't we still test that, but change what we're expecting the result to be?

  4. +++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
    @@ -191,7 +194,7 @@ public function testSerializerResponses() {
    -    $actual_xml = $this->drupalGet('test/serialize/entity');
    +    $actual_xml = $this->drupalGetWithFormat('test/serialize/entity', 'xml');
    

    I think same here. I agree with having the explicit test, but shouldn't we also retain the test where we don't pass in an explicit format?

  5. +++ b/core/modules/views_ui/src/ViewEditForm.php
    @@ -414,15 +415,25 @@ public function getDisplayDetails($view, $display) {
    +            try {
    +              if (!parse_url($path, PHP_URL_SCHEME)) {
    +                // @todo Views should expect and store a leading /. See:
    +                //   https://www.drupal.org/node/2423913
    +                $url = Url::fromUserInput('/' . ltrim($path, '/'));
    +              }
    +              else {
    +                $url = Url::fromUri("base:$path");
    +              }
                 }
    -            else {
    -              $url = Url::fromUri("base:$path");
    +            catch (NotAcceptableHttpException $e) {
    +              $url = '/' . $path;
                 }
    

    I think this try/catch is too broad. I think it's only needed around fromUserInput(). And then the catch for that can do the same thing that the else is doing: use base:$path. Or am I missing a subtlety for why we want $url to not be a Url object?

  6. if there is a "regular" HTML view on the same path, it will serve JSON

    Do we have test coverage for this case? I don't think I see it, but maybe it's there and I missed it?

damiankloip’s picture

Status: Needs work » Needs review
FileSize
19.11 KB
4.75 KB

Some very good points.

1. Hmm, not sure about this exactly but tried to improve it a bit :)
2. I don't think we really need it but I have restored it anyway, so it can work as a mechanism for setting the default available format rather than != html OR default.
3. Removed that hunk, it is indeed the same as the block a little below. Added but isn't testing without an explicit format the same as HTML as Drupal will default to html anyway?
4. This is just testing the response formats content, but I guess no harm in adding some other assertions for that...
5. Won't both fromUserInput() and fromUri() throw these Not found exceptions? I would need to refresh my memory though. There seemed to be a good reason to put both in the try() block to initially.
6. Hmm, not sure if there is explicit coverage for that... We might need a new test case for that, with a new test view that contains a page and rest export on the same path.

Status: Needs review » Needs work

The last submitted patch, 126: 2449143-126.patch, failed testing. View results

damiankloip’s picture

Sorry, not sure what I was doing with those tests last night. They should all return 406 as html is no longer supported on the RestExport routes. Which makes the additional coverage with no explicit format passed (exactly the same as trying to pass 'html' ?) even more moot?

damiankloip’s picture

So I think this is more correct, and was actually pretty much correct beforehand. testResponseFormatConfiguration tests all the cases with configured formats and their responses, from no format, to html explicitly, to full browser accept headers. testSerializerResponses just tests that, the actual serialized responses. So now that we do not support a fallback through allowing HTML in on the routes this coverage is not needed.

dawehner’s picture

Status: Needs work » Needs review

We talked about this issue on the REST major issue triage and agreed this is major, given it blocks for example taxonomy terms to be served by default.

Given the latest failure I think we have proved that we no longer do the fallback, yeah!

damiankloip’s picture

Yeah, totally!

dawehner’s picture

@damiankloip
Any idea why the testbot doesn't pick up the patch?

Wim Leers’s picture

Let's reupload.

dawehner’s picture

Status: Needs review » Needs work

The last submitted patch, 134: 2449143-129.patch, failed testing. View results

damiankloip’s picture

Status: Needs work » Needs review
FileSize
18.22 KB
722 bytes

No idea what is going on with that other patch. The fail is just my bad, if we request no format, it will default to HTML.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

AFAICT committer feedback has been addressed.

damiankloip’s picture

@Wim Leers I think possibly the only thing outstanding is an additional test that has a page and rest export view on the same path, and both are accessible. I don't think we have that yet (A good spot from Alex!).

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

NW for #138 if that test is still missing. If it's in the patch, please set back to RTBC.

Wim Leers’s picture

Issue tags: +Needs tests

Do we want that additional test coverage in \Drupal\rest\Tests\Views\StyleSerializerTest::testSerializerResponses(), or in \Drupal\Tests\views\Functional\Wizard\BasicTest::testViewsWizardAndListing(), or both?

dawehner’s picture

I think the StyleSerializerTest is the better place.

damiankloip’s picture

Agree, adding this is StyleSerializerTest is the best option I think. I'll try and do this today.

damiankloip’s picture

Here is some test coverage to show failing in the current world, and should all pass with the patch here.

The last submitted patch, 143: 2449143-143-tests-only-FAIL.patch, failed testing. View results

Wim Leers’s picture

Status: Needs review » Needs work

That is … super clear :)

Just one remark:

+++ b/core/modules/rest/src/Tests/Views/StyleSerializerTest.php
@@ -217,6 +217,19 @@ public function testSerializerResponses() {
+    $this->drupalGetWithFormat('test/serialize/shared', 'html');

This generates a /test/serialize/shared?_format=html URL. I think it'd be better to use drupalGet() instead, which would result in a /test/serialize/shared URL, which is the actual real-world scenario.

damiankloip’s picture

Yeah, I totally agree :) I was going to change that at the time, then.. didn't. We could even just have both?

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -Needs tests
FileSize
21.72 KB
1.24 KB

How about we match most of the other test assertions and check no format, and XML, like this?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Perfect. All edge cases covered.

dawehner’s picture

More tests is always better!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 147: 2449143-147.patch, failed testing. View results

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

#147 doesn't apply anymore, to either 8.4.x or 8.5.x.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
20.93 KB
6.9 KB

Reroll

Status: Needs review » Needs work

The last submitted patch, 154: 2449143-153.patch, failed testing. View results

damiankloip’s picture

Status: Needs work » Needs review
FileSize
20.98 KB
1.25 KB

Missed a couple of method renames/conversions.

Status: Needs review » Needs work

The last submitted patch, 156: 2449143-156.patch, failed testing. View results

damiankloip’s picture

Status: Needs work » Needs review
FileSize
21.83 KB
2.47 KB