Post Content Block: Disable when editing content #24006
Conversation
ockham
added
[Feature] Full Site Editing
[Block] Post Content
|
Size Change: +65 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
| if ( in_array( $post->post_type, array( 'wp_template', 'wp_template_part' ), true ) ) { | ||
| return $allowed_block_types; | ||
| } | ||
| return array_diff( $allowed_block_types, array( 'core/post-content' ) ); |
There was a problem hiding this comment.
I think this doesn't work because the $allowed_block_types arg that's passed to this filter isn't an array but true (meaning "enable all blocks") 😢
There was a problem hiding this comment.
For the time being, we'll probably address this on the client side, see e.g. https://developer.wordpress.org/block-editor/developers/filters/block-filters/#using-a-deny-list.
For the long term, I've filed WordPress/wordpress-develop#419.
| return array_diff( $allowed_block_types, array( 'core/post-content' ) ); | ||
| } | ||
|
|
||
| add_filter( 'allowed_block_types', 'disallow_core_post_content_block_outside_templates', 10, 2 ); |
There was a problem hiding this comment.
We might need to move this line into an action that's called upon init.
Edit: No, that's not it. The filter does seem to be called 👍
There was a problem hiding this comment.
Note that the filter doesn't seem to be called for the Site Editor -- I think it's only applied through Core's edit-form-blocks.php. If we want to run it for the Site Editor, we'll need to add it to edit-site-page.php.
|
Just looking further (I may be echoing something I wrote in slack). It looks like (at least for Seedlet-blocks) that the Post Content block added to the Index template is actually added by the Query Loop block. So we never actually have a need to insert the 'Post Content' block ourselves in this case. I am wondering if we should restrict the ability to insert the Post Content block altogether, and only allow it to be inserted by blocks that are specifically designed to add it in a proper context such as Query Loop. 🤔 As @noahtallen points out, the Singular template doesn't use any special block for this though and relies on the post content block. So maybe we do... 🤷♀️ |
|
I wonder if some combination of allowed blocks (https://developer.wordpress.org/block-editor/tutorials/block-tutorial/nested-blocks-inner-blocks/#allowed-blocks) on the That still wouldn't solve the problem in the normal post editor though. :/ |
ockham
requested review from
ajitbohra,
Soean,
talldan and
youknowriad
as code owners
| [ 'wp_template', 'wp_template_part' ].includes( | ||
| postType | ||
| ) | ||
| ? postContent |
There was a problem hiding this comment.
should we exclude more site blocks, like postAuthor or postComments, etc?
There was a problem hiding this comment.
🤔 The main rationale for this PR was to prevent blockception (infinite recursion), which wouldn't happen with those blocks. But yeah, we might want to exclude the class of FSE relevant post... blocks from the content editor. They used to be kind of handy for testing, but maybe that's no longer needed now that we can do that in the Site Editor.
There was a problem hiding this comment.
Please no, or at least not yet! 😄
The site editor is unreliable when it comes to loading post attributes into FSE blocks, while it's very easy to test post blocks in the post editor.
I agree that we should eventually limit FSE blocks to the Site Editor, but let's do it once we make it work appropriately.
|
I had another idea how we might remove the block on the server side, I'll give that another quick shot. |
No, that's a dead end for now 😕 (see #24114). |
|
A snapshot test is currently failing ( |
|
The failing test loops over all blocks (or rather a serialized HTML version of each of them), parses them, and compares the result to a JSON file for each. Experimental core blocks are loaded through gutenberg/test/integration/full-content/full-content.test.js Lines 75 to 77 in 1a16a0f The problem is that as of this PR, this won't load the Post Content block, which results in the above error. A quick fix would be to add diff --git a/test/integration/full-content/full-content.test.js b/test/integration/full-content/full-content.test.js
index 008239bd61..7151f7fcc5 100644
--- a/test/integration/full-content/full-content.test.js
+++ b/test/integration/full-content/full-content.test.js
@@ -73,7 +73,9 @@ describe( 'full post content fixture', () => {
require( '../../../packages/editor/src/hooks' );
registerCoreBlocks();
if ( process.env.GUTENBERG_PHASE === 2 ) {
- __experimentalRegisterExperimentalCoreBlocks( settings );
+ __experimentalRegisterExperimentalCoreBlocks( settings, {
+ postType: 'wp_template',
+ } );
}
} );
but that would lock the post context for the test to the (Run |
|
Maybe we should exempt the Post Content block from these test 🤔 There's a bit of a precedent for the oembed and blocks, but that's a different part of the test -- they're not exempted from loading and parsing: gutenberg/test/integration/full-content/full-content.test.js Lines 216 to 223 in 1a16a0f |
|
FWIW, those tests are documented at https://github.com/WordPress/gutenberg/blob/1a16a0fe76586889b338eaf805c4a13bec2359a3/packages/e2e-tests/fixtures/blocks/README.md |
|
Decided to go with the 'quick fix' since it seems okay here (52e2057). |
|
Weird, the test is still failing in CI, even though it's passing locally for me. I wonder if it's some sort of cache that persists across CI runs, but I don't see anything obvious in gutenberg/.github/workflows/unit-test.yml Lines 47 to 48 in 1e6d80c |
This reverts commit fb6bf8f.
ockham
force-pushed
the
remove/post-content-block-from-post-editor
branch
from
52e2057 to
43e4a90
Compare
|
Okay, this should be ready for another look! |
There was a problem hiding this comment.
This works well for me. Post Content is not available in the post and page editors as expected. If there was some way for a post or page to have been saved with a post content block, instead of crashing it will just show this in place of the block:
It's not the most appropriate message for that case, as it could still be available for the site in the site editor. But practically speaking it would be a rare case to come across this.
| @@ -58,7 +58,9 @@ export function initialize( id, settings ) { | |||
|
|
|||
| registerCoreBlocks(); | |||
| if ( process.env.GUTENBERG_PHASE === 2 ) { | |||
| __experimentalRegisterExperimentalCoreBlocks( settings ); | |||
| __experimentalRegisterExperimentalCoreBlocks( settings, { | |||
| postType: 'wp_template', | |||
There was a problem hiding this comment.
I feel like this could be a little confusing. postType makes a lot of sense in edit-post, but thinking of this for edit-site it seems a bit more funky. Is the post type always wp_template in edit-site? Even when we are selecting specific pages or posts? If we are going to hardcode a value here maybe it should just be something like 'edit_site'.
Perhaps something like editorType, editorEntity, or some better name I cant think of atm instead of postType.
So for edit post we would pass in { editorType: postType }.
For site editor { editorType: 'site_editor' }, and add that as a 3rd valid option in the array checked in the ternary?
Or maybe just a comment here explaining why we are going with wp_template as the hardcoded value, and some block(s) wont be registered if this isn't passed as wp_template or wp_template_part. 🤷♀️
There was a problem hiding this comment.
I feel like this could be a little confusing.
postTypemakes a lot of sense in edit-post, but thinking of this for edit-site it seems a bit more funky. Is the post type alwayswp_templatein edit-site? Even when we are selecting specific pages or posts? If we are going to hardcode a value here maybe it should just be something like 'edit_site'.
Yeah, it will be wp_template in those cases, too -- it's still the wp_template CPT we're editing. (However, it can also be wp_template_part, if we're editing a template part.)
A few factors come into play here. One is that we're only setting postType upon initialization -- we don't re-initialize upon switching between templates or template parts. We know that the Site Editor initially always shows a template (and not a template part) -- the home page template -- so we hard-code to wp_template. It just so happens that template parts allow for the same block types (and disallow Post Content).
Perhaps something like
editorType,editorEntity, or some better name I cant think of atm instead ofpostType.
So for edit post we would pass in{ editorType: postType }.
For site editor{ editorType: 'site_editor' }, and add that as a 3rd valid option in the array checked in the ternary?
Yep, which brings us to the question whether postType is the right argument to base these allow/deny lists on anyway. The reason for choosing postType (but really post context -- the second argument to __experimentalRegisterExperimentalCoreBlocks is an object that would also allow for additional fields such as e.g. post ID etc) is that it aligns with the allowed_block_types filter, which accepts a post object. Arguably, our client-side filter should be aligned with the server-side one, API-wise, since they're semantically doing the same thing.
However, it's arguable that a post object simply isn't the completely right fit anyway, and that we should replace it with something like editorType. Coincidentally, @TimothyBJacobs arrived at a similar conclusion in a separate issue (about providing the list of allowed blocks via the REST API). So maybe it's time to consider that, and maybe even deprecate/replace the current allowed_block_types filter (with a filter that accepts a context rather than a post argument) (cc/ @youknowriad) We would lose the ability to filter on a per-post basis, but OTOH, a post object might also not be the best fit for some (edge?) cases that Timothy lists in that discussion.
Or maybe just a comment here explaining why we are going with
wp_templateas the hardcoded value, and some block(s) wont be registered if this isn't passed aswp_templateorwp_template_part. 🤷♀️
Agree, writing this reply I realized there's a lot of considerations that have gone into this.
|
Closing, since the underlying issue has apparently been resolved. |
Description
Tentative partial fix for #22080: Remove the Post Content block when editing content, in order to prevent blockception (an infinite recursion of the Post Content block embedding the current post content, which includes itself, and so on).
Note that I'm passing the
postTypeas an extra argument to__experimentalRegisterExperimentalCoreBlocks. I think this is probably okay (since it's experimental, and we don't seem to be too picky about what we're passing to that function, since the existingsettingsargument lumps together a bunch of different concerns already).How has this been tested?
Screenshots
N/A
Types of changes
Bug fix.
Checklist: