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

WP_Theme_JSON_Gutenberg Unit tests: fix phpunit warnings about set_spacing_sizes #55313

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

matiasbenedetto
Copy link
Contributor

@matiasbenedetto matiasbenedetto commented Oct 12, 2023

What?

WP_Theme_JSON_Gutenberg unit tests: fix phpunit warnings about set_spacing_sizes().

Why?

To avoid the warnings on the console:

There were 5 warnings:

1) WP_Theme_JSON_Gutenberg_Test::test_set_spacing_sizes_when_invalid with data set "invalid_spacing_scale_values_missing_operator" (array('', 1.5, 1, 4, 'rem'), null)
Expecting E_STRICT, E_NOTICE, and E_USER_NOTICE is deprecated and will no longer be possible in PHPUnit 10.

2) WP_Theme_JSON_Gutenberg_Test::test_set_spacing_sizes_when_invalid with data set "invalid_spacing_scale_values_non_numeric_increment" (array('+', 'add two to previous value', 1, 4, 'rem'), null)
Expecting E_STRICT, E_NOTICE, and E_USER_NOTICE is deprecated and will no longer be possible in PHPUnit 10.

3) WP_Theme_JSON_Gutenberg_Test::test_set_spacing_sizes_when_invalid with data set "invalid_spacing_scale_values_non_numeric_steps" (array('+', 1.5, 'spiral staircase preferred', 4, 'rem'), null)
Expecting E_STRICT, E_NOTICE, and E_USER_NOTICE is deprecated and will no longer be possible in PHPUnit 10.

4) WP_Theme_JSON_Gutenberg_Test::test_set_spacing_sizes_when_invalid with data set "invalid_spacing_scale_values_non_numeric_medium_step" (array('+', 1.5, 5, 'That which is just right', 'rem'), null)
Expecting E_STRICT, E_NOTICE, and E_USER_NOTICE is deprecated and will no longer be possible in PHPUnit 10.

5) WP_Theme_JSON_Gutenberg_Test::test_set_spacing_sizes_when_invalid with data set "invalid_spacing_scale_values_missing_unit" (array('+', 1.5, 5, 4), null)
Expecting E_STRICT, E_NOTICE, and E_USER_NOTICE is deprecated and will no longer be possible in PHPUnit 10.

How?

using core function _doing_it_wrong() to throw the notice and using setExpectedIncorrectUsage to assert the incorrect usage notice.

Testing Instructions

Run PHP unit tests with and without this change.
You should not see the warning running this PR.

@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

@matiasbenedetto matiasbenedetto added [Type] Enhancement A suggestion for improvement. [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Oct 12, 2023
@matiasbenedetto matiasbenedetto removed the [Type] Enhancement A suggestion for improvement. label Oct 12, 2023
Copy link
Contributor

@glendaviesnz glendaviesnz left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for fixing this.

@matiasbenedetto matiasbenedetto merged commit bbbb0a1 into trunk Oct 13, 2023
50 of 53 checks passed
@matiasbenedetto matiasbenedetto deleted the fix/set_spacing_size_test_warnings branch October 13, 2023 14:31
@github-actions github-actions bot added this to the Gutenberg 16.9 milestone Oct 13, 2023
@anton-vlasenko
Copy link
Contributor

anton-vlasenko commented Oct 13, 2023

This code is in Core, so I wonder how changing trigger_error() to _doing_it_wrong() could affect backward compatibility, @matiasbenedetto.

@matiasbenedetto
Copy link
Contributor Author

Thanks @glendaviesnz and @anton-vlasenko for the review.

This code is in Core, so I wonder how changing trigger_error() to _doing_it_wrong() could affect backward compatibility.

I think this is not in core because this is the Gutenberg version of this class (WP_Theme_JSON_Gutenberg). Apart from that _doing_it_wrong() function is just calling trigger_error() as the line this PR replaces, just in a standardized way:

https://github.com/WordPress/wordpress-develop/blob/781953641607c4d5b0743a6924af0e820fd54871/src/wp-includes/functions.php#L5860-L5928

So I think there won't be disruptions.

@anton-vlasenko
Copy link
Contributor

I've explained why replacing trigger_error() with _doing_it_wrong() could be an issue here: #55354.
I would appreciate your review, @matiasbenedetto.

anton-vlasenko added a commit that referenced this pull request Oct 16, 2023
trigger_error( __( 'Some of the theme.json settings.spacing.spacingScale values are invalid', 'gutenberg' ), E_USER_NOTICE );
_doing_it_wrong( __METHOD__, __( 'Some of the theme.json settings.spacing.spacingScale values are invalid', 'gutenberg' ), '6.1.0' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

But I can also see that Gutnberg trunk does not contain this change

if ( ! empty( $spacing_scale ) ) {
trigger_error( __( 'Some of the theme.json settings.spacing.spacingScale values are invalid', 'gutenberg' ), E_USER_NOTICE );
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I appears this change was reverted in #55354

Copy link
Contributor

Choose a reason for hiding this comment

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

...and phpunit changes were commit as part of WordPress/wordpress-develop#5911

@getdave getdave added Needs PHP backport Needs PHP backport to Core and removed Needs PHP backport Needs PHP backport to Core labels Jan 25, 2024
@getdave
Copy link
Contributor

getdave commented Jan 25, 2024

✅ I updated the PHP Sync Tracking Issue for WP 6.5 to note this PR does not require a backport for WP 6.5.

Why? Because related changes were backported in #55313 instead.

cc @matiasbenedetto and @anton-vlasenko for 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[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