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 a Terms Indexable #1443

Merged
merged 35 commits into from
Jan 29, 2020
Merged

Add a Terms Indexable #1443

merged 35 commits into from
Jan 29, 2020

Conversation

dkotter
Copy link
Contributor

@dkotter dkotter commented Aug 16, 2019

Description of the Change

A new filter was added to WordPress core (coming in 5.3), that allows term queries to be short-circuited with your own results. This provides the ability for ElasticPress to take over any queries that utilize WP_Term_Query.

This PR adds a new Terms Indexable that will index all terms across a site and then takes over term queries if ep_integrate is set to true.

Benefits

Terms will now be stored in their own index and they can be searched across. Any queries that make use of WP_Term_Query can gain potential performance benefits by utilizing elasticsearch over a normal database query.

Possible Drawbacks

Not all queries will be made faster by this integration, so testing will need to be done to determine if it's worth using elasticsearch for term queries.

Anytime objects are updated that have terms associated with them, we re-index those terms, resulting in possibly longer save runs.

Verification Process

Tested these changes locally on a multisite install, with 4 sites on the network. Each site had a handful of categories and tags set; some nested, some not; some with term meta, most not; most associated with at least one object, some associated with nothing.

  • Verified that running a full index will add all term information into a new terms index within elasticsearch.
  • Verified that adding a new term or updating an existing term will add/update that term within the correct index
  • Verified that running a WP_Term_Query, with ep_integrate set to true, runs the query against elasticsearch
  • Tested all the various query params that WP_Term_Query supports, to make sure those work when run against elasticsearch

Note further testing is recommended here, to further ensure all the various query params work, especially when used in conjunction with each other. Also more testing around updating terms and adding/updating posts that use terms, to make sure things are updated correctly, particularly the term counts.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

@jeffpaul
Copy link
Member

@brandwaffle @tlovett1 @tott please let me know once this gets through code review as we'll look to have someone else from the team help with any changes from that review... thanks!

@adamsilverstein adamsilverstein self-assigned this Aug 19, 2019
@jeffpaul
Copy link
Member

@tlovett1 the two build failures appear related to the weighting work. Are you seeing weighting-related issues in this PR or is this good to be merged in alongside the WP 5.3 release in November? Essentially, do you need anything else from the team?

@jeffpaul jeffpaul added this to the 3.3.0 milestone Sep 17, 2019
@tlovett1
Copy link
Member

@jeffpaul we are good to go for now. Thank you for you and @dkotter's help!

@dkotter dkotter mentioned this pull request Oct 18, 2019
6 tasks
@jeffpaul jeffpaul assigned tlovett1 and unassigned adamsilverstein Nov 5, 2019
@tlovett1 tlovett1 modified the milestones: 3.3.0, 3.4 Jan 7, 2020
@tlovett1
Copy link
Member

@petenelson what's the status of unit tests on this one? Thanks.

@petenelson
Copy link
Contributor

With the latest develop branch merged in, there were a couple errors in the unit tests when running locally.

1) ElasticPressTest\TestTerm::testTermQueryOrderParent
Undefined offset: 0

/var/www/html/wp-content/plugins/elasticpress/tests/php/indexables/TestTerm.php:500

2) ElasticPressTest\TestTerm::testBasicTermQuery
Failed asserting that null is true.

/var/www/html/wp-content/plugins/elasticpress/tests/php/indexables/TestTerm.php:220

I'll take a look at these, as well as code coverage for the rest of the term unit tests.

@petenelson petenelson added the wip label Jan 22, 2020
@tlovett1 tlovett1 merged commit bcfb1ab into develop Jan 29, 2020
@tlovett1 tlovett1 deleted the feature/terms-indexable branch January 29, 2020 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants