Make serialization_class optional

Well, the deserialization should always happen, right? You just don't need to register a custom normalizer in the serializer chain, but you still want the JSON for example to be converted to arrays. So this should work as is, or do you get an exception?

+++ b/core/modules/rest/src/RequestHandler.php
@@ -63,7 +63,13 @@ public function handle(RouteMatchInterface $route_match, Request $request, Route
         $definition = $resource->getPluginDefinition();
...
         try {
-          $unserialized = $serializer->deserialize($received, $class, $format, array('request_method' => $method));
+          if ($class) {
+            $unserialized = $serializer->deserialize($received, $class, $format, array('request_method' => $method));
+          }
+          // Avoid denormalization because we need to instantiate a class.
+          else {
+            $unserialized = $serializer->decode($received, $format, array('request_method' => $method));
+          }
         }

so this means that the serialization_class key is now optional in the plugin definition. We should add documentation to the RestResource.php annotation class what keys there are and that this one is optional.

And if it is optional you will get a PHP notice here, because it is not defined in $definition['serialization_class'], correct?

So we should check if serialization_class is set.

And we need a test case here. Either we add one to DBLogResource.php or we create our own simple rest_test module with the test plugin to check this.

Otherwise I do think that this approach is a good idea by just using the decode() method of the serializer.

We should add documentation to the RestResource.php annotation class what keys there are and that this one is optional.

\Drupal\rest\Annotation\RestResource is not yet updated with this documentation. Marking NW for that.

The issue title & summary are also not very descriptive. It'd be great if you could make them more specific/descriptive.

rerolled for 8.1.x

forgot attachment ;)

Marking Needs review so testbot tests it.

Thanks for picking this up again! :)

Here's a review. There are lots and lots of small problems with this patch. Fortunately, they're all super easy to fix :)

1. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -54,9 +54,15 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +          // If the plugin does not specify a serialization class just decode the received data.
   

   80 cols violation.

2. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -54,9 +54,15 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +          // Example: received JSON is decoded into a PHP array.
   

   This example is unclear.

3. +++ b/core/modules/rest/src/Tests/ResourceTest.php
   @@ -104,6 +104,40 @@ public function testAuthentication() {
   +   * Tests that we must decode instead of deserialize.
   

   This sentence does not actually make sense.

4. +++ b/core/modules/rest/src/Tests/ResourceTest.php
   @@ -104,6 +104,40 @@ public function testAuthentication() {
   +    // Enabling the service.
   

   No service is being enabled here.

5. +++ b/core/modules/rest/src/Tests/ResourceTest.php
   @@ -104,6 +104,40 @@ public function testAuthentication() {
   +    $resources = [
   +      ['type' => 'serialization_test', 'method' => 'POST'],
   +    ];
   ...
   +    foreach ($resources as $resource) {
   

   There is only one resource. This foreach is overkill.

6. +++ b/core/modules/rest/src/Tests/ResourceTest.php
   @@ -104,6 +104,40 @@ public function testAuthentication() {
   +    $format = 'json';
   ...
   +    $serialized = $this->container->get('serializer')->serialize(['foo'], 'hal_json');
   

   json vs. hal_json

7. +++ b/core/modules/rest/src/Tests/ResourceTest.php
   @@ -104,6 +104,40 @@ public function testAuthentication() {
   +    $this->rebuildCache();
   

   I don't think this is necessary anymore.

8. +++ b/core/modules/rest/src/Tests/ResourceTest.php
   @@ -104,6 +104,40 @@ public function testAuthentication() {
   +    // Add permission to anonymous user.
   ...
   +    // Create a JSON.
   ...
   +    // Post to the REST service to verify that the result is correct.
   

   These comments are all unnecessary. (And each contains grammatical errors.)

9. +++ b/core/modules/rest/src/Tests/ResourceTest.php
   @@ -104,6 +104,40 @@ public function testAuthentication() {
   +    $this->assertResponse('200', 'HTTP response code is correct.');
   +    $this->assertResponseBody('"foo"', 'The Response Body is correct.');
   

   Both of these messages actually are more harmful than they help: the assert methods they use already provide far better messages.

Finally, #7 also still needs to be addressed.

Looks great for me. We still need a change record though according to the issue.

Looks much, much better :) Very close to ready IMO. Just a few more small things, then this is RTBC.

1. +++ b/core/modules/rest/src/Tests/ResourceTest.php
   @@ -104,6 +105,29 @@ public function testAuthentication() {
   +    $settings['serialization_test']['POST']['supported_formats'][] = 'json';
   +    $settings['serialization_test']['POST']['supported_auth'] = $this->defaultAuth;
   

   Why not use $this->enableService()?

2. +++ b/core/modules/rest/src/Tests/ResourceTest.php
   @@ -104,6 +105,29 @@ public function testAuthentication() {
   +    $this->rebuildCache();
   

   This should not be necessary?

3. +++ b/core/modules/rest/tests/modules/rest_test/src/Plugin/rest/resource/SerializationClassTestResource.php
   @@ -0,0 +1,32 @@
   +class SerializationClassTestResource extends ResourceBase {
   

   I'd call this NoSerializationClassTestResource.

+++ b/core/modules/rest/src/Tests/ResourceTest.php
@@ -104,6 +105,29 @@ public function testAuthentication() {
+    $serialized = $this->container->get('serializer')->serialize(['foo'], 'json');
...
+    $this->assertResponseBody('"foo"');

+++ b/core/modules/rest/tests/modules/rest_test/src/Plugin/rest/resource/SerializationClassTestResource.php
@@ -0,0 +1,32 @@
+    return new ResourceResponse($array[0], 200, []);

IMHO we just just return the array, so that we check arrays as well

#19++

@marthinal
You uploaded the wrong patch ...

+++ b/core/modules/rest/src/Tests/ResourceTest.php
@@ -104,6 +105,22 @@ public function testAuthentication() {
+    $this->assertResponse('200');

Nitpick: Its weird that we use '200' and not 200, but feel free to fix that on commit.

1. +++ b/core/modules/rest/src/Tests/ResourceTest.php
   @@ -104,6 +107,23 @@ public function testAuthentication() {
   +    $encoder = new JsonEncode();
   

   This is dead code, can be removed.

2. +++ b/core/modules/rest/tests/modules/rest_test/src/Plugin/rest/resource/NoSerializationClassTestResource.php
   @@ -0,0 +1,32 @@
   +   * @param array $array
   

   Nit: s/$array/$data/

@Wim Leers I can't see the code referred to in #27.1 - what am I missing.

This is perfect for me now

Still needs a change record

#28: Weird, sorry regarding #27.1, I have no idea how that happened!

#31: agreed, CR is missing. But … that points to another problem in REST's current code: \Drupal\rest\Annotation\RestResource doesn't list this serialization_class annotation attribute at all. I think this issue should fix that, and then immediately document it on the annotation class as being optional. If you want to see an example of how to do that, see \Drupal\Core\Annotation\Action.

Almost there! :)

Here is a CR: https://www.drupal.org/node/2736393

Do we still need this now that the user login functionality will no longer be a REST resource?

Well, it won't be longer a blocker (as we already realized) but others kind of code might have the same problem.

Fair.

However, that makes this a normal feature request IMO.

I would argue its a normal bug.

Well, it won't be longer a blocker (as we already realized) but others kind of code might have the same problem.

Prescient words, because #2662284 is now blocked on this, see #2662284-55: Return complete entity after successful PATCH.

This is an API addition, should go into 8.2.

All that's missing from this patch is an update to the annotation class, like I said in #32. Adding that brings it back to RTBC. My addition is mind-bogglingly trivial, so I think it's fine to re-RTBC.

Committed b3d0a73 and pushed to 8.2.x. Thanks!

Wow, that was incredibly fast!

However, the commit has not yet been pushed. Could you do that? :) Thanks!

+++ b/core/modules/rest/src/Annotation/RestResource.php
@@ -38,4 +38,11 @@ class RestResource extends Plugin {
+   * The serialization class to deserialize serialized data into.
...
+  public $serialization_class;

looks that backportable to 8.1

#44: but it's an API addition.

It wouldn't harm 8.1 in any way, but we don't do API additions to the current version unless absolutely necessary.