Don't cast wp_error object to int on failure#471
Don't cast wp_error object to int on failure#471superbuggy wants to merge 1 commit into10up:developfrom superbuggy:master
Conversation
|
Hi @superbuggy! Thanks for opening this PR. I can see how the int cast could cause buggy behavior. As @helen pointed out elsewhere, this feels like part of a larger issue we have with the plugin we plan to address in an upcoming release: that a lot of failures have little to no messaging about them. Can you describe the issue you are trying to resolve with this change or the bug you experienced? If you have time, can you please create an Issue (https://github.com/10up/distributor/issues/new/choose) describing the bug you are seeing, then we can discuss a plan to resolve there. |
| $post_id = intval( $_POST['postId'] ); | ||
| $remote_id = $external_connection->push( $post_id, $push_args ); No newline at end of file |
There was a problem hiding this comment.
were these lines left by accident?
| } else { | ||
| $external_push_results[ (int) $connection['id'] ] = array( | ||
| 'post_id' => (int) $remote_id, | ||
| 'post_id' => $post_id, |
There was a problem hiding this comment.
can we add an error attribute here with the (error) results from $external_connection->push? That will set us up for displaying the error on the front end.
jeffpaul
added
type:bug
|
@superbuggy Thanks for this PR. Wondering if you've had a chance to review the feedback @adamsilverstein left here or if you're looking for more direction on this PR? Thanks! |
|
@superbuggy are you able to respond to the feedback on this PR? |
|
Hi @superbuggy, in #630 we update the error reporting system, so I'm closing this PR. Thanks again for your contribution! If you have any questions or updates, feel free to comment/open this PR again. |
5 participants
Description of the Change
Avoid casting an error into an int (
1) and labeling it as a post_id.Alternate Designs
n/a
Benefits
Clear information about the failure rather than an arbitrary value coercion.
Possible Drawbacks
None come to mind.
Verification Process
Test failing push.
Checklist: