Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add block data to REST API #2649

Open
wants to merge 69 commits into
base: trunk
Choose a base branch
from

Conversation

@adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Sep 2, 2017

This pull request adds Gutenberg block data to the post endpoints

This provides a way to get the data and content of each block of a Gutenberg-built page from the front end. This would be very useful for building component based front ends, because the components could map one-to-one with gutenberg blocks. With these endpoints, an App could easily get the data it needs to render each component. This might also provide a patch for a standard component library matching Gutenberg blocks.

@adamsilverstein adamsilverstein requested a review from rmccue Sep 2, 2017
@BE-Webdesign
Copy link
Contributor

@BE-Webdesign BE-Webdesign commented Sep 2, 2017

#2503 is similar. They are addressing two different problems but, it would be good to have coordination between this and #2503 .

@adamsilverstein
Copy link
Member Author

@adamsilverstein adamsilverstein commented Sep 2, 2017

@BE-Webdesign thanks for pointing out #2503 - I searched for a similar PR and couldn't find one.

As you pointed out, these PRs are pretty different; from a brief view it looks like #2503 is an API for (reusable) block objects themselves, while this PR is focused on exposing a (gutenberg built) post's blocks (and their data)

Copy link
Contributor

@rmccue rmccue left a comment

I think rather than creating separate endpoints, we should build this into the existing endpoints instead. In particular .content on the post type is intentionally an object to allow adding extra properties.

(I'll follow this up in a minute; GH's review comment UI isn't great for leaving long comments.)

lib/class-wp-rest-gutenberg-blocks.php Outdated Show resolved Hide resolved
lib/class-wp-rest-gutenberg-blocks.php Outdated Show resolved Hide resolved
lib/class-wp-rest-gutenberg-blocks.php Outdated Show resolved Hide resolved
lib/class-wp-rest-gutenberg-blocks.php Outdated Show resolved Hide resolved
@rmccue
Copy link
Contributor

@rmccue rmccue commented Sep 4, 2017

In the post object, content contains the various representations of the same piece of data: the post content. As I understand it, with Gutenberg, the blocks are simply another representation of that same piece of data. (We also intentionally designed content for this purpose; although Gutenberg wasn't in sight at the time, other page builders were, and making content an object gave us more extensibility there.)

It also seems like (maybe) Gutenberg distinguishes between rendered and raw block content, based on this PR. (I could be wrong there.) Given that, I'd probably add a new sub-field under content called blocks, which could also contain its own raw and rendered fields (based on the provided context).

Something like this:

{
    "content": {
        "raw": "<!-- gutenberg comments --> interspersed with HTML",
        "rendered": "<div class='block-foo'><p>bar</p></div><p>interspersed with HTML</p>",
        "blocks": [
            {
                "type": "foo",
                "attributes": {
                    "value": "bar"
                },
                "rendered": "<p>bar</p>"
            },
            {
                "type": "html",
                "attributes": {
                    "content": "interspersed with HTML"
                },
                "rendered": "<p>interspersed with HTML</p>"
            }
        ]
    }
}

(Disclaimer: I only know the data model in very broad strokes here.)

@noisysocks
Copy link
Member

@noisysocks noisysocks commented Sep 4, 2017

As you pointed out, these PRs are pretty different; from a brief view it looks like #2503 is an API for (reusable) block objects themselves, while this PR is focused on exposing a (gutenberg built) post's blocks (and their data)

While the two endpoints address different use cases, it'd be highly useful to have both endpoints represent a block in a consistent way, e.g.

{
    "type": "core/paragraph",
    "attributes": {
        "content": "How are you?",
        "dropCap": true
    },
    "content": "<!-- wp:core/paragraph {\"dropCap\":true} -->\n<p class=\"has-drop-cap\">How are you?</p>\n<!-- /wp:core/paragraph -->",
    "rendered": "<p class=\"has-drop-cap\">How are you?</p>"
}

In the post object, content contains the various representations of the same piece of data: the post content. As I understand it, with Gutenberg, the blocks are simply another representation of that same piece of data.

+1

@adamsilverstein
Copy link
Member Author

@adamsilverstein adamsilverstein commented Sep 4, 2017

While the two endpoints address different use cases, it'd be highly useful to have both endpoints represent a block in a consistent way.

Good point @noisysocks, I was simply passing the data back as provided by gutenberg_parse_blocks(). looks like I need to transform the naming anyway so I'll try to match the block object format from #2503.

Thanks for the feedback @rmccue - I like your suggestion of adding this data onto the regular post endpoint content object - my only concern would be the overhead introduced in block parsing the post content, even if it hasn't been edited in gutenberg, seems like we need a performat has_been_edited_in_gutenberg() helper, if there is one I couldn't find it.

@adamsilverstein
Copy link
Member Author

@adamsilverstein adamsilverstein commented Sep 4, 2017

I remapped the response fields to match #2503 and also moved the data to the post object response as content->blocks.

Here is what the response from the demo page provided by my install of gutenberg looks like:

https://gist.github.com/adamsilverstein/5369160faca6097c5e22e0256e380962

@adamsilverstein
Copy link
Member Author

@adamsilverstein adamsilverstein commented Sep 4, 2017

@noisysocks & @rmccue can you please take another look after my recent changes where I have addressed your suggestions.

@rmccue
Copy link
Contributor

@rmccue rmccue commented Sep 5, 2017

my only concern would be the overhead introduced in block parsing the post content, even if it hasn't been edited in gutenberg

Won't this point become moot at the point where Gutenberg is the default editor anyway? Also, we could choose to only expose all the block pieces for context=edit if it's a frontend concern.

lib/blocks.php Outdated

// Set up the item data.
$item_data = array();
$item_data['type'] = $block['blockName'];
Copy link
Contributor

@rmccue rmccue Sep 5, 2017

This can be $block_name instead

Copy link
Member Author

@adamsilverstein adamsilverstein Sep 7, 2017

fixed in 5937d1a

lib/blocks.php Outdated
$block_name = isset( $block['blockName'] ) ? $block['blockName'] : null;
$attributes = is_array( $block['attrs'] ) ? $block['attrs'] : null;
$raw_content = isset( $block['rawContent'] ) ? $block['rawContent'] : null;
if ( null !== $block_name ) {
Copy link
Contributor

@rmccue rmccue Sep 5, 2017

This should be inverted with a continue instead:

if ( null === $block_name ) {
    continue;
}

Copy link
Member Author

@adamsilverstein adamsilverstein Sep 7, 2017

fixed in d990979

lib/blocks.php Outdated
$item_data = array();
$item_data['type'] = $block['blockName'];
if ( null !== $attributes ) {
$item_data['attributes'] = $block['attrs'];
Copy link
Contributor

@rmccue rmccue Sep 5, 2017

This should be $attributes not $block['attrs']. Also, properties should always be set if they're exposed in a given context to avoid index errors.

Copy link
Member Author

@adamsilverstein adamsilverstein Sep 7, 2017

fixed in 862bccc

lib/blocks.php Outdated
if ( null !== $attributes ) {
$item_data['attributes'] = $block['attrs'];
}
$item_data['content'] = $block['rawContent'];
Copy link
Contributor

@rmccue rmccue Sep 5, 2017

$block['rawContent'] -> $raw_content

Also, should this only be exposed for $context === 'edit'?

Copy link
Member Author

@adamsilverstein adamsilverstein Sep 7, 2017

This is perhaps misnamed when it is returned from the parser - its not really the raw content and doesn't include the html markers for example. This really represents the rendered html and is returned as content for a block (matching naming from #2503). As far as I can see, rendered is only used for a couple of block types where their content is dynamic, for example a recent post list.

Copy link
Member Author

@adamsilverstein adamsilverstein Sep 7, 2017

Can you review the data thats output, it seems public? we could show blocks only in edit context, but that would make my idea of using them for a front end data source less useful.

lib/blocks.php Outdated
*
* @return array Array of block data.
*/
function get_block_data_for_api_from_post_content( $content ) {
Copy link
Contributor

@rmccue rmccue Sep 5, 2017

Maybe gutenberg_add_blocks_to_post_resource()? This won't be used apart from the filter, so should probably be a prefixed name rather than a convenient one.

Copy link
Member Author

@adamsilverstein adamsilverstein Sep 7, 2017

fixed in 36f1000

lib/blocks.php Show resolved Hide resolved
lib/blocks.php Outdated
$blocks = get_block_data_for_api_from_post_content( $post->post_content );
if ( $blocks ) {
$response->data['content']['blocks'] = $blocks;
}
Copy link
Contributor

@rmccue rmccue Sep 5, 2017

The blocks field should always be set, even if it's null.

Also, this should probably use ->set_data() instead of reaching into the object.

Copy link
Member Author

@adamsilverstein adamsilverstein Sep 7, 2017

Addressed in 957dc7e & bc5f31e

@joemcgill
Copy link
Member

@joemcgill joemcgill commented Sep 6, 2017

+1 for this being a part of the regular content endpoint(s) and glad to see that #1516 was mentioned.

Architecturally, we should be thinking about more than just default post content here too since the blocks concept will likely be used in other scenarios (i.e. widgets and menus) in the future.

@adamsilverstein
Copy link
Member Author

@adamsilverstein adamsilverstein commented Apr 11, 2019

I refreshed this PR against master and updated the parse method to use WP_Block_Parser. I will work on a matching issue.

@adamsilverstein
Copy link
Member Author

@adamsilverstein adamsilverstein commented Apr 11, 2019

I realized that wp.blocks.parse provides the same data as long as the user has access to raw content, so adding this to the REST API seems excessive. Closing.

@mtias
Copy link
Contributor

@mtias mtias commented Oct 7, 2020

@adamsilverstein I think there's still value in an endpoint returning the WP_Block_Parser response, even if it's not as granular as wp.blocks.parse!

@lkraav
Copy link

@lkraav lkraav commented Oct 7, 2020

@adamsilverstein I think there's still value in an endpoint returning the WP_Block_Parser response, even if it's not as granular as wp.blocks.parse!

I browsed through this PR comment list, but @mtias could you clarify for "what's the difference"?

@mtias
Copy link
Contributor

@mtias mtias commented Oct 9, 2020

Sourced attributes (like the content of a paragraph) rely on a DOM tree, which is not easily accessed server side.

@adamsilverstein
Copy link
Member Author

@adamsilverstein adamsilverstein commented Nov 5, 2020

@adamsilverstein I think there's still value in an endpoint returning the WP_Block_Parser response, even if it's not as granular as wp.blocks.parse!

Happy to re-open and rebase the PR. Seemed useful to me as well, so happy to see this considered.

@TimothyBJacobs
Copy link
Member

@TimothyBJacobs TimothyBJacobs commented Nov 5, 2020

Is there a way to ensure the content is only parsed once with parse_blocks() instead of separately for when calling do_blocks() on the rendered content and then again when building this representation.

@adamsilverstein
Copy link
Member Author

@adamsilverstein adamsilverstein commented Nov 5, 2020

Removed unrelated changed and cleaned up return logic. Rest API data looks like this (from the default Gutenberg post):

image

@adamsilverstein
Copy link
Member Author

@adamsilverstein adamsilverstein commented Nov 5, 2020

Is there a way to ensure the content is only parsed once with parse_blocks() instead of separately for when calling do_blocks() on the rendered content and then again when building this representation.

@TimothyBJacobs Can you clarify where else parse_blocks would normally already be called for a REST API request? Is it already called by core in the post route for the rendered content?

@noisysocks
Copy link
Member

@noisysocks noisysocks commented Nov 5, 2020

Could you make the attribute names match the property names in WP_Block? In other words, rename blockNamename and attrsattributes. It's regrettable that parse_block() uses this non-consistent naming 🙂

How do you feel about moving blocks to content.blocks?

@adamsilverstein
Copy link
Member Author

@adamsilverstein adamsilverstein commented Nov 5, 2020

Could you make the attribute names match the property names in WP_Block? In other words, rename blockName → name and attrs → attributes. It's regrettable that parse_block() uses this non-consistent naming 🙂

I can remap the names if that is helpful: can you point me to the full list of names in WP_Block?

How do you feel about moving blocks to content.blocks?

It is already there, my screenshot was incomplete, here is a full capture.

@noisysocks
Copy link
Member

@noisysocks noisysocks commented Nov 5, 2020

I can remap the names if that is helpful: can you point me to the full list of names in WP_Block?

Sure. There's only a few public properties:

  • attributes (computed dynamically using __get)
  • parsed_block (not relevant)
  • name
  • block_type (not relevant)
  • context (not relevant)
  • inner_blocks
  • inner_html
  • inner_content

https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/class-wp-block.php

@TimothyBJacobs
Copy link
Member

@TimothyBJacobs TimothyBJacobs commented Nov 6, 2020

Is there a way to ensure the content is only parsed once with parse_blocks() instead of separately for when calling do_blocks() on the rendered content and then again when building this representation.

@TimothyBJacobs Can you clarify where else parse_blocks would normally already be called for a REST API request? Is it already called by core in the post route for the rendered content?

As a result of do_blocks as a result of apply_filters( 'the_content' )

@adamsilverstein
Copy link
Member Author

@adamsilverstein adamsilverstein commented Nov 6, 2020

@noisysocks One potential issue with trying to transform these on the fly is that the structure is nested, so we would need to recurse through each inner_block section to rename keys there, eg

image

Thinking about this more and your statement that "It's regrettable that parse_block() uses this non-consistent naming" I am wondering why these keys are the way they are? Trying to transform them on the fly seems tricky.

@adamsilverstein
Copy link
Member Author

@adamsilverstein adamsilverstein commented Nov 6, 2020

As a result of do_blocks as a result of apply_filters( 'the_content' )

@TimothyBJacobs - Ah, I see that now, I guess that is required to get the rendered content? The only approach I can imaging here would be to store the parsed block data in a global, then if available the endpoint can use that data directly instead of recalculating it. Any other suggestions for how we might make this work?

@TimothyBJacobs
Copy link
Member

@TimothyBJacobs TimothyBJacobs commented Nov 6, 2020

As a result of do_blocks as a result of apply_filters( 'the_content' )

@TimothyBJacobs - Ah, I see that now, I guess that is required to get the rendered content? The only approach I can imaging here would be to store the parsed block data in a global, then if available the endpoint can use that data directly instead of recalculating it. Any other suggestions for how we might make this work?

Yeah. I was thinking parse_blocks() would essentially memoize. Maybe we better leave that for a separate ticket.

@noisysocks
Copy link
Member

@noisysocks noisysocks commented Nov 8, 2020

@noisysocks One potential issue with trying to transform these on the fly is that the structure is nested, so we would need to recurse through each inner_block section to rename keys there, eg

WP_Block already does something like this in its constructor. You might be able to reuse that.

$blocks = array();
foreach ( parse_blocks( $content ) as $parsed_block ) {
	$blocks[] = new WP_Block( $parsed_block, array() );
}

@skorasaurus skorasaurus mentioned this pull request Dec 14, 2020
@mtias mtias mentioned this pull request Dec 24, 2020
16 tasks
Base automatically changed from master to trunk Mar 1, 2021
@gziolo
Copy link
Member

@gziolo gziolo commented Jul 6, 2021

I filed two tickets in WordPress core hoping we can move forward efforts for this POC:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet