Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add GraphQL API #51
Changes from 30 commits
b1ee4fa
5d9a6a6
a386950
bf73c1c
83bfd74
7e42577
cf0286d
61bee9d
4c00b0d
d810a0f
71dd8ae
d98425a
d5ab09e
778509d
75e992f
04237ed
145ea17
1df6165
830894b
e06b15a
7e5ae7e
33fc4dd
1dda9a7
df10512
5ad55f1
a07df2b
984f519
43560a8
9f03a0c
0e63fe4
47f2efb
17000db
3c6cdd0
5bb6030
e4acbf8
bd255c2
a25b630
272e92b
fac6ebf
d578fe4
8cdc1e2
90c77de
a64af6d
6e2c367
f89ed3c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment could be better (either here or below), to explain that it hooks into the
graphql_register_types
action, which only fires if WPGraphQL is installed and enabled, and is further controlled by thevip_block_data_api__is_graphql_enabled
filter.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made this change as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taken it out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
init
method is being called on load (at the end of this file, not hooked into WordPressinit
or another lifecycle hook). Therefore, plugin / theme code may not be able to add a filter before this is executed. I would move this inside theregister_types
method.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I have moved this into the
register_types
now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to add this filter if WPGraphQL isn't installed or active?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have taken it out now entirely.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can access on $model->fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had tried to do it that way, but no matter what I did the fields were null. That's why I did it this way. The plugin internally calls get_post already so the info is quickly returned.
Is there anyone special I need to do? I had called it the way u suggested 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized I summed up the same comment that's in the
blocks.php
within the decoupled bundle itself. That's really why it's been done this way as that same reasoning applies.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this is called via a filter from
ContentParser
. Why not just call it directly inget_blocks_data
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's 2 reasons why it was done that way:
So this way, while its odd, gives us the best of both worlds. With that said, I do think it can be done in a better way even if that means we go over the entire block list twice.
Talking it over with @alecgeatches, I'll change it up to:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched it over so the transformation is done after the parser comes back, so more filter weirdness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming is hard, but isn't this a
BlockAttribute
? BlockData is the collection?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yip, changed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good practice to designate fields as non-nullable whenever possible. That way the consumer can have confidence that a field will always have a value and can skip inspection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I have done this now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDs in WPGraphQL are always non-nullable and are typically of type
ID
meaning a Relay ID. Example. Create a Relay ID withRelay::toGlobalId
example.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I have done this now for the ID and the parentId.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review all these fields for non-nullability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I have done this now for all the fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I have done this now
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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
orinnerBlocksCount
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.
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 tographql_debug()
where it can be conditionally displayed in theerrors
payload. Better control for the site maintainer.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.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 forregister_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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
field name should be camel case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like you could just keep the arity at 4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yip, cleaned this up