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

Remove action to fix tests. #54806

Merged
merged 3 commits into from
Sep 26, 2023
Merged

Remove action to fix tests. #54806

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7744d77
Remove action to fix tests.
spacedmonkey Sep 25, 2023
4d14f40
Try fixing '__unstableResolvedAssets'
Mamaduka Sep 26, 2023
08ce95e
Try temp e2e test fixes
Mamaduka Sep 26, 2023
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
4 changes: 4 additions & 0 deletions lib/compat/wordpress-6.3/script-loader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,13 @@ function _gutenberg_get_iframed_editor_assets() {
}
}

// Remove the deprecated `print_emoji_styles` handler.
// It avoids breaking style generation with a deprecation message.
remove_action( 'wp_print_styles', 'print_emoji_styles' );
ob_start();
wp_print_styles();
$styles = ob_get_clean();
add_action( 'wp_print_styles', 'print_emoji_styles' );

ob_start();
wp_print_head_scripts();
Expand Down
4 changes: 4 additions & 0 deletions lib/compat/wordpress-6.4/script-loader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,13 @@ function _gutenberg_get_iframed_editor_assets_6_4() {
}
}

// Remove the deprecated `print_emoji_styles` handler.
// It avoids breaking style generation with a deprecation message.
remove_action( 'wp_print_styles', 'print_emoji_styles' );
Comment on lines +161 to +163
Copy link
Contributor

Choose a reason for hiding this comment

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

While this works, it feels a little hacky to me. Why do we need to remove core actions in order for the output to work correctly?

Copy link
Member

Choose a reason for hiding this comment

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

The deprecation print_emoji_styles function is still hooked into wp_print_styles for backward compatibility. Core handles its removal differently, so we must resort to manual removal.

I'm open to suggestions for a better fix.

ob_start();
wp_print_styles();
$styles = ob_get_clean();
add_action( 'wp_print_styles', 'print_emoji_styles' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check to see if remove_action() is truthy before re-adding the action? I think that would mean we'd only add it back in again if it was originally present 🤔

Copy link
Member

Choose a reason for hiding this comment

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

This is a common pattern for temporarily removing actions. We do the same above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Where do we do it above? Sorry if I'm missing something obvious!

I can see add_filter( 'should_load_block_editor_scripts_and_styles', '__return_false' ); followed by remove_filter( 'should_load_block_editor_scripts_and_styles', '__return_false' );. This one is different in a subtle way, as it first removes before then adding. If when this function is called there was never an action registered for print_emoji_styles (e.g. it was unregistered elsewhere), then the add_action could restore it unexpectedly?

I'm a little unsure of the consequences of making changes here, which is why I'm a little bit cautious about the changes. I don't quite understand the scope of what was introduced in WordPress/wordpress-develop#4824, so am a tiny bit nervous about the fix in this PR, particularly since this PR didn't have any explanation as to why the fix was required.

I'd probably lean more toward reviewing whether WordPress/wordpress-develop#4824 should be reverted, as it introduced a regression in core. That said, I'm very much in favour of pragmatic fixes that unblock merging in Gutenberg, so please don't take my comments here as a blocker to landing this PR if it fixes things 🙂


ob_start();
wp_print_head_scripts();
Expand Down
1 change: 1 addition & 0 deletions phpunit/experimental/block-editor-settings-mobile-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class Gutenberg_REST_Block_Editor_Settings_Controller_Test extends WP_Test_REST_
public function set_up() {
parent::set_up();
switch_theme( 'block-theme' );
remove_action( 'wp_print_styles', 'print_emoji_styles' );
}

/**
Expand Down