Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi’s picture

Status: Active » Needs review
FileSize
10.25 KB

First ugly patch attached, based on #1816354: Add a REST module, starting with DELETE. The format application/vnd.drupal.ld+json (drupal_jsonld) is hard coded right now. Also includes a test case for entity_test, the other entity typed do not support EntityNG with properties right now.

Crell’s picture

+++ b/lib/Drupal/rest/RequestHandler.php
@@ -39,7 +39,17 @@ class RequestHandler extends ContainerAware {
+        // Otherwise we serialize the data.
+        $serializer = drupal_container()->get('serializer');

This class is ContainerAware, so it should just call $this->container->get().

ygerasimov’s picture

I have altered patch according to #2.

Also:
- changed format 'application/vnd.drupal.ld+json' to 'application/ld+json' as application/vnd.drupal.ld+json is going to be used for content staging (according to help message in jsonld module)
- changed dblog resource to use serializer and not returing Response object. I found out that our normalizer does not support non EntityNG objects so now they are converted to array before serializing
- added dependency to jsonld module
- added verbose message for test rest calls

Shall we create normalizer that will support all kind of objects or keep this converting to array for now?

Crell’s picture

Any entity should eventually become Entity NG, and therefore be supported that way. Anything else, eh, can make its own integration.

ygerasimov’s picture

I haven't explained quite right. For dblog resource we return record of the database (object of stdClass). This is where our normalizer fails. And for such cases I propose to convert object to array.

Grayside’s picture

Will #1833440: Implement partial matcher based on content negotiation MIME type remove the dependency on the jsonld module?

And given that issue and #1803586: Give all entities their own URI, why does the REST module handle GET at all? Isn't the Router essentially Drupal's GET support?

Anonymous’s picture

- changed format 'application/vnd.drupal.ld+json' to 'application/ld+json' as application/vnd.drupal.ld+json is going to be used for content staging (according to help message in jsonld module)

Better to use 'application/vnd.drupal.ld+json' for the moment, as that's likely to stabilize more quickly.

- changed dblog resource to use serializer and not returing Response object. I found out that our normalizer does not support non EntityNG objects so now they are converted to array before serializing

There is an example of a Normalizer which just converts an object to an array in #1819760: Add a REST export display plugin and serializer integration.. All object conversion in the serialization process should be handled by a Normalizer.

However, we probably don't want to return JSON-LD for non-entities. While any valid JSON is technically valid JSON-LD, a JSON-LD parser can't do anything with it if it doesn't have a @context, and the @context is dependent on the Typed Data API.

And given that issue and #1803586: Give all entities their own URI, why does the REST module handle GET at all? Isn't the Router essentially Drupal's GET support?

The Router allows modules to register routes. However, something needs to register the route and provide the controller. That's where REST module would come in.

Other modules might also register routes for the same URI. However, they would probably be registering a route for the _format "text/html", not for the other available mime types.

klausi’s picture

Issue summary: View changes

Updated working repo URL

Grayside’s picture

Other modules might also register routes for the same URI. However, they would probably be registering a route for the _format "text/html", not for the other available mime types.

Okay, I understand that each route will be bound to a particular representation format. I'm still unclear on what the REST module is providing for GET that shouldn't be part of how the core Routing system works. Is it automatic glue between routes and every serialization format known in the system?

Crell’s picture

Is it automatic glue between routes and every serialization format known in the system?

That's really all it's offering for any HTTP method. The routing system handles all of the mapping of method/path/format to a controller; The rest module is a convenience system to make wiring stuff up to that mechanism easier.

Berdir’s picture

Component: base system » rest.module

Not sure what to do with the issue title but rest.module has a component now, no longer necessary to use the title for that :)

Crell’s picture

Title: REST module: GET/read » Support non-HTML GET requests
webchick’s picture

Priority: Normal » Major

This seems like a pretty important feature to having web services in core. Escalating to a "major" feature request.

klausi’s picture

Updated patch attached.

This introduces settings per request type for the purpose of serialization and returning the correct response codes. Goal: a resource plugin should be able to easily override the input/output semantics of a request method. Example: per default we do not return a response body on POST requests, but a plugin might want to return the serialized representation after creating a resource.

I came up with the following default mapping:

GET
  - success response: 200
  - incoming data: FALSE
  - outgoing data: TRUE
POST
  - success response: 201
  - incoming data: TRUE
  - outgoing data: FALSE
PUT
  - success response: 200
  - incoming data: TRUE
  - outgoing data: FALSE
DELETE
  - success response: 204
  - incoming data: FALSE
  - outgoing data: FALSE

This is of course not final, just my current suggestion.

It might be cool to have that settings as annotations on the methods of the plugin, but that is just an implementation detail right now. An idea for the time after feature freeze.

My current vision is to keep request/response specific logic out of the plugin method implementation. Example: the get() method should only return the loaded object. It should not serialize the object, it should not set any response HTTP headers. Example: the post() method should receive an already de-serialized object, save it and return it. Problem: the response to the POST request should contain a Location header with the URI to the newly created object. Who will set that header? The RequestHandler does not differentiate between the request methods and does not know where to set it and where not. The plugin does not create response objects, so it would have to also return the headers. Anyway, I digress, this is not a problem for this issue but later for #1839346: REST module: POST/create.

klausi’s picture

Oops, forgot to add the simpletest file to the patch. Interdiff is still against #3.

Grayside’s picture

Responses seem good. I rather like having the new/updated representation echo'd back in the response, but since that is a performance overhead can be done without. It would be nice if, say, Contrib could extend to add a debug mode that provided that behavior.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.php
@@ -83,22 +90,73 @@ public function routes() {
+      return $this->reuqestMethods[$method];
...
+    return $this->reuqestMethods;

Typo.

webchick’s picture

Issue tags: +Needs tests

Oops. That means we need test coverage for that too. :)

klausi’s picture

@webchick: simpletests are already included in the ReadTest class. Or do you mean something specific?

Fixed the variable name as reported in #15.

This patch now bases on #1801570: DX: Replace hook_route_info() with YAML files and RouteBuildEvent. Please review rest-review-1834288-17.txt.

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

The last submitted patch, rest-1834288-17.patch, failed testing.

klausi’s picture

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

#17: rest-1834288-17.patch queued for re-testing.

webchick’s picture

I mean that we should not have a green patch with a typo like in #15, so we're obviously missing some test coverage for that branch.

klausi’s picture

In this case I applied the typo consistently everywhere, so there was no functional error. Thank you netbeans for auto completing my mistake everywhere :-)

Crell’s picture

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.php
@@ -83,22 +90,73 @@ public function routes() {
+    if ($this->requestMethods == NULL) {
+      $this->requestMethods = array(
+        'HEAD' => array(
+          'success_code' => 200,
+          'incoming_data' => FALSE,
+          'outgoing_data' => FALSE,
+        ),
+        'GET' => array(

This whole block can be put straight into the object property. No need to lazy-init here.

That said, I don't actually understand the purpose of this array. It feels very kludgy. Why is this needed, exactly?

+++ b/core/modules/rest/lib/Drupal/rest/RequestHandler.php
@@ -33,17 +34,41 @@ class RequestHandler extends ContainerAware {
+      // If the plugin returns response objects itself we pass them through.
+      if ($data instanceof Response) {
+        return $data;
+      }
+      // If there is no response body data to return we can send a response
+      // immediately.
+      if (empty($settings['outgoing_data'])) {
+        return new Response('', $settings['success_code']);
+      }
+      // Otherwise we serialize the data.
+      $serializer = $this->container->get('serializer');
+      // Convert object to array as normalizer does not support
+      // non EntityNG objects.
+      if (is_object($data) && !$data instanceof EntityNG) {
+        $data = (array) $data;
+      }
+      // @todo Replace the format here with something we get from the HTTP
+      //   Accept headers. See http://drupal.org/node/1833440
+      $output = $serializer->serialize($data, 'drupal_jsonld');
+      return new Response($output, $settings['success_code'], array('Content-Type' => 'application/ld+json'));

This seems like it's reimplementing the kernel.view listener, which already fires when a controller returns something other than a Response object.

Crell’s picture

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.php
@@ -83,22 +90,73 @@ public function routes() {
+    if ($this->requestMethods == NULL) {
+      $this->requestMethods = array(
+        'HEAD' => array(
+          'success_code' => 200,
+          'incoming_data' => FALSE,
+          'outgoing_data' => FALSE,
+        ),
+        'GET' => array(

This whole block can be put straight into the object property. No need to lazy-init here.

That said, I don't actually understand the purpose of this array. It feels very kludgy. Why is this needed, exactly?

+++ b/core/modules/rest/lib/Drupal/rest/RequestHandler.php
@@ -33,17 +34,41 @@ class RequestHandler extends ContainerAware {
+      // If the plugin returns response objects itself we pass them through.
+      if ($data instanceof Response) {
+        return $data;
+      }
+      // If there is no response body data to return we can send a response
+      // immediately.
+      if (empty($settings['outgoing_data'])) {
+        return new Response('', $settings['success_code']);
+      }
+      // Otherwise we serialize the data.
+      $serializer = $this->container->get('serializer');
+      // Convert object to array as normalizer does not support
+      // non EntityNG objects.
+      if (is_object($data) && !$data instanceof EntityNG) {
+        $data = (array) $data;
+      }
+      // @todo Replace the format here with something we get from the HTTP
+      //   Accept headers. See http://drupal.org/node/1833440
+      $output = $serializer->serialize($data, 'drupal_jsonld');
+      return new Response($output, $settings['success_code'], array('Content-Type' => 'application/ld+json'));

This seems like it's reimplementing the kernel.view listener, which already fires when a controller returns something other than a Response object.

Anonymous’s picture

Status: Needs review » Needs work
+++ b/core/modules/rest/lib/Drupal/rest/RequestHandler.phpundefined
@@ -33,17 +34,41 @@ class RequestHandler extends ContainerAware {
+      // Convert object to array as normalizer does not support
+      // non EntityNG objects.
+      if (is_object($data) && !$data instanceof EntityNG) {
+        $data = (array) $data;

As I mentioned in my last comment, I feel pretty strongly that we should handle even simple object to array conversion with a Normalizer rather than doing manipulation on the data before it hits Serializer. Otherwise it sets a bad example for contrib, IMHO.

13 days to next Drupal core point release.

klausi’s picture

Status: Needs work » Needs review
FileSize
12.31 KB
18.97 KB

Better approach: instead of the complex requestMethod() the plugins can return response objects. That has the advantage that they can set all sort of stuff like status code, location header etc. themselves. I created a ResourceResponse as subclass of Symfony's Response class. It has one additional variable that holds a data structure that should be serialized later. I do not want to misuse the content variable of the original Response class for that.

So now all methods like get(), post(), delete() etc. on the resource plugins have to return ResourceResponse objects. If the method provides outgoing data the request handler will serialize it. So we keep the separation of serialization from resource plugins.

Commit log:
a1fdb56 Fixed @return docs about the new ResourceResponse object.
dd074bf Changed empty response body to NULL for consistency.
c36a239 removed unused use statement, explained dblog response object.
31eb0de Removed unused variable, removed object => array conversion from request handler.
9889b0f Removed unecessary db_select() call from dblog resource. Hello Crell!
ce44b0c Added a custom resource response class that can hold data for serialization. Reverted the complex requestMethod settings in reso
9a84066 Merge branch '8.x' into GET-read-1834288

fubhy’s picture

Status: Needs review » Reviewed & tested by the community

Okay this looks like a good temporary solution. However we should consider moving the serialization out of the responsibility of the respond object at all if you ask me. Instead we should consider serializing through a even subscriber for the route to either un-serialize the incoming data or serialize the outgoing data before printing it. We have to keep in mind that this is not specific to the REST module but will be used elsewhere. I have been playing around with that idea in my head for some time now but always forgot to set up an issue for that.

I quickly set up a placeholder issue for this feature request over here: #1842744: Automatically unserialize incomming request data / serialize outgoing response data and will post a more detailed proposal shortly.

As for this patch: I think it's RTBC. We will have to re-factor the hard-coded bits later.

moshe weitzman’s picture

Title: Support non-HTML GET requests » RESTfully request an entity with JSON-LD serialized response
Status: Reviewed & tested by the community » Needs review

This looks solid to me. This is a huge step for web services in core. Nicely done, folks.

+++ b/core/modules/rest/lib/Drupal/rest/Tests/RESTTestBase.phpundefined
@@ -54,13 +56,73 @@ protected function httpRequest($url, $method, $body = NULL, $format = 'applicati
+    $this->verbose($method . ' request to: ' . $url .
+      '<hr />Code: ' . curl_getinfo($this->curlHandle, CURLINFO_HTTP_CODE) .
+      '<hr />Response: ' . $response);
+

looks like debug code?

klausi’s picture

Status: Needs review » Reviewed & tested by the community

This is intended "debug" code same as in WebTestBase::drupalGet() for example. It helps to quickly identify errors in HTTP responses when developing simpletests. You can turn off these debug messages with the "Provide verbose information when running tests" setting.

Back to RTBC I guess, please set the status accordingly if you disagree.

moshe weitzman’s picture

Thanks for the explanation. Yes, RTBC.

ygerasimov’s picture

I am happy with the patch. Thanks guys!

Grayside’s picture

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/DBLogResource.php
@@ -7,17 +7,19 @@
+use Drupal\Core\Annotation\Translation;
+use Drupal\rest\Plugin\ResourceBase;
+use Drupal\rest\ResourceResponse;

+++ b/core/modules/rest/lib/Drupal/rest/RequestHandler.php
@@ -38,12 +38,26 @@ public function handle($plugin, Request $request, $id = NULL) {
+        $response->headers->set('Content-Type', 'application/ld+json');

@linclark in #7 suggested application/vnd.drupal.ld+json as a first step.

klausi’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
8.75 KB
21.84 KB

@Grayside: right.

Commit log:
Changed mime type to vnd.drupal.ld+json.
Added assertHeader() method for testing HTTP response headers.

Please review.

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

The last submitted patch, rest-1834288-32.patch, failed testing.

klausi’s picture

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

#32: rest-1834288-32.patch queued for re-testing.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

This looks good as well. Committed to 8.x. Thanks.

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

Anonymous’s picture

Issue summary: View changes

updated branch name