WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 4 weeks ago

Last modified 2 weeks ago

#52920 closed enhancement (fixed)

Editor: Abstract block editor configuration

Reported by: gziolo Owned by: gziolo
Milestone: 5.8 Priority: normal
Severity: normal Version:
Component: Editor Keywords: has-patch has-unit-tests needs-dev-note
Focuses: Cc:

Description (last modified by gziolo)

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.

Prior art in Gutenberg: https://github.com/WordPress/gutenberg/pull/28701.

Required for https://github.com/WordPress/gutenberg/pull/29969 - new REST API endpoint for the editor settings.

Changes included so far:

  • new method get_default_block_categories were introduced to make it 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
  • block_categories_all
  • block_editor_rest_api_preload_paths
  • block_editor_settings_all

Change History (51)

#1 @gziolo
3 months ago

  • Description modified (diff)

This ticket was mentioned in PR #1125 on WordPress/wordpress-develop by gziolo.


3 months ago

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

Ensures that wp-format-library assets are always loaded for the block editor.

#3 @gziolo
3 months ago

  • Description modified (diff)

#4 @prbot
3 months ago

azaozz commented on PR #1118:

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.

#5 @prbot
3 months ago

gziolo commented on PR #1125:

@youknowriad and @ellatrix – do you think it's safe to assume that we always need to load format library?

We do something similar for:

  • block directory with wp_enqueue_editor_block_directory_assets
  • block styles from WP_Block_Styles_Registry with enqueue_editor_block_styles_assets

#6 @prbot
3 months ago

ellatrix commented on PR #1125:

You mean for WordPress? I guess so, otherwise it should just have no formats available without errors.

#7 @prbot
3 months ago

gziolo commented on PR #1125:

You mean for WordPress? I guess so, otherwise it should just have no formats available without errors.

Yes, for WordPress. It's going to be loaded for all pages that render the block editor moving forward. Related commit: https://github.com/WordPress/wordpress-develop/commit/0df28171ed9c3c31ebc93add82b64df8f690f87c.

#8 @prbot
3 months ago

gziolo commented on PR #1118:

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 https://github.com/WordPress/wordpress-develop/pull/1118#discussion_r605516157 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.

#9 @gziolo
3 months ago

  • Keywords has-unit-tests dev-feedback added

#10 @gziolo
3 months ago

  • Description modified (diff)

#11 @gziolo
3 months ago

  • Description modified (diff)

#12 @SergeyBiryukov
3 months ago

Just linking [50620] for reference, apparently it missed the ticket.

#13 @SergeyBiryukov
3 months ago

In 50629:

Editor: Consolidate enqueueing block editor assets in wp-includes/default-filters.php.

Follow-up to [44157], [46111], [48537], [50620].

See #52920.

#14 @prbot
2 months ago

gziolo commented on PR #1118:

A note to myself.

The list of filters covered with unit tests:

  • [x] allowed_block_types
  • [x] allowed_block_types_{$editor_name}
  • [x] block_categories
  • [ ] block_editor_preload_paths
  • [x] block_editor_settings
  • [x] block_categories_{$editor_name}
  • [ ] block_editor_preload_paths_{$editor_name}
  • [x] block_editor_settings_{$editor_name}

The list of methods covered with unit tests

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

#15 @prbot
2 months ago

nosolosw commented on PR #1118:

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 https://github.com/WordPress/gutenberg/pull/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?

#17 @prbot
2 months ago

gziolo commented on PR #1118:

@nosolosw, it's a tough problem. 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.

#18 @prbot
2 months ago

gziolo commented on PR #1118:

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

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

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

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

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.

#19 @gziolo
2 months ago

In 50776:

Editor: Abstract block editor configuration

There are several WordPress hooks defined on the server that depend on $post object that isn’t present on the new screens like edit site, edit widgets, or edit navigation. This patch deprecates existing filters and introduces replacements that are context-aware.

Props azaozz, andraganescu, jeremyfelt, nosolosw, youknowriad.
See #52920.

#20 @prbot
2 months ago

gziolo commented on PR #1118:

I merged most of the changes included in this PR with https://github.com/WordPress/wordpress-develop/commit/a19f589a5f9d5c1c44b55971fa33ddf6fda653f3. I left out block_editor_preload_api_requests that requires more work.

#21 @gziolo
2 months ago

  • Description modified (diff)

#22 @gziolo
2 months ago

  • Keywords needs-dev-note added

#23 @gziolo
2 months ago

In 50777:

Editor: Shape block editor filters to work better with the Gutenberg plugin

This should allow to use new filters in the Gutenberg plugin and therefore it prevents deprecation warnings when in the debug mode.

See #52920.

#24 @jeremyfelt
2 months ago

From [50777]:

$block_categories = apply_filters( 'block_categories_all', $block_categories, $editor_name );

If the variable filter block_categories_{$editor_name} is not needed, and nothing is changing about the $block_categories parameter, then it may be that block_categories does not need to be deprecated. We can just add $editor_name as a new parameter. Otherwise it seems like the block_categories_all and block_categories filters are effectively the same thing.

#25 follow-up: @gziolo
2 months ago

@jeremyfelt, that would be ideal to go with the approach you outlined. We are discussing the same issue in https://github.com/WordPress/gutenberg/pull/31027. Let me share the part that worries me the most if we go this route:

block_categories and other filters involved here, they have the $post object as a param which is not set outside of the edit post page. More importantly, we provide examples for this filter that depend on unguarded $post usage at https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/filters/block-filters.md#managing-block-categories:

<?php
// my-plugin.php

function my_plugin_block_categories( $categories, $post ) {
        if ( $post->post_type !== 'post' ) {
                return $categories;
        }
        return array_merge(
                $categories,
                array(
                        array(
                                'slug' => 'my-category',
                                'title' => __( 'My category', 'my-plugin' ),
                                'icon'  => 'wordpress',
                        ),
                )
        );
}
add_filter( 'block_categories', 'my_plugin_block_categories', 10, 2 );

If you have some ideas on how to address it safely then we should do it.

#26 @gziolo
8 weeks ago

In 50798:

Editor: Fix typo in image default size setting

Related change in Gutneberg: https://github.com/WordPress/gutenberg/pull/31324

Props mamaduka.
See #52920.

#27 @chrisvanpatten
7 weeks ago

A big concern I have with this change is that while it solves for making the filters more abstract, the lack of $post contextual awareness in the post-editor is a big change, in particular for post-type aware settings.

It's an incredibly common use-case to restrict the available blocks for a given post type in particular; an "events" post type you define may be dramatically different from an "article" type, for instance. If settings are ultimately abstracted and exposed through API endpoints without any way to access post context (as has been proposed in a few Gutenberg repo issues) that represents a massive functionality change and ultimately will lead to breaking a lot of complex needs that we have at the enterprise level.

#28 @gziolo
7 weeks ago

@chrisvanpatten, in the discussion you started in https://github.com/WordPress/wordpress-develop/pull/1118#discussion_r627396445, @TimothyBlynJacobs suggested we introduce a context class instance instead of the $post that would provide access to the $post. I'm curious what other thing about going that route and providing access to the post properties as if they were present in the context object through overloading. This way we could avoid all the dance with deprecations of existing filters.

#29 @TimothyBlynJacobs
6 weeks ago

I wouldn't do the overloading, because while the properties can be made available. It will still break any code that is actually expecting a WP_Post object, either causing a fatal error or unexpected behavior. So I'm not sure we can really get away with not deprecating the filters.

#30 @gziolo
5 weeks ago

In 50920:

Editor: Remove editor type specific filters for block editor configuration

Aligns with changes introduced in the Gutenberg plugin. The planned follow-up replace the editor name string with a context object to bring back an optional reference to the actual post.

Props youknowriad.
See #52920.

#31 @gziolo
5 weeks ago

  • Description modified (diff)

#32 @prbot
5 weeks ago

gziolo commented on PR #1118:

@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.

#33 @prbot
5 weeks ago

chrisvanpatten commented on PR #1118:

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

#34 @prbot
5 weeks ago

azaozz commented on PR #1118:

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?

#35 @prbot
5 weeks ago

azaozz commented on PR #1118:

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?

#36 @prbot
5 weeks ago

azaozz commented on PR #1118:

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.

#37 @prbot
4 weeks ago

gziolo commented on PR #1118:

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 22f6768faff35013d89feb8ba6804d7d65426992.

#38 @prbot
4 weeks ago

gziolo commented on PR #1118:

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.

#39 @prbot
4 weeks ago

gziolo commented on PR #1118:

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 https://github.com/WordPress/wordpress-develop/pull/1118#discussion_r627396445. We will need more data to be passed as context. The work on the widgets screen https://github.com/WordPress/wordpress-develop/pull/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 assocative array, too.

#40 @gziolo
4 weeks ago

  • Owner set to gziolo
  • Resolution set to fixed
  • Status changed from new to closed

In 50956:

Editor: Extract block_editor_rest_api_preload method for use with different editor screens

It is going to be used on the new widgets editor screen. This patch also introduced a new class WP_Block_Editor_Context that is going to be used with revised block editor filters to let extenders to keep their existing behavior. It should also allow to provide more settings through the context class as new screens get introduced like the navigation editor.

Props azaozz, chrisvanpatten, timothyblynjacobs, youknowriad.
Fixes #52920.

#41 @gziolo
4 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#42 @gziolo
4 weeks ago

  • Keywords dev-feedback removed

#43 @gziolo
4 weeks ago

In 50957:

Editor: Add missing class WP_Block_Editor_Context

Follow-up for [50956].
See #52920.

#44 @prbot
4 weeks ago

gziolo commented on PR #1118:

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.

#45 @gziolo
4 weeks ago

In 50963:

Editor: Remove unused param in get_default_block_editor_settings

Follow-up for [50776].

Props nosolosw.
See #52920.

#46 @prbot
4 weeks ago

chrisvanpatten commented on PR #1118:

Thanks for the work on this @gziolo!

#47 @gziolo
4 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 50983:

Editor: Use the block editor context in filters that used the editor name

Follow-up for [50956].
Props azaozz, chrisvanpatten, timothyblynjacobs, youknowriad.
Fixes #52920.

#48 @prbot
4 weeks ago

gziolo commented on PR #1118:

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

#49 @gziolo
4 weeks ago

  • Description modified (diff)

#50 in reply to: ↑ 25 @jadpm
4 weeks ago

Replying to gziolo:

@jeremyfelt, that would be ideal to go with the approach you outlined. We are discussing the same issue in https://github.com/WordPress/gutenberg/pull/31027. Let me share the part that worries me the most if we go this route:

block_categories and other filters involved here, they have the $post object as a param which is not set outside of the edit post page.
...
If you have some ideas on how to address it safely then we should do it.

My 2c on this topic although I know I am a bit late: why no just keep them?

As stated above, there are lost of wild usages of the now deprecated filters that rely precisely on the parameter that is not available on the broad, generic new filters. I would suggest keeping the previous, existing filters, and document that they are only available for the post-editor context. This is not a breaking change at all: right now, they are only available there anyway!

Otherwise, we will end up finding that developers are just repeating the logic now placed under deprecation, but manually: they will hook into block_categories_all, check that the editor name is post-editor, get the current post with get_post and evaluate whether they want to include their block categories on the post editor based on the post properties. I really wonder why we would want to force them to do so when the logic for that already exists.

#51 @gziolo
2 weeks ago

@jadpm, thank you for your feedback. Actually, it's very similar to what @chrisvanpatten shared on GitHub in the accompanying PR. When you check [50983], you can see that the current approach is slightly different. All new filters have access to the instance of WP_Block_Editor_Context class that has optional post property. It replaced the string that represented the block editor context that was limiting in its application.

In theory, we could keep old filters, too. My biggest concern is that in the long run, they would cause confusion because we now use the same post editor to edit not only the content of the post but also the template for the individual post. We kept in mind all that I mentioned when opting for the final solution that provides the flexibility that the WP_Block_Editor_Context class ebables.

Note: See TracTickets for help on using tickets.