Add test coverage to ensure it's possible to reference File entities

@Grimreaper, would you be kind enough to check if this issue is still relevant? If not, please feel free to close it.

Yay, a failing test, thank you! 👏

The failure says it's a 422 instead of a 201 response. If I print the exact error response:

diff --git a/tests/src/Functional/JsonApiFunctionalTest.php b/tests/src/Functional/JsonApiFunctionalTest.php
index c5ff8fd..8871064 100644
--- a/tests/src/Functional/JsonApiFunctionalTest.php
+++ b/tests/src/Functional/JsonApiFunctionalTest.php
@@ -471,6 +471,7 @@ class JsonApiFunctionalTest extends JsonApiFunctionalTestBase {
       'headers' => ['Content-Type' => 'application/vnd.api+json'],
     ]);
     $created_response = Json::decode($response->getBody()->__toString());
+    var_dump((string)$response->getBody());
     $this->assertEquals(201, $response->getStatusCode());
     $this->assertArrayHasKey('uuid', $created_response['data']['attributes']);
     $uuid = $created_response['data']['attributes']['uuid'];
wim.leers at acquia in ~/Work/d8/modules/jsonapi on 8.x-1.x*

… then I see:

string(330) "{"errors":[{"title":"Unprocessable Entity","status":422,"detail":"field_image.0: You do not have access to the referenced entity (\u003Cem class=\u0022placeholder\u0022\u003Efile\u003C\/em\u003E: \u003Cem class=\u0022placeholder\u0022\u003E1\u003C\/em\u003E).","code":0,"source":{"pointer":"\/data\/attributes\/field_image\/0"}}]}"

In other words: the entity you're trying to reference is not accessible to you. That error is generated by \Drupal\Core\Entity\Plugin\Validation\Constraint\ReferenceAccessConstraint+\Drupal\Core\Entity\Plugin\Validation\Constraint\ReferenceAccessConstraintValidator.

I did some debugging and … the reason this test is failing is:

+++ b/tests/src/Functional/JsonApiFunctionalTestBase.php
@@ -129,6 +136,14 @@ abstract class JsonApiFunctionalTestBase extends BrowserTestBase {
+      'uri' => 'vfs://default_image.png',

because \Drupal\file\FileAccessControlHandler::checkAccess() only grants view access to public:// files. Updated the test to use that, and now it should be green.

So I'm going to bet that you're test with a private:// file.

#9 now fails in testRead() because the expected normalization is now different. But it now does pass testWrite().

Needs confirmation from @Grimreaper that he's using private://.

#2899109: Trouble PATCHing a File Field might be a duplicate of this.

#13: ok then this definitely Needs steps to reproduce at minimum, and Needs tests ideally (because then we don't need manual steps to reproduce anymore).

#2932031: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases added \Drupal\Tests\jsonapi\Functional\MediaTest. Updating its \Drupal\Tests\jsonapi\Functional\MediaTest::getPostDocument() would be sufficient to add test coverage for this!

No movement, so I'm doing it for you. Passes tests just fine, so either this was indeed a permission problem as I said before, or JSON API fixed this in the mean time.

Ideally, this would have the client upload a file via a HTTP API. Support for that landed only days ago in Drupal core's REST module. Created #2958554: Allow creation of file entities from binary data via JSON API requests to port it to JSON API.

For now, this test can just have PHP create a File entity, just to test that referencing File entities works.

(No interdiff, because new patch.)

Sadly, #17 fails due to a subtle difference in normalization. Not sure where/why/how this is happening, we need to dig into that.

Let's get this going again.

The interdiff in #18 is completely wrong. Correct interdiff attached.

It's strange that #18 did not pass, because it passes locally for me, at least today. Maybe other changes in the past month made a difference.

Attached is also a rebased #18, which passes for me locally, on D8.6.

Interesting! That failed. Let's see if the same happens on 8.6. Queued an 8.6 test.

D'oh 😅

Yay, green! We can't commit it just yet though, because:

+++ b/tests/src/Functional/ResourceTestBase.php
@@ -1693,7 +1693,8 @@ abstract class ResourceTestBase extends BrowserTestBase {
-        $this->assertSame($created_entity_document, $decoded_response_body);
+        // @todo remove the casts before comitting this!
+        $this->assertSame(static::castToString($created_entity_document), static::castToString($decoded_response_body));

This is that subtle difference in normalization that #18 referred to. Digging in…

Locally, this seems to pass just fine now. Let's see if DrupalCI agrees with that.

I was stupidly testing MediaTest::testGetIndividual() instead of MediaTest::testPostIndividual() 😳

Turns out the root cause is quite mundane: Entity/Field API. When it loads entities from the database, all field values of the in-memory EntityInterface object are strings! This is AFAIK even accepted as normal (even though it certainly is annoying).

The correct solution would be to make property normalization respect the underlying type, i.e. to do the appropriate casting. Even though Entity/Field API really should've taken care of that for us. So let's do that!

So I did.

And in doing so, found that several of HEAD's assertions also had incorrect values due to the same root cause, and those now are correct too! 🤘

I think it's clear we've disproven the original bug report. Let's make this a task to add test coverage that proves this is possible. And to ensure we don't ever regress!

1. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -112,7 +113,9 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
   +          $metadata[$property_key] = $property instanceof PrimitiveInterface
   +          ? $property->getCastedValue()
   +          : $property->getValue();
   

   Indentation is off.

2. There is actually one assertion in the PATCH test coverage that also does this string casting business … and I think it may be possible to remove that now!
   

   $this->assertArraySubset(static::castToString($field_normalization), $updated_entity->get($field_name)->getValue(), TRUE);
   

   (At which point \Drupal\Tests\jsonapi\Functional\ResourceTestBase::castToString() can be removed too, because zero callers remain!)

1. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -112,7 +113,9 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
   +          $metadata[$property_key] = $property instanceof PrimitiveInterface
   

   Do we need to be doing this type casting elsewhere too? Like FieldItemNormalizer? Perhaps it's only needed here because we're special casing entity reference?

   In FieldItemNormalizer, we pass field item properties into the serialization system:

   foreach ($field_item as $property_name => $property) {
     $values[$property_name] = $this->serializer->normalize($property, $format, $context);
   }
   

2. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -112,7 +113,9 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
   +            ? $property->getCastedValue()
   

   I've always wondered when/where this method would actually come in handy. TIL.

3. +++ b/tests/src/Functional/MediaTest.php
   @@ -60,9 +60,9 @@ class MediaTest extends ResourceTestBase {
   -          $this->grantPermissionsToTestedRole(['create media']);
   +          $this->grantPermissionsToTestedRole(['create media', 'access content']);
   ...
   -        $this->grantPermissionsToTestedRole(['create camelids media']);
   +        $this->grantPermissionsToTestedRole(['create camelids media', 'access content']);
   

   Can we add a comment explaining the new permission?

4. +++ b/tests/src/Functional/MediaTest.php
   @@ -141,7 +148,7 @@ class MediaTest extends ResourceTestBase {
   -    $thumbnail = File::load(2);
   +    $thumbnail = File::load(3);
   

   Nit: Is there a way to make these integers dynamic or make then a static property?

5. +++ b/tests/src/Functional/MediaTest.php
   @@ -203,8 +210,8 @@ class MediaTest extends ResourceTestBase {
   -                'width' => '180',
   -                'height' => '180',
   +                'width' => 180,
   +                'height' => 180,
   

   This is satisfying.

6. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -2129,30 +2131,6 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -  protected static function castToString(array $normalization) {
   

   😍

I realized my review in #32 doesn't actually have any points that need work. It's mostly questions and suggestions, so it shouldn't technically be "Needs Work", but there's not a better status.

#33:
Thanks for the review!

1. ✔️ D'OH OF COURSE. Although doing this will make description: null (instead of description: '') and display: null (instead of display: false) return… But fixing that is arguably out of scope.
2. Heh :)
3. ✔️
4. ✔️
5. 🙂
6. 🙂

Also fixed the CS violation.

🍻