blocks are being exposed to all post types, even those that don't use Gutenberg

[jasonbahl]

NOTE: This was reported to me in Slack by @kidunot89, just passing it on here:

Might want to tell to Content Blocks team to adjust the way they are targeting CPTs, they are adding editorBlocks to all of them even the ones that don't use gutenberg.

[theodesp]

Hey @jasonbahl . Thank you we will take a look.

[theodesp]

This is strange because we do have a filter here:

https://github.com/wpengine/wp-graphql-content-blocks/blob/97bb00e3c3c2556808c248d9890d129bf8631ac0/includes/Registry/Registry.php#L170-L203

is there anything else we are missing?

[justlevine]

@theodesp I havnt tried to replicate the original issue but the following jumped out at me:

1. It's using post_type_supports(), instead of the filtered use_block_editor_for_post_type() ref
2. It's using the get_post_types(), instead of the filtered WPGraphQL::get_allowed_post_types() ref

If either woo or WooGraphQL use the underlying filters (or any other extension for that matter), that could explain the discrepancies.

[theodesp]

@justlevine maybe a combination of get_allowed_post_types and use_block_editor_for_post_type would make it more robust?

[jasonbahl]

@josephfusco this was closed but I don't see an associated PR or anything? Can you link to the PR that addresses this issue?

[josephfusco]

@jasonbahl Here is the Jira ticket tracking this issue. https://wpengine.atlassian.net/browse/MERL-786

[jasonbahl]

@josephfusco the JIRA ticket isn't public so folks tracking this issue like @justlevine have no context about what happened to resolve this.

I have access to the JIRA but even in the JIRA ticket there's no information about how this was resolved. 🤔 👀

[justlevine]

thanks for looking out @jasonbahl ! I'm sure many in the community would find closing comments helpful - especially when there is no publicly linked PR

[mindctrl]

The Jira ticket says "Works as intended", and it was implied internally that we could not reproduce this bug. I'm going to reopen so we can revisit and confirm.

[theodesp]

I have a PR using missing replacement function based on the comments above:

https://github.com/wpengine/wp-graphql-content-blocks/pull/120

This should cover this issue.

[justlevine]

I have a PR using missing replacement function based on the comments above:

120

This should cover this issue.

Just in case the context was lost between here and JIRA, my hunches were untested and only address 'best practices', so even after #120 is merged I'd recommend a quick check explicitly against WPGraphQL for WC (if it hasn't been tested against already) before closing this issue to make sure nothing else is being overlooked.

[jasonbahl]

If this does fix the original issue, it will be a breaking change to the Schema for any users that update

[jasonbahl]

I'd recommend a quick check explicitly against WPGraphQL for WC (if it hasn't been tested against already) before closing this issue

With ACF 6.1+ supporting Post Type registration we should test with that as well.

But more basic, we should at least have a test that registers a post type that doesn't have gutenberg enabled, and make sure the Schema doesn't add editorBlocks to that post type, then register a post type that does have gutenberg enabled and make sure that editorBlocks can be queried on that post type.

Shouldn't matter if it's ACF, WooCommerce or any other plugin that adds a custom post type, if the post type is set to show in graphql AND uses the block editor, then we should expose the blocks.

[mindctrl]

@jasonbahl @justlevine thank you both. I added some tasks to the PR as a reminder. If you think of anything else let me know.

[mindctrl]

Hey @justlevine, thanks for your help here. You had mentioned tracking activity via diffs and commit logs. I imagine this is partly due to our inconsistency and lack of detail in the change logs. Is that correct? We're putting together some things now to improve our disciplines here. We'll be updating our PR templates to add confirmations for change logs, and we'll be updating our review processes to ensure all changes are documented with sufficient information to describe the changes. We'll also be sure to leave comments when things are closed. Is there anything else we could do better that would help you avoid having to read diffs and commits?

[justlevine]

@mindctrl - when I realized how negatively my previous comment came out, I edited it as quickly as I could. Apologies, and I'm happy you took it in the spirit I intended.

I don't want to get too into the weeds on this comment (I'm happy to chat in depth over on Slack or Discord). In addition to documenting intentional changes. I also think it's important to get the CI robost enough to both prevent unintentional changes, and help indicate to the contributor when a change needs to be documented as breaking. A few examples:

• Linting the generated GraphQL schema.
• Adding PHPStan as a CI
• Using a stricter PHPCS ruleset that lints for more than just formatting.
• Disabling auto-updates for major versions and/or showing Plugin Dashboard warnings when a manual update will be a breaking one.

The WPGraphQL ecosystem is still fairly fragile, and usually this fragility is conveyed to the user by a combination of the 0.x versioning and gatekeeping users to GitHub. Since wp-graphql-content-blocks just dropped the former (my assumption being in preparation to drop the latter), ensuring the quality of releases (regressions are caught, breaking changes are intentional and conveyed to the user) is paramount. More-so when it's from a company as established in the space as WPEngine, since losing trust in ya'll would likely tank the entire ecosystem.

[mindctrl]

@justlevine excellent feedback! You'll be happy to know that we have most of those 4 bullet points in our backlog already and I'll be sure to get the rest covered.

re: disabling auto updates, we had a discussion about this yesterday and @blakewilson put together a nice POC last night and we'll be looking to that as a starting point for what we do in this plugin: https://github.com/blakewilson/semantic-versioning-plugin

We all really appreciate your time and thoughts here, and everywhere else for that matter. 🙌

[justlevine]

Thanks for the update @mindctrl , that's very encouraging to hear!

I started on a similar POC to help with the WPGraphQL-ACF migration, hoping to get it pushed to a public branch for review over the weekend. Will check out that repo for inspiration, thanks! (A few more, and we could have a solid enough candidate to make this a core WPGraphQL API extensions can use instead of having to roll their own 🤞)