Automatically provide HEAD support when a REST resource supports GET

4. Now repeat number 2. This and all subsequent HEAD requests are now successful with a 200 status.

Oh interesting, this is a bug in page_cache module ... it should include some logic for HEAD requests maybe.

#3: I'm not sure it's a bug in page_cache, I suspect it's thanks to page_cache that subsequent HEAD requests work.

I think the bigger problem is that apparently HEAD requests do not always work. Which is yet another reason for #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method.

Thanks for reporting this bug!

Added this to the top priorities: #2721489-17: REST: top priorities for Drupal 8.2.x.

#3: I'm not sure it's a bug in page_cache, I suspect it's thanks to page_cache that subsequent HEAD requests work.

Well, right, but let's assume we are outside of rest, but rather in some other custom code. page_cache should still not cache act on those HEAD requests like that.

I think the bigger problem is that apparently HEAD requests do not always work.

Right, so this is an additional problem on top.

Well, right, but let's assume we are outside of rest, but rather in some other custom code. page_cache should still not cache act on those HEAD requests like that.

The HTTP spec does not agree with you, https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.4 says:

The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response. The metainformation contained in the HTTP headers in response to a HEAD request SHOULD be identical to the information sent in response to a GET request. This method can be used for obtaining metainformation about the entity implied by the request without transferring the entity-body itself. This method is often used for testing hypertext links for validity, accessibility, and recent modification.

In other words, the Cache-Control headers persist, so the response to a GET request is also valid for the HEAD response, it just needs to strip the response body. That's it.

So Page Cache works correctly IMHO AFAICT, it's just that REST module doesn't support HEAD requests properly.

The reason it doesn't work in the REST module is simple: none of our @RestResource plugins implement a head() method. Therefore, you cannot make HEAD requests. That's it.

So the question is very simple: do we require that @RestResource plugins implement head() (so they can minimize the amount of data fetching, access checking …), or do we automatically implement it?

I think it's best to require explicit implementation.

For EntityResource, that'd require an addition like:

  /**
   * Responds to entity HEAD requests.
   *
   * @param \Drupal\Core\Entity\EntityInterface $entity
   *   The entity object.
   *
   * @return \Drupal\rest\ModifiedResourceResponse
   *   The response containing the entity with its accessible fields.
   *
   * @throws \Symfony\Component\HttpKernel\Exception\HttpException
   */
  public function head(EntityInterface $entity) {
    $entity_access = $entity->access('view', NULL, TRUE);
    if (!$entity_access->isAllowed()) {
      throw new AccessDeniedHttpException();
    }

    $response = new ResourceResponse([], 200);
    $response->addCacheableDependency($entity);
    $response->addCacheableDependency($entity_access);

    return $response;
  }

i.e. this doesn't need to do all of the field-level access checking that the get() method must do.

In other words, the Cache-Control headers persist, so the response to a GET request is also valid for the HEAD response, it just needs to strip the response body. That's it.

Well, there is nothing inside page cache which ensures that the output varies by HTTP method. The interdiff provides an example route to show an example. Once you have cached the HEAD request, it will stick in there, even if you ask for GET requests. For me this is a bug of the page_cache module, which is 100% independent from whatever REST is doing.

Steps to reproduce:

1. Apply this mini patch
2. Rebuild everything
3. HTTP HEAD d8.dev/test
4. HTTP HEAD d8.dev/test # this should trigger a cache HIT
5. HTTP GET d8.dev/test # this triggers a cache HIT, even there was no such thing as an existing GET request yet

In the other usecase, when we have a cached GET request, we could totally provide a fast HEAD request inside the page_cache, as we would just have to strip the content and call it a day.

So the question is very simple: do we require that @RestResource plugins implement head() (so they can minimize the amount of data fetching, access checking …), or do we automatically implement it?

IMHO we should implement a default implementation and then let people figure out alternative better implementations.

Well, there is nothing inside page cache which ensures that the output varies by HTTP method.

Well, that sure is a bug. Great catch :) Filed #2753741: Page Cache caches all safe HTTP methods (GET+HEAD), but generates the same cache ID for either: add test coverage to prove this is correct, with an initial patch.

Here is an implementation of HEAD responses in REST module using existing GETs.

See #9: #15 has one big downside: it does too much work. And in doing so, it can make the result uncacheable — e.g. when all users can view an entity (cacheable HEAD response), but cannot view all fields within the entity (uncacheable GET response).

See #9: #15 has one big downside: it does too much work. And in doing so, it can make the result uncacheable — e.g. when all users can view an entity (cacheable HEAD response), but cannot view all fields within the entity (uncacheable GET response).

Yeah I focused more on a generic implementation not just for entity resources but the generic usecase.

This should apply now

Looks great :) Now just needs tests.

This patch totally applies for me.

Here is a quick fix. @marthinal might work on adding tests and adding a entity specific implementation.

Let's see what the testbot says.

From Symfony\Component\Routing\Matcher\UrlMatcher

            // check HTTP method requirement
            if ($requiredMethods = $route->getMethods()) {
                // HEAD and GET are equivalent as per RFC
                if ('HEAD' === $method = $this->context->getMethod()) {
                    $method = 'GET';
                }

                if (!in_array($method, $requiredMethods)) {
                    $this->allow = array_merge($this->allow, $requiredMethods);

                    continue;
                }
            }

Permissions for HEAD don't work. Basically HEAD is treated as GET AFAIK. So I think that we can use our new ModifiedResourceResponse class. IMHO makes no sense to cache a HEAD request because we are caching the GET request with an empty body.

Needs test!

@Wim @dawehner what do you think about that?

#27: can you provide an interdiff? It's not clear to me what the difference is.

@Wim yes sure. Attached!

@marthinal
Its sad that you removed the generic implemention in ResourceBase ... its quite nice that one doesn't have to develop it for other rest plugins. Do you mind adding it back?

@dawehner Sure. Sorry! I thought we only want the entity specific implementation. Missing integration tests. I'll try to continue working on it later.

1. Adding Integration test.

2. Removing $response->setContent(NULL); from the generic implementation. By default we set the content to NULL here:
Symfony\Component\HttpFoundation\Response

            if ($request->isMethod('HEAD')) {
                // cf. RFC2616 14.13
                $length = $headers->get('Content-Length');
                $this->setContent(null);
                if ($length) {
                    $headers->set('Content-Length', $length);
                }
            }

3. I think we don't need an entity implementation here. At least to be honest I'm not sure how to reproduce the situation explained here #17. But probably I'm missing something :)

ANyway HEAD is a GET but in the Response we remove the content.

1. +++ b/core/modules/rest/src/Plugin/ResourceBase.php
   @@ -149,11 +149,11 @@ public function routes() {
   +
        return $response;
   

   Can we document that responses handle that automatically by default?

2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -114,26 +114,6 @@ public function get(EntityInterface $entity) {
   -   * Responds to entity HEAD requests.
   -   *
   -   * @param \Drupal\Core\Entity\EntityInterface $entity
   -   *   The entity object.
   -   *
   -   * @return \Symfony\Component\HttpFoundation\Response
   -   *   The response.
   -   *
   -   * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
   -   */
   -  public function head(EntityInterface $entity) {
   -    $entity_access = $entity->access('view', NULL, TRUE);
   -    if (!$entity_access->isAllowed()) {
   -      throw new AccessDeniedHttpException();
   -    }
   -
   -    return new ModifiedResourceResponse();
   -  }
   -
   

   Well, IMHO having the entity specific one would be fine, as its faster

1. Done.

2. Sure! Done

Thanks!

The first HEAD request is not cacheable because we are using ModifiedResourceResponse.

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -114,6 +114,24 @@ public function get(EntityInterface $entity) {
+
+    return new ModifiedResourceResponse();
+  }

IMHO this is semantically wrong. Those requests are totally safe to use and by that we can add the cacheability metadata.

1. +++ b/core/modules/rest/src/Plugin/ResourceBase.php
   @@ -141,6 +142,26 @@ public function routes() {
   +   * Provides an implementation of a HEAD response using an existing GET.
   

   Generates a HEAD response by reusing the existing GET implementation, if any.

2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -114,6 +114,24 @@ public function get(EntityInterface $entity) {
   +    return new ModifiedResourceResponse();
   

   return (new ResourceResponse())->addCacheableDependency($entity_access);

3. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -119,7 +120,11 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
   +        throw new MethodNotAllowedHttpException($resource->availableMethods(), "$method requests are not supported for this resource.");
   

   "for" or "by"?

   More importantly: we have no test coverage for this.

4. +++ b/core/modules/rest/src/Tests/PageCacheTest.php
   @@ -135,4 +136,39 @@ public function testConfigChangePageCache() {
   +    $entity = $this->entityCreate('entity_test');
   +    $entity->save();
   

   We never use $entity. So then we can just do:

   $this->entityCreate('entity_test')->save();
   

5. +++ b/core/modules/rest/src/Tests/PageCacheTest.php
   @@ -135,4 +136,39 @@ public function testConfigChangePageCache() {
   +    $url = Url::fromUri('internal:/entity_test/1?_format=' . $this->defaultFormat);
   

   Hm, not a big fan of the use of internal: here, but it's probably ok.

6. +++ b/core/modules/rest/src/Tests/PageCacheTest.php
   @@ -135,4 +136,39 @@ public function testConfigChangePageCache() {
   +    $this->httpRequest($url, 'HEAD', NULL, $this->defaultMimeType);
   +    $this->assertResponse(200, 'HTTP response code is correct.');
   +    $this->assertResponseBody('');
   +
   +    $this->httpRequest($url, 'HEAD', NULL, $this->defaultMimeType);
   +    $this->assertResponse(200, 'HTTP response code is correct.');
   +    $this->assertResponseBody('');
   

   What's the point of doing the same thing twice? Shouldn't this then verify X-Drupal-Cache headers?

   Or shouldn't one of them do a GET request?

What's the point of doing the same thing twice? Shouldn't this then verify X-Drupal-Cache headers?

Or shouldn't one of them do a GET request?

Well, this clashes a bit with #2753741: Page Cache caches all safe HTTP methods (GET+HEAD), but generates the same cache ID for either: add test coverage to prove this is correct if we want to test it properly :)

Right! Thanks :)

39.1 Done. Doc comment short description must be on a single line. Removing "if any"

39.2 Done

39.3 Done

39.4 Done

Missing 39.6. I'll try to do it later.

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -128,7 +128,7 @@ public function head() {
-    return new ModifiedResourceResponse();
+   return (new ResourceResponse())->addCacheableDependency($entity_access);
   }

One space is missing :P

Oops @Wim++ :)

Ok! So IMHO the entity implementation should return ModifiedResourceResponse(). In this test we can see that the first HEAD (MISS) is correct but the next request using GET (HIT) is not correct because the body is empty. So this test should fail. And... I think it makes sense because a HEAD is a GET but in the Response we remove the content.

If someone can explain me this ModifiedResourceResponse for HEAD? HEAD is an independent operation, there is nothing to modify here ... I mean we went with using ModifiedResourceResponse and not something like NonGetResourceResponse, because there is HEAD.

#46++.

The failure here is not related to using ModifiedResourceResponse instead of ResourceResponse. In fact, the test even shows that: the test expects the presence of certain cache tags, but ModifiedResourceResponse cannot even carry cache tags!

Rerolled.

@Wim We have 2 different failures there(#45). And we are using ResourceResponse there.

'entity_test:1' is present in the X-Drupal-Cache-Tags header.

Value NULL is equal to value 'entity_test'.

Removing the assertion about cache tags in the current patch(simply to show the problem). IMHO we are caching the HEAD response and then the GET response will have the body empty and x-drupal-cache will be set to HIT. And this is a bug because we want to obtain the body for GET requests.

So using ModifiedResourceResponse we avoid to cache HEAD...

Am I missing something here???

We can use the Response class directly... then we avoid to cache and to use the ModifiedResourceResponse class...

Attached 1 patch that should fail to see the error + the new patch using Response class + the interdiff to see the difference.

thanks!

Why do we want to avoid caching HEAD?

We don't want to avoid caching HEAD. :) Using ResourceResponse the test 2752325-49-sould-fail.patch fails because the body (for GET requests) is empty when doing a HEAD and later a GET.

So something like this will fail:

    $url = Url::fromUri('internal:/entity_test/1?_format=' . $this->defaultFormat);

    $this->enableService('entity:entity_test', 'GET');

    $this->httpRequest($url, 'HEAD', NULL, $this->defaultMimeType);
    $this->assertResponse(200, 'HTTP response code is correct.');
    $this->assertHeader('x-drupal-cache', 'MISS');
    $this->assertResponseBody('');

    // FAILS the body is empty
    $response = $this->httpRequest($url, 'GET', NULL, $this->defaultMimeType);
    $this->assertResponse(200, 'HTTP response code is correct.');
    $this->assertHeader('x-drupal-cache', 'HIT');
    $this->assertCacheTag('config:rest.resource.entity.entity_test');
    $this->assertCacheTag('entity_test:1');
    $data = Json::decode($response);
    $this->assertEqual($data['type'][0]['value'], 'entity_test');

On the other hand if we do a GET and then a HEAD, the next GET will be correct. Something like this:

    $url = Url::fromUri('internal:/entity_test/1?_format=' . $this->defaultFormat);

    $this->enableService('entity:entity_test', 'GET');

    $response = $this->httpRequest($url, 'GET', NULL, $this->defaultMimeType);
    $this->assertResponse(200, 'HTTP response code is correct.');
    $this->assertHeader('x-drupal-cache', 'MISS');
    $this->assertCacheTag('config:rest.resource.entity.entity_test');
    $this->assertCacheTag('entity_test:1');
    $data = Json::decode($response);
    $this->assertEqual($data['type'][0]['value'], 'entity_test');

    $this->httpRequest($url, 'HEAD', NULL, $this->defaultMimeType);
    $this->assertResponse(200, 'HTTP response code is correct.');
    $this->assertHeader('x-drupal-cache', 'HIT');
    $this->assertResponseBody('');

    $response = $this->httpRequest($url, 'GET', NULL, $this->defaultMimeType);
    $this->assertResponse(200, 'HTTP response code is correct.');
    $this->assertHeader('x-drupal-cache', 'HIT');
    $this->assertCacheTag('config:rest.resource.entity.entity_test');
    $this->assertCacheTag('entity_test:1');
    $data = Json::decode($response);
    $this->assertEqual($data['type'][0]['value'], 'entity_test');

There's a related issue. #2753741: Page Cache caches all safe HTTP methods (GET+HEAD), but generates the same cache ID for either: add test coverage to prove this is correct

Maybe this is a problem with the cache system...

@marthinal
What about opening up a follow up to look into that again after #2753741: Page Cache caches all safe HTTP methods (GET+HEAD), but generates the same cache ID for either: add test coverage to prove this is correct went in?

#53: wrong issue link?

This is what happens if you try to not copy and paste stuff.

@dawehner To reproduce the problem we should apply the current patch otherwise HEAD is not working by default. So IMHO we need fix the problem here.

Testing a little bit more I found that this test works as expected:

    $url_head = Url::fromUri('internal:/entity_test/1');
    $url_get = Url::fromUri('internal:/entity_test/1?_format=' . $this->defaultFormat);

    $this->enableService('entity:entity_test', 'GET');

    $this->httpRequest($url_head, 'HEAD', NULL, $this->defaultMimeType);
    $this->assertResponse(200, 'HTTP response code is correct.');
    $this->assertHeader('x-drupal-cache', 'MISS');
    $this->assertResponseBody('');

    // FAILS the body is empty
    $response = $this->httpRequest($url_get, 'GET', NULL, $this->defaultMimeType);
    $this->assertResponse(200, 'HTTP response code is correct.');
    $this->assertHeader('x-drupal-cache', 'MISS');
    $this->assertCacheTag('config:rest.resource.entity.entity_test');
    $this->assertCacheTag('entity_test:1');
    $data = Json::decode($response);
    $this->assertEqual($data['type'][0]['value'], 'entity_test');

I mean we are caching per url...

This is a very important comment https://www.drupal.org/node/2753741#comment-11325449

To be honest I can only think 2 different possibilities:

1) We don't need to implement HEAD for EntityResource. So we are caching the GET response and then Symfony\Component\HttpFoundation\Response will remove the content by default:

            if ($request->isMethod('HEAD')) {
                // cf. RFC2616 14.13
                $length = $headers->get('Content-Length');
                $this->setContent(null);
                if ($length) {
                    $headers->set('Content-Length', $length);
                }
            }

Drupal\rest\Plugin\ResourceBase

    // By default the content is set to NULL when preparing a HEAD Response
    // before it is sent to the client.
    // @see \Symfony\Component\HttpFoundation\Response
    /** @var \Symfony\Component\HttpFoundation\Response $response */
    $response = call_user_func_array([$this, 'get'], func_get_args());

2) We want to implement HEAD for ENtityResource. Then IMHO we need to avoid caching the Response because a HEAD response will be cached with an empty body (makes no sense the return the body here because we already have GET). And makes sense because AFAIK we're caching per url.

@Wim @dawehner what do you think about that? I need your opinion here.

I'm going through this patch now. Trying to fix my nits at the same time, because @marthinal said at #2291055-184: REST resources for anonymous users: register that he doesn't have time for this this week, and this probably needs to land before feature freeze.

1. +++ b/core/modules/rest/src/Plugin/ResourceBase.php
   @@ -141,6 +142,26 @@ public function routes() {
      /**
   +   * Generates a HEAD response by reusing the existing GET implementation.
   +   *
   

   Missing docs. Fixed.

2. +++ b/core/modules/rest/src/Plugin/ResourceBase.php
   @@ -141,6 +142,26 @@ public function routes() {
   +      throw new MethodNotAllowedHttpException($this->availableMethods(), "HEAD requests are not supported for this resource.");
   

   Not just HEAD, but also GET. If it wouldn't support HEAD, and only HEAD, then that'd be a severe bug. So, should mention both.

3. +++ b/core/modules/rest/src/Plugin/ResourceBase.php
   @@ -141,6 +142,26 @@ public function routes() {
   +    // By default the content is set to NULL when preparing a HEAD Response
   +    // before it is sent to the client.
   +    // @see \Symfony\Component\HttpFoundation\Response
   

   This had me confused at first. What you meant is not "by default", but "automatically" :) And by "preparing", you were referring to Response::prepare(). So, let's make that more explicit. Fixed.

4. +++ b/core/modules/rest/src/Plugin/ResourceBase.php
   @@ -141,6 +142,26 @@ public function routes() {
   +    $response = call_user_func_array([$this, 'get'], func_get_args());
   +
   +    return $response;
   

   We're assigning something here only to return it. Why not just do that right away?

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -113,6 +114,25 @@ public function get(EntityInterface $entity) {
+  public function head() {
+    $entity = func_get_args()[0];

Why would you retrieve a passed in function argument like this? Why not just be explicit, just like EntityResource::get()?

Oh… if you change this like I would like to see:

 core/modules/rest/src/Plugin/rest/resource/EntityResource.php | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
index 29dd34e..f90693b 100644
--- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -117,15 +117,16 @@ public function get(EntityInterface $entity) {
   /**
    * Responds to entity HEAD requests.
    *
+   * @param \Drupal\Core\Entity\EntityInterface $entity
+   *   The entity object.
+   *
    * @return \Symfony\Component\HttpFoundation\Response
    *   The response.
    *
    * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
    */
-  public function head() {
-    $entity = func_get_args()[0];
+  public function head(EntityInterface $entity) {
     $entity_access = $entity->access('view', NULL, TRUE);
-
     if (!$entity_access->isAllowed()) {
       throw new AccessDeniedHttpException();
     }

… then you'll get this error:

Declaration of Drupal\rest\Plugin\rest\resource\EntityResource::head() should be compatible with Drupal\rest\Plugin\ResourceBase::head()

And so that is why you did this.

I think ResourceBase::head() is unacceptable if it means REST resources cannot be written in a correct manner anymore. Even more so because it is just a base class.

This made me understand what @marthinal was getting at in #27 (and why he removed this method in that reroll): because Symfony and Drupal treat GET and HEAD as the same internally (it maps both to the same route). And so there is literally no point in having an optimized head() method, because Symfony will still call get() anyway!

This reroll

Attached is a reroll that:

1. hardcodes EntityResource::head() to print a message
2. adds test coverage for HEAD, that verifies that there's no response body (which would FAIL if EntityResource::head() would actually be called!) and verifies that all headers are identical to the GET response

So, given that #58 proves that adding a head() method to REST resource plugins is utterly pointless, I've:

1. removed all changes in ResourceBase
2. removed all changes in EntityResource

+++ b/core/modules/rest/src/RequestHandler.php
@@ -117,6 +118,9 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
+      if (!method_exists($resource, $method)) {
+        throw new MethodNotAllowedHttpException($resource->availableMethods(), "$method requests are not supported by this resource.");
+      }

+++ b/core/modules/rest/tests/src/Kernel/RequestHandlerTest.php
@@ -119,6 +119,33 @@ public function testSerialization($data, $expected_response = FALSE) {
+  public function testMethodNotAllowed() {
+    $request = new Request();
+    $request->setMethod('POST');
+
+    $route_match = new RouteMatch('test', new Route('/rest/test', ['_rest_resource_config' => 'restplugin'], ['_format' => 'json', '_content_type_format' => 'json']));
+
+    $resource = $this->prophesize(StubMethodNotAllowedResourcePlugin::class);
+    $resource->availableMethods()->willReturn(['POST']);
+
+    // Setup the configuration.
+    $config = $this->prophesize(RestResourceConfigInterface::class);
+    $config->getResourcePlugin()->willReturn($resource->reveal());
+    $config->getCacheContexts()->willReturn([]);
+    $config->getCacheTags()->willReturn([]);
+    $config->getCacheMaxAge()->willReturn(12);
+    $this->entityStorage->load('restplugin')->willReturn($config->reveal());
+
+    $data = ['error' => 'post requests are not supported by this resource.'];
+
+    $handler_response = $this->requestHandler->handle($route_match, $request);
+
+    $this->assertEquals(json_encode($data), $handler_response->getContent());
+  }

This is not something we should implement in rest.module. It's something the routing system should provide for us.

And it will, once #2659070: REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response lands. It's currently RTBC.

So, let's remove all this and focus just on HEAD request support.

Just a random / offtopic thought. We could put this generic HEAD support into the routing layer or somewhere there. This would allow us to support HEAD basically everywhere and we don't have to deal with this issue described in #58

Turns out I made two mistakes in #59:

1. First, Symfony does treat GET and HEAD the same. But it actually is possible to implement a route that only replies to HEAD requests. And if you implement a route that supports it, then that route will be picked over GET routes.
2. Second, I was being fooled by the exact same thing as the person who opened this issue. It seems HEAD requests work, but that's just because PageCache cached them.

However, that just means that the "proper" solution just looks different from what this issue had been doing so far. This is what AFAICT is the correct, and most Symfony-esque flow, that closely matches the existing expectations of Symfony:

1. Incoming HEAD request
2. Router picks a GET route
3. PageCache::handle() caches the GET response (i.e. including body)
4. DrupalKernel::handle() receives the GET response and calls $response->prepare() on it, which transforms a GET response to a HEAD response: it strips the body and updates the Content-Length header

The end result: HEAD support! A HEAD request will cause the PageCache to be populated with a GET response. Which makes total sense when you take RFC2616 into account. And to get this, all we need to do is Use Symfony As Intended™!

1. +++ b/core/modules/rest/src/Entity/RestResourceConfig.php
   @@ -250,7 +250,7 @@ protected function getRestResourceDependencies() {
   -    $valid_methods = ['GET', 'POST', 'PATCH', 'DELETE'];
   +    $valid_methods = ['GET', 'POST', 'PATCH', 'DELETE', 'HEAD'];
   

   This is actually no longer necessary.

2. +++ b/core/modules/rest/src/RequestHandler.php
   --- a/core/modules/rest/src/Tests/PageCacheTest.php
   +++ b/core/modules/rest/src/Tests/PageCacheTest.php
   

   This specifically tests REST + PageCache.

3. +++ b/core/modules/rest/src/Tests/ReadTest.php
   @@ -55,9 +55,28 @@ public function testRead() {
   +      // Verify that it exists: use a HEAD request.
   +      $this->httpRequest($this->getReadUrl($entity), 'HEAD');
   +      $this->assertResponseBody('');
   +      $head_headers = $this->drupalGetHeaders();
   +
          // Read it over the REST API.
          $response = $this->httpRequest($this->getReadUrl($entity), 'GET');
   +      $get_headers = $this->drupalGetHeaders();
          $this->assertResponse('200', 'HTTP response code is correct.');
   +
   +      // Verify that the GET and HEAD responses are the same, that the only
   +      // difference is that there's no body.
   +      unset($get_headers['date']);
   +      unset($head_headers['date']);
   +      unset($get_headers['content-length']);
   +      unset($head_headers['content-length']);
   +      unset($get_headers['x-drupal-dynamic-cache']);
   +      unset($head_headers['x-drupal-dynamic-cache']);
   +      $this->assertIdentical($get_headers, $head_headers);
   +      $this->assertResponse('200', 'HTTP response code is correct.');
   +
   

   This implicitly tests PageCache, but that's not the point. The What it really does test is that the responses are the same, as RFC2616 demands.

4. 2+3 together mean that this has full test coverage.

As far as I'm concerned, this is RTBC. But I've worked too much on it at this point to RTBC it.

+++ b/core/modules/rest/src/RequestHandler.php
@@ -62,6 +62,21 @@ public static function create(ContainerInterface $container) {
+    if ($method === 'head') {
+      $method = 'get';
+    }

And to prove that this is really the only change that is necessary… here's a patch identical to #65, but also a test-only patch that will fail. (No interdiff because no changes

This looks good! RTBC!

+++ b/core/modules/rest/src/RequestHandler.php
@@ -62,6 +62,21 @@ public static function create(ContainerInterface $container) {
 
+    // Symfony is built to transparently map HEAD requests to a GET request. In
+    // the case of the REST module's RequestHandler though, we essentially have
+    // our own light-weight routing system on top of the Drupal/symfony routing
+    // system. So, we have to do the same as what the UrlMatcher does: map HEAD
+    // requests to the logic for GET. This also guarantees response headers for
+    // HEAD requests are identical to those for GET requests, because we just
+    // return a GET response. Response::prepare() will transform it to a HEAD
+    // response at the very last moment.
+    // @see https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.4
+    // @see \Symfony\Component\Routing\Matcher\UrlMatcher::matchCollection()
+    // @see \Symfony\Component\HttpFoundation\Response::prepare()
+    if ($method === 'head') {
+      $method = 'get';
+    }

I still have the feeling that this could be even done on the routing level or so, and make it more generic than just for rest module. It could be also useful for jsonapi for example, or whatever.

I still have the feeling that this could be even done on the routing level or so, and make it more generic than just for rest module.

How? The routing system is already deciding to route a HEAD request to a controller that knows how to deal with GET.

\Drupal\rest\RequestHandler::handle() is a very generic controller, that itself routes an incoming request to:

1. a @RestResource plugin (depending on the route)
2. a specific PHP method on an instance of that plugin (depending on the HTTP method

It's that second point where things go wrong: Symfony already decided to map HEAD to GET, but now RequestHandler does $request->getMethod() which of course must return the actual HTTP method. We just need to do the same normalization that the (Symfony) routing system does.

The only possible alternative is to change ResourceBase to also encode the PHP method to call in the route definition in some route option, and for RequestHandler to then look at that. This comes with one enormous downside: not every \Drupal\rest\Plugin\ResourceInterface implementation is required to use \Drupal\rest\Plugin\ResourceBase, so it will make the REST module's API even more dependent on that base class.

Conclusion: the solution in the patch is IMO the best.

Again in other words, why can't the routing system itself handle all possible HEAD requests, when there is a GET route.

Moving this to 8.1.x since it's a straight bugfix.

Here is my alternative idea.

This issue is not looking particularly rtbc.

So yeah here is a more complete patch.

Retesting because #75 applies cleanly here.

I applied #75 and ran it locally. It fails, as I predicted in #70.

With #75, RequestHandler tries to invoke a head() method on the REST resource plugin, but that doesn't exist, so it fatals:

call_user_func_array() expects parameter 1 to be a valid callback, class 'Drupal\rest\Plugin\rest\resource\EntityResource' does not have a method 'head'	

This is why we will always need the first hunk of #66, no matter what other nice additions we make to our routes to better describe what they support. But again, the additions that #73/#75 make to make our routes be more self-descriptive are also rather pointless, because Symfony already does that automatically for all routes anyway. Symfony does that automatically, because that's what RFC2616 prescribes.

Hopefully the failures in #75 (that should appear in ~30 mins) will convince you, and help you arrive at the same conclusion as I had in #70 :)

Well, I am still 100% convinced that we could implement a generic HEAD support, no matter how much text you write, its just though a little bit tougher than expected. Let's do that in a follow up.

15:43:09 <WimLeers> dawehner: ping
15:43:13 <dawehner> WimLeers: pong
15:43:26 <WimLeers> dawehner: you said "Let's do that in a follow up."
15:43:33 <WimLeers> dawehner: do you mean, the approach you're proposing?
15:43:34 <WimLeers> Or?
15:43:54 <dawehner> WimLeers: yeah
15:44:23 <WimLeers> dawehner: so do you propose that I reupload my last patch? (From #66)
15:44:30 <dawehner> WimLeers: yeah pretty miuch
15:44:36 <WimLeers> ok thanks

So, restoring the patch from #66. But since @catch moved this to 8.1.x in #72, there's now a patch for 8.1.x and another for 8.2.x. It's impossible to roll a patch that applies cleanly to both. (Because: tests have changed, a single REST settings config file vs REST Resource Config Entities…)

Follow-up created: #2775479: Try to remove the "map HEAD to GET" logic in \Drupal\rest\RequestHandler::handle().

I'll let @dawehner RTBC.

Thank you wim and sorry for trying to solve things on a more fundamental level.

Please don't apologize, I wish that were possible — and maybe we'll still find a way in #2775479: Try to remove the "map HEAD to GET" logic in \Drupal\rest\RequestHandler::handle() :)

Committed 7766139 and pushed to 8.2.x. Thanks!
Committed 8baec91 and pushed to 8.1.x. Thanks!

Just a heads up, not sure if the outcome will be Symfony implementing HEAD request support, but I have a valid use case now that I've brought to their attention.

https://github.com/symfony/symfony/issues/28743