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

Check that block type styles are an array before foreach. #4014

Conversation

tellthemachines
Copy link
Contributor

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

Tries to fix this PHP warning .


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.

Copy link
Member

@SergeyBiryukov SergeyBiryukov left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@hellofromtonya
Copy link
Contributor

The more I think about this change, the more concerned I am that it is masking a real problem of doing it wrong. The root cause is not this code, but rather, a block type being registered with 'styles' property set as something other than an array. That's doing it wrong.

By introducing a is_array() here, it will mask the problem by skipping over this code. That can make it harder for a developer to debug their block type and styles.

So I think leaving the PHP Warning is a better practice as it'll serve to alert the developer something is wrong. The Warning though won't tell them that they've registered styles of their block type incorrectly. That's where this issue can help WordPress/gutenberg#48039.

I think this PR should not be committed. But instead, the test that is incorrectly registering a block type should be fixed. I've opened a new Trac ticket #57706 to fix the test that is throwing the PHP Warning.

@tellthemachines
Copy link
Contributor Author

The more I think about this change, the more concerned I am that it is masking a real problem of doing it wrong. The root cause is not this code, but rather, a block type being registered with 'styles' property set as something other than an array. That's doing it wrong.

That makes sense! It's more useful and informative to validate the block data and warn if it's the wrong type.

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.

3 participants