Hello,

In the case of image fields and file fields, there are respectively alt, title text fields and description, display fields which are stored in the reference field value.

Is it possible currently to get the values of those kind of fields?

Thanks for any help.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Grimreaper created an issue. See original summary.

e0ipso’s picture

Status: Active » Postponed (maintainer needs more info)

I'm not sure I understand the question. Please be more specific. You should be able to see all the fields in the image entity via the image resource or using the includes.

Grimreaper’s picture

Hello @eOipso,

Thanks for the quick reply.

Yes, I see the fields in the image entity but the problem is that the alt text is not stored in the image entity but in the field value on the article.

I will attached screenshots of what I obtain.

The node:

The value of the field image in the database:

jsonapi/node/article:

jsonapi/node/article with include:

jsonapi/node/article/uuid/field_image:

jsonapi/node/article/uuid/relationships/field_image:

I don't see the alt text anywhere. Am I missing something?

Do you need more details?

Thanks fot you reply.

e0ipso’s picture

Status: Postponed (maintainer needs more info) » Active

Thanks for the details!

dawehner’s picture

I also ran into the problem. As of now there seems to be now way to fetch it.

Grimreaper’s picture

Hello,

Also for comment field type, there are some conf values in the "attributes" but nothing in the "relationships" to be able to get the comment.

"field_comment": {
  "status": "2",
  "cid": "3",
  "last_comment_timestamp": "1489764885",
  "last_comment_name": "",
  "last_comment_uid": "1",
  "comment_count": "3"
},

Also I wonder if the "cid" and the "last_comment_uid" should be converted into the UUID of the entities.

edwardaa’s picture

Besides the alt text, the image width and height values are also useful.

Grimreaper’s picture

Issue tags: +DevDaysSeville

Hello,

I discussed about this issue with @Wim Leers.

In a first time for image fields only, a strategy would be to look at the classes:
- jsonapi/src/Normalizer/EntityReferenceFieldNormalizer.php
- jsonapi/src/Normalizer/Relationship.php
- jsonapi/src/Normalizer/RelationshipItem.php

And to see how and if we need, to create sub-classes to add the extra information in the relationship.

@eOipso: Are you ok with this strategy?

e0ipso’s picture

That seems reasonable. There can be dedicated normalizers for this.

Grimreaper’s picture

Status: Active » Needs work
FileSize
7.06 KB
66.57 KB

Hello,

Here is a first attempt.

It adds the additionnal properties into a meta in the relationship value, as you can see on the attached screenshot.

It breaks the tests and I want some feedback before to continue working on this issue.

I worked with beram (https://www.drupal.org/u/beram).

Also, we put some TODO where we had interrogations?

Thanks for the review.

dawehner’s picture

  1. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
    @@ -85,17 +85,34 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
    +      // TODO: Add computed values?
    

    I think its fine to skip computed fields for now. Computed fields adds its own complexity.

  2. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
    @@ -85,17 +85,34 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
    +      // TODO: What about entity reference revisions?
    

    Conceptually I think that is its own complexity, maybe a submodule of ERR would be good on tp.

  3. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
    @@ -85,17 +85,34 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
    +      // TODO: Add access control on those properties?
    

    I guess you should check for \Drupal\Core\Access\AccessibleInterface for each property here, but core doesn't do it, nor there are properties which have that.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.02 KB

I started with some additional test coverage.

Status: Needs review » Needs work

The last submitted patch, 12: 2860886-test.patch, failed testing.

Grimreaper’s picture

Thanks @dawehner

Here is a quick review.

  1. +++ b/tests/src/Kernel/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php
    @@ -223,6 +251,7 @@ class JsonApiDocumentTopLevelNormalizerTest extends JsonapiKernelTestBase {
    +    print_r($normalized);
    

    Should be removed? :)

  2. +++ b/tests/src/Kernel/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php
    @@ -208,6 +230,12 @@ class JsonApiDocumentTopLevelNormalizerTest extends JsonapiKernelTestBase {
    +    ], $normalized['data']['attributes']['field_image']['data']['meta']);
    

    $normalized['data']['relationships']['field_image']['data']['meta'] instead?

dawehner’s picture

@Grimreaper
Oh yeah good point. I just wanted to put out some test ...

Grimreaper’s picture

Assigned: Unassigned » Grimreaper

@Dawehner no problem :)

I am currently using your test to test my previous patch. Trying to have as much green as possible.

Grimreaper’s picture

Here is the patch that fix @dawehner test and put it into green.

But I still get the failures from patch in comment 10.

It will need to investigate.

