-
Notifications
You must be signed in to change notification settings - Fork 19
Orders: Add support for multiple statuses in list request #31
Conversation
|
|
||
| // Set post_status. | ||
| if ( isset( $status ) ) { | ||
| $args['post_status'] = array_map( function( $status ) { return 'wc-' . $status; }, $status ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work on PHP 5.2 :( #blamewordpress
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I saw this page and assumed WC had bumped to higher requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish that was the minimum, but alas we support 5.2 like wordpress. https://github.com/woocommerce/woocommerce/blob/master/readme.txt#L91
| $args = parent::prepare_objects_query( $request ); | ||
|
|
||
| // Set post_status. | ||
| if ( isset( $status ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will always be isset because you set it above. You need to move the isset to check $request['status']
| protected function prepare_objects_query( $request ) { | ||
| global $wpdb; | ||
|
|
||
| // This is a hack to get around WC_REST_Orders_Controller::prepare_objects_query |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why this hack is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't really need this code, since you can override after the results from WC_REST_Orders_Controller::prepare_objects_query.
Or even filter it using woocommerce_rest_shop_order_object_query.
I agree that is required to change the value to not get any "array to string" notice, but it's possible change in the old class too to help with it and avoid any hack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think filtering will help here, the $request is still sent with the status array causing the array to string notice. I've updated the comment to be a bit clearer about why it's unsetting status.
If we can update the core endpoint whenever this is merged, that would be great, but since we probably want this in by the calypso rollout on thursday, this seems best for now.
| * @return array | ||
| */ | ||
| protected function prepare_objects_query( $request ) { | ||
| global $wpdb; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not required here.
…mment, remove unused global var
|
@mikejolley @claudiosanches Can one of you give this another look today? I've fixed most of the feedback, the only thing I didn't really change was the post_status work around, but I explained better in the comment and left my reasoning here. |
mikejolley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryelle This is good to merge pending 1 change. There is some PHP 5.4 only syntax in this which should be swapped out.
| unset( $request['status'] ); | ||
| $args = parent::prepare_objects_query( $request ); | ||
|
|
||
| $args['post_status'] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is PHP 5.4.0 + only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙁 I blame javascript — fixed now.
emdashcodes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
This PR updates the order parameter schema to accept a list of statuses (still using only the set of
get_order_statuses), and the query builder to use an array of statuses. Now you can make a request for both completed and cancelled orders:I also updated the version number in the PR, though we don't need to make a release immediately after merging if anything else should be updated.
To test:
WC_Helper_Order::create_order)/orders?status=pending,processing,on-holdFixes #26