Skip to content

Remove action to fix tests. #54806

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

Merged
spacedmonkey merged 3 commits into from
Sep 26, 2023
Merged

Remove action to fix tests. #54806

Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
Try fixing '__unstableResolvedAssets'
  • Loading branch information
@Mamaduka
Mamaduka committed Sep 26, 2023
commit 4d14f40c4febd08c11d3b1f0703d592e9b5c59ec
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

@andrewserong andrewserong Sep 26, 2023

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

@Mamaduka Mamaduka Sep 26, 2023

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

@andrewserong andrewserong Sep 26, 2023

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

@Mamaduka Mamaduka Sep 26, 2023

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

@andrewserong andrewserong Sep 26, 2023

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