EntityResource: Provide comprehensive test coverage for Item entity

Working on this one instead since #2843755: EntityResource: Provide comprehensive test coverage for Message entity is currently blocked :)

Woot, thanks! :)

I'm attaching a WIP patch here.

It's failing because \Drupal\aggregator\Entity\Item has no canonical url but still returns text/html; charset=UTF-8. Hence the test below fails:

 // DX: 404 when resource not provisioned, but HTML if canonical route.
    $response = $this->request('POST', $url, $request_options);
    if ($has_canonical_url) {
      $this->assertSame(404, $response->getStatusCode());
      $this->assertSame(['text/html; charset=UTF-8'], $response->getHeader('Content-Type'));
    }
    else {
      $this->assertResourceErrorResponse(404, 'No route found for "GET ' . str_replace($this->baseUrl, '', $this->getUrl()->setAbsolute()->toString()) . '"', $response);
    }

Here's a related issue with some more info: #2853211: EntityResource::post() incorrectly assumes that every entity type has a canonical URL.

Wim, can you advise on how to fix the fails here? Thanks.

Note: I've marked testPatch and testPatch as skipped for now.

Great catch!

We're in testPost(), yet the code said this:

$this->assertResourceErrorResponse(404, 'No route found for "GET ' . str_replace($this->baseUrl, '', $this->getUrl()->setAbsolute()->toString()) . '"', $response);

Note the mention of "GET"!

That does not make sense. I mirrorred this after testGet(). But of course that doesn't make sense. When you don't specify a format, then html is assumed by Drupal. Hence, you'll always get a HTML response.

A similar problem exists a dozen lines of code further down.

I fixed both of those. Now you're getting this error:

--- Expected
+++ Actual
@@ @@
-{"message":"Unprocessable Entity: validation failed.\ntitle: Title: this field cannot hold more than 1 values.\n"}
+{"message":"Unprocessable Entity: validation failed.\nfid: This value should not be null.\ntitle: Title: this field cannot hold more than 1 values.\n"}

/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:361
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/ResourceTestBase.php:377
/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:677

I'll leave that to you again :)

Thanks @Wim Leers. This is super helpful.

Adding another WIP patch here. This one fails (see the fail for testPost) because \Drupal\aggregator\Entity\Item has no uuid.

// DX: 422 when invalid entity: UUID field too long.
$response = $this->request('POST', $url, $request_options);
$this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nuuid.0.value: UUID: may not be longer than 128 characters.\n", $response);

Which is a good thing because now we can write the tests for #2835576: InvalidArgumentException: Field uuid is unknown. in Drupal\Core\Entity\ContentEntityBase->getTranslatedField().

@Wim Leers I believe we will have the same issues in #7 for testPatch and testDelete?

Should we fix it in the related ticket here: #2853211: EntityResource::post() incorrectly assumes that every entity type has a canonical URL? I've updated the title of the ticket.

#10 + #12:

1. yay, looking forward to your patch for #2835576: InvalidArgumentException: Field uuid is unknown. in Drupal\Core\Entity\ContentEntityBase->getTranslatedField()!
2. However, please note this in \Drupal\aggregator\Entity\Item::baseFieldDefinitions():
   

       // @todo Convert to a real UUID field in
       //   https://www.drupal.org/node/2149851.
       $fields['guid'] = BaseFieldDefinition::create('string_long')
         ->setLabel(t('GUID'))
         ->setDescription(t('Unique identifier for the feed item.'));
   

   I just got that issue going again: #2149851-14: Remove todo about GUID field on the 'aggregator_item' entity and add UUID field. If Item entities behaved as intended, then this would be a non-issue.

3. A problem with [#28570856] is that it is then no longer testing a particular kind of failure anymore for Item entities. So I'd rather not have that committed, and would prefer #2149851: Remove todo about GUID field on the 'aggregator_item' entity and add UUID field to be fixed instead.

#14 + #15:

@Wim Leers I believe we will have the same issues in #7 for testPatch and testDelete?

Indeed.

Should we fix it in the related ticket here?

I think so, yes. Posted a review there!

#2853211: EntityResource::post() incorrectly assumes that every entity type has a canonical URL landed, this is now unblocked!

Thanks @Wim Leers. Working on it now.

Woot, thanks!

OK let's try this one.

Note: I added a check for the uuid field and a @todo with a link to the issue. Let me know if this works.

Forgot to include the EntityResourceTestBase fix. Let's try again.

So close!

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -695,8 +695,11 @@ public function testPost() {
   +    if ($this->entity->uuid()) {
   

   We should change this to:

   if ($this->entity->getEntityType()->hasKey('uuid')) {
   

   That's more explicit.

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Item/ItemResourceTestBase.php
   @@ -0,0 +1,178 @@
   +    $feed = Feed::create(array(
   ...
   +    $item = Item::create([
   

   s/array()/[]/

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Item/ItemResourceTestBase.php
   @@ -0,0 +1,178 @@
   +          'target_id' => (int) $feed->id(),
   

   We can hardcode this ID to 1 AFAICT?

Updated. Thanks @Wim Leers :)

Sorry… I noticed a few more small things:

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Item/ItemResourceTestBase.php
   @@ -0,0 +1,178 @@
   +      'title' => t('Llama'),
   

   Let's omit the t() call here. t() is for interface translation. This is content.

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Item/ItemResourceTestBase.php
   @@ -0,0 +1,178 @@
   +    $item->setPostedTime(123456789)
   

   Why use setPostedTime(), but not setFeedId(), setTitle() or setLink()?

   Let's use those setters instead of passing all the values in the Item::create() call. That's better/clearer.

   (It's also how e.g. \Drupal\Tests\rest\Functional\EntityResource\Node\NodeResourceTestBase::createEntity() does it.)

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Item/ItemResourceTestBase.php
   @@ -0,0 +1,178 @@
   +          'url' => $feed->toUrl()->toString(),
   

   We can even hardcode this URL! :)

After this, I promise it's RTBC!

All changes completed.

Thanks!

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Item/ItemResourceTestBase.php
@@ -98,7 +97,7 @@
-          'url' => $feed->toUrl()->toString(),
+          'url' => '/aggregator/sources/1',

Ahhh, this only works on D8 installations that are installed in a domain's root, it will fail for subdir installations.

Use base_path() here. See e.g. \Drupal\Tests\rest\Functional\EntityResource\Comment\CommentResourceTestBase::getExpectedNormalizedEntity().

Updated.

Committed and pushed 8419d5e to 8.4.x and 769eb4b to 8.3.x. Thanks!

#2857086: EntityResourceTestBase: only test uuid for entity types that have uuid was closed as a duplicate of this :)