alleyinteractive / apple-news

The Publish to Apple News plugin enables your WordPress blog content to be published to your Apple News channel.
https://wordpress.org/plugins/publish-to-apple-news/
GNU General Public License v3.0
153 stars 71 forks source link

update_post_metadata Filter Can Crash on PHP 8 #1034

Closed alamedapost closed 8 months ago

alamedapost commented 8 months ago

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

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!

kevinfodness commented 8 months ago

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?

alamedapost commented 8 months ago

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 commented 8 months ago

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

alamedapost commented 8 months ago

Classic and Elementor. I’ve never used Gutenberg.

Yes, Wordpress 6.4.2

--

http://linkedin.com/company/alameda-post/ [image: google+ image of google+ icon] https://instagram.com/alameda.post https://facebook.com/alamedapost

Donate Now! https://omella.com/AlamedaPost

Adam Gillitt, Publisher

(He/him/his)

https://alamedapost.com

Phone 510.263.8176 Mobile 510.457.1421 Email @.*** Web alamedapost.com Office 1516 Oak St. #203, Alameda, CA 94501

Alameda Post Inc. is a 501(c)(3) nonprofit corporation. Our tax ID is 874572048.

On Wed, Jan 3, 2024 at 8:55 AM Kevin Fodness @.***> wrote:

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

— Reply to this email directly, view it on GitHub https://github.com/alleyinteractive/apple-news/issues/1034#issuecomment-1875676757, or unsubscribe https://github.com/notifications/unsubscribe-auth/BFAA2BZS7SIAMIRDILVECCTYMWEPRAVCNFSM6AAAAABBG6Q7JOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZVGY3TMNZVG4 . You are receiving this because you authored the thread.Message ID: @.***>

kevinfodness commented 8 months ago

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.

alamedapost commented 8 months ago

Thank you for the detailed explanation.

To clarify, I don’t use Elementor for individual posts, just for pages.

This bug has been happening for a while, so our production staff is used to flipping off the plug-in before creating a new post, but it will be nice to not have to do that anymore.

Thanks again.

--

http://linkedin.com/company/alameda-post/ [image: google+ image of google+ icon] https://instagram.com/alameda.post https://facebook.com/alamedapost

Donate Now! https://omella.com/AlamedaPost

Adam Gillitt, Publisher

(He/him/his)

https://alamedapost.com

Phone 510.263.8176 Mobile 510.457.1421 Email @.*** Web alamedapost.com Office 1516 Oak St. #203, Alameda, CA 94501

Alameda Post Inc. is a 501(c)(3) nonprofit corporation. Our tax ID is 874572048.

On Wed, Jan 3, 2024 at 9:09 AM Kevin Fodness @.***> wrote:

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.

— Reply to this email directly, view it on GitHub https://github.com/alleyinteractive/apple-news/issues/1034#issuecomment-1875695493, or unsubscribe https://github.com/notifications/unsubscribe-auth/BFAA2B65Z54GHFJZZI6PYB3YMWGC5AVCNFSM6AAAAABBG6Q7JOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZVGY4TKNBZGM . You are receiving this because you authored the thread.Message ID: @.***>

attackant commented 8 months ago

@alamedapost Do you want to test out https://github.com/alleyinteractive/apple-news/pull/1037 and see if this fixes your issue? I didn't see any other likely issues with usages of count() elsewhere.

alamedapost commented 8 months ago

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 commented 8 months ago

Unfortunately, it did not solve the issue. Some images work, others do not. This is an error from one that did not.

2024/01/03 23:33:44 [error] 120216#120216: *1465164 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/apple-news-release-v2.4.1/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, 55127, '_thumbnail_id', 55210, '')

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, 55127, '_thumbnail_id', 55210, '')

3 /www/alamedapost_328/public/wp-includes/post.php(2558):

update_metadata('post', 55127, '_thumbnail_id', 55210, '')

4 /www/alamedapost_328/public/wp-includes/post.php(7657):

update_post_meta(55127, '_thumbnail_id', 55210)

5 /www/alamedapost_328/public/wp-includes/post.php(4625):

set_post_thumbnail(Object(WP_Post), 55210)

6 /www/alamedapost_328/public/wp-includes/post.php(4862):" while reading

response header from upstream, client: 2600:1700:442:2ed0:80a8:8bf4:2975:60b0, 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=55127&action=edit"

--

http://linkedin.com/company/alameda-post/ [image: google+ image of google+ icon] https://instagram.com/alameda.post https://facebook.com/alamedapost

Donate Now! https://omella.com/AlamedaPost

Adam Gillitt, Publisher

(He/him/his)

https://alamedapost.com

Phone 510.263.8176 Mobile 510.457.1421 Email @.*** Web alamedapost.com Office 1516 Oak St. #203, Alameda, CA 94501

Alameda Post Inc. is a 501(c)(3) nonprofit corporation. Our tax ID is 874572048.

On Wed, Jan 3, 2024 at 1:02 PM Damian @.***> wrote:

Closed #1034 https://github.com/alleyinteractive/apple-news/issues/1034 as completed via 58c6e81 https://github.com/alleyinteractive/apple-news/commit/58c6e819e2420a2f65b9020afc8a09a781d18a04 .

— Reply to this email directly, view it on GitHub https://github.com/alleyinteractive/apple-news/issues/1034#event-11383150014, or unsubscribe https://github.com/notifications/unsubscribe-auth/BFAA2BZBR57HPMHGUN5SRBTYMXBQDAVCNFSM6AAAAABBG6Q7JOVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJRGM4DGMJVGAYDCNA . You are receiving this because you were mentioned.Message ID: @.*** com>

attackant commented 8 months ago

thanks for the follow up... investigating.

attackant commented 8 months ago

@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 commented 8 months ago

No, it's not. 559 is

if ( 1 === count( $old_value ) ) {

I will delete and re-upload and check again

--

http://linkedin.com/company/alameda-post/ [image: google+ image of google+ icon] https://instagram.com/alameda.post https://facebook.com/alamedapost

Donate Now! https://omella.com/AlamedaPost

Adam Gillitt, Publisher

(He/him/his)

https://alamedapost.com

Phone 510.263.8176 Mobile 510.457.1421 Email @.*** Web alamedapost.com Office 1516 Oak St. #203, Alameda, CA 94501

Alameda Post Inc. is a 501(c)(3) nonprofit corporation. Our tax ID is 874572048.

On Wed, Jan 3, 2024 at 4:00 PM Damian @.***> wrote:

@alamedapost https://github.com/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.

— Reply to this email directly, view it on GitHub https://github.com/alleyinteractive/apple-news/issues/1034#issuecomment-1876128782, or unsubscribe https://github.com/notifications/unsubscribe-auth/BFAA2B6KZRC2EQOLBZZHRF3YMXWIZAVCNFSM6AAAAABBG6Q7JOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZWGEZDQNZYGI . You are receiving this because you were mentioned.Message ID: @.***>

attackant commented 8 months ago

Yes, that's the old version then.

alamedapost commented 8 months ago

How may I download the updated version again? The version i have saved locally does not have the correct code at line 559.

--

http://linkedin.com/company/alameda-post/ [image: google+ image of google+ icon] https://instagram.com/alameda.post https://facebook.com/alamedapost

Donate Now! https://omella.com/AlamedaPost

Adam Gillitt, Publisher

(He/him/his)

https://alamedapost.com

Phone 510.263.8176 Mobile 510.457.1421 Email @.*** Web alamedapost.com Office 1516 Oak St. #203, Alameda, CA 94501

Alameda Post Inc. is a 501(c)(3) nonprofit corporation. Our tax ID is 874572048.

On Wed, Jan 3, 2024 at 4:05 PM Damian @.***> wrote:

Yes, that's the old version then.

— Reply to this email directly, view it on GitHub https://github.com/alleyinteractive/apple-news/issues/1034#issuecomment-1876132995, or unsubscribe https://github.com/notifications/unsubscribe-auth/BFAA2BYDPD65C3XBPVVGMFDYMXW45AVCNFSM6AAAAABBG6Q7JOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZWGEZTEOJZGU . You are receiving this because you were mentioned.Message ID: @.***>

alamedapost commented 8 months ago

Or do i just need to edit that one line?

attackant commented 8 months ago

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

alamedapost commented 8 months ago

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

attackant commented 8 months ago

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

alamedapost commented 8 months ago

Thank you Damian. We got through the rest of production with no crashes.

--

http://linkedin.com/company/alameda-post/ [image: google+ image of google+ icon] https://instagram.com/alameda.post https://facebook.com/alamedapost

Donate Now! https://omella.com/AlamedaPost

Adam Gillitt, Publisher

(He/him/his)

https://alamedapost.com

Phone 510.263.8176 Mobile 510.457.1421 Email @.*** Web alamedapost.com Office 1516 Oak St. #203, Alameda, CA 94501

Alameda Post Inc. is a 501(c)(3) nonprofit corporation. Our tax ID is 874572048.

On Wed, Jan 3, 2024 at 4:17 PM Damian @.***> wrote:

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

— Reply to this email directly, view it on GitHub https://github.com/alleyinteractive/apple-news/issues/1034#issuecomment-1876142766, or unsubscribe https://github.com/notifications/unsubscribe-auth/BFAA2B6NDLDIXD5Z2X64V6TYMXYKLAVCNFSM6AAAAABBG6Q7JOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZWGE2DENZWGY . You are receiving this because you were mentioned.Message ID: @.***>

kevinfodness commented 8 months ago

@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 commented 8 months ago

Thanks very much, it is installed.

--

http://linkedin.com/company/alameda-post/ [image: google+ image of google+ icon] https://instagram.com/alameda.post https://facebook.com/alamedapost

Donate Now! https://omella.com/AlamedaPost

Adam Gillitt, Publisher

(He/him/his)

https://alamedapost.com

Phone 510.263.8176 Mobile 510.457.1421 Email @.*** Web alamedapost.com Office 1516 Oak St. #203, Alameda, CA 94501

Alameda Post Inc. is a 501(c)(3) nonprofit corporation. Our tax ID is 874572048.

On Thu, Jan 4, 2024 at 1:31 PM Kevin Fodness @.***> wrote:

@alamedapost https://github.com/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.

— Reply to this email directly, view it on GitHub https://github.com/alleyinteractive/apple-news/issues/1034#issuecomment-1877787980, or unsubscribe https://github.com/notifications/unsubscribe-auth/BFAA2B655A55TTEVVH3572DYM4NT7AVCNFSM6AAAAABBG6Q7JOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZXG44DOOJYGA . You are receiving this because you were mentioned.Message ID: @.***>