REST module: POST/create

This seems pretty important to having web services in core.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.php
@@ -54,24 +54,41 @@ public function permissions() {
+        // Special case for POST and resource creation: Add a route that does
+        // not require an ID.

This is worded awkwardly. "Special case for resource creation via POST" would make more sense, I think.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.php
@@ -54,24 +54,41 @@ public function permissions() {
+            // @todo Once http://drupal.org/node/1793520 is committed we will have
+            // route object avaialble in the controller so 'plugin' property
+            // should be changed to '_plugin'.

Typo. "available".

That said, I don't see why naming this plugin vs. _plugin is in any way related to the routing issue... It's an internal value with magic meaning not intended for use in the signature of the controller callable. So it gets an _ prefix.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/DBLogResource.php
@@ -38,23 +40,24 @@ public function routes() {
+      $result = db_query("SELECT * FROM {watchdog} WHERE wid = :wid", array(':wid' => $id))
+        ->fetchObject();

Pedantic nitpick. $result is usually used only when we are getting back an interatable result object. In this case, we're getting back a single record. So, this should be called $record.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/DBLogResource.php
@@ -38,23 +40,24 @@ public function routes() {
+        // Serialization is done here, so we indicate with NULL that there is no
+        // subsequent serialization necessary.
+        $response = new ResourceResponse(NULL, 200, array('Content-Type' => 'application/json'));

I am not comfortable with this sort of flagging. "Pass NULL to mean this thing or pass something else to mean this other thing" is a code smell.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/EntityResource.php
@@ -18,21 +20,66 @@
+      // Create responses have an empty body.
+      return new ResourceResponse(NULL, 201, array('Location' => $url));

This reads oddly. I think you mean "201 Created responses have an empty body." (Otherwise it sounds like "Create" is a verb, not a noun.)

+++ b/core/modules/rest/lib/Drupal/rest/RequestHandler.php
@@ -38,12 +38,32 @@ public function handle($plugin, Request $request, $id = NULL) {
+        // @todo what do we pass here as "type" to the serializer?
+        // De-serialization does not work yet, see http://drupal.org/node/1838596
+        // $received = $serializer->deserialize($received, $plugin, 'drupal_jsonld');
+        // @todo Remove hard-coded test entity here.
+        $received = entity_create('entity_test', array('name' => 'test', 'user_id' => 1));

I think resolving this to not hard code entity_test is an RTBC blocker.

+++ b/core/modules/rest/lib/Drupal/rest/ResourceResponse.php
@@ -0,0 +1,48 @@
+  /**
+   * Returns response data that should be serialized.
+   *
+   * @return mixed
+   *   Response data that should be serialized.
+   */
+  public function getResponseData() {
+    return $this->responseData;
+  }

Doesn't the Response class have a getContent() method that works just as well? I'm confused why this is necessary.

+++ b/core/modules/rest/lib/Drupal/rest/Tests/ReadTest.php
@@ -0,0 +1,83 @@
+  /**
+   * Tests several valid and invalid read requests on all entity types.
+   */
+  public function testRead() {

False. :-) Test method should test one thing each, only. If there's common setup that needs to be done for them, that's what the setUp() method is for.

Looked at the small recent interdiffs and decided that this one is RTBC too. Hopefully we can reroll this quickly its dependency #1838596: Add deserialize for JSON-LD goes in.

Reroll after #1838596: Add deserialize for JSON-LD. I had to fix one conflict in ResourceBase.php that happened (I think) when we renamed to _plugin (with underscore). Hope I got it right.

-      $this->assertHeader('Content-Type', 'application/vnd.drupal.ld+json');
+      $this->assertHeader('content-type', 'application/vnd.drupal.ld+json');

This seems wrong, or at least weird.

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/Derivative/EntityDerivative.php
@@ -43,6 +43,7 @@ public function getDerivativeDefinitions(array $base_plugin_definition) {
+          'serialization_class' => $entity_info['class'],
+++ b/core/modules/rest/lib/Drupal/rest/RequestHandler.php
@@ -34,19 +34,28 @@ public function handle(Request $request, $id = NULL) {
+      $class = $definition['serialization_class'];
+      $unserialized = $serializer->deserialize($received, $class, 'drupal_jsonld');

As I mentioned in #1838676-27: Support dynamic entity type management, I think it would be better to pass the entity type for the 2nd parameter to deserialize() rather than the entity class. According to linclark in #1838676-30: Support dynamic entity type management, this parameter is required to be a class name, but I disagree: in the Symfony update that went in in #1834594-24: Update dependencies (Symfony and Twig) follow up fixes for Composer, the parameter got renamed to $type (in the interface, but not in some of Symfony's implementations), and regardless of its name, it just gets passed to the denormalizer, so it's up to the denormalizer to decide what meaning to give it. But, I'm ok with this patch going in as-is, and me taking up this issue in #1852812: Use cache to get entity type/bundle metadata from JSON-LD @type URI where some discussion of it is already under way.

oops. didn't mean to change status.

Regarding lowercasing in #21, RestTestbase::httpRequest is lowercasing the headers that the web server sends. It does so for a good reason I'll comment its quote here

// Store the header keys lower cased to be more robust. Headers are case
// insensitive according to RFC 2616.
$this->responseHeaders[strtolower($parts[0])] = isset($parts[1]) ? trim($parts[1]) : '';

So I think this code is robust and reasonable. I assure you that the web server is sending Content-type (mixed case) as is conventional.

Thanks effulgentsia for agreeing to handle #22 in a follow-up.

Tests have passed so back to RTBC.

  /**
   * Responds to entity POST requests.
   *
   * @param mixed $id
   *   Ignored.

Is it me, or this is *very* sloppy? When we are going to have other operations on a collection (for example, GET), are we *also* going to have an empty variable being passed around? Couldn't we make the resource handler properly handle those collection operations instead of hacking around like this?

Also, in its current form, would the deserializer allow to set the entity id and revision id?

@klausi: POST is a collection-level action, not a resource-level action. Those should not be handled the same way. It seems like the design needs to be fixed.

POST is not a collection-level action inherently. It's "instructions presented to a resource for processing", where that processing is undefined by HTTP. There's no reason you can't POST to a resource.

Yes. But in the context of this issue, only a POST to create a new resource is implemented. I don't mind the extra unused $id in the signature, since it creates consistency with the other http methods in the module, though if a follow up wants to explore cleaning that up, that's cool too.

It seems like Moshe, Crell and others are fine with the implementation (myself included), despite DamZ not being fully on board with the implementation details. I decided to go ahead and commit this. We can do clean up in a follow-up patch if enough people care.