Add support for Gutenberg inner blocks

[chriszarate]

Add support for Gutenberg inner blocks in contentBlocks output. This required a little refactoring to make the process function callable recursively.

A classic use case is columns:

{
  "name": "core/columns",
  "tagName": null,
  "innerHTML": null,
  "outerHTML": null,
  "attributes": [],
  "innerBlocks": [
    {
      "name": "core/column",
      "tagName": "div",
      "innerHTML": "",
      "outerHTML": "<div class=\"wp-block-column\"></div>",
      "attributes": [],
      "innerBlocks": [
        {
          "name": "core/paragraph",
          "tagName": "p",
          "innerHTML": "<strong>column 1</strong>",
          "outerHTML": "<p><strong>column 1</strong></p>",
          "attributes": []
        }
      ]
    },
    {
      "name": "core/column",
      "tagName": "div",
      "innerHTML": "",
      "outerHTML": "<div class=\"wp-block-column\"></div>",
      "attributes": [],
      "innerBlocks": [
        {
          "name": "core/paragraph",
          "tagName": "p",
          "innerHTML": "<em>column 2</em>",
          "outerHTML": "<p><em>column 2</em></p>",
          "attributes": []
        }
      ]
    }
  ]
}

Note that fetching nested inner blocks requires an explicit query to a fixed depth:

fragment ContentBlockParts {
  name
  tagName
  innerHTML
  outerHTML
  attributes {
    name
    value
  }
}

query MyQuery {
  posts {
    nodes {
      title
      contentBlocks {
        blocks {
          ...ContentBlockParts
          innerBlocks {
            ...ContentBlockParts
            innerBlocks {
              ...ContentBlockParts
            }
          }
        }
      }
    }
  }
}

[elazzabi]

Tested locally and looks good! Left a small question, but otherwise looks good to go 🎉

[elazzabi]

I think this is a good way of versioning the API-response and improving communication between developers on both spectrums (WP and Node).

But I believe we don't have a way to make two versions co-exist. Breaking changes to GraphQL APIs will always be so frustrating 😭

[chriszarate]

