Stats: avoid Fatal errors when trying to fetch malformed data#35746
Stats: avoid Fatal errors when trying to fetch malformed data#35746
Conversation
This is a follow-up to #34557. It was discussed a bit here: https://github.com/Automattic/jetpack/pull/34557/files#r1475310509 I ran into the following issue in the post editor today: ``` PHP Fatal error: Uncaught TypeError: array_merge(): Argument #2 must be of type array, WP_Error given in wp-content/plugins/jetpack/jetpack_vendor/automattic/jetpack-stats/src/class-wpcom-stats.php:453 Stack trace: wp-content/plugins/jetpack/jetpack_vendor/automattic/jetpack-stats/src/class-wpcom-stats.php(453): array_merge() wp-content/plugins/jetpack/jetpack_vendor/automattic/jetpack-stats/src/class-wpcom-stats.php(215): Automattic\Jetpack\Stats\WPCOM_Stats->fetch_post_stats() ``` This added security fixes the issue.
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
darssen
left a comment
There was a problem hiding this comment.
Changes look sensible to me. I was able to test the flow after adding a Blog Stats showing the data for the individual post and things went smooth.
I added a small comment on the initial check but it is not a blocker.
| if ( | ||
| ! is_array( $data ) | ||
| || empty( $data ) | ||
| || is_wp_error( $data ) |
There was a problem hiding this comment.
Do we need is_wp_error here? Feels like ! is_array( $data ) should have taken care of it.
There was a problem hiding this comment.
I wasn't sure, but it was the only check that was there before my change so I figured it may cover a use-case I haven't thought about.
I think I'll leave it here for now. Better be safe than sorry. :)
|
Thanks for fixing this @jeherve 👍 |
Proposed changes:
This is a follow-up to #34557. It was discussed a bit here:
https://github.com/Automattic/jetpack/pull/34557/files#r1475310509
I ran into the following issue in the post editor today:
The added safety checks in this PR fix the issue, by not assuming anything about the format of the returned data.
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
add_filter( 'jetpack_blocks_variation', function () { return 'beta'; } );