#52920 closed enhancement (fixed)
Editor: Abstract block editor configuration
Reported by: |
|
Owned by: |
|
---|---|---|---|
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 )
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 (53)
This ticket was mentioned in PR #1125 on WordPress/wordpress-develop by gziolo.
4 years ago
#2
4 years ago
#4
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.
4 years ago
#5
@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
withenqueue_editor_block_styles_assets
4 years ago
#6
You mean for WordPress? I guess so, otherwise it should just have no formats available without errors.
4 years ago
#7
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.
4 years ago
#8
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.
4 years ago
#14
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
4 years ago
#15
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 theblock_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) orget_block_editor_settings
(if they are only relevant to a specific editor).
Does this sound about right to you?
4 years ago
#16
Gutenberg some of the deprecated filters:
allowed_block_types
block_categories
block_editor_preload_paths
block_editor_settings
- https://github.com/WordPress/gutenberg/blob/2c0ba0758d48a530596f4977c272d3a35906b215/lib/global-styles.php#L171
- https://github.com/WordPress/gutenberg/blob/7c134eb66f1e5ed67db646e19234d9edf6e3090e/lib/client-assets.php#L673
- https://github.com/WordPress/gutenberg/blob/7c134eb66f1e5ed67db646e19234d9edf6e3090e/lib/client-assets.php#L690
- and more ...
4 years ago
#17
@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.
4 years ago
#18
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.
4 years ago
#20
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.
#24
@
4 years 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:
↓ 50
@
4 years 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.
#27
@
4 years 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
@
4 years 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
@
4 years 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.
4 years ago
#32
@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 commented on PR #1118:
4 years ago
#33
@gziolo Looks like a good approach for my needs! Thanks for putting the work into this.
4 years ago
#34
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
?
4 years ago
#35
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?
4 years ago
#36
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
likeWP_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.
4 years ago
#37
Nitpick: the
preload_paths
part ofblock_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 likeblock_editor_initial_data
orblock_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 functionblock_editor_rest_api_preload_paths
for the filter
Applied in 22f6768faff35013d89feb8ba6804d7d65426992.
4 years ago
#38
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.
4 years ago
#39
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
likeWP_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
@
4 years ago
- Owner set to gziolo
- Resolution set to fixed
- Status changed from new to closed
In 50956:
4 years ago
#44
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 commented on PR #1118:
4 years ago
#46
Thanks for the work on this @gziolo!
4 years ago
#48
The last batch of changes committed in https://core.trac.wordpress.org/changeset/50983.
#50
in reply to:
↑ 25
@
4 years 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
@
4 years 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.
Trac ticket: https://core.trac.wordpress.org/ticket/52920
Ensures that
wp-format-library
assets are always loaded for the block editor.