-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fonts API: Add is_array check before using array_merge #55974
Conversation
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/experimental/fonts-api/class-wp-fonts-resolver.php |
Flaky tests detected in e50f0a5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6800859890
|
Alternative #55981 |
Was looking at #55981. It checks both values separately which I think would be better. Also have another question: #55981 (comment). |
@@ -202,6 +202,11 @@ private static function get_settings() { | |||
$settings = static::set_tyopgraphy_settings_array_structure( $settings ); | |||
} | |||
|
|||
// Skip if typography.fontFamilies.theme from the global settings and the variation are not arrays. | |||
if ( ! is_array( $settings['typography']['fontFamilies']['theme'] ) && ! is_array( $variation['settings']['typography']['fontFamilies']['theme'] ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this should be ||
not &&
as one of these may be null or perhaps not set? Also perhaps checking them separately may be better? Then the $settings['typography']['fontFamilies']['theme']
can probably be above the loop as it cannot change to non-array in the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense, thank you!
I noticed #55981 is probably closer to a better fix, so happy to close this PR is favour of that. Or also happy to continue working on this one if it ends up being needed.
Going to close this in favour of #55981. |
What?
Adds an
is_array
check for$settings['typography']['fontFamilies']['theme']
and$variation['settings']['typography']['fontFamilies']['theme']
before usingarray_merge
on both values.Why?
To prevent potential errors such as
Uncaught TypeError: array_merge(): Argument #1 must be of type array
.How?
Checks if both values are arrays, and if not, skips the current variation and moves on to the next one.