EntityResource::post() incorrectly assumes that every entity type has a canonical URL

This makes it difficult to test for cases where the Content-Type header is different from application/json.

That's the point.

The point is that \Drupal\Tests\rest\Functional\ResourceTestBase::assertResourceResponse() and \Drupal\Tests\rest\Functional\ResourceTestBase::assertResourceErrorResponse() are the methods you should use 95% of the time. They make things simple, short, clean, nice.

Whenever you need to deviate from that, suffer the consequences. Usually this is because of an unfortunate edge case or a bug! So let it be long, ugly, in-your-face.

Oh hahah, that's because #2805281: ?_format=hal_json error responses are application/json, yet should be application/hal+json should have simplified that but forgot to do so! That definitely should be improved, of course :)

No, because:

1. A \Symfony\Component\HttpKernel\Exception\NotFoundHttpException exception is thrown by the routing system.
2. The format is json (or hal_json, or …), which is one of the serialization formats.
3. Hence the NotFoundHttpException (404) is caught by \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber, which converts it to a JSON/HAL+JSON/… response.

#7: because $this->assertResourceErrorResponse() takes care of asserting the Content-Type response header for us :) That's the thing: anything that behaves in a strange/unexpected way is going to have additional/ugly looking assertions. The fact that we get a HTML response is strange. So the assertions stand out.

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -578,7 +578,6 @@ public function testPost() {
   -    $has_canonical_url = $this->entity->hasLinkTemplate('canonical');
   

   Hah, interesting! Apparently testPost() didn't even need $has_canonical_url at all then!

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -597,15 +596,10 @@ public function testPost() {
   -    // DX: 404 when resource not provisioned, but HTML if canonical route.
   +    // DX: 404 when resource not provisioned. HTML because no ?_format.
   
   @@ -621,16 +615,11 @@ public function testPost() {
   -    // DX: 415 when no Content-Type request header, plain text if canonical URL.
   +    // DX: 415 when no Content-Type request header. HTML because no ?_format.
   

   These aren't consistent with the comments in testGet(). Fixed.

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -783,7 +772,7 @@ public function testPatch() {
   -    // DX: 404 when resource not provisioned, but 405 if canonical route.
   +    // DX: 405 if canonical route, 404 for non canonical. HTML because no ?_format.
   

   Same remark as above, and exceeds 80 cols. Fixed.

4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -791,17 +780,23 @@ public function testPatch() {
   -    // DX: 405 when resource not provisioned.
   +    // DX: 405 when resource not provisioned. 404 for non canonical.
   

   This comment is no longer accurate. Fixed.

5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -791,17 +780,23 @@ public function testPatch() {
   -    $this->assertSame(['GET, POST, HEAD'], $response->getHeader('Allow'));
   -    $this->assertResourceErrorResponse(405, 'No route found for "PATCH ' . str_replace($this->baseUrl, '', $this->getUrl()->setAbsolute()->toString()) . '": Method Not Allowed (Allow: GET, POST, HEAD)', $response);
   +    if ($has_canonical_url) {
   +      $this->assertSame(405, $response->getStatusCode());
   +      $this->assertSame(['GET, POST, HEAD'], $response->getHeader('Allow'));
   +    }
   +    else {
   +      $this->assertResourceErrorResponse(404, 'No route found for "PATCH ' . str_replace($this->baseUrl, '', $this->getUrl()->setAbsolute()->toString()) . '"', $response);
   +    }
   

   The "if" case" now is missing an assertion for the content type.

   And AFAICT, this can use the original assertion!

   Fixed.

6. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -809,16 +804,11 @@ public function testPatch() {
   -    // DX: 415 when no Content-Type request header, but HTML if canonical route.
   +    // DX: 415 when no Content-Type request header.
        $response = $this->request('PATCH', $url, $request_options);
   -    if ($has_canonical_url) {
   -      $this->assertSame(415, $response->getStatusCode());
   -      $this->assertSame(['text/plain; charset=UTF-8'], $response->getHeader('Content-Type'));
   -      $this->assertTrue(FALSE !== strpos((string) $response->getBody(), htmlspecialchars('No "Content-Type" request header specified')));
   -    }
   -    else {
   -      $this->assertResourceErrorResponse(415, 'No "Content-Type" request header specified', $response);
   -    }
   +    $this->assertSame(415, $response->getStatusCode());
   +    $this->assertSame(['text/plain; charset=UTF-8'], $response->getHeader('Content-Type'));
   +    $this->assertTrue(FALSE !== strpos((string) $response->getBody(), htmlspecialchars('No "Content-Type" request header specified')));
   

   Interesting, so we never actually hit the "else" case here. Great catch.

   However, I suspect this is because we have no test coverage for entity types without a canonical link template.

   I think that before we make this change, we should add test coverage for one of these, e.g. \Drupal\entity_test\Entity\EntityTestLabel, so that we can be certain that we really don't need this.

   (We do e.g. have ConfigTest, which also doesn't have a canonical link template, but that one only supports GET, because we don't yet support modifications to config entities.)

7. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -977,7 +967,7 @@ public function testDelete() {
   -    // DX: 404 when resource not provisioned, but 405 if canonical route.
   +    // DX: 405 when resource not provisioned. 404 for non canonical.
   

   I think this was clearer before. Although the bit about "plain text or HTML" should be appended. Fixed.

8. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -985,18 +975,22 @@ public function testDelete() {
   -
   

   Unnecessary change, reverting.

9. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -985,18 +975,22 @@ public function testDelete() {
   -    // DX: 405 when resource not provisioned.
   +    // DX: 405 when resource not provisioned. 404 for non canonical.
   

   Correct change, but made consistent with comments in testGet() again. Fixed.

10. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -985,18 +975,22 @@ public function testDelete() {
    -    $this->assertSame(['GET, POST, HEAD'], $response->getHeader('Allow'));
    -    $this->assertResourceErrorResponse(405, 'No route found for "DELETE ' . str_replace($this->baseUrl, '', $this->getUrl()->setAbsolute()->toString()) . '": Method Not Allowed (Allow: GET, POST, HEAD)', $response);
    -
    +    if ($has_canonical_url) {
    +      $this->assertSame(405, $response->getStatusCode());
    +      $this->assertSame(['GET, POST, HEAD'], $response->getHeader('Allow'));
    +    }
    +    else {
    +      $this->assertResourceErrorResponse(404, 'No route found for "DELETE ' . str_replace($this->baseUrl, '', $this->getUrl()->setAbsolute()->toString()) . '"', $response);
    +    }
    

    The "if" case" now is missing an assertion for the content type.

    And AFAICT, this can use the original assertion!

    Fixed.

I fixed all my own nits. You only need to address point 6.

#14: heh glad you liked my review :)

The failures of the test-only patch in #18 prove that indeed the existing assertions are wrong. And like I said in #13.6: we never actually hit the "else" case with the existing tests. This new test does and fails. Perfect.

Thanks to this additional test coverage, this guarantees that the changes made by this patch are actually correct.

#18 looks perfect. And #18 is identical to #13, the only changes are test coverage expansions. Unfortunately, perfect with one exception:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTestLabel/EntityTestLabelResourceTestBase.php
@@ -0,0 +1,163 @@
+  public function testPost() {
+    $this->markTestSkipped();
+  }

This we can't have.

You described in #14 why this is failing. In fact, you have the answer right there in the exception that you quoted! :) The root cause is that EntityTestLabel has no canonical link template (which is why we're testing this entity type in the first place, see #13.6), yet EntityResource::post() and EntityResourceTestBase::testPost() ignore this, and assume that it's possible to always send a Location header. But that's not the case. Also note that https://tools.ietf.org/html/rfc2616#section-10.2.2 says "SHOULD", not "MUST" (about providing a location in the response).

(I RTBC'ed the work by @arshadcn + @vaplas, and made only trivial changes in #21 to address the last small remark. I think that's acceptable, given that I'm a REST module maintainer.)

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -189,8 +189,12 @@ public function post(EntityInterface $entity = NULL) {
-      $url = $entity->urlInfo('canonical', ['absolute' => TRUE])->toString(TRUE);
-      return new ModifiedResourceResponse($entity, 201, ['Location' => $url->getGeneratedUrl()]);
+      $headers = [];
+      if (in_array('canonical', $entity->uriRelationships(), TRUE)) {
+        $url = $entity->urlInfo('canonical', ['absolute' => TRUE])->toString(TRUE);
+        $headers['Location'] = $url->getGeneratedUrl();
+      }
+      return new ModifiedResourceResponse($entity, 201, $headers);

Doesn't this mean we're fixing a bug here? The only run-time change in this patch was introduced in #21 - the comment that rtbc'd the patch. Setting back to needs review just to make sure that this bit gets further review. It looks sensible to me - but perhaps the issue title or summary needs an update to detail what is being fixed. At the moment it sounds all about tests.

Yes, the title/IS need to be updated. This is fixing a bug in test coverage, which led us to discover a small bug in EntityResource, i.e. outside test coverage.

Eh… turns out that the bug that I fixed in #21 was reported at #2836121: PHP error when POSTing content entities without a "canonical" link template.

I'll migrate the necessary information onto this one. This issue includes the test coverage already.

IS completely rewritten. Title updated.

This blocks the https://www.drupal.org/project/paragraphs contrib module because it's impossible to POST paragraph content entities: they result in an unhandled exception and hence a 500 response.

This also blocks #2843752: EntityResource: Provide comprehensive test coverage for Item entity.

Back to RTBC.

Closed #2836121: PHP error when POSTing content entities without a "canonical" link template as a duplicate. Migrated its tags (accidentally did that in #29).

I believe I'm running into this issue when using the Entity Pilot git module. My specific issue is that whenever it uses TypeLinkManager::getTypeInternalIds is called the $type_uri that is supplied is like /rest/type/taxonomy_term/category whereas the type_uri that is retrieved from TypeLinkManager::getTypes looks like http://:/rest/type/taxonomy_term/product_category

So I've been investigating whether the types are being stored incorrectly or retrieved incorrectly. I guess i don't know what my expectations should be because to me the json looks right.

"_links": {
        "self": {
            "href": "\/taxonomy\/term\/6?_format=hal_json"
        },
        "type": {
            "href": "http:\/\/:\/rest\/type\/taxonomy_term\/category"
        }
    },

but somehow when json_decode is finished with decoding that json it converts to /rest/type/taxonomy_term/category

Which leads me to this issue and how it appears that this canonical url needs to be fixed. Is there any chance we can get this patch working with Drupal 8.2 / 8.3 ?

@cosmicdreams: what you are describing is all specific to the HAL module. AFAICT it's completely unrelated to this issue. Please open a new issue against the hal.module component.

Thanks Wim. Will do.

Thank you! Will respond to that issue as soon as you open it. Please make sure to provide steps to reproduce, then I'm certain we can get it fixed very quickly.

Cool, here ya go #2859641: Need to setup proper Request object for console commands

This is also blocking #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler.

Committed 0edd07b and pushed to 8.4.x. Thanks!

I think we should commit this to 8.3.x - going to ask other committers.

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -189,8 +189,12 @@ public function post(EntityInterface $entity = NULL) {
-      $url = $entity->urlInfo('canonical', ['absolute' => TRUE])->toString(TRUE);
-      return new ModifiedResourceResponse($entity, 201, ['Location' => $url->getGeneratedUrl()]);
+      $headers = [];
+      if (in_array('canonical', $entity->uriRelationships(), TRUE)) {
+        $url = $entity->urlInfo('canonical', ['absolute' => TRUE])->toString(TRUE);
+        $headers['Location'] = $url->getGeneratedUrl();
+      }
+      return new ModifiedResourceResponse($entity, 201, $headers);

This is a pure bugfix.

Contrib modules having their own entity types without a canonical URL would also run into this bug. In fact, that's already the case, see #2836121: PHP error when POSTing content entities without a "canonical" link template, which reported it for the paragraph entity type!

Therefore I strongly agree this should be committed to 8.3.x.

So let's make this a bugfix.

Yep #44 is the only real change here, and agreed it's just a bugfix and that we should backport it.

Yeah it was originally a task (improve test coverage), then in doing so we found an actual bug outside of test coverage. Sorry for not changing the category earlier.

Fixed a couple of merge conflicts on commit in comments and what with what is in the 8.4.x patch. The conflicts where caused by what was in HEAD.

Thanks!

Can we please also assign issue credit to @ruloweb (for his contributions at #2836121-2: PHP error when POSTing content entities without a "canonical" link template, which was closed as a duplicate). Perhaps also @dagmar.

I ran into this bug. How will this patch end up in normal Drupal installations? When Drupal 8.4 arrives? Or with a new Paragraphs version?