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

Add support for multiple aggregations #2682

Merged
merged 4 commits into from
Mar 29, 2022
Merged

Conversation

felipeelia
Copy link
Member

Description of the Change

Currently, the plugin only supports a single array as the aggs parameter of WP_Query. This PR changes that to also support an array of aggregations. This will be needed to implement #1440 in a cleaner way.

Closes #636

Changelog Entry

Added: Support for an array of aggregations in the aggs parameter of WP_Query.

Credits

Props @felipeelia

@felipeelia felipeelia added this to the 4.1.0 milestone Mar 27, 2022
@felipeelia felipeelia self-assigned this Mar 27, 2022
$formatted_args['aggs'][ $agg_name ]['aggs'] = $agg_obj['aggs'];
if ( isset( $args['aggs'] ) ) {
// An array of aggregations.
if ( isset( $args['aggs'][0] ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to check if this is an array with is_array() ? Right now I don't think it would do any harm, but i'm thinking if there's ever a case where we offer an aggregations filter for example, and people can manipulate the array indexes. If there's not $args['aggs'][0] but there's $args['aggs'][1] this code will not run.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've made the changes we discussed in slack @oscarssanchez . Do you mind giving yet another look at it? Thanks!

@felipeelia felipeelia merged commit 256db79 into develop Mar 29, 2022
@felipeelia felipeelia deleted the feature/multiple-aggs branch March 29, 2022 17:04
felipeelia added a commit that referenced this pull request Mar 30, 2022
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.

Support multiple aggregations
2 participants