Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WC_Object_Query & WC_Order_Query #14733

Merged
merged 18 commits into from May 7, 2017
Merged

WC_Object_Query & WC_Order_Query #14733

merged 18 commits into from May 7, 2017

Conversation

claudiulodro
Copy link
Contributor

@claudiulodro claudiulodro commented Apr 26, 2017

Relates to #13646.

This is the implementation of WC_Object_Query and WC_Order_Query. The next PR will integrate these into WC_Order_Data_Store_CPT::get_orders and wc_get_orders.

Functions like `wc_get_orders` are almost certainly used widely and in a million different ways, so I think we should take a conservative approach to releasing the query helpers: Have them just hanging out and available for developers to use in 3.1 to iron out any problems or things we didn't think of. In 3.2, we can update existing code like `wc_get_orders` to use the query objects knowing that many developers will have switched over to the query objects and the query objects are bug-free. This is similar to how WP stretched out releasing term meta over a few releases. Thoughts?
Never mind. Overthought this ^.

Copy link
Member

@mikejolley mikejolley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functions like wc_get_orders are almost certainly used widely and in a million different ways, so I think we should take a conservative approach to releasing the query helpers

Why are you concerned with this? wc_get_orders only has a handful of props which I imagine will be included in this helper?

All the function would need to keep is the backwards compatibility handling, with which it would convert the old props to the new props for the helper.

These helpers should act as a way to get objects or ids for WC core objects. I don't think they need to match WP_Query exactly, simply because it does a lot of bloated unnecessary things we shouldn't have to worry about.

Search Helper is the layer devs work with. Search Helper then has an internal layer to translate the supported args -> WP_Query (or if we ever had custom tables, our own queries instead).

Does that make sense?

protected function get_default_query_vars() {

return array(
'tax_query' => array(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do args like this feel too deeply coupled to WP_Query? The whole idea of meta_query/tax_query even things like meta_key and meta_value - these don't feel like things we should be carrying over to an abstract search helper.

I feel we should start small, support core props and some more advanced searches, like date ranges, but handle the translation between this and WP_Query internally, behind the scenes...

* @param mixed $value Value to set for query variable.
*/
public function set( $query_var, $value ) {
$this->query_vars[$query_var] = $value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spaces around [ ]

'exact' => '',
'sentence' => '',

'fields' => '',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this too. We should support ids and objects. These follow out CRUD more closely.

* Stores query data.
* @var array
*/
public $query_vars = array();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep this protected and have a getter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I think I was trying to make this too similar to WP_Query. I'll try and steer away from that from now on. :)

'in' => array(),
'not_in' => array(),

'status' => array( 'publish', 'pending', 'draft', 'future', 'private', 'inherit' ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we leave this out and let the sub-classes deal with it? They may not match WP 1:1

'parent__in' => array(),
'parent__not_in' => array(),
'in' => array(),
'not_in' => array(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in and not_in seem less useful to me - that kind of inclusion/exclusion would be easy to handle externally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed wc_get_orders has exclude. Maybe that is ok to use? in though... not sure why you'd need to search if you have the ids already?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not_in/exclude comes in handy a lot. e.g. if you want to get products that aren't the current product. in isn't very useful. I'll change not_in to exclude and ditch in.


'status' => array( 'publish', 'pending', 'draft', 'future', 'private', 'inherit' ),

'per_page' => -1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wc_get_orders has page, limit, offset. Personally prefer that over per_page.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also has paginate which returns some extra page args if used which is handy I guess. Maybe good to factor in to the return values?

'order' => 'DESC',
'orderby' => 'date',

'date_before' => '',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should leave this one to subclasses and make it more prop based. e.g. orders have date_paid, date_completed, date_created and so on which could benefit from these type of query.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes that may make the later queries harder to map/generate, but I think it's more useful.

* @param array $query_vars query vars from a WC_Object_Query
* @return array
*/
public function get_wp_query_args( $query_vars ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why public?

@claudiulodro
Copy link
Contributor Author

@mikejolley I've implemented your requested changes. I think this will be easier to maintain and migrate to custom tables now. 👍

Copy link
Member

@mikejolley mikejolley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some timezone questions to resolve.

Is this all we need for v1? Provide some examples/what is supported?

$this->query_vars = wp_parse_args( $args, $this->get_default_query_vars() );
}

public function get_query_vars() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docblock


if ( isset( $query_vars[ 'date_created_after'] ) && '' !== $query_vars[ 'date_created_after' ] ) {
$wp_query_args['date_query'][] = array(
'column' => 'post_date',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we work with GMT or store timezone here or support both?

if ( isset( $query_vars[ 'date_completed_before' ] ) && '' !== $query_vars[ 'date_completed_before' ] && strtotime( $query_vars[ 'date_completed_before' ] ) ) {
$wp_query_args['meta_query'][] = array(
'key' => '_date_completed',
'value' => strtotime( $query_vars[ 'date_completed_before' ] ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may also be confusing when you think about timezones.

return $query->posts;
}

$orders = array_map( 'wc_get_order', $query->posts );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filter out bad results with array_filter

@mikejolley mikejolley added needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. and removed Status: Needs Review labels Apr 28, 2017
@claudiulodro claudiulodro added Status: Needs Review and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Apr 28, 2017
@claudiulodro
Copy link
Contributor Author

I'm going to make a wiki for the object query args, similar to the WP_Query docs. You can query for any combination of the props in WC_Object_Query::get_default_query_vars and WC_Order_Query::get_default_query_vars. There's a few examples in the unit tests at tests/unit-tests/order/query.php.

I'm going to make a ticket for integrating WC_Order_Query into wc_get_orders and other similar situations for the next sprint. The 3.0.5 release took up my morning. :)

@mikejolley
Copy link
Member

@claudiulodro This looks good to go so #14795 can proceed. I do have one more idea which could be implemented to change the date queries, but I'll note it in #13646 for feedback.

@mikejolley mikejolley merged commit dd3be12 into master May 7, 2017
@mikejolley mikejolley deleted the feature/13646 branch May 7, 2017 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants