Entity Field Fetch should return a value via JSON:API

Changing this to a bug. This is a place where EFF is out-of-compliance with what Drupal expects a field to be able to do.

Adding a note that we will want to make sure the grabbing of the data to put in the jsonapi still respects the established caching of the content as it is controlled now.

In comparing GraphQL results for a fetch paragraph, the problem becomes more visible:

GraphQL results for a fetched paragraph:

          "fieldCcAboveTopOfPage": {
            "fetched": {
              "fieldWysiwyg": [
                {
                  "value": "<h2>Questions about copay balance</h2>\r\n\r\n<p>For questions about the copay balance of your VA health care bill, call us toll free at the number below. You won't need to pay any copays for X-rays, lab tests, preventative tests, and services like health screenings or immunizations.</p>\r\n",
                  "format": "rich_text",
                  "processed": "<h2>Questions about copay balance</h2>\n\n<p>For questions about the copay balance of your VA health care bill, call us toll free at the number below. You won't need to pay any copays for X-rays, lab tests, preventative tests, and services like health screenings or immunizations.</p>"
                }
              ]
            },
            "fetchedBundle": "wysiwyg",
            "targetField": null,
            "targetId": "109546",
            "targetType": "paragraph",
            "value": false
          }
        },

JSON API

"field_cc_top_of_page_content": false,

I believe the JSON API "false" is coming from the value property, and the other properties are not included. The false is present because it is a computed field, and the rest of the properties are computed.

It might be tied to this #3055163: Computed field properties do not appear in JSON:API responses unless their definition explicitly calls setInternal(FALSE) →

I think I found the solution in this article
https://gorannikolovski.com/blog/add-computed-field-jsonapi-response

In the data properties, there is no class to call for the computed properties
https://git.drupalcode.org/project/entity_field_fetch/-/blob/1.0.x/src/P...

I think the solution will involve wiring up the existing Fetcher class that handles all the heavy lifting to be called from maybe a shell class(es) in these property definitions. The trick will be in making it keep state across the properties so that the fetch does not happen four times for each fetched field, because currently the fetcher builds all the properties in one shot.

Digging at this a bit more I think we don't have to worry about multiple calls to the Fetcher, because the only property that needs the heavy lifting of the Fetcher is the "fetched" property

  $properties['target_type'] = DataDefinition::create('string')...
  $properties['target_id'] = DataDefinition::create('string') ...
  $properties['target_field'] = DataDefinition::create('string')
  $properties['fetched_bundle'] = DataDefinition::create('string')...
  // Each of these just needs a little simple logic to determine the property value. They are currently handled by the Fetcher, but could be moved to their own classes if needed.

  $properties['fetched'] = DataDefinition::create('map') ...
  // This is the one that needs the getEntityData() call from the Fetcher.  It is the thing that does the heavy lifting. 

swirt → credited gnikolovski → .

Crediting @gnikolovski for his article that showed me what was missing.

It is possible that moving the computed field setting to classes on the property definition will also resolve this bug 🐛 Node save required to get newly added fetched field to show up Active as it may remove the need for the existence of setting them in the call to setValue() https://git.drupalcode.org/project/entity_field_fetch/-/blame/1.0.x/src/...

This might not be a final solution, but I did a bit of digging on the blog article and connected Drupal issues.

Adding a class to the field property definition, like "->setClass(MyComputedField::class)" in the blog article, didn’t work for me, and I kept getting errors on the code trying to get the property definitions correctly.

Then I saw the note about "->setInternal(FALSE)" and the code around that. The patch in https://www.drupal.org/project/jsonapi/issues/3055163 → got me onto the "$properties['value']" definition in the EntityFieldFetchItem's "propertyDefinitions".

I updated the type to "DataDefinition::create('any')"...cause that's what I do in TypeScript all day ;), but also to test this out without proper typing getting in the way. Then in "setValues" I added "$values['value'] = $values['fetched'];". Loading the JSON:API response gave me the field info I was expecting!

So, I think the fields have "FALSE" in JSON:API responses since the value is set to a boolean and the default is always false. I also tried setting the value property data type to a string with a default of "foo" to see that in the JSON:API response. And the field output was "foo".

I'll leave this here for comment as to whether this direction makes sense to pursue and create an actual patch or MR.

I had some questions that I jotted down and tried to answer after a discussion with @swirt.

1. JSON:API schema - With "value" as a boolean is that what the schema output says in Open API docs?

This comes out correctly as "any" in the schema, and the title and description can be updated accordingly.

2. Check the database record for an EFF with the updated change to value. Does all the content get saved?

Changing the property definition for "value" to "any" type and setting its value as the fetched data caused the value to be "1" instead of "0" in my testing. So, it is still saved as a boolean, but I think any data presented is regarded as TRUE.

3. Do any errors appear in logs if you save a node with an EFF on it?

I did not see any errors in the watchdog logs or added to the edit screen.

---

So I think this solution looks promising enough to put up a MR, and I will work on that now. I still have the following questions to answer.

1. GraphQL field - Does that function the same after updating the value property?
2. How to add a test to prove this works? Maybe start with GraphQL and JSON:API tests since that's the way these fields are being used currently in my project.
3. I tried this on a site where the module is already installed. I think a test can prove this works on a new site, but we could also manually test.

Note: afinnarn and I met on zoom for a bit where we tossed questions and concerns around. I did not ignore his comment with questions above.

There was much discussion about whether to put all the fetched content into the 'value' property or not. I opted not to for the following reasons:

1. Given the nature and variety of what is fetched, it did not seem right to return any variety of things with a call to getValue().
2. It seemed like a potential breaking change to take 'value' that has always been a boolean false and suddenly start putting arrays into it.
3. We could let Drupal know that "fetched" is the main thing by setting it in mainPropertyName(), so I opted to do that.

afinnarn → changed the visibility of the branch 3300098-entity-field-fetch to hidden.

I checked out the MR and tested it on an existing site and a new Drupal 10.2 install. All the testing steps passed, but I didn't test the GraphQL part on the new Drupal site I created since I've never set that up. The GraphQL output worked on my existing site. Also on the new Drupal 10.2 install the fetched field still saved the db value as "0".

I think the MR looks good to me. I'll put some comments below but not anything to prevent merging.

1. I've seen "setClass('Drupal\entity_field_fetch\PropertyTargetType')" done more like "setClass(PropertyTargetType::class)" with adding an addiitonal use statement. I'm unsure if there is a Drupal coding standard preference on this.
2. I wonder if the value property is needed if the "mainPropertyName" is now changed to "fetched". Just curious on this one...
3. Should the new TpyedData-related classes live next to the FieldType definition instead of at the src root?

afinnarn → changed the visibility of the branch 3300098-entity-field-fetch to active.

Those were great suggestions @afinnarn. I moved the property classes into their own directory but not inside the plugin. Sometimes there are discovery things built into Plugins so I wanted to keep that directory clean with only plugins in them
I also switched everything over to call the classes by Use and name.

I could not remove the `value` though. If needed we can handle that with a separate issue.

This has been merged and will go out with 1.0.5 shortly.

Thank you @timcosgrove for reporting this.
Thank you @gnikolovski for writing a great article that helped identify the problem
Huge thank you to @afinnarn for working on this with me and continually pushing to make this solution the "right solution"

Released as 1.0.5 →