ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty

Super nice numbers. What happens when someone calls getResponseData() on an unserialized result though?

1. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -0,0 +1,210 @@
   +    $format = $this->getResponseFormat($this->routeMatch, $request);
   

   Given we are in a subscriber and not inside the routing layer any longer, I~m wondering whether using \Drupal\Core\Routing\RouteMatch::createFromRequest would be safer here.

2. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -129,118 +129,13 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
        return $response;
      }
   

   I'm wondering whether having some direct documentation here pointing towards the subscriber would make it clearer what is going on

This patch makes a lot of sense. I learned a lot from just reading it, thanks!

Some notes:

1. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -0,0 +1,209 @@
   +    $response = $this->renderResponseBody($request, $response, $this->serializer, $format);
   ...
   +  protected function renderResponseBody(Request $request, ResourceResponseInterface $response, SerializerInterface $serializer, $format) {
   ...
   +    return $response;
   

   Minor, but seems unnecessary to return the response, when it's being passed in and modified.

   Without looking at the docs I would guess the return value of a method called renderResponseBody() to be a $response_body, not the $response. So not returning anything would be clearer in my opinion. I realize, that that's arguable, though, so feel free to leave as is, as well.

2. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -0,0 +1,209 @@
   +  protected function flattenResponse(ResourceResponseInterface $response) {
   +    $final_response = ($response instanceof CacheableResponseInterface) ? new CacheableResponse() : new Response();
   +    $final_response->setContent($response->getContent());
   +    $final_response->setStatusCode($response->getStatusCode());
   +    $final_response->headers->add($response->headers->all());
   +    if ($final_response instanceof CacheableResponseInterface) {
   +      $final_response->addCacheableDependency($response->getCacheableMetadata());
   +    }
   +    return $final_response;
   +  }
   

   This is pretty nifty! Nice work.

   I looked at Symfony's Response and besides the stuff that is being taken care of here, there is the following state in a response object: version (the HTTP version), statusText, and charset.

   I can't actually think of those ever being very useful (maybe statusText, but I think we should take those over as well, just for completeness' sake.

3. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
   @@ -0,0 +1,334 @@
   + * @group restkak
   

   :-)

4. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
   @@ -0,0 +1,334 @@
   +
   +
   

   Double newline

5. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
   @@ -0,0 +1,334 @@
   +      $handler_response = new ResourceResponse($method !== 'DELETE' ? ['REST' => 'Drupal'] : NULL);
   +      $this->assertInstanceOf(ResourceResponseInterface::class, $handler_response);
   +      $this->assertInstanceOf(CacheableResponseInterface::class, $handler_response);
   

   Am I missing or are these assertions pretty pointless?

6. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
   @@ -0,0 +1,334 @@
   +      $this->assertInstanceOf(CacheableResponseInterface::class, $handler_response);
   

   Seems there is no test coverage for uncacheable responses?

   Not sure how hard that would be to integrate, though.

7. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
   @@ -0,0 +1,334 @@
   +  protected function getFunctioningResourceResponseSubscriber(RouteMatchInterface $route_match) {
   

   Minor, but in my opinion the class would read more fluently if this were the last method.

8. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
   @@ -0,0 +1,334 @@
   +    $renderer = $this->prophesize(RendererInterface::class);
   +    $renderer->executeInRenderContext(Argument::type(RenderContext::class), Argument::type('callable'))
   +      ->will(function ($args) {
   +        $callable = $args[1];
   +        return $callable();
   +      });
   

   Wow

9. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
   @@ -0,0 +1,334 @@
   +/**
   + * Stub class where we can prophesize methods.
   + */
   +class StubRequestHandlerResourcePlugin extends ResourceBase {
   +
   +  function get() {}
   +  function post() {}
   +  function patch() {}
   +  function delete() {}
   +
   +}
   

   Unused here, I think.

Thank you, the fixes look great. Also looked through the patch again, and - again - found it very readable and a great cleanup. I'm general not an expert on the request processing system, but this A) just makes sense all around B) @dawehner and @neclimdul looked at it and didn't find it too offensive and C) is mostly moving code around, so I'm going to be a tad bold and RTBC this one.

1. Nice to see the performance improvements this patch brings and the evidence on the issue :)

2. +++ b/core/modules/rest/rest.services.yml
   @@ -28,3 +28,8 @@ services:
   +  resource_response.subscriber:
   

   I think we should prefix the service name with rest. - whilst it is not a standard - it should be and definitely will make this addition less likely to clash.

3. FILE: ...drupal/core/modules/rest/tests/src/Kernel/RequestHandlerTest.php
   ----------------------------------------------------------------------
      5 | WARNING | [x] Unused use statement
     13 | WARNING | [x] Unused use statement
     14 | WARNING | [x] Unused use statement
   

   There are now some unused uses in this test.

1. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -0,0 +1,206 @@
   +/**
   + * Response subscriber to handle resource responses.
   + */
   +class ResourceResponseSubscriber implements EventSubscriberInterface {
   

   i wonder whether we could describe better what this class is doing

2. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -0,0 +1,206 @@
   +        if (!$context->isEmpty()) {
   +          $response->addCacheableDependency($context->pop());
   +        }
   +      }
   +      else {
   +        $output = $serializer->serialize($data, $format);
   +      }
   

   I'm wondering whether we could put the if into the if($context->isEmpty()) bit, so we keep just one $serializer->serialize() call

3. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -0,0 +1,206 @@
   +    $final_response = ($response instanceof CacheableResponseInterface) ? new CacheableResponse() : new Response();
   +    $final_response->setContent($response->getContent());
   +    $final_response->setStatusCode($response->getStatusCode());
   +    $final_response->setProtocolVersion($response->getProtocolVersion());
   +    $final_response->setCharset($response->getCharset());
   +    $final_response->headers->add($response->headers->all());
   +    if ($final_response instanceof CacheableResponseInterface) {
   +      $final_response->addCacheableDependency($response->getCacheableMetadata());
   

   Is it just me, or is this kinda sad? We mostly copy an existing response, right? I'm a bit confused about the function name.

4. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
   @@ -0,0 +1,206 @@
   +    $events[KernelEvents::RESPONSE][] = ['onRespond'];
   

   nitpick: We could use onResponse

5. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
   @@ -0,0 +1,371 @@
   +      // The default data for \Drupal\rest\ResourceResponse.
   +      [NULL, ''],
   +      [''],
   +      ['string'],
   +      ['Complex \ string $%^&@ with unicode ΑΒΓΔΕΖΗΘΙΚΛΜΝΞΟΣὨ'],
   +      [[]],
   +      [['test']],
   +      [['test' => 'foobar']],
   +      [TRUE],
   +      [FALSE],
   +      // @todo Not supported. https://www.drupal.org/node/2427811
   +      // [new \stdClass()],
   +      // [(object) ['test' => 'foobar']],
   

   Did you considered to use named data provider, but yeah, meh

Done. Though I wanna call out that the reason I named it like I did before, is that we have html_response.subscriber and ajax_response.subscriber in core.services.yml. Still, I agree this is better.

Well, if we don't start now we never start.

This code was just being moved. But, improved it since you asked.

Mh sure, well, I try to improve things :)

+++ b/core/modules/rest/src/RequestHandler.php
@@ -129,118 +129,13 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
-  protected function getResponseFormat(RouteMatchInterface $route_match, Request $request) {
...
-  protected function renderResponse(Request $request, ResourceResponseInterface $response, SerializerInterface $serializer, $format, RestResourceConfigInterface $resource_config) {

I'm pondering about the BC implications of this change. The only usage in core is \Drupal\rest\Plugin\ResourceBase::getBaseRoute() as the controller class. We've already said that controllers are internal (see https://www.drupal.org/core/d8-bc-policy#controllers). So I think this change is allowed.

Committed 0f3f279 and pushed to 8.3.x. Thanks!