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.

Sign up for GitHub

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

Jump to bottom

update_post_metadata Filter Can Crash on PHP 8 #1034

Closed
alamedapost opened this issue Dec 29, 2023 · 21 comments
Closed

update_post_metadata Filter Can Crash on PHP 8 #1034

alamedapost opened this issue Dec 29, 2023 · 21 comments
Assignees
@attackant
Labels
bug Something isn't working php Requires understanding PHP
Milestone
v2.4.1

Comments

@alamedapost
Copy link

alamedapost commented Dec 29, 2023 •
edited by kevinfodness
Loading

PHP 8 now throws a TypeError on values that are not iterable. In at least one location there is a case where the plugin does not check the return type from a function and can crash if the function returns something that is not iterable.

Acceptance Criteria

  • Analyze the code for uses of the count() function and ensure that, where necessary, a check is done to ensure the value is iterable before passing to the function.

Original Submission

When plugin is enabled, setting a featured image and saving the post cause the entire site to crash. Only workaround is to disable plugin while creating posts then reactivate plugin before publication

Steps To Reproduce

have plugin enabled
create new post
assign featured image
save
crash!
@alamedapost alamedapost added the bug Something isn't working label Dec 29, 2023
@kevinfodness
Copy link
Member

I can't reproduce this locally on WordPress 6.4.2 with Publish to Apple News 2.4.0. It's likely that there is a conflict with another plugin or with your theme. Can you see what the error message is in the PHP logs?

@kevinfodness kevinfodness added evaluating and removed bug Something isn't working labels Jan 3, 2024
@alamedapost
Copy link
Author

2024/01/03 16:11:05 [error] 120216#120216: *1408060 FastCGI sent in stderr: "PHP message: PHP Fatal error: Uncaught TypeError: count(): Argument #1 ($value) must be of type Countable|array, int given in /www/alamedapost_328/public/wp-content/plugins/publish-to-apple-news/includes/class-apple-news.php:559
Stack trace:
#0 /www/alamedapost_328/public/wp-includes/class-wp-hook.php(324): Apple_News->filter_update_post_metadata(NULL, 55065, '_thumbnail_id', 55084, '')
#1 /www/alamedapost_328/public/wp-includes/plugin.php(205): WP_Hook->apply_filters(NULL, Array)
#2 /www/alamedapost_328/public/wp-includes/meta.php(235): apply_filters('update_post_met...', NULL, 55065, '_thumbnail_id', 55084, '')
#3 /www/alamedapost_328/public/wp-includes/post.php(2558): update_metadata('post', 55065, '_thumbnail_id', 55084, '')
#4 /www/alamedapost_328/public/wp-includes/post.php(7657): update_post_meta(55065, '_thumbnail_id', 55084)
#5 /www/alamedapost_328/public/wp-includes/post.php(4625): set_post_thumbnail(Object(WP_Post), 55084)
#6 /www/alamedapost_328/public/wp-includes/post.php(4862): wp_i" while reading response header from upstream, client: 2600:1700:ab14:8a00:4c30:d48c:5e19:e42b, server: alamedapost.com, request: "POST /wp-admin/post.php HTTP/2.0", upstream: "fastcgi://unix:/var/run/php8.0-fpm-alamedapost.sock:", host: "alamedapost.com:29052", referrer: "https://alamedapost.com/wp-admin/post.php?post=55065&action=edit"

@kevinfodness
Copy link
Member

Are you using Gutenberg or the Classic Editor? Are you on WordPress 6.4.2?

@alamedapost
Copy link
Author

alamedapost commented Jan 3, 2024 via email

@kevinfodness
Copy link
Member

I tested this using the Classic Editor plugin and it works properly there also. I did not test it using Elementor.

The line in question is comparing the result of get_metadata with the $single parameter set to false (the default) which should return an array, but for some reason is returning an int on your site. I suspect that there's a filter at play in third party or custom code that you have on your site that is returning the wrong data type. Calling get_metadata with $single set to false should return an array of all postmeta values for that key, and it should be an empty array if no values are set, an array with one item if only one is set, etc. You may want to look for instances of the filter get_post_metadata which may not be respecting the $single parameter and would return an integer instead of an array.

I'm going to apply the bug label on this issue though, because the plugin could be doing a better job of validating the data type before using it.

@kevinfodness kevinfodness added bug Something isn't working and removed evaluating labels Jan 3, 2024
@kevinfodness kevinfodness changed the title Site crashes with plugin enabled update_post_metadata Filter Can Crash on PHP 8 Jan 3, 2024
@alamedapost
Copy link
Author

alamedapost commented Jan 3, 2024 via email

@attackant attackant self-assigned this Jan 3, 2024
@attackant attackant added this to the v2.4.1 milestone Jan 3, 2024
@attackant attackant mentioned this issue Jan 3, 2024
Merged
@attackant
Copy link
Member

attackant commented Jan 3, 2024 •
edited
Loading

@alamedapost Do you want to test out #1037 and see if this fixes your issue? I didn't see any other likely issues with usages of count() elsewhere.

@alamedapost
Copy link
Author

Just uploaded 2.4.1 and did a quick test with an empty post and added the featured image. The post saved successfully with no crash. We will test further during today's production cycle and report back if there are any problems.

Thank you!

@alamedapost
Copy link
Author

alamedapost commented Jan 3, 2024 via email

@attackant
Copy link
Member

thanks for the follow up... investigating.

@attackant attackant reopened this Jan 3, 2024
@attackant
Copy link
Member

@alamedapost Can you confirm that /www/alamedapost_328/public/wp-content/plugins/apple-news-release-v2.4.1/includes/class-apple-news.php:559 is this?
if ( false !== $old_value && is_array( $old_value ) && 1 === count( $old_value ) ) {
In that case the fatal should not be possible, so I'm wondering if the old version of that line is somehow still being loaded.

@alamedapost
Copy link
Author

alamedapost commented Jan 4, 2024 via email

@attackant
Copy link
Member

Yes, that's the old version then.

@alamedapost
Copy link
Author

alamedapost commented Jan 4, 2024 via email

@alamedapost
Copy link
Author

Or do i just need to edit that one line?

@attackant
Copy link
Member

You can try editing that line or download the zip here: https://github.com/alleyinteractive/apple-news/releases/tag/v2.4.1

@alamedapost
Copy link
Author

Thanks. I'll let you know if that does not solve the issue in a little bit... I really appreciate the assistance.

@attackant
Copy link
Member

The new release won't be on wordpress.org until tomorrow probably.

@alamedapost
Copy link
Author

alamedapost commented Jan 4, 2024 via email

@kevinfodness
Copy link
Member

@alamedapost the new version just went live about an hour ago (v2.4.3). If you update, this should fix the issue for you. Please let me know if it does not.

@alamedapost
Copy link
Author

alamedapost commented Jan 4, 2024 via email

@kevinfodness kevinfodness added the php Requires understanding PHP label Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@attackant attackant

Labels
bug Something isn't working php Requires understanding PHP
Projects
None yet
Milestone
v2.4.1
Development

No branches or pull requests
3 participants
@kevinfodness @attackant @alamedapost