-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix: #57469 fatal error when a sidebar widgets set to null #9603
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: #57469 fatal error when a sidebar widgets set to null #9603
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
…` and `wp_map_sidebars_widgets()`
|
@DarkMatter-999 thanks for adding phpunit tests. Here's my test report for this PR: Test ReportDescriptionThis report validates whether the indicated patch works as expected. Patch tested: #9603 Environment
Actual Results
Additional Notes
Delete existing configInsert config with null valuesGet config to confirm new valuesGet config after switching themes in wp-admin |
src/wp-includes/widgets.php
Outdated
|
|
||
| // Replace null values inside the array with an empty array. | ||
| foreach ( $sidebars_widgets as $key => $value ) { | ||
| if ( null === $value ) { |
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.
What if it was set to something other than an array or null, like a string, boolean, integer, or object? Wouldn't this be more appropriate:
| if ( null === $value ) { | |
| if ( ! is_array( $value ) ) { |
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.
moving forward with this approach
src/wp-includes/widgets.php
Outdated
|
|
||
| // Replace null values inside the array with an empty array. | ||
| foreach ( $new_sidebars_widgets as $key => $value ) { | ||
| if ( null === $value ) { |
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.
| if ( null === $value ) { | |
| if ( ! is_array( $value ) ) { |
src/wp-includes/widgets.php
Outdated
| // Replace null values inside the array with an empty array. | ||
| foreach ( $sidebars_widgets as $key => $value ) { | ||
| if ( null === $value ) { | ||
| $sidebars_widgets[ $key ] = array(); | ||
| } | ||
| } |
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.
Alternatively:
| // Replace null values inside the array with an empty array. | |
| foreach ( $sidebars_widgets as $key => $value ) { | |
| if ( null === $value ) { | |
| $sidebars_widgets[ $key ] = array(); | |
| } | |
| } | |
| $sidebars_widgets = array_filter( $sidebars_widgets, 'is_array' ); |
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.
I think checking for is_array will be a better solution and is a drop in replacement because it preserves sidebar keys and guarantees any downstream operations receive arrays, this is consistent with how older PHP version like 7.4 handle array_merge and array_search.
Using array_filter($sidebars_widgets, 'is_array') will remove entries that are not arrays, which can drop keys and change control flow in the downstream code that expects those keys to exist.
src/wp-includes/widgets.php
Outdated
| // Replace null values inside the array with an empty array. | ||
| foreach ( $new_sidebars_widgets as $key => $value ) { | ||
| if ( null === $value ) { | ||
| $new_sidebars_widgets[ $key ] = array(); | ||
| } | ||
| } |
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.
Alternatively:
| // Replace null values inside the array with an empty array. | |
| foreach ( $new_sidebars_widgets as $key => $value ) { | |
| if ( null === $value ) { | |
| $new_sidebars_widgets[ $key ] = array(); | |
| } | |
| } | |
| $new_sidebars_widgets = array_filter( $new_sidebars_widgets, 'is_array' ); |
westonruter
left a comment
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 looks good. Just a minor suggestion to merge a couple assertions.
Trac ticket: https://core.trac.wordpress.org/ticket/57469
The PR ensures that any
NULLvalues in thesidebars_widgetsarray are replaced with empty arrays during widget retrieval and mapping.This is needed as PHP 8.x introduces stricter type and warning behaviors that can break assumptions made in earlier versions.
Code used to testing:
Before:
sidebar-null-values-before.mov
After:
sidebar-null-values-after.mov