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 GraphQL API #51

Merged
merged 45 commits into from
Dec 14, 2023
Merged
Show file tree
Hide file tree
Changes from 38 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
b1ee4fa
Add WP-GraphQL to Block Data API, import it on plugin startup and mak…
ingeniumed Oct 31, 2023
5d9a6a6
Add the skeleton layout of the graphQL endpoint
ingeniumed Oct 31, 2023
a386950
Add the parent_id and id to each block
ingeniumed Oct 31, 2023
bf73c1c
Transform the block attributes, and add an extra param to the sourced…
ingeniumed Nov 1, 2023
83bfd74
Require the new graphQL api file
ingeniumed Nov 1, 2023
7e42577
Set the minimum version of the plugin to be 8.0
ingeniumed Nov 1, 2023
cf0286d
Remove the graphQL plugin from the plugin itself.
ingeniumed Nov 8, 2023
61bee9d
Remove the lib folder from the PHPCS
ingeniumed Nov 13, 2023
4c00b0d
Attempting to flatten the nested blocks
ingeniumed Nov 13, 2023
d810a0f
Flattened the innerblocks, but still have a duplicate showing up
ingeniumed Nov 13, 2023
71dd8ae
Fixed the bug with duplicates in the child list
ingeniumed Nov 14, 2023
d98425a
Downgrade php version to 7.4
ingeniumed Nov 14, 2023
d5ab09e
Add some php docs and fix linting problems
ingeniumed Nov 14, 2023
778509d
Tweak todo comments
ingeniumed Nov 14, 2023
75e992f
Remove the new filter and instead just add a new arg to the existing one
ingeniumed Nov 16, 2023
04237ed
Move the id/parentId logic to the graphQL logic instead of the conten…
ingeniumed Nov 16, 2023
145ea17
Fix linting errors
ingeniumed Nov 16, 2023
1df6165
Fix unknown error
ingeniumed Nov 28, 2023
830894b
Fix unknown error
ingeniumed Nov 28, 2023
e06b15a
Fix unknown error
ingeniumed Nov 28, 2023
7e5ae7e
Fix unknown error
ingeniumed Nov 28, 2023
33fc4dd
Fix unknown error
ingeniumed Nov 28, 2023
1dda9a7
add tests and clean up the code
ingeniumed Nov 30, 2023
df10512
Fix the linting problems
ingeniumed Nov 30, 2023
5ad55f1
Add more comments
ingeniumed Nov 30, 2023
a07df2b
Add docs for graphql and add analytics
ingeniumed Dec 1, 2023
984f519
tweak the table
ingeniumed Dec 1, 2023
43560a8
Update README.md
ingeniumed Dec 4, 2023
9f03a0c
Add the utility function
ingeniumed Dec 4, 2023
0e63fe4
Add the utility function
ingeniumed Dec 4, 2023
47f2efb
Re-write the graphQL generation separate from a filter
ingeniumed Dec 7, 2023
17000db
fix failures in tests
ingeniumed Dec 7, 2023
3c6cdd0
fix failures in tests
ingeniumed Dec 7, 2023
5bb6030
fix failures in tests
ingeniumed Dec 7, 2023
e4acbf8
fix failures in tests
ingeniumed Dec 7, 2023
bd255c2
Add a mock for a graphQL specific function
ingeniumed Dec 7, 2023
a25b630
fix linting
ingeniumed Dec 7, 2023
272e92b
Add a flatten option for the innerblocks
ingeniumed Dec 8, 2023
fac6ebf
Address README feedback, standardize code spacing, update TOC
alecgeatches Dec 11, 2023
d578fe4
Add minor spacing and documentation changes
alecgeatches Dec 11, 2023
8cdc1e2
Explicitly strval block attribute values
alecgeatches Dec 12, 2023
90c77de
Add note about mocked integer ID values in README
alecgeatches Dec 12, 2023
a64af6d
add a mention of the flatten paramter
ingeniumed Dec 13, 2023
6e2c367
Incorporate post ID into GraphQL relay IDs to prevent cache collisions
alecgeatches Dec 13, 2023
f89ed3c
Fix linting errors, make Relay mock return more deterministic numbers
alecgeatches Dec 13, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
358 changes: 325 additions & 33 deletions README.md
smithjw1 marked this conversation as resolved.
Show resolved Hide resolved

Large diffs are not rendered by default.

1 change: 0 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
"automattic/vipwpcs": "^3.0",
"yoast/phpunit-polyfills": "^2.0",
"dms/phpunit-arraysubset-asserts": "^0.5.0"

},
"config": {
"allow-plugins": {
Expand Down
2 changes: 1 addition & 1 deletion phpcs.xml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
<rule ref="WordPress-Extra"/>
<!-- For help in understanding these custom sniff properties:
https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties -->
<config name="minimum_supported_wp_version" value="5.8"/>
<config name="minimum_supported_wp_version" value="5.9"/>

<rule ref="WordPress-VIP-Go">
<!-- These disallow anonymous functions as action callbacks -->
Expand Down
255 changes: 255 additions & 0 deletions src/graphql/graphql-api.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,255 @@
<?php
/**
* GraphQL API
*
* @package vip-block-data-api
*/

namespace WPCOMVIP\BlockDataApi;

use GraphQLRelay\Relay;

defined( 'ABSPATH' ) || die();

/**
* GraphQL API to offer an alternative to the REST API.
*/
class GraphQLApi {
/**
* Initiatilize the graphQL API by hooking into the graphql_register_types action,
* which only fires if WPGraphQL is installed and enabled, and is further controlled
* by the vip_block_data_api__is_graphql_enabled filter.
*/
public static function init() {
add_action( 'graphql_register_types', [ __CLASS__, 'register_types' ] );
}

/**
* Extract the blocks data for a post, and return back in the format expected by the graphQL API.
*
* @param \WPGraphQL\Model\Post $post_model Post model for post.
*
* @return array
*/
public static function get_blocks_data( $post_model ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a PHP version policy for this plugin? Can you use type declarations?

Suggested change
public static function get_blocks_data( $post_model ) {
public static function get_blocks_data( \WPGraphQL\Model\Post $post_model ) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have set it at 7.4, but will be upping it to 8.0. At that point we were gonna go in and set the type declarations. That is on the cards, so I haven't done that here.

That said, I do have to figure out when I do how to pass in post model of type \WPGraphQL\Model\Post when we dont bundle in WP-GraphQL with the plugins. The tests fail due to it, I realized.

I'll leave this one as is for this PR

$post_id = $post_model->ID;
$post = get_post( $post_id );

$content_parser = new ContentParser();

$parser_results = $content_parser->parse( $post->post_content, $post_id );

// We need to not return a WP_Error object, and so a regular exception is returned.
if ( is_wp_error( $parser_results ) ) {
Analytics::record_error( $parser_results );

// Return API-safe error with extra data (e.g. stack trace) removed.
return new \Exception( $parser_results->get_error_message() );
chriszarate marked this conversation as resolved.
Show resolved Hide resolved
}

$parser_results['blocks'] = array_map(function ( $block ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there no phpcs on this repo?

Suggested change
$parser_results['blocks'] = array_map(function ( $block ) {
$parser_results['blocks'] = array_map( function ( $block ) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, weird! We do have phpcs configured, and it seems to spot other problems in this file just fine, but it's not enforcing parentheses spacing. Our phpcs configuration is a close relation to vip-go-mu-plugins's config which appears to be roughly the same. Looking into what sniff might be missing here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured it out. I noticed if I add a missing space a couple of lines up, PHPCS will report it:

$block['id'] = Relay::toGlobalId('ID', wp_unique_id() );
//                               ^ missing space
FILE: src/graphql/graphql-api.php
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
 73 | ERROR | [x] Expected 1 spaces after opening parenthesis; 0 found
    |       |     (PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket)

However, that same PEAR.Functions.FunctionCallSignature.SpaceAfterOpenBracket sniff doesn't kick in for array_map(). After looking at our configuration, I noticed we have some sniffs disabled related to anonymous functions and function calls. If I remove those, we'll now get multiple errors for the multi-line function call:

 72 | ERROR | [x] Opening parenthesis of a multi-line function call must be the last content on the line
    |       |     (PEAR.Functions.FunctionCallSignature.ContentAfterOpenBracket)
 74 | ERROR | [x] Only one argument is allowed per line in a multi-line function call (PEAR.Functions.FunctionCallSignature.MultipleArguments)
 74 | ERROR | [x] Closing parenthesis of a multi-line function call must be on a line by itself (PEAR.Functions.FunctionCallSignature.CloseBracketLine)

So, in short, if we want to disable those sniffs to use some standard PHP array_map( function( ... ) { practices, we also lose the sniff that tells us about weird spacing after array_map().

Fixed manually.

return self::transform_block_format( $block );
}, $parser_results['blocks'] );

return $parser_results;
}

/**
* Transform the block's format to the format expected by the graphQL API.
*
* @param array $block An associative array of parsed block data with keys 'name' and 'attribute'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for type declarations instead of hand-written descriptions

Suggested change
* @param array $block An associative array of parsed block data with keys 'name' and 'attribute'.
* @param array $block An associative array of parsed block data with keys 'name' and 'attributes'.

*
* @return array
*/
public static function transform_block_format( $block ) {
// Generate a unique ID for the block.
$block['id'] = Relay::toGlobalId( 'ID', wp_unique_id() );
Copy link
Member

@chriszarate chriszarate Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first argument to this function should the type name of the object in question, so BlockData in this case. This is what ensures uniqueness across the graph. Also, since wp_unique_id just increments integers, you should probably also incorporate the post ID. Otherwise, you risk collisions in client caches which will be really hard to diagnose. (I've seen this happen and it's bewildering when a paragraph from post 2 shows up in post 1).

Suggested change
$block['id'] = Relay::toGlobalId( 'ID', wp_unique_id() );
$block['id'] = Relay::toGlobalId( 'BlockData', sprintf( "%d:%d", $post->ID, wp_unique_id() ) );

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for clarifying this @chriszarate! Changed in 6e2c367.


// Convert the attributes to be in the name-value format that the schema expects.
$block = self::map_attributes( $block );

if ( isset( $block['innerBlocks'] ) && ! empty( $block['innerBlocks'] ) ) {
$block['innerBlocks'] = array_map( function ( $block ) {
return self::transform_block_format( $block );
}, $block['innerBlocks'] );
}

return $block;
}

/**
* Convert the attributes to be in the name-value format that the schema expects.
*
* @param array $block An associative array of parsed block data with keys 'name' and 'attribute'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param array $block An associative array of parsed block data with keys 'name' and 'attribute'.
* @param array $block An associative array of parsed block data with keys 'name' and 'attributes'.

*
* @return array
*/
public static function map_attributes( $block ) {
// check if type of attributes is stdClass and unset it as that's not supported by graphQL.
if ( isset( $block['attributes'] ) && is_object( $block['attributes'] ) ) {
unset( $block['attributes'] );
Comment on lines +89 to +91
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how / why would this happen?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ingeniumed Can you explain a bit more about the stdClass comment here? I'm not sure where that comes from either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is because in the contentParser we set the actual attributes to be (object)[] which allows it to be given back as empty json in the rest api rather than as an empty array. Due to that, the internal data type comes back as stdClass rather than an array. This workaround is due to that.

I experimented with switching the (object)[] to a new ArrayObject but in that case it adds an empty array with key storage under the attributes which requires an even worse workaround. That's why it's been done this way.

I tried to search for a better solution but couldn't quite find one and I didn't want to break the rest response or the way the public function works because there are customers already using it.

This is the line I'm referencing btw in the contentParser.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully that explains it properly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ingeniumed That sounds familiar now! As a side-effect of returning data in JSON via the REST API, if we want to return attributes: {} in a response, we need to cast [] into an object to avoid returning an array instead. This is just dealing with that unusual data in the GraphQL layer.

} elseif ( isset( $block['attributes'] ) && ! empty( $block['attributes'] ) ) {
$block['attributes'] = array_map(
function ( $name, $value ) {
return [
'name' => $name,
'value' => $value,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess WPGraphQL is doing the casting to string? You might want to do it here in code just to be explicit and test it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess WPGraphQL is doing the casting to string?

I think you're right! I tested with a block delimiter value like {"test": 123}, which is returned as an integer from the parser and REST API. However, once we return it in a BlockAttribute, it is converted to a string. Added explicit conversion in 8cdc1e2 anyway.

];
},
array_keys( $block['attributes'] ),
array_values( $block['attributes'] )
);
}

return $block;
}

/**
* Flatten the inner blocks, no matter how many levels of nesting is there.
*
* @param array $inner_blocks the inner blocks in the block.
* @param string $parent_id ID of the parent block, that the inner blocks belong to.
* @param array $flattened_blocks the flattened blocks that's built up as we go through the inner blocks.
*
* @return array
*/
public static function flatten_inner_blocks( $inner_blocks, $parent_id, $flattened_blocks = [] ) {
foreach ( $inner_blocks as $inner_block ) {
// Set the parentId to be the ID of the parent block whose inner blocks are being flattened.
$inner_block['parentId'] = $parent_id;

// This block doesnt have any inner blocks, so just add it to the flattened blocks. Ensure the parentId is set.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indenting is off here

if ( ! isset( $inner_block['innerBlocks'] ) ) {
array_push( $flattened_blocks, $inner_block );
// This block is has inner blocks, so go through the inner blocks recursively.
} else {
$inner_blocks_copy = $inner_block['innerBlocks'];
unset( $inner_block['innerBlocks'] );
// first add the current block to the flattened blocks, and then go through the inner blocks recursively.
array_push( $flattened_blocks, $inner_block );
$flattened_blocks = self::flatten_inner_blocks( $inner_blocks_copy, $inner_block['id'], $flattened_blocks );
}
}

return $flattened_blocks;
}

/**
* Register types and fields graphql integration.
*
* @return void
*/
public static function register_types() {
/**
* Filter to enable/disable the graphQL API. By default, it is enabled.
*
* @param bool $is_graphql_to_be_enabled Whether the graphQL API should be enabled or not.
*/
$is_graphql_to_be_enabled = apply_filters( 'vip_block_data_api__is_graphql_enabled', true );

if ( ! $is_graphql_to_be_enabled ) {
return;
}

// Register the type corresponding to the attributes of each individual block.
register_graphql_object_type(
'BlockAttribute',
[
'description' => 'Block attribute',
'fields' => [
'name' => [
'type' => [ 'non_null' => 'String' ],
'description' => 'Block data attribute name',
],
'value' => [
'type' => [ 'non_null' => 'String' ],
'description' => 'Block data attribute value',
],
],
],
);

// Register the type corresponding to the individual block, with the above attribute.
register_graphql_type(
'BlockData',
chriszarate marked this conversation as resolved.
Show resolved Hide resolved
[
'description' => 'Block data',
ingeniumed marked this conversation as resolved.
Show resolved Hide resolved
'fields' => [
'id' => [
'type' => [ 'non_null' => 'ID' ],
'description' => 'ID of the block',
],
'parentId' => [
'type' => 'ID',
'description' => 'ID of the parent for this inner block, if it is an inner block. Otherwise, it will not be set. If it set, this will match the ID of the block',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last part confusing / redundant

Suggested change
'description' => 'ID of the parent for this inner block, if it is an inner block. Otherwise, it will not be set. If it set, this will match the ID of the block',
'description' => 'ID of the parent for this inner block, if it is an inner block. Otherwise, it will be null.',

],
'name' => [
'type' => [ 'non_null' => 'String' ],
'description' => 'Block name',
],
'attributes' => [
'type' => [
'list_of' => 'BlockAttribute',
],
'description' => 'Block attributes',
],
'innerBlocks' => [
'type' => [ 'list_of' => 'BlockData' ],
'description' => 'Flattened list of inner blocks of this block',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get why these are flattened, but is the "reparenting" going to be annoying / error-prone? Is the nesting issue severe enough to warrant this approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core problem we wanted to solve was that, you should be able to get back the complicated block hierarchy in a post without having any knowledge of the depth involved. This gets us past that, and to ensure that the re-parenting isn't annoying we added a little utility function in the README that could be used. Admittedly that function could be simplified further, but I kept it simple enough to follow. By ensuring that each block gets an id and if applicable, a parent id the likelihood of an error should be minimal (one would hope, but obviously things can go wrong).

The other approach would be to flatten the innerBlocks alongside all the blocks but that proved to be even more complicated (in terms of code and the outcome) and wouldn't necessarily match the structure of a parent-child in a post with Gutenberg blocks.

So with that in mind, this was the ultimate solution that was picked to allow graphQL to be added on top of the block data api without the innerBlocks being a problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I totally get the why, just wondering how big a deal this actually is in practice:

you should be able to get back the complicated block hierarchy in a post without having any knowledge of the depth involved

If you use fragments in your query, adding another layer isn't a huge deal. And since you are reconstructing the deep tree on the client side, your block parsing code isn't less complex.

Yes, if you add another level beyond what you are querying for, it simply won't be in the response. But adding a hasInnerBlocks or innerBlocksCount property could allow the queryer to determine if they are "missing out" on blocks.

Another drawback of this approach is that it makes human inspection of the response data much less friendly, possibly impossible. Human debugging is useful and common, especially with the built-in GraphiQL client.

Again, totally understand where this is coming from, just want to make sure the devex has been fully considered.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an idea: If you use the same type for both inner and outer blocks, you could add a GraphQL field directive that allows the queryer to choose whether to flatten or not. If would look like this:

query FlattenBlocks {
   post(id: "abc123=") {
      blockData(flatten: true) {
         # ...
      }
   }
}

query NestedBlocks {
   post(id: "abc123=") {
      blockData(flatten: false) { # or omit the directive
         # ...
      }
   }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the research specifically mentioned flattening the hierarchy and using parent/child relationships. No one balked at it.

I'm not trying to suggest that flattening is the right choice. I think optionality is good, as are Chris' points.

My take is we have options that customers will accept, and we can choose from among them, which is a great place to be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to the re-write I did of the way the parser's result is transformed into the right graphQL schema, I was able to add in this flatten option. So by default it's set to true because we want the inner blocks to be flattened. Omitting it sets it to true by default. If you set it to false, it will not flatten the inner blocks, and send back the original hierarchy.

So best of both options are available, but we give back the flattened hierarchy by default as thats the best option we want to be used.

Copy link
Member

@chriszarate chriszarate Dec 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! FYI it is not mentioned in the README

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, added it in under usage!

],
],
],
);

// Register the type corresponding to the list of individual blocks, with each item being the above type.
register_graphql_type(
'BlocksData',
[
'description' => 'Data for all the blocks',
'fields' => [
'blocks' => [
'type' => [ 'list_of' => 'BlockData' ],
'description' => 'List of blocks data',
'args' => [
'flatten' => [
'type' => 'Boolean',
'description' => 'Collate the inner blocks under each root block into a single list with a parent-child relationship. This is set to true by default, and setting it to false will reverse that to preserve the original block hierarchy, at the expense of knowing the exact depth when querying the inner blocks. Default: true',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'description' => 'Collate the inner blocks under each root block into a single list with a parent-child relationship. This is set to true by default, and setting it to false will reverse that to preserve the original block hierarchy, at the expense of knowing the exact depth when querying the inner blocks. Default: true',
'description' => 'Collate the inner blocks under each root block into a single list with a parent-child relationship. This is set to true by default, and setting it to false will preserve the original block hierarchy, but will require nested inner block queries to the desired depth. Default: true',

],
],
'resolve' => function ( $blocks, $args ) {
if ( ! isset( $args['flatten'] ) || true === $args['unflatten'] ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug, probably needs tests

Suggested change
if ( ! isset( $args['flatten'] ) || true === $args['unflatten'] ) {
if ( ! isset( $args['flatten'] ) || false === $args['flatten'] ) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I fixed this, tested it manually, and put it in d578fe4. I spent some time trying to add a test, but after my attempt I think @ingeniumed has already done a decent job trying to wrap the core logic from the resolver with a test:

'resolve' => function ( $blocks, $args ) {
if ( ! isset( $args['flatten'] ) || true === $args['flatten'] ) {
$blocks['blocks'] = array_map( function ( $block ) {
// Flatten the inner blocks, if any.
if ( isset( $block['innerBlocks'] ) ) {
$block['innerBlocks'] = self::flatten_inner_blocks( $block['innerBlocks'], $block['id'] );
}
return $block;
}, $blocks['blocks'] );
}
return $blocks['blocks'];
},

Our unit testing does not include the WPGraphQL plugin, so the logic that's in this resolver isn't directly tested in test_flatten_inner_blocks(). The bug here is in the thin shell of WPGraphQL code around the flatten_inner_blocks() function, which we test directly. Ideally we could have some end-to-end tests that include the request through WPGraphQL (like we do for our REST tests), but I think the testing here is a good enough start. As long as we catch the interface bugs, like this.

$blocks['blocks'] = array_map(function ( $block ) {
// Flatten the inner blocks, if any.
if ( isset( $block['innerBlocks'] ) ) {
$block['innerBlocks'] = self::flatten_inner_blocks( $block['innerBlocks'], $block['id'] );
}

return $block;
}, $blocks['blocks'] );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just do this, no?

Suggested change
$blocks['blocks'] = array_map(function ( $block ) {
// Flatten the inner blocks, if any.
if ( isset( $block['innerBlocks'] ) ) {
$block['innerBlocks'] = self::flatten_inner_blocks( $block['innerBlocks'], $block['id'] );
}
return $block;
}, $blocks['blocks'] );
// Flatten the inner blocks, if any.
$blocks['blocks'] = self::flatten_inner_blocks( $blocks['blocks'], null );

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This replacement doesn't work as-is, because flatten_inner_blocks() needs the $parent_id parameter, which is passed to innerBlocks to determine their parent IDs:

public static function flatten_inner_blocks( $inner_blocks, $parent_id, $flattened_blocks = [] ) {
foreach ( $inner_blocks as $inner_block ) {
// Set the parentId to be the ID of the parent block whose inner blocks are being flattened.
$inner_block['parentId'] = $parent_id;

I think this is due to the design that top level blocks are treated differently as "root" blocks, but their children are flattened beneath them all together. I'm sure it's possible to rewrite flatten_inner_blocks() so this is done internally, but I think this works fine.

Copy link
Contributor

@alecgeatches alecgeatches Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait a minute, I'm rereading this and I'm not sure why it's not working anymore. Let me take a second look.

}

return $blocks['blocks'];
},
],
'warnings' => [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning / error data typically doesn't ship in the data payload, but instead goes to graphql_debug() where it can be conditionally displayed in the errors payload. Better control for the site maintainer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still working on this, ran into some problems that are of my own making.

'type' => [ 'list_of' => 'String' ],
'description' => 'List of warnings related to processing the blocks data',
],
],
],
);

// Register the field on every post type that supports 'editor'.
register_graphql_field(
'NodeWithContentEditor',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you are following the pattern of the decoupled bundle here, but let's discuss the pros and cons of adding the field to this union.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had looked up this page as well as this one to see what it could support. From what I gathered this would be what we support - pages and posts that would be made using the content editor. We have a check with the contentParser that already checks to see if the provided post has blocks within it or not, so that should account for the classic editor.

That was my thinking, alongside this being used in the decoupled bundle already. Was there something else or a downside that I might have missed in using this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I was conflating this with querying for contentNodes, which is a bit more problematic. I think this is fine.

this would be what we support - pages and posts that would be made using the content editor.

The NodeWithContentEditor interface picks up any post type that supports the content editor regardless of its purpose or visibility. It's worth noting that the default value for register_post_type#supports includes 'editor' (and also many developers and plugin authors don't pay close attention to post type args), so in reality the field will probably get added to a bunch of random plugin-created post types where it might not make sense. But it's fine since the user is unlikely to expose them in GraphQL or query for them. Just FYI.

'blocksData',
[
'type' => 'BlocksData',
'description' => 'A block representation of post content',
'resolve' => [ __CLASS__, 'get_blocks_data' ],
]
);
}
}

GraphQLApi::init();
10 changes: 4 additions & 6 deletions src/parser/block-additions/core-image.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,20 @@ class CoreImage {
* @access private
*/
public static function init() {
add_filter( 'vip_block_data_api__sourced_block_result', [ __CLASS__, 'add_image_metadata' ], 5, 4 );
add_filter( 'vip_block_data_api__sourced_block_result', [ __CLASS__, 'add_image_metadata' ], 5, 2 );
}

/**
* Add size metadata to core/image blocks
*
* @param array $sourced_block Sourced block result.
* @param string $block_name Name of the block.
* @param int|null $post_id Id of the post.
* @param array $block Block being parsed.
* @param array $sourced_block Sourced block result.
* @param string $block_name Name of the block.
*
* @access private
*
* @return array Updated sourced block with new metadata information
*/
public static function add_image_metadata( $sourced_block, $block_name, $post_id, $block ) { // phpcs:disable Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastUsed
public static function add_image_metadata( $sourced_block, $block_name ) {
if ( 'core/image' !== $block_name ) {
return $sourced_block;
}
Expand Down
2 changes: 2 additions & 0 deletions src/parser/content-parser.php
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ public function should_block_be_included( $block, $block_name, $filter_options )
* @return array|WP_Error
*/
public function parse( $post_content, $post_id = null, $filter_options = [] ) {
Analytics::record_usage();

if ( isset( $filter_options['exclude'] ) && isset( $filter_options['include'] ) ) {
return new WP_Error( 'vip-block-data-api-invalid-params', 'Cannot provide blocks to exclude and include at the same time', [ 'status' => 400 ] );
}
Expand Down
2 changes: 0 additions & 2 deletions src/rest/rest-api.php
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,6 @@ public static function get_block_content( $params ) {
$post_id = $params['id'];
$post = get_post( $post_id );

Analytics::record_usage();
chriszarate marked this conversation as resolved.
Show resolved Hide resolved

$parse_time_start = microtime( true );

$content_parser = new ContentParser();
Expand Down
2 changes: 2 additions & 0 deletions tests/bootstrap.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,5 @@ function _manually_load_plugin() {

// Add custom test classes
require_once __DIR__ . '/registry-test-case.php';

require_once __DIR__ . '/mocks/graphql-relay-mock.php';
Loading
Loading