Editor: Abstract block editor configuration

[gziolo]

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

Related to WordPress/gutenberg#28701.

Required for WordPress/gutenberg#29969 - new REST API endpoint for the editor settings.

We are approaching the day where new screens with the block editor get included in WordPress core. It looks like there are several WordPress hooks defined on the server that depends on $post object that isn’t present on the edit site, edit widgets, or edit navigation screens. It feels like we should deprecate existing filters and create replacements that are going to be context-aware.

It's a work in progress.

Changes included so far:

• new method get_default_block_categories were introduced to make is possible to share default block categories
• new method get_allowed_block_types was introduced to handle filters depending on the editor type
• most of the settings that are defined on the client in the block editor package in https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/store/defaults.js are already exposed from the new get_default_block_editor_settings method
• another method get_block_editor_settings got introduced to ensure that editor settings can be filtered depending on the editor type
• the logic that preloads common data used with the block editor by processing an array of REST API paths is now abstracted in block_editor_rest_api_preload method

All existing filters that depend on $post object get deprecated with apply_filters_deprecated as suggested by @jeremyfelt:

• allowed_block_types
• block_categories
• block_editor_preload_paths
• block_editor_settings

New filters are introduced that are context-aware:

• allowed_block_types_all
• allowed_block_types_{$editor_name}
• block_categories_all
• block_editor_rest_api_preload_paths
• block_editor_preload_paths_{$editor_name}
• block_editor_settings_all
• block_editor_settings_{$editor_name}

In the case where there are two filters for a general use case that covers any editor type and a specific one, I followed https://core.trac.wordpress.org/ticket/46187 and the implementation for render_block + render_block_{$block_name}. @johnbillion shared more examples in https://core.trac.wordpress.org/ticket/46187#comment:9:
Detailed filters per editor type are on hold upon feedback received from @youknowriad, @chrisvanpatten, and @TimothyBJacobs . We want to make it more flexible and it might end up with a special object that holds context.

> This is a fairly common pattern in core, see for example: `gettext/gettext_{$domain}`, `plugin_action_links/plugin_action_links_{$plugin_file}`, `manage_posts_columns/manage_{$post_type}_posts_columns`, etc. The dynamically named filter should come after the generic one.

---

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.

[gziolo]

I opened WordPress/gutenberg#30245 to exercise how get_default_block_editor_settings integrates with the Gutenberg plugin.

[azaozz]

It feels like we should deprecate existing filters and create replacements that are going to be context-aware.

The changes so far look good imho. Thinking this can go a step further, perhaps, and clearly separate/define the (PHP) filters that run for the actual editor (settings, UI, etc.) and the filters that run for the things around the editor (tags, categories, etc.). Would make it clearer/easier to understand and eventually add/tweak/remove filters in the future. At this point that's mostly documentation, and perhaps functions/filters naming.

[gziolo]

I applied several iterations and added tests to cover some of the methods and filters that are marked as deprecated. It still needs more tests and one more abstraction as discussed in #1118 (comment) but I think it's close to what I would expect. Any thoughts on the current shape? I'd like to start committing parts of the codebase next week if there are no further concerns.

[oandregal]

👋 is this meant to be here given that we also set it here?

[gziolo]

Yes, the related discussion is here:
#1118 (comment)

It's something that I'm not 100% sure about so all feedback is valuable.

[gziolo]

A note to myself.

The list of filters covered with unit tests:

• allowed_block_types
• allowed_block_types_all
• block_categories
• block_categories_all
• block_editor_preload_paths
• block_editor_preload_paths_all
• block_editor_settings
• block_editor_settings_all

The list of methods covered with unit tests

• get_allowed_block_types
• get_default_block_categories (indirectly – it's a hardcoded list)
• get_block_categories
• block_editor_preload_api_requests
• get_default_block_editor_settings
• get_block_editor_settings

[oandregal]

This looks good and brings clarity about the different editors and settings. Nice work! Trying to understand how to juggle these changes with the plugin and, particularly, with the addition of global styles settings to the mix. I'm thinking out loud here, it looks like the next steps are:

• Port the get_default_block_editor_settings as a shim to the plugin, verbatim. You already have this at Block Editor: Standardize loading default block editor settings gutenberg#30245 This is a shim to be removed as soon as the minimum WordPress version for the plugin is 5.8.
• In the plugin, all new editors start using the new editor-specific filters.
• In the plugin, for the post editor, the plugin hooks into the block_editor_settings and runs the block_editor_settings_post_editor filter. All our code uses the editor-specific filter. When the minimum WordPress version for the plugin is 5.8 we switch to hook into the editor-specific filter instead.
• In subsequent cycles, the new settings to port to WordPress core will be added to either get_default_block_editor_settings (if they're common to all editors) or get_block_editor_settings (if they are only relevant to a specific editor).

Does this sound about right to you?

[gziolo]

Gutenberg some of the deprecated filters:

1. allowed_block_types
   1. E2E test: https://github.com/WordPress/gutenberg/blob/3da717b8d0ac7d7821fc6d0475695ccf3ae2829f/packages/e2e-tests/plugins/allowed-blocks.php#L17-L24
2. block_categories
   1. https://github.com/WordPress/gutenberg/blob/12c23261ab469b91f84568f8ed9e6493bff6f187/lib/blocks.php#L358-L377
3. block_editor_preload_paths
   1. https://github.com/WordPress/gutenberg/blob/7c134eb66f1e5ed67db646e19234d9edf6e3090e/lib/compat.php#L154-L159
4. block_editor_settings
   1. https://github.com/WordPress/gutenberg/blob/2c0ba0758d48a530596f4977c272d3a35906b215/lib/global-styles.php#L171
   2. https://github.com/WordPress/gutenberg/blob/7c134eb66f1e5ed67db646e19234d9edf6e3090e/lib/client-assets.php#L673
   3. https://github.com/WordPress/gutenberg/blob/7c134eb66f1e5ed67db646e19234d9edf6e3090e/lib/client-assets.php#L690
   4. and more ...

It makes me wonder whether we should also add filters that allow changing multiple screens at once and don't depend on the $post object:

• allowed_block_types_all
• block_categories_all
• block_editor_preload_paths_all
• block_editor_settings_all

[gziolo]

@nosolosw, it's a tough problem to solve. I'm still unsure how to handle the deprecation of filters and add more general versions that don't depend on the $post object not present in some contexts. I did some searching in the Gutenberg plugin to better understand how we use those filters today. For example, the usage of the block_categories filter makes me want to have a way to set this category for all screens at once rather than calling it for every screen individually.

I think you got it all correct, but it still needs some thinking.

[gziolo]

When WP_DEBUG is set to true, I see a few deprecation warnings when testing with the Gutenberg plugin enabled:

  | Deprecated: block_categories is deprecated since version 5.8.0! Use block_categories_post-editor instead. in /var/www/src/wp-includes/functions.php on line 5242

  | Deprecated: block_categories is deprecated since version 5.8.0! Use block_categories_post-editor instead. in /var/www/src/wp-includes/functions.php on line 5242

  | Deprecated: block_editor_settings is deprecated since version 5.8.0! Use block_editor_settings_post-editor instead. in /var/www/src/wp-includes/functions.php on line 5242

It feels like we should run those deprecated filters in the Gutenberg plugin only when new functions aren't declared because they mostly add the same functionalities.

[gziolo]

I merged most of the changes included in this PR with a19f589. I left out block_editor_preload_api_requests that requires more work.

[chrisvanpatten]

Does this effectively mean you can no longer conditionally change what is preloaded based on the post type being edited? That's a substantial change.

[gziolo]

You can always use get_post() in the filter. In the Gutenberg plugin, we started simple, and there is only a filter that works with all editor types. We still need to figure out how to make it all simpler to use rather than manually detecting the context. The most pressing issue is that $post is not present on the widgets editor page so existing filters that have unguarded $post usage would error there.

[chrisvanpatten]

@gziolo That works until you start pulling the settings in via REST, and the API request doesn't have a post context :)

[gziolo]

The REST API endpoint is the very early stage of development. It might be a really good idea to provide more params to the API call so it could re-create the same behavior as you currently have when loading the edit post editor in WP-Admin.

[chrisvanpatten]

That'd be fantastic @gziolo :) Just want to make sure it's on the radar! 🚀

[chrisvanpatten]

I'm not sure I strongly care about distinguishing which editor is visible — I can understand how that might be ambiguous — but I do care about what specifically is being edited in terms of discrete pieces of site content (pages, posts, etc.) If that means that the context is "hey you're editing template part X" rather than "hey you're in the site editor" I think that's entirely reasonable; the context becomes about the edited object, which I think is the valuable bit.

I will say my primary interest here is being able to restrict available blocks based on what is being edited, although I can see, for example, wanting to say "allow custom spacing when editing a template part, but not when editing a specific piece of content". But that's less concerning to me vs. solving for block restrictions based on the environment you're in/what you're editing.

[gziolo]

With 0b6eb8d I removed editor type specific filters for block editor configuration. It mostly aligns with what we have in the Gutenberg plugin at the moment. The planned follow-up is to replace the editor name string with a context object to bring back an optional reference to the actual post as discussed in this thread.

[youknowriad]

@chrisvanpatten that's a very good way to think about it and it's clear and well defined that I can agree with.

The issue with that is that sometimes the "edited piece of content" changes dynamically inside the same editor instance.

A template with a Query block can edited multiple posts and single template at the same time which means a logic like you suggest would only work if these editor settings were specific to a given block container (InnerBlocks). Right now, these settings are unique in a single block editor instance and provided using php (inline script) so it's not something we can support at the moment unless we perform a big JS refactoring. (not saying it's not doable but I wanted to get expectations right :) )

[chrisvanpatten]

@youknowriad I totally get that. I do think longer term, there is value in InnerBlocks or other block containers being able to fetch their own settings at run-time — I've certainly always advocated for more control over InnerBlock containers 😅 — but I think it's also reasonable as a first step that the editor settings default to whatever object is being edited at launch and there is an inherent drift as you move through different screens and experiences.

Thinking on a longer horizon, I wonder if the editor settings are turning to a bit of a catch-all… maybe editor settings should be split into more logical groups, some of which are persistent across all editor experiences, and others which are contextual? That might make the whole thing a little easier to reason about and also potentially make it easier to load contextual stuff up front.

I'd also emphasize that for the enterprise use-cases I've been involved with (at Meredith and now at the NBA), we've generally been happy with object type level contextual fidelity. In other words, we're rarely (never?) saying "we need to configure settings for this individual specific post", but "we need to configure settings for post type X". Because we can't necessarily predict which specific objects might be edited in a given editor session, maybe it's more reasonable to pre-load settings for all object types up front instead?

[gziolo]

I included WP_Block_Editor_Context class that only allows setting the $post field for now. If we pass it to all places, we will be able to grow the class as we add new functionalities so we can decide based on individual cases. This way we don't have to predict what names of the editors we should use. It also gives the flexibility to the REST API endpoint to set the context in an expected state.

I'm planning to add tests today and integrate everything with the block editor settings functions. I feel like it satisfies all the feedback shared. If there are no further concerns I'd like to commit changes on Monday to have it ready for the WP 5.8 feature freeze.

[gziolo]

@chrisvanpatten and @TimothyBJacobs – I refactored this code to work with WP_Block_Editor_Context class and covered everything with unit tests. If you agree that this is the path forward then I will apply the same changes with the context class to other functions that are already in trunk. That remaining part should be straightforward.

[chrisvanpatten]

@gziolo Looks like a good approach for my needs! Thanks for putting the work into this.

[azaozz]

Really glad that the filter with the dynamic name was dropped. Such filters suck, really :) They are quite hard to use and if the dynamic part of the name is provided by a plugin, they are nearly impossible to use.

Looking through this again, seems several things could be a bit better. Adding separate comments for easier handling/review.

[azaozz]

Nitpick: the preload_paths part of block_editor_preload_paths_all doesn't sound good imho. What is being preloaded from these paths?

I see the new filter name follows the deprecated filter name, but perhaps it can be a bit more helpful/descriptive. Maybe something like block_editor_initial_data or block_editor_preload_data?

[azaozz]

Seems that names like:

• allowed_block_types_all,
• block_categories_all,
• block_editor_preload_paths_all
• block_editor_settings_all

should probably not be used. The _all part seems misleading.

Quick question: how does the block editor configuration handle multiple instances on the same page? Would filters ending in _all apply to a particular instance or all the instances?

[azaozz]

Not sure what the advantages are of using WP_Block_Editor_Context class vs. an associative array.

• Why is a class better? What benefits are there for core and for plugins if it is a class and not an associative array?
• Should it be final like WP_Post?
• Should it be restricted in some ways like perhaps make the core data (like $post) private?

I'm not sold on needing a class with all the extra built-in stuff just to pass some data around, about 50/50 for now. If there are benefits please add a sufficient explanation (in the docblock or as inline comments) what these benefits are and how this decision was reached. Such documentation will be really helpful in the future.

[gziolo]

Nitpick: the preload_paths part of block_editor_preload_paths_all doesn't sound good imho. What is being preloaded from these paths?
I see the new filter name follows the deprecated filter name, but perhaps it can be a bit more helpful/descriptive. Maybe something like block_editor_initial_data or block_editor_preload_data?

It allows providing REST API paths that are going to be preloaded and passed to the client.

How about:

• block_editor_rest_api_preload for the function
• block_editor_rest_api_preload_paths for the filter

Applied in 22f6768.

[gziolo]

Seems that names like:

• allowed_block_types_all,
• block_categories_all,
• block_editor_preload_paths_all
• block_editor_settings_all

should probably not be used. The _all part seems misleading.

Yes, it isn't perfect but there is no better alternative proposed so far. The agreement is that we need to deprecate old filters because $post passed as a param when used unguarded in checks will cause fatal errors when null passed for screens when there is no post selected like on the widgets screen.

Quick question: how does the block editor configuration handle multiple instances on the same page? Would filters ending in _all apply to a particular instance or all the instances?

There is no use case for multiple instances of the block editor on one page. It's hard to predict if there is going to be one. We focus on supporting different settings per screen, like the post editor vs widgets screen for now. In the future, there are going to be a navigation editor screen and maybe a site editor screen.

[gziolo]

Not sure what the advantages are of using WP_Block_Editor_Context class vs. an associative array.

• Why is a class better? What benefits are there for core and for plugins if it is a class and not an associative array?
• Should it be final like WP_Post?
• Should it be restricted in some ways like perhaps make the core data (like $post) private?

I'm not sold on needing a class with all the extra built-in stuff just to pass some data around, about 50/50 for now. If there are benefits please add a sufficient explanation (in the docblock or as inline comments) what these benefits are and how this decision was reached. Such documentation will be really helpful in the future.

I don't feel strongly about one or another. We discussed a class in #1118 (comment). We will need more data to be passed as context. The work on the widgets screen #1284 might help decide what that is going to be. One thing that was raised by @chrisvanpatten is that filters should still offer simple access to the $post object when it's set so plugins/sites could keep their old logic build on top of that. I will include the final keyword. We can change that to an associative array, too.

Update: see applied changes in 4f3dda8.

[gziolo]

In 1a4e6f2 I update the rest of the methods committed earlier to use the WP_Block_Editor_Context class. I need to apply the same changes in the Gutenberg plugin first.

[chrisvanpatten]

Thanks for the work on this @gziolo!

[gziolo]

The last batch of changes committed in https://core.trac.wordpress.org/changeset/50983.