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

Try application/hal+json

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

Still getting

{
message: "Not acceptable"
}

for

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

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

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

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.

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.

Can be fixed in 8.0.x (hopefully)

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

#13: that very much reminds me of the routing problems uncovered by/described in #2659070: REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response.

Should we move this to routing system?

#2658926: Entity resource urls shouldn't be the same as entity view urls was a duplicate of this. It was marked major,.

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

Per #18.

I think this is a clearer title.

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

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.

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?

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.

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.

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 ?

I think this was just a random failure.

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.

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.

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?

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)));

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?

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.

This also fixes the test fail in #2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available's patch (which is otherwise ready), so this blocks that issue.

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

Works for me.

Added #2730497: REST Views override existing REST routes for the other problem

Hurray, #2730497: REST Views override existing REST routes landed!

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

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

+1 for me

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.

Let's see if this improves things at all.

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.

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 :/

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

Sorry, that one is not good!

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.

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.

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.

IMHO we should add a pointer to #2852107: PathValidator::getUrlIfValid() does not support non-HTML/non-GET routes and call it a day.

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

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

Why not simply do this instead?

Nice idea!

+++ 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?

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.

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.

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

What do you think is still necessary here?

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

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.

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?

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.

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.

+++ 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

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

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

Let's start with a reroll.

Correctly rerolled.

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?

+++ 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

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.

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?

Annnd, need the patches too..

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

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

Drupal CI had a bad moment.

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?

Again.

Again.

Again.

This also blocks #2353611: Make it possible to link to an entity by UUID.

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

Rerolled - git resolved all conflicts.

git++, just saying

Fail reproduced. On it.

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

Simple fix.

Looks ok to me! Thanks, Wim.

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!

#2293697: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available is in, so this is no longer blocking that issue.

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?

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.

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?

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.

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!

Yeah, totally!

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

Let's reupload.

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.

AFAICT committer feedback has been addressed.

@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!).

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

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?

I think the StyleSerializerTest is the better place.

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

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

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.

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

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

Perfect. All edge cases covered.

More tests is always better!

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

Reroll

Missed a couple of method renames/conversions.

1. +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
   @@ -327,17 +328,20 @@ public function collectRoutes(RouteCollection $collection) {
   +        $formats = $style_plugin->getFormatOptions();
   
   +++ b/core/modules/rest/src/Plugin/views/style/Serializer.php
   @@ -197,7 +197,7 @@ public function calculateDependencies() {
   +  public function getFormatOptions() {
   

   Do we need a new interface for this? Now that we're expecting the style plugin to implement this method?

2. +++ b/core/modules/rest/tests/src/Functional/Views/StyleSerializerTest.php
   @@ -207,6 +210,28 @@ public function testSerializerResponses() {
   +    $this->assertHeader('content-type', 'text/html; charset=UTF-8');
   ...
   +    $this->assertHeader('content-type', 'text/html; charset=UTF-8');
   ...
   +    $this->assertHeader('content-type', 'application/json');
   ...
   +    $this->assertHeader('content-type', 'text/xml; charset=UTF-8');
   

   Should we also be asserting that we're getting the expected output in the body here? Don't our error handlers still set this header if something goes wrong?

Do we need a new interface for this? Now that we're expecting the style plugin to implement this method?

Those two plugins are coupled to each other. Maybe we could do an instanceof check or method_exists check and otherwise throw an exception?

Don't our error handlers still set this header if something goes wrong?

We're asserting that the responses have a 200 code.

Should we also be asserting that we're getting the expected output in the body here?

This patch is adding test coverage that fails in HEAD. Response bodies are already being asserted in \Drupal\Tests\rest\Functional\Views\StyleSerializerTest::testSerializerResponses(). We're adding test coverage for the thing that was broken.

Do we need a new interface for this? Now that we're expecting the style plugin to implement this method?

As @dawehner these classes are already coupled together. RestExport is already relying on the public function getFormats in \Drupal\rest\Plugin\views\style\Serializer that is not in an interface.

Maybe we could do an instanceof check or method_exists check and otherwise throw an exception?

This patch adds the method_exists calls for both getFormats and getFormatOptions since these are the 2 methods that are required of $style_plugin in \Drupal\rest\Plugin\views\display\RestExport::collectRoutes()
It doesn't make sense to just check for getFormatOptions since it would be confusing later on and imply that 1 method was part an interface and the other wasn't.

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -328,6 +328,10 @@ public function collectRoutes(RouteCollection $collection) {
+      if (!method_exists($style_plugin, 'getFormats') || !method_exists($style_plugin, 'getFormatOptions')) {
+        $class_name = get_class($style_plugin);
+        throw new \Exception("Views style plugins used with REST exports must provide methods getFormats and getFormatOptions. Plugin class {$class_name} doesn't provide these methods.");
+      }

This looks great, but wouldn't an instanceof check be cleaner? Is there any reason not to define an interface with these two methods?

At least for me an interface communicates extensibility. But the question is, do we actually want extensibility here?

I think the key feature of an interface is that it communicates expectations. For example, \Drupal\Core\TypedData\TranslatableInterface. And to use a style plugin within a REST export display, we're placing expectations on that plugin, so we should communicate those expectations in an interface.

Could we mark the interface as internal at least?

Yes, it's true they are pretty tied together. They would probably be one class if that's what views architecture permitted. I guess an interface is ok to add, it probably wont have much use in reality :) Marking it as internal could be a good idea too.

I guess it might be feasible someone would want their own variant of the serializer style plugin.... maybe...

I am conflicting on what to do. I chatted with @damiankloip about this.
Basically

11:14 tedbow: damiankloip: ok. I think so. I not really familiar with how this works. but I think about just update patch to check for `instanceof \Drupal\rest\Plugin\views\style\Serializer` this issue is not about making sure others can you use other serializer which was already a problem because we were calling getFormats()
11:14 damiankloip: tedbow exactly
11:15 damiankloip: and views plugins in general

So I was going to update to

if (!$style_plugin instanceof Serializer) {
        $class_name = get_class($style_plugin);
        throw new \Exception("Views style plugins used with REST exports must use be an instance of \\Drupal\\rest\\Plugin\\views\\style\\Serializer, {$class_name} found.");
      }

But that has the possibility of breaking any Style plugin that does not extend \Drupal\rest\Plugin\views\style\Serializer but works right now. Is that an edge case? Right now the developer would already have to figure out that Style plugin would have to have a getFormats() method for it to work. So either way whatever we are going to do break existing plugins that work with RestExport but don't extend \Drupal\rest\Plugin\views\style\Serializer right now because they won't have public getFormatOptions method.

So I could see having an new interface as @effulgentsia suggested. But then if we make it internal as @dawehner and @damiankloip suggested what is the point? I see not wanting to have a new public API but for Style plugins that we break they have to figure out they need to provide a public getFormatOptions. If we make the interface internal we would be basically making an interface that makes a style plugin work with RestExport but then telling contrib not to use it.

Looking at \Drupal\views\Plugin\views\style\StylePluginBase there is not interface here but the phpdoc says

 * Style plugins extend \Drupal\views\Plugin\views\style\StylePluginBase. They
 * must be annotated with \Drupal\views\Annotation\ViewsStyle
 * annotation, and they must be in namespace directory Plugin\views\style.

I could be wrong but looking at ViewsPluginManager though this doesn't seem to be enforced. Would it make sense to just follow this pattern and update the class phpdoc for RestExport say

* Style plugins that work with this class must  extend \Drupal\rest\Plugin\views\style\Serializer.

then although we have the edge of case of breaking a style plugin with this update in the future would could actually add public functions to Serializer without breaking other RestExport serializer Style plugins.
then would actually use

if (!$style_plugin instanceof Serializer) {
        $class_name = get_class($style_plugin);
        throw new \Exception("Views style plugins used with REST exports must use be an instance of \\Drupal\\rest\\Plugin\\views\\style\\Serializer, {$class_name} found.");
      }

What is the actual coupling between RestExport and Serializer? Looks to me like it's just that Serializer is the only style plugin that has an annotation of display_types = {"data"}? But why couldn't there be a contrib style plugin that also has that annotation?

The other coupling seems indeed to be the getFormats() and getFormatOptions() methods. Seems to me like those would perhaps make more sense on RestExport than on Serializer? If so, then I think moving getFormats() might be too much scope for this issue, but what about just moving getFormatOptions()? Or, if we think that both methods more logically belong to the "style" than to the "display", then I do think we need an interface for them. Perhaps named ConfigurableFormatInterface or DataFormatInterface or StylePluginForDataDisplayInterface or DataStylePluginInterface, or ...?

I think it would be fine to start with it being @internal, but I think eventually we'll want to better flesh out the pattern of how to define the interfaces that are expected of Views plugins that are specific to particular display_types.

I also wonder if this is a more generic problem. I.e., of where display plugins have an implicit requirement for a style plugin to have a particular "option". In which case, I wonder if a more generic API on style plugins would be appropriate: e.g., ->definesOption('format'), ->getOption('format'), ->getAllPossibleOptions('format')? But that might take time to get right, and we shouldn't hold up this issue on it.

@effulgentsia yes there is not coupling except the annotation and contrib style plugin could set this too.

If so, then I think moving getFormats() might be too much scope for this issue, but what about just moving getFormatOptions()?

I think moving getFormatOptions()to RestExportmakes sense because then we won't have the possibility of breaking existing style plugins like I describe in #169.

This patch does that. Then we don't have to do the check for method_exists('getFormatOptions') or worry about adding an interface. There is still the problem that a style plugin could be set to work RestExport and not have public method getFormats() but predates this issue.

I'm totally cool with the direction we went with. People are super scared of code duplication, because someone told them they should be. In this case though its a minimal amount of code. We could have a simple trait in case anyone cares.

I am ok with this direction too. But just to clarify to others, the coupling is not just the methods, but the response from the serializer plugin that is expected in the REST export display too. As discussed with ted, people could totally implement their own plugin, or extend the current Serializer one. It would still be easy either way here.

The only thing is this is now pretty awkward as we have the available list of options coming from the display plugin but the configuration for that stored on the style plugin config. That seems pretty questionable....

Sorry #172 was not exactly what the patch did because I didn't commit all my changes.

I didn't actually move getFormatOptions() from Serializer to RestExport but copy it there. So @dawehner's comment

I'm totally cool with the direction we went with. People are super scared of code duplication, because someone told them they should be.

As @dawehner says if we want to remove the duplication we can make SerializerFormatOptionsTrait or something.

So re @damiankloip's comment here

The only thing is this is now pretty awkward as we have the available list of options coming from the display plugin but the configuration for that stored on the style plugin config. That seems pretty questionable....

List of options is actually still coming from Serializer because right now the getFormatOptions() is duplicated.

This patch fixes the failing test and removes an unused use statement.

@effulgentsia++ for poking holes in this patch and therefore Views' architecture (hitherto hidden coupling) — there are indeed theoretical BC breaks. An understandable oversight made in the past. I'm glad we're uncovering problems now, and not after it's committed and rolled out to actual sites. Unfortunate we didn't notice this BC problem sooner!

I think @damiankloip summarized it well:

But just to clarify to others, the coupling is not just the methods, but the response from the serializer plugin that is expected in the REST export display too. As discussed with ted, people could totally implement their own plugin, or extend the current Serializer one.

Can't wait for @dawehner & @damiankloip to review this and hopefully get it back to RTBC :)

Yeah to not just blindly introduce interfaces ...

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -327,17 +338,21 @@ public function collectRoutes(RouteCollection $collection) {
+      // If there are no configured formats, add all formats that the serializer
+      // style plugin supports.

nit: This comment doesn't need the reference the style plugin now, I don't think?

Just amended the patch comment, this is a total +1 from me now. Leaving as RTBC.

Adding credit for @effulgentsia and myself as our reviews changed direction of patch

Committed as 4997192 and pushed to 8.5.x.

🎉

This means REST export views no longer break the HTML view if they're on the same path — I think that's worthy of the release notes…

Note: This seemed to have caused issues with all code which relied on not having to specify the format explicitely.
#2916212: Fix rest tests. is one of them.

Should we roll back?

No. That test was simply relying on the buggy behavior. This was committed to 8.5.x only, which means contrib has 6 months to fix their tests. That seems reasonable, doesn’t it?

@Wim Leers
Well, the question is, can we do something in some automated update path? Could we automatically select 'json' as one of the format, given it was the fallback previously as well? I guess we can't given that's the entire point of the issue.

Exactly.

We only use the release notes tag for thing that the site owner needs to know when updating. It's a good bug to have fixed, but I also don't think it has a place on the level of the frontpage post, so not tagging it for the highlights either.

Per #185 and #2937942: REST views: regression in 8.5.x: view with display using 'csv' format now returns a 406 response, need to add ?_format=csv to URL to make it work, this may need a mention in the release notes after all.

Which makes me wonder… shouldn't we create a change record for this?

Yes please to change notice

Now that Rest Views no longer specify HTML as a possible request format, getUrlIfValid('/path/to/rest') throws an error. See #2952432: [PP-1] Since 8.5.0, getUrlIfValid('/path/to/rest') will throw an exception if there is no route found for html; how to get a valid Url for a Json path?

Looks like we forgot to create that change record.

Created one: https://www.drupal.org/node/2954953.

I just ran into this issue on Drupal 8.5. I don't know if this is the right place for this comment, but in the view display, when it's REST export, it would be helpful to have the ?_format=csv appende to the PATH SETTINGS Path: field.

I think the change in Page.php causes another regresssion: #2969297: It is not longer possible to override /taxonomy/{}/view with a more specific path without brekaing /taxonomy/{}/edit