Skip to content

Fix test failures due to WP 6.4 change #881

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

Jump to bottom
Merged
merged 2 commits into from
Nov 14, 2023
Merged

Fix test failures due to WP 6.4 change #881

merged 2 commits into from
Nov 14, 2023

Conversation

felixarntz
Copy link
Member

@felixarntz felixarntz commented Nov 13, 2023 •
edited
Loading

Summary

As of the WordPress 6.4 release, our unit tests are failing. This hasn't happened yet in trunk, but only because no PRs have been opened since the release. It is however obvious in https://github.com/WordPress/performance/actions/runs/6851370708/job/18627496026?pr=864.

The failures happen simply because of deprecation warnings for print_emoji_styles() which per core behavior remains added as a callback for the wp_print_styles action but normally gets removed via another core function before it would be executed. In certain custom situations though (such as tests), it may be necessary to manually remove it. Even core itself does that for example in https://github.com/WordPress/wordpress-develop/blob/d66ef46847e2340c613d0a01c36d4fcc4b5f3a6b/src/wp-includes/block-editor.php#L363.

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@felixarntz felixarntz added [Type] Bug An existing feature is broken [Focus] JS & CSS no milestone PRs that do not have a defined milestone for release labels Nov 13, 2023
This was referenced Nov 13, 2023
Merged
Merged
@swissspidy
Copy link
Member

I feel like we should reconsider the approach with how this function is handled in core. I don't see why the function still needs to be hooked into wp_print_styles just so everyone has to unhook it again. That seems like worse DX than just unhooking it by default.

See also https://core.trac.wordpress.org/ticket/59892

@felixarntz
Copy link
Member Author

@swissspidy

I feel like we should reconsider the approach with how this function is handled in core. I don't see why the function still needs to be hooked into wp_print_styles just so everyone has to unhook it again. That seems like worse DX than just unhooking it by default.

This was done for backward compatibility with existing code. It's a clunky DX, but it's a tradeoff between breaking BC and clunky DX. I don't feel strongly either way. cc @westonruter

I've addressed your code feedback in a3bc55d.

@westonruter
Copy link
Member

What is this about:

Warning: file_put_contents(/var/www/html/wp-content/plugins/performance/.phpunit.result.cache): failed to open stream: Permission denied in phar:///usr/local/bin/phpunit/phpunit/Runner/DefaultTestResultCache.php on line 111

@swissspidy
Copy link
Member

This was done for backward compatibility with existing code.

Yes I saw that in the code comments, but I wonder if it actually addresses a real backward compatibility issue.

@swissspidy swissspidy merged commit 5b95f6b into trunk Nov 14, 2023
@swissspidy swissspidy deleted the fix/wp-64-test-incompatibility branch November 14, 2023 14:28
@felixarntz felixarntz mentioned this pull request Nov 14, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonruter westonruter westonruter approved these changes

@swissspidy swissspidy swissspidy approved these changes

@10upsimon 10upsimon Awaiting requested review from 10upsimon

@manuelRod manuelRod Awaiting requested review from manuelRod

Assignees
No one assigned
Labels
no milestone PRs that do not have a defined milestone for release [Type] Bug An existing feature is broken
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@felixarntz @swissspidy @westonruter