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

Block Editor: Apply allowed_block_types filter to registered blocks #419

Closed
wants to merge 2 commits into from
Closed

Block Editor: Apply allowed_block_types filter to registered blocks #419

wants to merge 2 commits into from

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Jul 20, 2020

The allowed_block_types hook filters a list of blocks. Alternatively, it's possible to pass a bool to indicate that either all blocks are allowed (true), or none of them are (false).

It's currently applied in lib/edit-form-blocks.php to a default value of true -- meaning all blocks are allowed.

However, this means that the filter can only really be used for an allowlist, but not for a denylist: Since we don't really have access to the full list of available blocks from the filter (but just that true bool), we cannot choose to remove a set of blocks from that list.

I'm thus suggesting to pass the full list of registered blocks to the filter as the default value instead.

Trac ticket: https://core.trac.wordpress.org/ticket/50706


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@ockham
Copy link
Contributor Author

ockham commented Jul 20, 2020

Use case example: WordPress/gutenberg#24006

@ockham
Copy link
Contributor Author

ockham commented Jul 21, 2020

cc/ @TimothyBJacobs @youknowriad

@TimothyBJacobs
Copy link
Member

Makes sense to me!

@youknowriad
Copy link
Contributor

No objections from me either but curious about backward compatibility implications.
it also seems we'd have to wait for trunk to target 5.6 to land this.

@ockham
Copy link
Contributor Author

ockham commented Jul 22, 2020

No objections from me either but curious about backward compatibility implications.
it also seems we'd have to wait for trunk to target 5.6 to land this.

I think this should be mostly backward compatible, since the hook always accepted either an array of block names, or a bool, so filters attached to the hook would have to check with which of either they were dealing. In practice, they couldn't count on getting an array from which they could remove something, so the filter was only used for allow lists rather than deny lists. (As e.g. the docs show.) So I think we're adding functionality without really impacting existing usage.

Furthermore, the filter is applied right before the $settings var is passed to the client, so all server-side block registrations can be expected to have happened by this time.

LMK if you're thinking of a scenario that I'm missing 🤔

@ockham
Copy link
Contributor Author

ockham commented Aug 4, 2020

Ping @TimothyBJacobs and @youknowriad -- Are we cool to merge this? 😄

@youknowriad
Copy link
Contributor

I've been thinking more here and I think we should not move forward with this. The reason being that all blocks are not registered on the server, so as implemented here, it means all the blocks registered on the frontend are not allowed by default.

@vindl
Copy link
Member

vindl commented Aug 24, 2020

@ockham sounds like we should close out this PR?

@ockham
Copy link
Contributor Author

ockham commented Oct 12, 2020

I've been thinking more here and I think we should not move forward with this. The reason being that all blocks are not registered on the server, so as implemented here, it means all the blocks registered on the frontend are not allowed by default.

@youknowriad That makes sense, but it also limits the potential use cases of the server-side filter significantly: It can basically never be used as a denylist. IMO, this raises the question of the general usefulness of this filter -- since there are also client-side mechanisms to both register or unregister a block. Should we:

  1. prescribe that blocks must be registered on the server side?
  2. deprecate the allowed_block_types hook?

@TimothyBJacobs
Copy link
Member

Yeah the only times I've been able to use allowed_block_types is when there are a very specific set of blocks allowed which is rarely what you want. Maybe add a disabled_block_types filter?

@ockham
Copy link
Contributor Author

ockham commented Oct 13, 2020

Yeah the only times I've been able to use allowed_block_types is when there are a very specific set of blocks allowed which is rarely what you want. Maybe add a disabled_block_types filter?

Yeah, that could work 🤔 It might not perfectly cover cases where e.g. you'd like to remove all blocks in a certain namespace (rather than listing them explicitly, because e.g. they might come from a plugin that has added more blocks in more recent versions), but I guess it's a start...

@TimothyBJacobs
Copy link
Member

We could support including a namespace in the list, so both my-plugin and other-plugin/block.

@hellofromtonya
Copy link
Contributor

Closing this PR as the associate Trac ticket https://core.trac.wordpress.org/ticket/50706 was closed 7 weeks ago.

If any maintainer or committer feels this ticket and PR should be reopened, please update accordingly.

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

Successfully merging this pull request may close these issues.

5 participants