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

Full Site Editing: Only allow FSE blocks in wp_template CPT #32657

Merged
merged 1 commit into from
Apr 30, 2019

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Apr 29, 2019

Changes proposed in this Pull Request

  • Restrict the usage of Content and Template blocks in the wp_template CPT.

Note: my first thinking was to use the allowed_block_types filter, which seems designed exactly for this purpose, but in fact falls short of our needs.
The first parameter (and return value) can be true if all blocks are allowed, false if none, or a whitelist of allowed blocks.
This means that, if we want to disallow FSE blocks from all post types except wp_template, we would need to return an array including all blocks except the FSE ones.
The filter doesn't make such list available, and as far as I know there is no way to get it in PHP.

JS-side, it's slightly easier but less elegant: once the DOM is ready, only register the blocks if the current post's type is wp_template.

Testing instructions

  • Create or edit a Template post, and make sure it's possible to insert both the Content and Template blocks. Also check on the front-end, and make sure Content output a temporary placeholder message, and Template the selected post.
  • Create or edit a post of any other type, and make sure the Content and Template blocks aren't available.

Part of #32618 and #32619

@Copons Copons added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Goal] Full Site Editing labels Apr 29, 2019
@Copons Copons requested a review from a team April 29, 2019 15:02
@Copons Copons self-assigned this Apr 29, 2019
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@mattwiebe
Copy link
Contributor

This approach is fine in the short term but will break once we move to a more universal editing model, since the wp_template template would, in the example of a homepage, be edited within the context of the page post type as that which contains the content being edited.

In lieu of better logic for showing/allowing blocks in varying contexts, we could also approach this by modifying the internal logic of the block's edit function to show warnings etc and also on the frontend to educate users on proper usage.

But I think that would be premature, this gets us moving just fine.

@Copons
Copy link
Contributor Author

Copons commented Apr 30, 2019

This approach is fine in the short term but will break once we move to a more universal editing model, since the wp_template template would, in the example of a homepage, be edited within the context of the page post type as that which contains the content being edited.

In lieu of better logic for showing/allowing blocks in varying contexts, we could also approach this by modifying the internal logic of the block's edit function to show warnings etc and also on the frontend to educate users on proper usage.

But I think that would be premature, this gets us moving just fine.

Good point, I didn't think about that.
I agree it would be premature: we'll have to find a custom solution anyway, but we don't have enough elements yet to figure out how to approach it.

@Copons Copons merged commit b2440a0 into master Apr 30, 2019
@Copons Copons deleted the update/fse-restrict-blocks-usage branch April 30, 2019 11:03
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 30, 2019
Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

I'm less of a fan of doing this in JavaScript even though the approach in abstract is how I'd prefer to do it - conditionally register the blocks based on if we're editing a wp_template

Doing it in JavaScript opens us up to even more data races that have already given us headaches with conditionally-registered block types.

How about conditionally queuing the script in PHP in the plugin based on whether the current page is a wp_template? Did you try that approach?

@Copons
Copy link
Contributor Author

Copons commented May 1, 2019

@dmsnell I did try that, but to be honest I haven't explored it enough, once I got stuck trying to figure out the action order.
The editor post type isn't available in init yet (unless we check $_GET, which doesn't seem a nice approach to me), and so I would have needed to move enqueues and registrations to other hooks - and then I saw the domReady example in the official docs. 🙂

I can push an alternative approach to this one, it would also solve the Core's core/template extension issue (paAmJe-fT-p2 #comment-569).

@dmsnell
Copy link
Member

dmsnell commented May 1, 2019

The editor post type isn't available in init yet

@Copons that sounds like a golden comment to add in the implementation here if you ask me

// Note: we _could_ have conditionally enqueued this script from the server
// but at the time that the `init` hook runs in PHP we don't know the post type
// so we can't make that choice there whether or not to load this
//
// It _may_ be possible to transition to a server-based approach and that could
// alleviate related issues with data-races affecting block registration.

@vindl
Copy link
Member

vindl commented May 2, 2019

For some reason I'm no longer able to insert this blocks to any post type (including wp_template). 🤔 It works fine on the commit before this was merged.

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

Successfully merging this pull request may close these issues.

5 participants