This issue is based in a discussion between @klausi, @damiankloip and me.

When serializing an object that has a file field, the URI of the image is just exposed in HAL+json format but not in other formats.

For example, requesting http://d8.local/entity/node/1 with Accept: application/json returns the following info for the file entity

"field_page_file":[
  {
    "target_id":"1",
    "display":"1",
    "description":""
  }
]

While if we request hal+json, we get the following (note, response is cropped):

{
  "_links": {
     ...
    "http:\/\/d8.local\/rest\/relation\/node\/page\/field_page_file": [
      {
        "href": "http:\/\/d8.local\/entity\/file\/1"
      }
    ]
  },
   ...
   "_embedded": {
    ...
    "http:\/\/d8.local\/rest\/relation\/node\/page\/field_page_file": [
      {
        "_links": {
          "self": {
            "href": "http:\/\/d8.local\/entity\/file\/1"
          },
          "type": {
            "href": "http:\/\/d8.local\/rest\/type\/file\/file"
          }
        },
        "uuid": [
          {
            "value": "73f796ef-8a15-4a0c-8e4b-75ea1caf8b12"
          }
        ]
      }
    ]
  },
  ...
}

So it seems that there is a way to obtain the file URI, although the keys within _embedded and _links are not easy to grok from a programmers perspective in order to request a page node and render a file URI contained in that node.

I am about to review EntityReferenceItemNormalizer (suggested by @damiankloip) to see how other formats such as JSON include the URI of the file.

CommentFileSizeAuthor
#44 interdiff-2124677-42.txt4.21 KBdamiankloip
#44 2124677-42.patch9.54 KBdamiankloip
#39 interdiff-2124677-39.txt5.23 KBdamiankloip
#39 2124677-39.patch9.35 KBdamiankloip
#38 interdiff-2124677-38.txt1.23 KBdamiankloip
#38 2124677-38.patch9.65 KBdamiankloip
#28 interdiff-2124677-28.txt659 bytesdamiankloip
#28 2124677-28.patch9.44 KBdamiankloip
#24 interdiff-2124677-24.txt7.54 KBdamiankloip
#24 2124677-24.patch9.42 KBdamiankloip
#21 interdiff-2124677-21.txt838 bytesdamiankloip
#21 2124677-21.patch1.87 KBdamiankloip
#19 interdiff-2124677-19.txt2.33 KBdamiankloip
#19 2124677-19.patch1.86 KBdamiankloip
#12 drupal-expose-file-uri-2124677-9.patch2.02 KBchr.fritsch
#8 drupal-expose-file-uri-2124677-8.patch2.57 KBjuampynr
#5 drupal-expose-file-uri-2124677-5.patch2.34 KBjuampynr
#5 drupal-expose-file-uri-2124677-5.interdiff.txt1.49 KBjuampynr
#4 drupal-expose-file-uri-2124677-4.interdiff.txt1.64 KBjuampynr
#4 drupal-expose-file-uri-2124677-4.patch2.22 KBjuampynr
#3 interdiff.txt1.3 KBjuampynr
#3 drupal-expose-file-uri-2124677-2.patch2.44 KBjuampynr
#1 drupal-expose-file-uri-2124677-1.patch2.25 KBjuampynr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

juampynr’s picture

Status: Active » Needs work
FileSize
2.25 KB

First pass. I added FileFieldItemNormalizer service. It is currently being picked up by Drupal core for File fields. Comments after uploading patch.

juampynr’s picture

Some observations from y patch:

  1. +++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FileFieldItemNormalizer.php
    @@ -0,0 +1,61 @@
    +   * Implements \Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize()
    ...
    +    $values = $field_item->getPropertyValues();
    

    I need to figure out how to obtain the file's URI and then add it to the $values array.

  2. +++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FileFieldItemNormalizer.php
    @@ -0,0 +1,61 @@
    +    dd('denormalize');
    +  }
    

    Until I am done with normalize() I won't take care of denormalisation.

  3. +++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FileFieldItemNormalizer.php
    @@ -0,0 +1,61 @@
    +    return $data;
    

    Not sure if I need this.

juampynr’s picture

OK, now file fields return the URI to the file like the following when requesting a node in JSON or XML format:

"field_page_file":[
  {
    "target_id":"1",
    "display":"1",
    "description":"",
    "uri":"http:\/\/d8.local\/sites\/default\/files\/myfile.txt"
  }
]

And since image fields are also file entities, they are listed too. Here I added an image field to the page content type, edited the node and then requested it as JSON:

...
"field_page_file": [
  {
    "target_id": "1",
    "display": "1",
    "description": "",
    "uri": "http:\/\/d8.local\/sites\/default\/files\/some_file.txt"
  }
],
"field_page_image": [
  {
    "target_id": "2",
    "display": null,
    "description": null,
    "uri": "http:\/\/d8.local\/sites\/default\/files\/some_image.jpg"
  }
]
...

Now I will take care of what needs to be done at method denormalize().

juampynr’s picture

Issue summary: View changes

Updated description

juampynr’s picture

This patch adds both the URL to access the full entity (as 'uri') and the URL to access to the content directly (as 'content'), so it looks like the following:

"field_page_file": [
  {
    "target_id": "1",
    "display": "1",
    "description": "",
    "uri": "http:\/\/d8.local\/entity\/file\/1",
    "content": "http:\/\/d8.local\/sites\/default\/files\/test.txt"
  }
]
juampynr’s picture

Status: Needs work » Needs review
FileSize
1.49 KB
2.34 KB

Here I did the following changes:

* Now using $entity->uri() in order to obtain the URI of the entity istead of building it manually.
* Added $langcode to the returned structure (as other normalizers do).
* Emptied the denormalize method since this is just needed for Normalizers that support hal+json format.

juampynr’s picture

Priority: Normal » Major

Bumping priority up to major. This makes the REST API pretty useless when a piece of content has files attached to it since their URLs are not exposed.

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FileFieldItemNormalizer.php
    @@ -0,0 +1,57 @@
    +  protected $formats = array('json', 'xml');
    

    why is that in hal module and not in the serialization module?

  2. +++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FileFieldItemNormalizer.php
    @@ -0,0 +1,57 @@
    +      $values['lang'] = $context['langcode'];
    

    why do you need the language? That is already on the entity object? please add a comment.

But otherwise I think you are on the right track. We need a test case that covers this change.

juampynr’s picture

Moved to serialization module as suggested in #7. Also, removed an unneeded constructor setter and the language field since it is not needed.

Now writing the test.

jsbalsera’s picture

Assigned: Unassigned » jsbalsera
Issue tags: +#D8SVQ, +#SprintWeekend2014
gloob’s picture

Issue tags: -#D8SVQ, -#SprintWeekend2014 +D8SVQ, +SprintWeekend2014
mgifford’s picture

Assigned: jsbalsera » Unassigned
chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
2.02 KB
mikey_p’s picture

Since file fields extend entity reference, wouldn't it make sense for this to also support all entity reference items?

chr.fritsch’s picture

Maybe yes, but what about circular references? There have to be some logic to make a "cut"

Berdir’s picture

I don't see how we could have circular references here? we're just calling url(), like hal serialization is too, that could easily live on a more generic normalizer.

Also needs an issue summary update (the examples there are very old) and the issue title should clarify that this is about the json format.

damiankloip’s picture

Agree it would make sense to just support all entity reference fields with this. We can rely on ->url() as it comes from EntityInterface, along with every other method under the sun.

damiankloip’s picture

damiankloip’s picture

File rename not in diff due to pure laziness.

Status: Needs review » Needs work

The last submitted patch, 19: 2124677-19.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.87 KB
838 bytes

Maybe not assuming there will always be an entity is a good idea.

damiankloip’s picture

Issue tags: +Needs tests

damiankloip queued 21: 2124677-21.patch for re-testing.

damiankloip’s picture

FileSize
9.42 KB
7.54 KB

The EntitySerializationTest should have actually semi-failed already, we just weren't actually using a user in the test, so no 'url' was added from the normalizer.

Here is a modified EntitySerializationTest and a new EntityReferenceFieldItemNormalizerTest unit test.

Status: Needs review » Needs work

The last submitted patch, 24: 2124677-24.patch, failed testing.

The last submitted patch, 24: 2124677-24.patch, failed testing.

dawehner’s picture

+1 even using prophecy would be nicer for the tests.

damiankloip’s picture

Issue tags: -Needs tests
FileSize
9.44 KB
659 bytes

Yeah, prophecy is not a bad idea. None of the other tests use it but hey..

klausi’s picture

Status: Needs work » Needs review

The last submitted patch, 1: drupal-expose-file-uri-2124677-1.patch, failed testing.

The last submitted patch, 3: drupal-expose-file-uri-2124677-2.patch, failed testing.

The last submitted patch, 4: drupal-expose-file-uri-2124677-4.patch, failed testing.

The last submitted patch, 1: drupal-expose-file-uri-2124677-1.patch, failed testing.

The last submitted patch, 3: drupal-expose-file-uri-2124677-2.patch, failed testing.

The last submitted patch, 4: drupal-expose-file-uri-2124677-4.patch, failed testing.

The last submitted patch, 8: drupal-expose-file-uri-2124677-8.patch, failed testing.

The last submitted patch, 8: drupal-expose-file-uri-2124677-8.patch, failed testing.

damiankloip’s picture

FileSize
9.65 KB
1.23 KB

Fixing a the rel param value passed to url() so config entities don't get the edit-form URL put into the normalized data.

damiankloip’s picture

FileSize
9.35 KB
5.23 KB

Converted to prophecy, that was pretty fun!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Converted to prophecy, that was pretty fun!

Of course it is!

klausi’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
    @@ -0,0 +1,39 @@
    +  protected $supportedInterfaceOrClass = 'Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem';
    

    can we use ::class here?

  2. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/EntityReferenceFieldItemNormalizerTest.php
    @@ -0,0 +1,106 @@
    +    $this->fieldItem = $this->prophesize('Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem');
    

    can we use ::class here?

  3. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/EntityReferenceFieldItemNormalizerTest.php
    @@ -0,0 +1,106 @@
    +    $entity = $this->prophesize('Drupal\Core\Entity\EntityInterface');
    

    can we use ::class here?

  4. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/EntityReferenceFieldItemNormalizerTest.php
    @@ -0,0 +1,106 @@
    +    $entity_reference = $this->prophesize('Drupal\Core\TypedData\TypedDataInterface');
    

    and here

  5. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/EntityReferenceFieldItemNormalizerTest.php
    @@ -0,0 +1,106 @@
    +    $entity_reference = $this->prophesize('Drupal\Core\TypedData\TypedDataInterface');
    

    and here

  6. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/EntityReferenceFieldItemNormalizerTest.php
    @@ -0,0 +1,106 @@
    +    $this->fieldItem->get('entity')->willReturn($entity_reference->reveal())->shouldBeCalled();
    

    over 80 characters, can we split this up into multiple lines as we do elsewhere with chained method calls? Also elsewhere.

damiankloip’s picture

I can change that, 80 chars is not a limit on lines of code though?

klausi’s picture

Sure, but if you have 5 "->" operators on one line my head starts spinning.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
9.54 KB
4.21 KB

And yes, you certainly can use it with interface too! :)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/serialization/tests/src/Unit/Normalizer/EntityReferenceFieldItemNormalizerTest.php
@@ -69,13 +74,19 @@ public function testSupportsNormalization() {
+    $entity_reference = $this->prophesize(TypedDataInterface::class);
+    $entity_reference->getValue()
+      ->willReturn($entity->reveal())

Meh, actually I think its much more readable if input and output is in one line. At least I don't have to do that in other patches, yeah!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 44: 2124677-42.patch, failed testing.

Lucasljj queued 44: 2124677-42.patch for re-testing.

damiankloip’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 44: 2124677-42.patch, failed testing.

damiankloip queued 44: 2124677-42.patch for re-testing.

damiankloip’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ba447d7 and pushed to 8.0.x. Thanks!

  • alexpott committed ba447d7 on 8.0.x
    Issue #2124677 by damiankloip, juampynr, chr.fritsch, klausi: Expose URI...
Berdir’s picture

damiankloip’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Wim Leers’s picture