JSON API: preserve valid sub-400 WP_Error statuses in serializable_error()#50173
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
martian77
left a comment
There was a problem hiding this comment.
Although I agree with the spirit of the original fix, the WP_Error code supports the other codes and has been used this way in WPCOM code so this is probably the correct way to guard against invalid responses.
Thanks for making this change! 🙇
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
Proposed changes
PR #50077 hardened
WPCOM_JSON_API::serializable_error()so that aWP_Errorcan no longer serialize a bogus status (the incident where an array without astatus_codekey cast to1). That change did two things: it made the status extraction robust, and it also coerced every status below 400 up to 400.The second part was too broad. Some callers return a
WP_Errorwith a status below 400, for example a302redirect from the checkout flow, and a couple of older endpoints that return100or200with an error. Coercing those to400changed their behavior and started failing the Payments PayPal Express test suite.This PR keeps the robust extraction from #50077 and narrows the default back to only handle a missing or invalid status. A value that is missing, non-numeric, or otherwise resolves to
0still falls back to400(so the original incident stays fixed), but a valid HTTP status code, including one below 400, is now left as the caller set it.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
status_code, a non-numeric value, and no data all default to400), and that valid statuses, including100,200, and302, pass through unchanged.