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

Fix PHP warning in WP_REST_Global_Styles_Controller if no styles exist in theme.json #2224

Conversation

ntsekouras
Copy link

Trac ticket: https://core.trac.wordpress.org/ticket/54900

During this GB PR I noticed a PHP warning in WP_REST_Global_Styles_Controller where we are assuming that every theme.json has styles property, which is not always the case.
It's fixed in GB now, but we should add this to 5.9 next minor release.


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@@ -563,7 +563,8 @@ public function get_theme_item( $request ) {
}

if ( rest_is_field_included( 'styles', $fields ) ) {
$data['styles'] = $theme->get_raw_data()['styles'];
$raw_data = $theme->get_raw_data();
$data['styles'] = isset( $raw_data['styles'] ) ? $raw_data['styles'] : array();
Copy link
Member

Choose a reason for hiding this comment

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

I thought a little bit more about this and I think ideally if there are no styles we should just not include the styles in the rest response.

			$raw_data       = $theme->get_raw_data();
			if( isset( $raw_data['styles'] ) ) {
				$data['styles'] = $raw_data['styles'];
			}

We avoid sending unnecessary bytes over the wire and we are more consistent with what is defined in theme.json. In that case, theme.json is not defining anything for styles if theme.json defines styles: {} in that case we send the empty array.

We should just make sure the Javascript code is not crashing if styles are omitted.

Copy link
Author

Choose a reason for hiding this comment

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

In all my testing, there was no crash if styles weren't set. I could use some testing though from you @jorgefilipecosta or André that you've worked so much on these parts.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for updating your PR and testing it 👍 I will just do a final checkup and if everything seems ok commit the patch.

@ntsekouras ntsekouras force-pushed the fix/global-styles-controller-styles-warning branch from 493bdc8 to 3c4c4df Compare February 8, 2022 07:23
@ntsekouras ntsekouras force-pushed the fix/global-styles-controller-styles-warning branch from 3c4c4df to 6431c43 Compare February 8, 2022 07:24
@ntsekouras
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants