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.

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

Don't change error handling in WP_Theme_JSON_Gutenberg::set_spacing_sizes() #55354

Conversation

anton-vlasenko
Copy link
Contributor

@anton-vlasenko anton-vlasenko commented Oct 13, 2023

What?

#55313 changed error handling in WP_Theme_JSON_Gutenberg::set_spacing_sizes(). Since changes from WP_Theme_JSON_Gutenberg will eventually be merged into Core, it's important to maintain parity between both codebases.

Why?

_doing_it_wrong() is not a direct replacement for trigger_error(), since _doing_it_wrong() only triggers a PHP error if WP_DEBUG is enabled.

How?

This PR reverts the changes introduced in #55313 while maintaining PHPUnit 10 compatibility.

Testing Instructions

  1. Make sure that the WP_Theme_JSON_Gutenberg_Test tests pass.

Testing Instructions for Keyboard

Screenshots or screencast

@github-actions
Copy link

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/class-wp-theme-json-gutenberg.php
❔ phpunit/class-wp-theme-json-test.php

@anton-vlasenko anton-vlasenko marked this pull request as ready for review October 13, 2023 15:23
@anton-vlasenko anton-vlasenko self-assigned this Oct 13, 2023
@anton-vlasenko anton-vlasenko added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Oct 13, 2023
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

The changes look good to me 👍

@anton-vlasenko anton-vlasenko merged commit 624f430 into trunk Oct 16, 2023
51 of 55 checks passed
@anton-vlasenko anton-vlasenko deleted the fix/dont-throw-doing-it-wrong-in-wp-theme-json-gutenberg-constructor branch October 16, 2023 10:51
@anton-vlasenko
Copy link
Contributor Author

Thank you for the review and approval, @jorgefilipecosta! I apreciate it.

@anton-vlasenko
Copy link
Contributor Author

@getdave getdave added the Needs PHP backport Needs PHP backport to Core label Jan 22, 2024
@getdave
Copy link
Contributor

getdave commented Jan 22, 2024

Thank you for working on this @anton-vlasenko 👍

FYI I updated the PHP Sync Tracking Issue to:

  • add the Trac link
  • add the Core PR backport link
  • remove the completion checkmark as this has yet to be merged into Core

Please could I ask you to check back on the Tracing Issue when the Core PR is merged and mark the line item as complete? This will help us keep track of what's outstanding. Very much appreciated 🙏

@youknowriad youknowriad added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Needs PHP backport Needs PHP backport to Core labels Jan 23, 2024
@anton-vlasenko
Copy link
Contributor Author

@getdave, the PR has been merged, and I've marked it as complete in the PHP Sync Tracking issue. Thank you.

@getdave
Copy link
Contributor

getdave commented Jan 25, 2024

Thank you Anton 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants