-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Remove action to fix tests. #54806
Conversation
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.3/script-loader.php ❔ lib/compat/wordpress-6.4/script-loader.php ❔ phpunit/experimental/block-editor-settings-mobile-test.php |
It looks like Puppeteer tests 1 & 3 are failing consistently with 500 errors. I'm not sure the cause, but these appear to have started failing around the same time as WordPress/wordpress-develop@54c4de1 landed (WordPress/wordpress-develop#4824). In running Could the deprecated messages in core be causing unintended side effects? I'm not too sure the context here since this PR doesn't have any explanatory comments, but just thought I'd mention it here since this PR appears to try to resolve a failing test related to the change to |
@andrewserong, can you test this again? I think 4d14f40 should fix the remaining issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewserong, can you test this again? I think 4d14f40 should fix the remaining issues.
Thanks @Mamaduka! That appears to fix the issue with the editor styles for me.
Also, it seems like a pragmatic change to get things passing again 👍. Let's see what happens with the e2e tests, as there were some other Puppeteer test failures that were failing that may or may not have been related to the iframed 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' ); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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' ); |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙂
It seems e2e test failures might have different causes. Looking into those now. |
Thanks! It very well could be different. @glendaviesnz and I did a little digging today, but haven't come up with much. It seems that some of the date related failures could be due to a missing Save button. (Screenshots thanks to Glen!) Then, other tests in Puppeteer 3 appear to fail with a 500 error when going to create a new post: The error message is:
That seems to be to do with setting the So there very well could have been a different commit around a similar time as WordPress/wordpress-develop#4824 causing an unrelated issue 🤔 Edit: potentially this one: WordPress/wordpress-develop@8d8b843 ? |
I think we're on the correct path. Both test suites start failing after one of the tests tries to modify the timezone via |
I found a consistent way to reproduce the issue:
To reset settings, go to There's a good chance that WordPress/wordpress-develop@8d8b843 caused the issue. Update - cause of the bug
ScreencastCleanShot.2023-09-26.at.09.03.51.mp4 |
I just pushed a temporary e2e test fixed to unblock development on the repo. |
Size Change: +53 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
@Mamaduka The follow bug bug regarding |
Thanks, @felixarntz! I'll revert the e2e test changes. |
I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: 3d51b98 |
* use the wporg cdn (#54795) * core-data: Fix nested property access with undefined name (#54790) * core-data: Fix nested property access with undefined name * Add a unit test * Social Links: add X (#54092) * Social Links: add X Fixes #53223 * Add Twitter keyword to variation This will allow people to find the new icon when searching for Twitter. See #53223 (comment) * Reorder links alphabetically Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> * No need for a capital letter Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> * Fix svg attributes See #54092 (comment) Co-authored-by: Rich Tabor <hi@richtabor.com> * Remove "icon" Co-authored-by: Nick Diego <nickmdiego@gmail.com> * Update X icon path See #54092 (comment) See #54092 (comment) --------- Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: Nick Diego <nickmdiego@gmail.com> * remove font files created by tests after tests run (#54771) * Check that new pattern is synced before replacing existing blocks with a synced pattern (#54804) * Patterns: Improve sentence case consistency of labels and notices (#54807) * Navigation block: fix padding on mobile overlay when global padding is 0 (#53725) * force min value for padding to be 2rem * fallback for when the css variables are not defined * Allow the padding to be smaller than 2rem * Add fix to avoid trigger hover state on links when overlay opens --------- Co-authored-by: scruffian <ben@scruffian.com> Co-authored-by: Dave Smith <getdavemail@gmail.com> * Always show the total number of patterns even with only one page (#54813) * Always show the total number of patterns even with only one page * Add to explorer too * Hide total number of 0 * Font Library: Avoid rendering Font Library UI outside Gutenberg plugin (#54830) * Remove action to fix tests. (#54806) * Conditionally remove deprecated 'print_emoji_styles' (#54828) * Fix Performance tests * Fix global styles revision --------- Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> Co-authored-by: Jeremy Herve <jeremy@jeremy.hu> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: Nick Diego <nickmdiego@gmail.com> Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com> Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> Co-authored-by: Maggie <maggie.cabrera@automattic.com> Co-authored-by: scruffian <ben@scruffian.com> Co-authored-by: Dave Smith <getdavemail@gmail.com> Co-authored-by: Kai Hao <kevin830726@gmail.com> Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
What?
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast