Fix "Array" values in GraphQL block array data

[alecgeatches]

Description

See #53 for a description of the problem. In short, the code that maps block attributes into GraphQL types coerces the value property into a string:

vip-block-data-api/src/graphql/graphql-api.php

Lines 93 to 102 in f89ed3c

	$block['attributes'] = array_map(
	function ( $name, $value ) {
	return [
	'name' => $name,
	'value' => strval( $value ),
	];
	},
	array_keys( $block['attributes'] ),
	array_values( $block['attributes'] )
	);

This causes an 'Array' string to replace the actual value, which makes array data inaccessible through WPGraphQL. Additionally, this coercion produces a notice:

PHP Warning:  Array to string conversion in /.../vip-block-data-api/src/graphql/graphql-api.php on line 97

Why JSON?

The fix in this PR checks for arrays in the value property, and converts those to a JSON string representation. This was selected instead of per-block custom properties or union types.

array-source values can represent arbitrarily complex data which could require adding a new type for each block. For example, the head, body, and foot of a table follow a structured cell-based pattern:

{
    "name": "core/table",
    "attributes": {
        "hasFixedLayout": false,
        "head": [
            { "cells": [
                { "content": "Header A", "tag": "th" },
                { "content": "Header B", "tag": "th" }
            ] }
        ],
        "body": [
            { "cells": [
                { "content": "Value A", "tag": "td" },
                { "content": "Value B", "tag": "td" }
            ] },
            { "cells": [
                { "content": "Value C", "tag": "td" },
                { "content": "Value D", "tag": "td" }
            ] }
        ],
        "foot": [
            { "cells": [
                { "content": "Footer A", "tag": "td"},
                { "content": "Footer B", "tag": "td" }
            ] }
        ]
    }
}

That's because core/table uses a query with a known structure to fill those values.

However, other uses of array values, like the tracks attribute of core/video use a freeform structure:

{
    "name": "core/video",
    "attributes": {
        "src": "http://my.site/wp-content/uploads/my-video.mp4",
        "tracks": [
            {
                "src": "http://my.site/wp-content/uploads/track-en.vtt",
                "label": "track-en",
                "srcLang": "en"
            },
            {
                "src": "http://my.site/wp-content/uploads/track-fr.vtt",
                "label": "track-fr",
                "srcLang": "fr",
                "kind": "subtitles"
            }
        ]
    }
}

If we tried to cover known core array types with custom types, this still wouldn't solve the problem for custom blocks. JSON representation will allow the GraphQL backend to model arbitrarily complex nested types without needing to know their structure beforehand.

The primary downside is that consumers of the data will be required to know that the block contains JSON-encoded information for array-type values.

However, that doesn't change the existing model for querying. All attribute values are already treated as String type as a trade-off for convenience. Also, given that array values were completely clobbered into an "Array" string previously, this is an improvement.

Steps to Test

A test using core/table was added in test_array_data_in_attribute(). To run tests:

1. Check out PR.
2. Run wp-env start
3. Run composer install
4. Run composer run test

Fixes #53

[Zamfi99]

What do you think about using !is_scalar( $value ) instead?

[Zamfi99]

I'm thinking if it would make sense to specify the actual type of $value (which would be the best approach imo), or at least specifying that the content is encoded (we may run into performance issues by trying to decode every attribute in frontend)

[alecgeatches]

Thanks a ton for your input, @Zamfi99! We just pushed a change that adds a isValueJsonEncoded boolean to the attributes query. Here's an example with a table:

Query

query MyPostQuery {
  post(id: $ID, idType: DATABASE_ID) {
    blocksData {
      blocks {
        attributes {
          name
          value
          isValueJsonEncoded      " <-- New attribute "
        }
        id
        name
        innerBlocks {
          attributes {
            name
            value
            isValueJsonEncode     " <-- New attribute "
          }
          id
          name
          parentId
        }
      }
    }
  }
}

→ Result

{
  "data": {
    "post": {
      "blocksData": {
        "blocks": [
          {
            "attributes": [
              {
                "name": "hasFixedLayout",
                "value": "",
                "isValueJsonEncoded": false
              },
              {
                "name": "head",
                "value": "[{\"cells\":[{\"content\":\"Header A\",\"tag\":\"th\"},{\"content\":\"Header B\",\"tag\":\"th\"}]}]",
                "isValueJsonEncoded": true
              },
              {
                "name": "body",
                "value": "[{\"cells\":[{\"content\":\"Value A\",\"tag\":\"td\"},{\"content\":\"Value B\",\"tag\":\"td\"}]},{\"cells\":[{\"content\":\"Value C\",\"tag\":\"td\"},{\"content\":\"Value D\",\"tag\":\"td\"}]}]",
                "isValueJsonEncoded": true
              },
              {
                "name": "foot",
                "value": "[{\"cells\":[{\"content\":\"Footer A\",\"tag\":\"td\"},{\"content\":\"Footer B\",\"tag\":\"td\"}]}]",
                "isValueJsonEncoded": true
              }
            ],
            "id": "QmxvY2tEYXRhOjQ1NTox",
            "name": "core/table",
            "innerBlocks": null
          }
        ]
      }
    }
  }
}

We liked the idea of an opt-in isValueJsonEncoded attribute. This solves a couple of problems:

1. You can avoid decoding every attribute by querying for isValueJsonEncoded along with attribute name and value, and only do extra decoding work when this value is true.
2. Folks who don't use blocks with array values can continue to query for simple name/value pairs and can use this feature optionally.

We also chose not to add the attribute's type as a queryable option, because we're actively trying to keep the GraphQL querying options simple. I also think it feels confusing to not use a "real" GraphQL type and specify a type another attribute instead. This is the same trade-off we made for original attribute typing as strings. I think the isValueJsonEncoded attribute is a good middle ground for attributes that can't be easily represented as a simple string.

Let me know what you think! We're look to get this shipped out this week. Thank you for your input!

[alecgeatches]

@ingeniumed Would appreciate your review here as well, thank you!

[ingeniumed]

Forgot to mention that the plugin version should be bumped