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

Add type check user cpt array #45275

Closed
wants to merge 1 commit into from

Conversation

cbravobernal
Copy link
Contributor

What?

Fixes #45240

Why?

If $user_cpt is not an array. array_key_exists( 'post_content', $user_cpt ) function can throw a Fatal error or a warning.

How?

Add the is_array() check to the conditional.

Testing Instructions

Screenshots or screencast

@cbravobernal cbravobernal added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Oct 25, 2022
@ockham ockham added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 25, 2022
Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

If $user_cpt was null I think this would still create a notice?

@ockham
Copy link
Contributor

ockham commented Oct 25, 2022

If $user_cpt was null I think this would still create a notice?

TBH I don't quite see why? is_array( $user_cpt ) seems perfectly fine with $user_cpt being null, and that check should short-circuit the whole if conditional and skip the array_key_exists one. Or am I missing something? 🤔

@ockham
Copy link
Contributor

ockham commented Oct 25, 2022

FWIW, I don't mind adding the in_array check, since it seems like it can't have any negative side effects.

Ideally, I'd like to be able to reproduce the issue (see) and verify the fix.

Finally, I'd like to understand which code path leads to $user_cpt being null here? Seems like get_user_data_from_wp_global_styles would fall back to an empty array pretty much always 🤔

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Okay, LGTM. See #45275 (comment) for some more thoughts.

@desrosj
Copy link
Contributor

desrosj commented Oct 25, 2022

I'm a bit hesitant to add a type check here without understanding what's actually going on at a deeper level. I worry this could be silencing an error that would alert someone of a problem in their code.

I'm a little confused as to the steps I should be using to try and reproduce this. Is the contributing factor using unregister_post_type( 'wp_global_styles' ) (which in my opinion is not expected and can't be guaranteed to result in a bug-free experience), or deleting the wp_global_styles post that holds the site's global styles (as @ockham's testing on WordPress/wordpress-develop#3525 reflects)?

@spacedmonkey
Copy link
Member

I think a unit test would be useful here. But the change looks good to me.

@ockham
Copy link
Contributor

ockham commented Oct 26, 2022

Update: Per this discussion, we've decided not to include this in 6.1. Tentatively bumping to 6.1.1.

@ockham ockham added Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 26, 2022
@Mamaduka
Copy link
Member

This is no longer an issue in the plugin or in the core. See: #45240 (comment).

@Mamaduka Mamaduka removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Nov 10, 2022
@Mamaduka
Copy link
Member

@c4rl0sbr4v0, I think we can close this and the core backport PR.

@scruffian scruffian deleted the fix/45240-add-type-check-user-cpt branch November 16, 2022 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Fatal error when user CPT for Global Styles is deleted
6 participants