I'm not sure how/why this started happening, but when I turn on rest.module and hit http://8x.localhost:8082/entity/node/1 in my browser, I get:

<response>
<nid>
<value>1</value>
</nid>
<uuid>
<value>1d0b7f7c-18e6-4023-a700-bef36f2e00c3</value>
</uuid>
<vid>
<value>1</value>
</vid>
<type>
<value>article</value>
</type>
...

Awesome! Didn't even need to carefully craft some cURL request to send an Accept header!

However, if I hit a REST export view in my browser, I get:

Error

The website has encountered an error. Please try again later.
Error message
Symfony\Component\Serializer\Exception\UnexpectedValueException: Serialization for the format html is not supported in Symfony\Component\Serializer\Serializer->serialize() (line 78 of /Users/webchick/Sites/8.x/core/vendor/symfony/serializer/Symfony/Component/Serializer/Serializer.php).

This is decidedly un-user friendly. :)

Let's figure out how rest.module is doing its magic for entities and get it to do it for Views as well. :)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
1.75 KB

That's just a simple fix, read for more information below and how to fix it on the longrun.

The way the rest modules handles it is the following:

It registers one route per format, so for example entity/node/{node} for json, xml. As there is no special content-type requested, the acceptable mime-types are:

text/html, application/xhtml+xml, application/xml, */*

(@see MimeTypeMatcher::filter()
so basically all routes match. As far as I understand the routing system, the one who comes first then wins, which is (maybe not on purpose) xml.

Views could get such a default behavior when it would use the route system. Though there is currently a dependency tree:

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, drupal-1969870-1.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +VDC

#1: drupal-1969870-1.patch queued for re-testing.

linclark’s picture

You're right about XML not being on purpose. I think that we should probably make it default to JSON in both places, XML is serialization module's red headed stepchild.

webchick’s picture

Status: Needs review » Postponed

Ok, sounds then like this is postponed on those other issues for now, since we don't want to hard-code in red-headedness.

damiankloip’s picture

Title: REST export view should default to XML » REST export view should default to JSON

Let's leave it postponed, but I agree with lin - JSON should be the default format served.

dawehner’s picture

I don't know but I think we should get this in first, because a) the other issues still need time (*hint* #1956896: Add role based access checker is RTBC)
and also this patch adds some kind of test, which is always helpful. Beside from this, it wouldn't block the other issue, as I guess we should not fix the needed behavior of this issue in #1800998: Use route system instead of hook_menu() in Views

damiankloip’s picture

Status: Postponed » Needs review
FileSize
2.13 KB
2.03 KB

Ok, let's just try and get this in then - Should be pretty simple.

I think this approach would be better; To set the contentType property on RestExport to 'json', and only override it if it's not html. Thoughts?

dawehner’s picture

I'm wondering whether it should be json or hal_json by default.

webchick’s picture

Well, it's tricky, right? Because it should be whatever the primary serialization format is. Currently, D8 ships with both json-ld and hal+jason, but both are optional modules, separate from REST.

dawehner’s picture

Should then logic then be:

If hal_json is enabled use hal_json
If json_ld is enabled use json_ld
else use json?

moshe weitzman’s picture

yes, that logic makes sense but even simpler since jsonld is poised for removal. #1935548: Remove JSON-LD module

dawehner’s picture

FileSize
2.03 KB
2.45 KB

Added this logic without taking care about json_ld.

Status: Needs review » Needs work

The last submitted patch, drupal-1969870-13.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.08 KB
2.44 KB

Just fixing it up. I'm not really a fan of having conditional module logic in the display plugin like this. Especially as hal will only work with entities aswell?

dawehner’s picture

Great, Thank you. The code looks fine, so it's just about a comment of lin.

linclark’s picture

Damian is right, HAL will only work with entities.

I think we can simply default to JSON in this case. The only thing that HAL buys you over JSON is that it has link relations... and link relations are only important to you if you are a machine. Since this is about what gets shown in the browser, we're probably working with a human user, so the hypermedia special sauce isn't necessary.

damiankloip’s picture

Thanks Lin. I was always of the mindset that JSON should be our default format, you explain things really well though :) Defaulting to hal just doesn't make sense.

This is a mix of previous patches (logic from #8 and test from #13), reverting hal_json back to json.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

It's good to see that the logic is quite simple

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.phpundefined
@@ -78,7 +78,13 @@ public function initDisplay(ViewExecutable $view, array &$display, array &$optio
 
-    $this->setContentType($negotiation->getContentType($request));
+    $request_content_type = $negotiation->getContentType($request);
+    // Only set the requested content type if it's not 'html'. The default
+    // 'json' content will be used in this case.
+    if ($request_content_type !== 'html') {
+      $this->setContentType($request_content_type);
+    }
+
     $this->setMimeType($request->getMimeType($this->contentType));

Shouldn't this be $this->setContentType($this->contentType);

alexpott’s picture

Or maybe the comment is just confusing...

dawehner’s picture

Status: Needs work » Needs review
FileSize
906 bytes
2.78 KB

Shouldn't this be $this->setContentType($this->contentType);

I don't think it's needed to set it to json if it's already set onto json?

Maybe this small change helps to understand the comment?

alexpott’s picture

Status: Needs review » Needs work

thanks @dawehner that comment is indeed clearer.

+++ b/core/modules/rest/lib/Drupal/rest/Tests/Views/StyleSerializerTest.phpundefined
@@ -120,12 +120,16 @@ public function testSerializerResponses() {
+    // Test that the default output will be JSON if html is requested.
+    $expected = $serializer->serialize($entities, 'json');
+    $actual = $this->drupalGet('test/serialize/entity');
+    $this->assertIdentical($actual, $expected, 'If hal module is enabled by default hal_json output is returned.');

I don't think the hal module has anything to do with this.

alexpott’s picture

EDIT: duplicate comment

dawehner’s picture

Status: Needs work » Needs review
FileSize
836 bytes
2.78 KB

Good catch.

dawehner’s picture

FileSize
810 bytes
2.75 KB

As always alex spotted that there is no json module, let's get rid of the module comment.

damiankloip’s picture

Looks good to me. Alex?

alexpott’s picture

+1

damiankloip’s picture

If Alex is happy with the new comment I think we're good to go again.

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

Oh

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Ok I've now understood why this patch confuses me...

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/views/display/RestExport.phpundefined
@@ -78,7 +78,13 @@ public function initDisplay(ViewExecutable $view, array &$display, array &$optio
+    $request_content_type = $negotiation->getContentType($request);
+    // Only determine the requested content type if it's not 'html'. The default
+    // 'json' content will be used else.
+    if ($request_content_type !== 'html') {
+      $this->setContentType($request_content_type);
+    }

To me this should be...

    $request_content_type = $negotiation->getContentType($request);
    // Only use the requested content type if it's not 'html'. If it is then default
    // to 'json' to aid debugging.
    if ($request_content_type !== 'html') {
      $this->setContentType($request_content_type);
    }
dawehner’s picture

Status: Needs work » Needs review
FileSize
902 bytes
2.75 KB
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Simple enough.

effulgentsia’s picture

Doesn't this violate HTTP spec? If the agent asked for html, why should we return json?

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed fc886f5 and pushed to 8.x. Thanks!

xjm’s picture

#34 to #35 was a crosspost; do we want to reopen the discussion?

effulgentsia’s picture

Re #34, I don't object that this was committed, given that in practice, browsers include */* in their Accept header, but we don't yet have #1505080: [META] Content Negotiation for Accept Headers in place in order to handle that correctly. Perhaps after that issue though, we can revisit the implementation of what was committed here in order to not return json for a hypothetical agent that requested html (and nothing but html).

effulgentsia’s picture

Since this issue is marked fixed and will therefore, fall off our radar, I also left a comment in #1505080-40: [META] Content Negotiation for Accept Headers and raised the priority of that issue.

damiankloip’s picture

@effulgentsia: so even if/when we do have this content negotiation, what do you propose to server when html is requested? serializer doesn't support html, or do you want to add an encoder for that? Throwing an error etc.. didn't fly with people before, so not sure.

effulgentsia’s picture

Once both content negotiation and #1800998: Use route system instead of hook_menu() in Views are in, then when dynamically creating the route for a View that has only a REST export display, we'll need to do a $route->setRequirement('_format', $supported_serialization_formats);. If an agent asks for the URL without anything in the Accept header that matches a supported format, then the routing system will return a HTTP 415 (Unsupported Media Type) status code.

Throwing an error etc.. didn't fly with people before

That's because 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.

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

Wim Leers’s picture

We're dealing with the fallout/consequences of this over at #2449143: 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: this breaks as soon as you have a non-REST view and a REST view on the same path: then the routing system will pick the REST view over the non-REST view, resulting in JSON/XML/… instead of HTML.

Wim Leers’s picture

Component: views.module » rest.module

This patch only touched the REST module.