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 Hooks: Respect allowedBlocks field and control for users #53989

Open
Tracked by #54904 ...
bph opened this issue Aug 28, 2023 · 8 comments
Open
Tracked by #54904 ...

Block Hooks: Respect allowedBlocks field and control for users #53989

bph opened this issue Aug 28, 2023 · 8 comments
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Extensibility The ability to extend blocks or the editing experience [Type] Enhancement A suggestion for improvement.

Comments

@bph
Copy link
Contributor

bph commented Aug 28, 2023

from the tracking issue:

  • Originally touched upon Auto-inserting Blocks #39439 (comment). We might want to give a block some level of control what blocks it allows as its (auto-inserted) children.

  • There's an allowedBlock attribute that's currently used by a number of blocks; AFAICT, it's only handled on the client side, and basically passed through to the InnerBlocks component as a prop. I'm a bit hesitant to use something like this as it seems currently really up to each individual block, with no standardization, especially on the server (e.g. in the shape of block-supports).

  • Implementing this later might amount to an API change: Blocks that were previously auto-inserted might cease to be if their parent disallows them 😕

full disclosure: I was reading through the tracking issue on my phone and must have fat-fingered it to create an issue of a to-do-item. Only noticed it today, when back at my computer.Tried to make the best of it by copy/pasting the content from the tracking issue.

@bph bph closed this as completed Aug 29, 2023
@bph bph reopened this Aug 29, 2023
@bph bph added [Feature] Block API API that allows to express the block paradigm. [Feature] Extensibility The ability to extend blocks or the editing experience [Feature] Custom Fields Anything related to the custom fields project - connecting block attributes and dynamic values labels Aug 29, 2023
@bph bph changed the title Respect field. Respect field and control for users Aug 29, 2023
@jordesign jordesign added the [Type] Enhancement A suggestion for improvement. label Aug 30, 2023
@ockham ockham changed the title Respect field and control for users Respect allowedBlock field and control for users Sep 28, 2023
@gziolo gziolo removed the [Feature] Custom Fields Anything related to the custom fields project - connecting block attributes and dynamic values label Oct 3, 2023
@fabiankaegy fabiankaegy changed the title Respect allowedBlock field and control for users Block Hooks: Respect allowedBlock field and control for users Oct 23, 2023
@ockham
Copy link
Contributor

ockham commented Jan 29, 2024

I've approved #58262. However, maybe there's another problem before we can use it to implement a solution to this issue:

If we'd like the Block Hooks mechanism -- which runs on the server side -- to respect the allowedBlocks setting, we need plugins that include hooked blocks to add the latter to the allowedBlocks of the parent block's that they'd like to insert into. E.g. if my (3rd-party) Like Button block would like to auto-insert itself into a Comment Template block, it will need to add itself to the Comment Template's allowedBlocks first; if it's not found in that list, Block Hooks should not render the block.

In practice, this could be a bit tricky: Core/GB blocks (like Comment Template) are registered before 3rd-party plugin blocks; so if the latter tries to extend the Comment Template block's allowedBlocks list via the block_type_metadata or block_type_metadata_settings filters, it'll be too late, as the Comment Template block will already have been registered 😕

I'm wondering if we need any additional filters, or if we'll simply need to un-register and re-register the block 🤔
cc/ @jsnajdr @tjcafferkey if either of you has any ideas 😊

@jsnajdr
Copy link
Member

jsnajdr commented Jan 29, 2024

The client-side blocks registration in JS has this covered with the reapplyBlockTypeFilters action. At some time during the editor initialization, when all blocks are supposed to be already registered, you call this action to explicitly rerun all the filters.

It seems we need the same thing on the server, too, if it's already not there.

@gziolo
Copy link
Member

gziolo commented Jan 29, 2024

In WordPress core all blocks get registered on init action so I don’t expect any issues here as it’s a way to make sure that all plugins can register their filters that will trigger at a proper time.

@ockham ockham changed the title Block Hooks: Respect allowedBlock field and control for users Block Hooks: Respect allowedBlocks field and control for users Jan 30, 2024
@jsnajdr
Copy link
Member