Grimreaper’s picture

Status: Needs work » Needs review
FileSize
11.68 KB

I forgot to change to needs review to trigger the test bot...

Status: Needs review » Needs work

The last submitted patch, 18: jsonapi-entity_reference_properties-2860886-17.patch, failed testing.

Grimreaper’s picture

Assigned: Unassigned » Grimreaper

I think I found a way to fix the tests.

I will upload a patch soon.

Grimreaper’s picture

Assigned: Grimreaper » Unassigned
Status: Needs work » Needs review
FileSize
11.74 KB
1 KB

Here is the patch.

Grimreaper’s picture

Tests are green \o/

Is it possible to have feedbacks to know if this approach will be merged?

After an image field, should we also test a file field?

e0ipso’s picture

Status: Needs review » Needs work

I like what I am seeing from a code perspective. A couple of comments:

  1. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
    @@ -85,17 +85,31 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
    +    $entity_reference_item_list = array_filter($field->getIterator()->getArrayCopy(), function ($item) {
    

    Can we use getReferencedEntities() instead?

  2. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
    @@ -85,17 +85,31 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
    +    $entity_list_metadata = array();
    

    Nit. Use [] instead of array(). (applies to other places as well)

  3. +++ b/src/Normalizer/RelationshipItem.php
    @@ -86,7 +96,11 @@ class RelationshipItem {
    -    return [$this->targetKey => $this->getTargetEntity()->uuid()];
    +    return [
    +      $this->targetKey => $this->getTargetEntity()->uuid(),
    +      'meta' => $this->metadata,
    +      'target_key' => $this->targetKey,
    +    ];
    
    +++ b/src/Normalizer/Value/RelationshipItemNormalizerValue.php
    @@ -39,10 +39,16 @@ class RelationshipItemNormalizerValue extends FieldItemNormalizerValue implement
    -    return [
    +    $rasterizedValue = [
           'type' => $this->resource->getTypeName(),
    -      'id' => $value,
    +      'id' => is_array($value) ? $value[$value['target_key']] : $value,
         ];
    

    What is the reasoning behind adding this? Would it make more sense to pass the UUID around under a static 'target_uuid' key that we can expect during "rasterization"?

e0ipso’s picture

However I am not convinced that companion properties to a relationship are metadata at all. They are first class content, and they should be treated as such.

I did a bunch of research as part of this issue and found some relevant threads.

I believe that ultimately it all boils down to an architectural problem derived from the 1-to-1 mapping of resource and bundles. If that was not a limitation we could consider doing:

{
  "data": {
    "type": "node--article",
    "id": "article-uuid",
    "attributes": {
      "title": "Foo!"
    },
    "relationships": {
      "field_image": {
        "data": {
          "type": "file--image-instance",
          "id": "some-generated-id"
        }
      }
    }
  },
  "included": [
    {
      "type": "file--image-instance",
      "id": "the-same-generated-id",
      "attributes": {
        "title": "Image title",
        "alt": "Image alt"
      },
      "relationships": {
        "file": {
          "data": {
            "type": "file",
            "id": "image-entity-uuid"
          }
        }
      }
    },
    {
      "type": "file",
      "id": "image-entity-uuid",
      "attributes": { … },
      "relationships": { … }
    }
  ]
}

As of now, that is not possible so I believe that meta on the relationship is the best we can do for now.

e0ipso’s picture

Also I wonder if the "cid" and the "last_comment_uid" should be converted into the UUID of the entities.

I don't think that is the case. If a module (even if it is core) wants to store some normalized data for performance reasons, we should not transform it in any way. We should only turn references into their UUID when they are declared as entity relationships explicitly.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.97 KB
2.55 KB

Can we use getReferencedEntities() instead?

Well, not really, because the entities don't contain the metadata we want here.

What is the reasoning behind adding this? Would it make more sense to pass the UUID around under a static 'target_uuid' key that we can expect during "rasterization"?

This is indeed a much nicer solution ...

Status: Needs review » Needs work

The last submitted patch, 26: 2860886-26.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
10.97 KB
644 bytes

One underscore too much.

e0ipso’s picture

Added some minor modifications. Merging if green.

  • e0ipso committed 3e40a1a on 8.x-1.x authored by dawehner
    feat(Add attributes on relationships to the meta (#2860886 by Grimreaper...
e0ipso’s picture

Status: Needs review » Fixed
Grimreaper’s picture

Hello,

Thanks @dawehner for completing the patch.

Thanks @e0ipso for the merge and the search in JSONAPI.

Status: Fixed » Closed (fixed)

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