-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fatal error when wxr_cdata() is called with non-string value
#10595
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
Conversation
|
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. |
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. |
src/wp-admin/includes/export.php
Outdated
| if ( ! is_string( $str ) ) { | ||
| return ''; | ||
| } |
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 believe I may be contradicting a previous suggestion I had here, but here's the latest informed by my latest research:
| if ( ! is_string( $str ) ) { | |
| return ''; | |
| } | |
| if ( ! is_string( $str ) && is_scalar( $str ) ) { | |
| $str = (string) $str; | |
| } |
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.
Or:
| if ( ! is_string( $str ) ) { | |
| return ''; | |
| } | |
| $str = (string) $str; |
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.
Hi @westonruter, I guess the first suggestion should be something like below,
if ( ! is_string( $str ) || is_scalar( $str ) ) {
$str = (string) $str;
}This is because, what if data is null? ! is_string( null ) would evaluate to true, but is_scalar( null ) would evaluate to false, hence $str will remain as is and would not change to string, it would then gives the fatal for 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.
I would like to go by casting it to str whatever value it is as per - https://core.trac.wordpress.org/ticket/64347#comment:14
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.
Oh, interesting. I wasn't aware that is_scalar( null ) is false. That's news to me and counter-intuitive.
| $wpdb->update( | ||
| $wpdb->posts, | ||
| array( | ||
| 'post_excerpt' => null, |
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.
The post_excerpt field actually cannot be null:
| post_excerpt text NOT NULL, |
Therefore, this test doesn't test a null value here. I'll remove this test.
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.
Thanks, but I have added that the usecase that even setting null would automatically convert to string, which will pass anyhow we add null or not.
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.
Well, I tried running these tests against trunk without the fix applied, and this test didn't error, which means that null wasn't passed to wxr_cdata().
| $wpdb->update( | ||
| $wpdb->users, | ||
| array( | ||
| 'display_name' => null, |
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.
The display_name cannot be null:
| display_name varchar(250) NOT NULL default '', |
So this isn't actually passing a null value to wxr_cdata(). I'll remove this test.
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 to me.
Review from Gemini:
The changes look solid and address the reported fatal error by explicitly casting the input to wxr_cdata() to a string, which satisfies the strict type requirement of wp_is_valid_utf8(). The added test cases cover posts, comments, and term meta, ensuring the fix works across different content types that might contain NULL meta values.
Here is a review of the changes:
src/wp-admin/includes/export.php
wxr_cdata():- The explicit cast
$str = (string) $str;correctly handlesNULLvalues by converting them to an empty string, preventing theTypeErrorinwp_is_valid_utf8(). - The DocBlock update to
@param string|null $straccurately reflects that the function can now safely accept (and expects)nullvalues from the database.
- The explicit cast
tests/phpunit/tests/admin/exportWp.php
- New Tests:
test_export_with_null_postmeta_values(): Correctly reproduces the issue by using$wpdb->insert()to bypass potentially strict sanitization inadd_post_meta()that might otherwise castNULLbefore insertion.test_export_with_null_comment_values()andtest_export_with_null_term_meta_values(): Appropriately cover the other metadata tables.- The assertions
assertNotFalse( $xml )and the check forchannel->itemcount are sufficient to verify the export process completed without a fatal error.
General
- Coding Standards: The code adheres to WordPress coding standards (spacing, indentation, casting).
- Safety: The fix is backward compatible with the behavior of
seems_utf8()(which was used previously) regardingNULLvalues, restoring functionality for exports containingNULLmeta.
I have no requested changes. The patch is ready for commit.
…ng passed value to `string`. This ensures that `wp_is_valid_utf8()` does not cause a type error since it only accepts strings. Developed in #10595 Follow-up to [60630]. Props hbhalodia, westonruter, desrosj, albigdd, jorbin. See #38044. Fixes #64347. git-svn-id: https://develop.svn.wordpress.org/trunk@61405 602fd350-edb4-49c9-b593-d223f7449a82
…ng passed value to `string`. This ensures that `wp_is_valid_utf8()` does not cause a type error since it only accepts strings. Developed in WordPress/wordpress-develop#10595 Follow-up to [60630]. Props hbhalodia, westonruter, desrosj, albigdd, jorbin. See #38044. Fixes #64347. Built from https://develop.svn.wordpress.org/trunk@61405 git-svn-id: http://core.svn.wordpress.org/trunk@60717 1a063a9b-81f0-0310-95a4-ce76da25c4cd
…ng passed value to `string`. This ensures that `wp_is_valid_utf8()` does not cause a type error since it only accepts strings. Developed in WordPress/wordpress-develop#10595 Follow-up to [60630]. Props hbhalodia, westonruter, desrosj, albigdd, jorbin. See #38044. Fixes #64347. Built from https://develop.svn.wordpress.org/trunk@61405 git-svn-id: https://core.svn.wordpress.org/trunk@60717 1a063a9b-81f0-0310-95a4-ce76da25c4cd
wxr_cdatafunction, and typecast the incoming input to string.Trac ticket: https://core.trac.wordpress.org/ticket/64347
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.