jsnajdr commented Jan 31, 2024

A block registration on the server, in a plugin PHP code, looks like this:

add_action( 'init', function() {
  register_block_type( /* my/block */ );
} );

add_action( 'register_block_metadata', function( $metadata ) {
  if ( $metadata->name === 'core/navigation' ) {
   /* add 'my/block' to $metadata->allowedBlocks */
  }
  return $metadata;
} );

When the block registrations run on init, all the filters from plugins are already registered. Is that right?

@gziolo
Copy link
Member

gziolo commented Jan 31, 2024

When the block registrations run on init, all the filters from plugins are already registered. Is that right?

Yes, it should work correctly. When we land WordPress/wordpress-develop#5988 then the changes from the server should also get correctly propagated to the client.

@ockham
Copy link
Contributor

ockham commented Feb 1, 2024

Ah, thanks for clarifying that, folks! That'd be quite the relief 😄

BTW I've thought about this some more, and I'm starting to wonder if we should actually implement this behavior (i.e. Block Hooks respecting allowedBlocks). Arguably, if someone makes a block a hooked block, they state their intention pretty clearly that they'd like to insert it in a given position; requiring them to also write a filter to add that block to the parent's allowedBlocks feels like a bit of busywork.

This is particularly clear with firstChild/lastChild insertion, where the parent block is also the anchor block. It's a bit less clear cut for before/after insertion, where we don't know what the parent block will be; it might make a bit more sense to respect allowedBlocks there.


We could go as far as to "invert" this issue's idea: We could make Block Hooks extend a parent block's allowedBlocks to include the hooked block automatically. This would be handy for cases such as this:

[...] a scenario where a user (e.g. accidentally) deletes a hooked block from a Navigation menu, and then wants to re-insert it -- in which case we'd want the hooked block to show up in the inserter, to which end it needs to be in the list of allowed blocks.

Once again, this feels easier to answer for firstChild/lastChild insertion than before/after insertion.

@jsnajdr
Copy link
Member

jsnajdr commented Feb 2, 2024

The only place where parent, ancestor and allowedBlocks are checked is canInsertBlockType. There you can add code that uses blockType.blockHooks as another criterion for the decision. And possibly override what the other criteria say.

For example, parent, ancestor and allowedBlocks can already override each other sometimes:

  • ancestor is not overridable, if a block says that it must have a certain ancestor, then canInsertBlockType result is always false if the ancestor block is not there.
  • parent and allowedBlocks can override each other. If block A specifies allowedBlocks: [ 'B' ] and there is a block C that specifies parent: [ 'A' ], then C can be a child of A even though A's allowedBlocks allow only B.
  • and vice versa, if B specifies allowedBlocks: [ 'C' ], then C can be a child of B even though C allows only A as a parent.

Similarly, blockHooks could possibly say an explicit YES and override the other criterions.

@gziolo
Copy link
Member

gziolo commented Feb 6, 2024

Now that Jarda shared the context of how parent works, always forcing the block to be exposed in the inserter, ignoring other settings, I remember it was the design decision. In the past, we were talking about that ancestor is not going to be possible to fully respect on the server because we often deal with patterns or template parts that don’t have full knowledge about ancestor blocks present higher in the tree of blocks that gets constructed in the block editor.

Similarly, blockHooks could possibly say an explicit YES and override the other criterions.

I think we should consider blockHooks a power feature that always takes precedence. This is also why we provide PHP filters that make it possible to fine-tune where and how it gets injected. In addition to that, it's always possible for the user to go to the editor and modify the template to disable the unwanted block or tap into the rendering pipeline with one of the WP hooks to prevent rendering of the block.

One thing that might be actionable is respecting the allowedBlocks setting in the toggle exposed in the sidebar panel. In effect, the user shouldn't be able to insert the block in case they wouldn't be able to do it with the inserter. Possibly, by reusing the same logic that checks whether the block can be inserted with the inserter. I'm only not entirely sure how that would work with auto-inserted blocks 😅

@gziolo gziolo mentioned this issue Oct 10, 2024
68 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Extensibility The ability to extend blocks or the editing experience [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

5 participants