Feature/faceting #7

jakewrfoster · 2022-04-25T16:34:10Z

Overview

Adds DSL class for creating query ES DSL.
Adds aggregations to VIP Adapter for the following types:
- WP Category
- WP Tag
- Custom taxonomies
- WP Post date*
- WP Post Type
Allows for enabling faceting on empty search queries for VIP Search

*Post date DSL is in this PR, but it is unfinished.

…pter

…aceting for VIP Search

renatonascalves

💯

Left a few suggestions!

renatonascalves · 2022-04-26T13:32:06Z

lib/adapters/class-adapter.php

+ /**
+ * Enables faceting on empty search query strings.
+ */
+ public function enable_empty_search_faceting(): void {


Do we need a disable option?

since it is likely that we would want to have faceting on in most cases, perhaps this should be changed to a disable function and the option defaults to true.

to address your question more directly, I don't think a function to revert this one is needed since this is something that would either be on or off for the full duration of a request lifecycle - it only needs to be set once.

The defaults are also different by platform. SearchPress, for example, has this on by default, whereas ElasticPress / VIP Enterprise Search has it off by default.

renatonascalves · 2022-04-26T13:34:41Z

lib/adapters/class-vip-enterprise-search.php

+ case 'taxonomy':
+ $query['query']['function_score']['query']['bool']['must'][] = [
+ 'terms' => [
+ 'terms.' . $configured_facets[ $facet_label ]['taxonomy'] . '.slug' => [


Is it possible or easy to also map via term_id as well as slug? I think the current VIP Search widget has a filter that allows for that.

I'm not sure I follow - what would we gain by adding an additional clause here?

It's my understanding it is another option. You could match by the term_id instead of the term slug.

This might be useful in combination with the suggestion above about how the data makes its way to this plugin for applying the facets. On a headless site, it might use the term ID instead of the term slug. However, the idea behind this plugin is that it handles faceting for you, so being opinionated about how it does that is fine. If we discover that we need additional flexibility around accepting facet configurations via REST or GraphQL we could revisit the idea of allowing this to be a term ID instead of a term slug.

renatonascalves · 2022-04-26T13:36:57Z

lib/adapters/class-vip-enterprise-search.php

 *
 * @param array $request_args Request arguments.
 * @param string $path Request path.
 * @param string $index Index name.
 * @param string $type Index type.
 *
 * @return array New request arguments.
+ * @see \ElasticPress\Elasticsearch


The @see tag SHOULD have a description to provide additional information regarding the relationship between the element and its target.

Source: https://docs.phpdoc.org/guide/references/phpdoc/tags/see.html

The location of the tag is also not the recommended placement.

renatonascalves · 2022-04-26T13:40:27Z

lib/class-controller.php

+ *
+ * @return Controller The instance of the class to allow for chaining.
+ */
+ public function enable_empty_search_faceting() {


Or Controller?

Suggested change

public function enable_empty_search_faceting() {

public function enable_empty_search_faceting(): self {

: Controller

renatonascalves · 2022-04-26T13:41:45Z

lib/adapters/class-vip-enterprise-search.php

- // Parse face data.
+ /*
+ * Parse face data. Added on 11 to ensure that the results and aggs have been set.
+ * @see VIP_Enterprise_Search::set_results()


Same phpDoc suggestion.

renatonascalves · 2022-04-26T13:41:56Z

lib/adapters/class-vip-enterprise-search.php

+ *
+ * @param array $formatted_args Formatted ES args.
+ * @param array $args WP args.
+ * @see \ElasticPress\Indexable\Post\Post


Same phpDoc suggestion.

renatonascalves · 2022-04-26T13:44:20Z

lib/adapters/class-vip-enterprise-search.php

+ *
+ * @param array $query
+ * @return array
+ * @see \ElasticPress\Indexable\Post\Post.


Same phpDoc suggestion.

renatonascalves · 2022-04-26T13:44:32Z

lib/adapters/class-vip-enterprise-search.php

+ * @return array
+ * @see \ElasticPress\Indexable\Post\Post.
+ */
+ public function add_facets_to_ep_query( $query ) {


Suggested change

public function add_facets_to_ep_query( $query ) {

public function add_facets_to_ep_query( $query ): array {

renatonascalves · 2022-04-26T13:45:25Z

lib/adapters/class-vip-enterprise-search.php

+ * Add facets to EP query.
+ * Filters `ep_post_formatted_args`.
+ *
+ * @param array $query


Is this a WP_Query? An EP query? A custom query? It is an array so unlikely to be a WP query....

Comes from an EP filter. It is an array containing the formatted query.

renatonascalves · 2022-04-26T13:47:53Z

lib/adapters/class-vip-enterprise-search.php

+ */
+ public function add_facets_to_ep_query( $query ) {
+ // Do we have any facets specified?
+ $searched_facets = get_query_var( 'fs' );


This makes the assumption that only searches using the global query are relevant. Headless projects, like Irving, don't always use the global wp-query in its searching functionality.

good point. do you have a suggestion on how we might account for headless projects? cc @kevinfodness

Yeah, we could make a generic function that accepts searched facets as an argument, which could be populated here using get_query_var or could be populated from a request to GraphQL or the REST API's search endpoint, and could also be filtered. Adding support for a standard structure in REST and GraphQL makes sense, and then the filter would allow for further customization on a per-site basis. However, this feels like something we can add as an enhancement later vs. being a blocker for merging this PR.

Feature/post date facets

kevinfodness

This is great. Left some suggestions. 🍣

kevinfodness · 2022-04-28T15:26:50Z

lib/adapters/class-adapter.php

+ }
+
+ foreach ( $items as $item ) {
+ $datum = apply_filters( 'elasticsearch_extensions_facet_datum', false, $item, $this->facets );


Need docblocks for any custom action hooks or filters.

kevinfodness · 2022-04-28T15:29:21Z

lib/adapters/class-adapter.php

+ /**
+ * Enables faceting on empty search query strings.
+ */
+ public function enable_empty_search_faceting(): void {


The defaults are also different by platform. SearchPress, for example, has this on by default, whereas ElasticPress / VIP Enterprise Search has it off by default.

kevinfodness · 2022-04-28T15:30:56Z

lib/adapters/class-adapter.php

+ /**
+ * Enables an aggregation based on WP Tags.
+ */
+ public function enable_tag_aggregation(): void {


Is this because WordPress handles categories and tags differently in terms of what it calls them in query construction? Since we need to be able to enable aggregations on taxonomies generally (to support custom taxonomies) I'm wondering whether it makes sense to have categories and tags be treated in a "special" way here.

kevinfodness · 2022-04-28T15:33:23Z

lib/adapters/class-vip-enterprise-search.php

+ */
+ public function add_facets_to_ep_query( $query ) {
+ // Do we have any facets specified?
+ $searched_facets = get_query_var( 'fs' );


Yeah, we could make a generic function that accepts searched facets as an argument, which could be populated here using get_query_var or could be populated from a request to GraphQL or the REST API's search endpoint, and could also be filtered. Adding support for a standard structure in REST and GraphQL makes sense, and then the filter would allow for further customization on a per-site basis. However, this feels like something we can add as an enhancement later vs. being a blocker for merging this PR.

kevinfodness · 2022-04-28T15:35:32Z

lib/adapters/class-vip-enterprise-search.php

+ case 'taxonomy':
+ $query['query']['function_score']['query']['bool']['must'][] = [
+ 'terms' => [
+ 'terms.' . $configured_facets[ $facet_label ]['taxonomy'] . '.slug' => [


This might be useful in combination with the suggestion above about how the data makes its way to this plugin for applying the facets. On a headless site, it might use the term ID instead of the term slug. However, the idea behind this plugin is that it handles faceting for you, so being opinionated about how it does that is fine. If we discover that we need additional flexibility around accepting facet configurations via REST or GraphQL we could revisit the idea of allowing this to be a term ID instead of a term slug.

kevinfodness · 2022-04-28T15:39:29Z

lib/class-controller.php

+ $this->adapter->enable_post_date_aggregation();
+ $defaults = [
+ 'count' => 1000,
+ 'calendar_interval' => 'month',


Recommend defaulting to year here. I think bucketing by year is likely the most common scenario, and would result in the smallest number of buckets, and folks could zoom in from there if they need/want to.

kevinfodness · 2022-04-28T15:40:03Z

lib/class-controller.php

 if ( isset( $this->adapter ) ) {
 $this->adapter->enable_post_type_aggregation();
+ $defaults = [
+ 'count' => 1000,
+ 'name' => 'Post Type',


Strings like these should be translated if they aren't being pulled from already-translated strings (like taxonomy labels).

kevinfodness · 2022-04-28T15:40:46Z

lib/class-controller.php

 if ( isset( $this->adapter ) ) {
 $this->adapter->enable_taxonomy_aggregation( $taxonomy );
+ $defaults = [
+ 'count' => 1000,
+ 'name' => $taxonomy,


Here we could extract the name from the taxonomy object based on its slug.

kevinfodness · 2022-04-28T15:41:56Z

lib/class-facet.php

+/**
+ * Elasticsearch facet/aggregation.
+ */
+class Facet {


Same comment here - recommend calling this Aggregation instead of Facet

kevinfodness · 2022-04-28T15:42:28Z

lib/class-facet.php

+ *
+ * @var string
+ */
+ public string $subtype;


Nitpicky, but I like to alphabetize my class properties and method names to keep everything organized.

kevinfodness · 2022-04-28T18:43:14Z

I'm going to merge this so I can build on it.

jakewrfoster added 19 commits April 13, 2022 09:11

add facet types

7a33bfb

add field mapping to controller

73e6c6f

map core fields to adapters

7b1aaa3

add DSL

6b82b23

add facet parsing, set/get for config, getter for facet data

633ef4a

add tax aggregations to vip adapter

eef0570

add tag and category aggs to vip adapter, refactor map_* funcs to ada…

4d5b869

…pter

add helpers to get facet data and set up facet configs

90dd6c6

add Facet class

f554db7

phpcs edits

757937f

add generic adapter allow empty search faceting functionality

e6be068

add facet config to controller

bde4b23

move facet dsl to VIP Adapter class, add func to allow empty search f…

dd4c75f

…aceting for VIP Search

add tag and category faceting config setters

1ee22ab

add args to post tag and category agg functions

eed15a1

add name setter to facet class

7a11fd8

inject facet name from config into facet class constructor

51bf61a

please phpcs gods

c3c68f7

add TODO to README

c6a7aed

renatonascalves approved these changes Apr 26, 2022

View reviewed changes

jakewrfoster added 10 commits April 26, 2022 10:52

add TODO to date DSL

b4ecc47

add handling for post_tag item query vars, add TODOs

c34c6ec

add enable_post_date_aggregation to controller

e722737

add facet config to tFacet objects

3a9db46

add calendar interval to post_date Facet Type class

2250ce3

add Post_Date aggs to qp query request args

7abadb9

add post_date aggs to adapter

6d974c8

Merge pull request #9 from alleyinteractive/feature/post-date-facets

c27cdc1

Feature/post date facets

update @see tags

399a9db

add phpcs fixes

f006b5e

jakewrfoster requested a review from kevinfodness April 26, 2022 17:11

Merge branch 'main' into feature/faceting

210e417

kevinfodness approved these changes Apr 28, 2022

View reviewed changes

kevinfodness merged commit 7d58a38 into main Apr 28, 2022

kevinfodness deleted the feature/faceting branch April 28, 2022 18:43

This was referenced May 4, 2022

Add ability to register facets #2

Closed

Add ability to fetch searched facet data #3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/faceting #7

Feature/faceting #7

jakewrfoster commented Apr 25, 2022 •

edited

Loading

renatonascalves left a comment

renatonascalves Apr 26, 2022

jakewrfoster Apr 26, 2022

jakewrfoster Apr 26, 2022 •

edited

Loading

kevinfodness Apr 28, 2022

renatonascalves Apr 26, 2022

jakewrfoster Apr 26, 2022

renatonascalves Apr 26, 2022

kevinfodness Apr 28, 2022

renatonascalves Apr 26, 2022

renatonascalves Apr 26, 2022

renatonascalves Apr 26, 2022

jakewrfoster Apr 26, 2022

renatonascalves Apr 26, 2022

renatonascalves Apr 26, 2022

renatonascalves Apr 26, 2022

renatonascalves Apr 26, 2022

renatonascalves Apr 26, 2022

jakewrfoster Apr 26, 2022

renatonascalves Apr 26, 2022

jakewrfoster Apr 26, 2022

kevinfodness Apr 28, 2022

kevinfodness left a comment

kevinfodness Apr 28, 2022

kevinfodness Apr 28, 2022

kevinfodness Apr 28, 2022

kevinfodness Apr 28, 2022

kevinfodness Apr 28, 2022

kevinfodness Apr 28, 2022

kevinfodness Apr 28, 2022

kevinfodness Apr 28, 2022

kevinfodness Apr 28, 2022

kevinfodness Apr 28, 2022

kevinfodness commented Apr 28, 2022

	public function enable_empty_search_faceting() {
	public function enable_empty_search_faceting(): self {

	public function add_facets_to_ep_query( $query ) {
	public function add_facets_to_ep_query( $query ): array {

Feature/faceting #7

Feature/faceting #7

Conversation

jakewrfoster commented Apr 25, 2022 • edited Loading

Overview

renatonascalves left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakewrfoster Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinfodness left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinfodness commented Apr 28, 2022

jakewrfoster commented Apr 25, 2022 •

edited

Loading

jakewrfoster Apr 26, 2022 •

edited

Loading