This change is purely additive (it only adds innerBlocks and doesn't modify or remove any existing fields), so luckily this isn't a breaking schema change since it won't break any existing queries.

For future breaking schema changes, I agree we need a strategy. WPGraphQL (or GraphQL for that matter) doesn't have any mechanisms for navigating breaking schema changes. WPGraphQL more or less relies on semver and a CHANGELOG to communicate changes.

Generally speaking, I think a good strategy is:

• When adding new types or modifying existing types, always introduce new fields.
• Don't remove or change the type of existing fields. You can mark them as @deprecated to communicate deprecation, but allow them to span the release window and later remove them after you're sure consumers have migrated to the new fields.

Interestingly, I think we do have the opportunity to help with upstream breaking schema changes with WPGraphQL and, by extension, graphql-php. Since we conditionally include WPGraphQL in code, we could ship multiple versions of the plugin and decide at runtime which one to load. In theory, we could inspect the request for a special query parameter or header and use it to determine which plugin version to load (X-WPGraphQL-Version: 2.2.0).

[joemcgill]

Overall, this is working great!

The need to manually define the recursion level is less than ideal, but understandably a current limitation of the GraphQL spec until something like @recursion is added (see: graphql/graphql-spec#237), unless you would want to preemptively implement that syntax.

Left a couple of comments, but would love to see this be available. Nice work!

[joemcgill]

I like that you're trying to include attributes that are saved in post content, and not parsed via the PHP parser here, but this seems a bit clunky to me to only enhance the image attributes and not others. Instead, maybe add a filter here so that additional attributes can be added to any block based on the block name and move the image attributes to that callback.

Ultimately, this is one of the annoying shortcomings of the PHP parser in WP, that it doesn't know how to parse out the attributes that are stored in markup and not on the block delimiters.

[chriszarate]

Filter added in bb07461

Ultimately, this is one of the annoying shortcomings of the PHP parser in WP, that it doesn't know how to parse out the attributes that are stored in markup and not on the block delimiters.

Sighs deeply 😭

[joemcgill]

I'm curious about the decision here to change the properties from what parse_blocks() implements out of the box? As a WP developer, I would expect the innerHTML property of a block to be equivalent to what the block parser returns. But here, it looks like GraphQL would return just the markup found inside the wrapping element and return the equivalent as outerHTML.

I like the intent of trying to return the wrapping tag name and inner markup separately, but worry that this would be confusing as it's inconsistent with what other implementations of block data would call innerHTML.

Additionally, parse_blocks() returns an innerContent property which can be useful to include in responses so a custom component can reconstruct the shape of a block that mixes innerBlocks and innerHTML.

See: https://github.com/WordPress/WordPress/blob/5.8-branch/wp-includes/class-wp-block-parser.php#L56-L68

[chriszarate]

I added innerContent in 2eefb78. Great suggestion.

As a WP developer, I would expect the innerHTML property of a block to be equivalent to what the block parser returns. But here, it looks like GraphQL would return just the markup found inside the wrapping element and return the equivalent as outerHTML.

@joemcgill The reason we made this choice is that it matches how this data is often consumed by component-driven front-ends. Keeping the wrapping tag in the innerHTML string makes it impossible to do something like this:

function Paragraph ( props ) {
	return <p dangerouslySetInnerHTML={ { __html: props.block.innerHTML }} />;
}

I know your point is that it's confusing to call it innerHTML when it differs from what the Gutenberg parser calls innerHTML, but this aligns with how React and other frameworks think about "innerHTML." Of course, we could call it something else here like componentHtml, but we'd lose that alignment. I also don't know how many people are familiar enough with the output of parse_blocks for this to be genuinely confusing.

But you raise a good enough point that I am a bit conflicted. Any further thoughts?

[joemcgill]

I also don't know how many people are familiar enough with the output of parse_blocks for this to be genuinely confusing.

I'm equally conflicted about this, because if I were not expecting the GraphQL response to mimic the data for blocks as it is modeled inside WordPress, I would want innerHTML and outerHTML to work exactly as you are proposing here. However, I think WordPress is assuming that innerHTML references all of the HTML that is contained inside the block delimiters, and not just what is inside the outermost wrapping element. For that matter, I think it's technically possible with v2 of the blocks API for there to be multiple elements inside a block's innerHTML, like this for example:

<!-- example:my-block -->
<div>
  <p>Some text</p>
</div>
<div>
  <p>Some other text</p>
</div>
<!-- /example:my-block -->

If that's true (I've not verified), then returning all of the content between the delimiters as innerHTML makes the most sense to me.

The new WP_REST_Block_Renderer_Controller that will be included in WP 5.9 takes the approach of modeling block content based on the parse_blocks data (see reference), so there's a case to be made that the block parser modeling is the defacto standard for how developers will interact with block data in various contexts.

[chriszarate]

Hey @joemcgill! Thanks for the conversation and pointers here. The idea that the output of parse_blocks is the de facto schema for block data under the hood is pretty exciting and convincing enough for me. I've made a few changes in f560e03 as a result:

1. Deprecate outerHTML (still available, just marked as deprecated).
2. By default, do not attempt to strip the wrapping tag from innerHTML.
3. Introduce a boolean removeWrappingTag field directive to innerHTML (default: false) that allows queries to request the removal of the wrapping tag if they find it useful:

fragment ContentBlockParts {
  name
  tagName
  innerHTML(removeWrappingTag: true)
  attributes {
    name
    value
  }
}

If requested and it's not possible to remove the wrapping tag (i.e., because of the multiple element scenario you posed), then the field will return null.

Interested if this feels more aligned to you.

[joemcgill]

Thanks @chriszarate. This new approach does seem more aligned to me.

The only problem I'm seeing now when testing is that the innerHTML and outerHTML are both returning null right now. Here's the query I'm testing in the GraphiQL IDE and the result I'm seeing.

Query:

{
  pages {
    nodes {
      contentBlocks {
        blocks {
          attributes {
            name
            value
          }
          innerContent
          innerHTML
          name
          outerHTML
          tagName
        }
      }
    }
  }
}

Result:

{
  "data": {
    "pages": {
      "nodes": [
        {
          "contentBlocks": {
            "blocks": [
              {
                "attributes": [],
                "innerContent": [
                  "\n<p>This is an example page. It's different from a blog post because it will stay in one place and will show up in your site navigation (in most themes). Most people start with an About page that introduces them to potential site visitors. It might say something like this:</p>\n"
                ],
                "innerHTML": null,
                "name": "core/paragraph",
                "outerHTML": null,
                "tagName": "p"
              }
            ]
          }
        }
      ]
    }
  }
}

If I update the query to pass innerHTML(removeWrappingTag: true) then I get the expected result of the innerHTML with the wrapping p tags stripped. Explicitly passing false also returns null.

[chriszarate]

@joemcgill Sorry about that, pushed up a two-liner (1b49d40 and 638456d) to fix that. Will also be adding tests, soon. :)

[joemcgill]

Yup. That did it 👍🏻. This looks really great. Thanks for all the effort you've put into it.