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.

Sign up for GitHub

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

Plugin: Call deprecated hooks only when new filters not present #31027

Merged
merged 2 commits into from
May 4, 2021
Merged

Plugin: Call deprecated hooks only when new filters not present #31027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
659e17a
Plugin: Call deprecated hooks only when new filters not present
gziolo Apr 21, 2021
a5a7686
Update PHP filters for block editor to be a type agnostic
gziolo Apr 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions lib/block-editor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ function gutenberg_get_block_categories( $editor_name_or_post ) {
*
* @param array[] $default_categories Array of categories for block types.
*/
$block_categories = apply_filters( "block_categories_{$editor_name}", $default_categories );
$block_categories = apply_filters( 'block_categories_all', $default_categories );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I read WordPress/wordpress-develop#1118 (comment) and this issue's description I thought that we'd do this: check if block_categories_${editor_name} exists, and don't run the deprecated block_categories filter if it does (same for the other filters). Would it work if we limit this PR to do that and so fixing the warnings when WP_DEBUG is true?

Related: what do you think of not introducing a *_all filters at the moment? I think it'd be fine to do in the case all take the same config:

add_filter( 'block_categories_post_editor', callback );
add_filter( 'block_categories_site_editor', callback );
...

I'm thinking that *_all is just a particular use case (all editor use the same settings) but there may be more (post and site use the same but widgets doesn't, etc), that's why I'm not fully convinced is worth introducing the _all filters at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check if block_categories_${editor_name} exists

What does this mean? That there are subscribers (has_filter), or that there are consumers? I'm trying to understand how this would help.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: what do you think of not introducing a *_all filters at the moment? I think it'd be fine to do in the case all take the same config:

It's a completely opposite direction that @youknowriad advocates for in #31027 (comment) and expands on in #31027 (comment).

if ( 'post-editor' === $editor_name ) {
$post = is_object( $editor_name_or_post ) ? $editor_name_or_post : get_post();

Expand All @@ -99,7 +99,7 @@ function gutenberg_get_block_categories( $editor_name_or_post ) {
* @param array[] $block_categories Array of categories for block types.
* @param WP_Post $post Post being loaded.
*/
$block_categories = apply_filters_deprecated( 'block_categories', array( $block_categories, $post ), '5.8.0', "block_categories_{$editor_name}" );
$block_categories = apply_filters_deprecated( 'block_categories', array( $block_categories, $post ), '5.8.0', 'block_categories_all' );
}

return $block_categories;
Expand Down Expand Up @@ -131,7 +131,7 @@ function gutenberg_get_allowed_block_types( $editor_name ) {
* @param bool|array $allowed_block_types Array of block type slugs, or
* boolean to enable/disable all.
*/
$allowed_block_types = apply_filters( "allowed_block_types_{$editor_name}", $allowed_block_types );
$allowed_block_types = apply_filters( 'allowed_block_types_all', $allowed_block_types );
if ( 'post-editor' === $editor_name ) {
$post = get_post();

Expand All @@ -146,7 +146,7 @@ function gutenberg_get_allowed_block_types( $editor_name ) {
* boolean to enable/disable all.
* @param WP_Post $post The post resource data.
*/
$allowed_block_types = apply_filters_deprecated( 'allowed_block_types', array( $allowed_block_types, $post ), '5.8.0', "allowed_block_types_{$editor_name}" );
$allowed_block_types = apply_filters_deprecated( 'allowed_block_types', array( $allowed_block_types, $post ), '5.8.0', 'allowed_block_types_all' );
}

return $allowed_block_types;
Expand Down Expand Up @@ -267,13 +267,13 @@ function gutenberg_get_block_editor_settings( $editor_name, $custom_settings = a
);

/**
* Filters the settings to pass to the block editor for a given editor type.
* Filters the settings to pass to the block editor for all editor types.
*
* @since 5.8.0
*
* @param array $editor_settings Default editor settings.
*/
$editor_settings = apply_filters( "block_editor_settings_{$editor_name}", $editor_settings );
$editor_settings = apply_filters( 'block_editor_settings_all', $editor_settings );
if ( 'post-editor' === $editor_name ) {
$post = get_post();

Expand All @@ -286,7 +286,7 @@ function gutenberg_get_block_editor_settings( $editor_name, $custom_settings = a
* @param array $editor_settings Default editor settings.
* @param WP_Post $post Post being edited.
*/
$editor_settings = apply_filters_deprecated( 'block_editor_settings', array( $editor_settings, $post ), '5.8.0', "block_editor_settings_{$editor_name}" );
$editor_settings = apply_filters_deprecated( 'block_editor_settings', array( $editor_settings, $post ), '5.8.0', 'block_editor_settings_all' );
}

return $editor_settings;
Expand Down
6 changes: 4 additions & 2 deletions lib/blocks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,8 +380,10 @@ function gutenberg_register_theme_block_category( $categories ) {
);
return $categories;
}

add_filter( 'block_categories', 'gutenberg_register_theme_block_category' );
// This can be removed when plugin support requires WordPress 5.8.0+.
if ( ! function_exists( 'get_default_block_categories' ) ) {
add_filter( 'block_categories', 'gutenberg_register_theme_block_category' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question I have is whether these should be deprecated in the first place. I know you've thought about the _all prefix to extend all screens at the same time, did you consider using these existing filters for that use case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to use block_categories? It has the $post object as a param which is not set outside of the edit post page. We even provide examples for this filter that depend on $post 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm right :) if it's already receiving $post, it's not a good fit for the "all" use-case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeremyfelt raised the same issue in his comment on WordPress trac: https://core.trac.wordpress.org/ticket/52920#comment:24.

}

/**
* Checks whether the current block type supports the feature requested.
Expand Down
18 changes: 16 additions & 2 deletions lib/client-assets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,8 @@ function gutenberg_register_vendor_script( $scripts, $handle, $src, $deps = arra
/**
* Extends block editor settings to remove the Gutenberg's `editor-styles.css`;
*
* This can be removed when plugin support requires WordPress 5.8.0+.
*
* @param array $settings Default editor settings.
*
* @return array Filtered editor settings.
Expand Down Expand Up @@ -671,11 +673,18 @@ function gutenberg_extend_block_editor_styles( $settings ) {

return $settings;
}
add_filter( 'block_editor_settings', 'gutenberg_extend_block_editor_styles' );
// This can be removed when plugin support requires WordPress 5.8.0+.
if ( function_exists( 'get_block_editor_settings' ) ) {
add_filter( 'block_editor_settings_all', 'gutenberg_extend_block_editor_styles' );
} else {
add_filter( 'block_editor_settings', 'gutenberg_extend_block_editor_styles' );
}

/**
* Adds a flag to the editor settings to know whether we're in FSE theme or not.
*
* This can be removed when plugin support requires WordPress 5.8.0+.
*
* @param array $settings Default editor settings.
*
* @return array Filtered editor settings.
Expand All @@ -688,7 +697,12 @@ function gutenberg_extend_block_editor_settings_with_fse_theme_flag( $settings )

return $settings;
}
add_filter( 'block_editor_settings', 'gutenberg_extend_block_editor_settings_with_fse_theme_flag' );
// This can be removed when plugin support requires WordPress 5.8.0+.
if ( function_exists( 'get_block_editor_settings' ) ) {
add_filter( 'block_editor_settings_all', 'gutenberg_extend_block_editor_settings_with_fse_theme_flag' );
} else {
add_filter( 'block_editor_settings', 'gutenberg_extend_block_editor_settings_with_fse_theme_flag' );
}

/**
* Sets the editor styles to be consumed by JS.
Expand Down
7 changes: 6 additions & 1 deletion lib/editor-settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,12 @@ function gutenberg_extend_post_editor_settings( $settings ) {

return $settings;
}
add_filter( 'block_editor_settings', 'gutenberg_extend_post_editor_settings' );
// This can be removed when plugin support requires WordPress 5.8.0+.
if ( function_exists( 'get_block_editor_settings' ) ) {
add_filter( 'block_editor_settings_all', 'gutenberg_extend_post_editor_settings' );
} else {
add_filter( 'block_editor_settings', 'gutenberg_extend_post_editor_settings' );
}

/**
* Initialize a block-based editor.
Expand Down
10 changes: 9 additions & 1 deletion lib/global-styles.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ function gutenberg_experimental_global_styles_enqueue_assets() {
/**
* Adds the necessary data for the Global Styles client UI to the block settings.
*
* This can be removed when plugin support requires WordPress 5.8.0+.
*
* @param array $settings Existing block editor settings.
* @return array New block editor settings
*/
Expand Down Expand Up @@ -168,7 +170,13 @@ function gutenberg_experimental_global_styles_register_user_cpt() {
}

add_action( 'init', 'gutenberg_experimental_global_styles_register_user_cpt' );
add_filter( 'block_editor_settings', 'gutenberg_experimental_global_styles_settings', PHP_INT_MAX );
// This can be removed when plugin support requires WordPress 5.8.0+.
if ( function_exists( 'get_block_editor_settings' ) ) {
geriux marked this conversation as resolved.
Show resolved Hide resolved
add_filter( 'block_editor_settings_all', 'gutenberg_experimental_global_styles_settings', PHP_INT_MAX );
} else {
add_filter( 'block_editor_settings', 'gutenberg_experimental_global_styles_settings', PHP_INT_MAX );

}
add_action( 'wp_enqueue_scripts', 'gutenberg_experimental_global_styles_enqueue_assets' );


Expand Down
9 changes: 8 additions & 1 deletion lib/widgets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,14 +210,21 @@ function gutenberg_get_legacy_widget_settings() {
/**
* Extends default editor settings with values supporting legacy widgets.
*
* This can be removed when plugin support requires WordPress 5.8.0+.
*
* @param array $settings Default editor settings.
*
* @return array Filtered editor settings.
*/
function gutenberg_legacy_widget_settings( $settings ) {
return array_merge( $settings, gutenberg_get_legacy_widget_settings() );
}
add_filter( 'block_editor_settings', 'gutenberg_legacy_widget_settings' );
// This can be removed when plugin support requires WordPress 5.8.0+.
if ( function_exists( 'get_block_editor_settings' ) ) {
add_filter( 'block_editor_settings_all', 'gutenberg_legacy_widget_settings' );
} else {
add_filter( 'block_editor_settings', 'gutenberg_legacy_widget_settings' );
}

/**
* Function to enqueue admin-widgets as part of the block editor assets.
Expand Down
25 changes: 7 additions & 18 deletions phpunit/block-editor-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,17 +242,7 @@ function test_get_default_block_editor_settings() {
/**
* @ticket 52920
*/
function test_get_block_editor_settings_returns_default_settings() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is bad conceptually, at some point the default settings and the block editor settings might diverge. More importantly, in the context of the plugin hooks applied change the output of gutenberg_get_block_editor_settings 😄

$this->assertSameSets(
gutenberg_get_block_editor_settings( 'my-editor' ),
gutenberg_get_default_block_editor_settings()
);
}

/**
* @ticket 52920
*/
function test_get_block_editor_settings_overrides_default_settings_my_editor() {
function test_get_block_editor_settings_overrides_default_settings_all_editors() {
function filter_allowed_block_types_my_editor() {
return array( 'test/filtered-my-block' );
}
Expand All @@ -271,15 +261,15 @@ function filter_block_editor_settings_my_editor( $editor_settings ) {
return $editor_settings;
}

add_filter( 'allowed_block_types_my-editor', 'filter_allowed_block_types_my_editor', 10, 1 );
add_filter( 'block_categories_my-editor', 'filter_block_categories_my_editor', 10, 1 );
add_filter( 'block_editor_settings_my-editor', 'filter_block_editor_settings_my_editor', 10, 1 );
add_filter( 'allowed_block_types_all', 'filter_allowed_block_types_my_editor', 10, 1 );
add_filter( 'block_categories_all', 'filter_block_categories_my_editor', 10, 1 );
add_filter( 'block_editor_settings_all', 'filter_block_editor_settings_my_editor', 10, 1 );

$settings = gutenberg_get_block_editor_settings( 'my-editor' );

remove_filter( 'allowed_block_types_my-editor', 'filter_allowed_block_types_my_editor' );
remove_filter( 'block_categories_my-editor', 'filter_block_categories_my_editor' );
remove_filter( 'block_editor_settings_my-editor', 'filter_block_editor_settings_my_editor' );
remove_filter( 'allowed_block_types_all', 'filter_allowed_block_types_my_editor' );
remove_filter( 'block_categories_all', 'filter_block_categories_my_editor' );
remove_filter( 'block_editor_settings_all', 'filter_block_editor_settings_my_editor' );

$this->assertSameSets( array( 'test/filtered-my-block' ), $settings['allowedBlockTypes'] );
$this->assertSameSets(
Expand All @@ -297,7 +287,6 @@ function filter_block_editor_settings_my_editor( $editor_settings ) {

/**
* @ticket 52920
* @expectedDeprecated block_categories
* @expectedDeprecated block_editor_settings
*/
function test_get_block_editor_settings_deprecated_filter_post_editor() {
Expand Down