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

refactor meta key allowed check #163

Merged
merged 9 commits into from
Jul 20, 2022
Merged

Conversation

pschoffer
Copy link

Description

When meta key is deleted from a post, this does NOT trigger reindex of that post due to the way of allowed metas are detected.

This change makes it so that we have the logic that evaluates if given meta_key is allowed separate from the logic that prepares metas for indexing. That way we can trigger even for meta key that is not being prepared to indexed (because it was just deleted) but is allowed to be in index (was in the indexed document before).

Checklist

Please make sure the items below have been covered before requesting a review:

  • This change works and has been tested locally (or has an appropriate fallback).
  • This change works and has been tested on a Go sandbox.
  • This change has relevant unit tests (if applicable).
  • This change has relevant documentation additions / updates (if applicable).
  • This change has the fix PRed upstream (if applicable). If not applicable, it has the relevant "// VIP: reason for the discrepancy with upstream" comment in places where the code is discrepant.

Steps to Test

  1. Add this filter impl:
add_filter( 'vip_search_post_meta_allow_list', function ( $allow, $post = null ) {
	$allow['testing'] = true;
	return $allow;
}, 10, 2 );
  1. Add meta - vip dev-env exec -- wp post meta set 1 testing foobar
  2. Check the document in ES - vip dev-env exec -- wp vip-search documents get post 1 --format=json | jq '.[0].meta'
  3. Remove meta vip dev-env exec -- wp post meta delete 1 testing
  4. Check the document in ES - vip dev-env exec -- wp vip-search documents get post 1 --format=json | jq '.[0].meta'

Copy link

@rebeccahum rebeccahum left a comment

Choose a reason for hiding this comment

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

@pschoffer Makes sense of a re-factor! Just a few minor notes added, but I think we should PR this upstream first so that we can try to be as close to upstream as possible when we sync.

includes/classes/Indexable/Post/Post.php Outdated Show resolved Hide resolved
includes/classes/Indexable/Post/Post.php Outdated Show resolved Hide resolved
@pschoffer
Copy link
Author

Upstream change: 10up#2862

pschoffer and others added 2 commits June 30, 2022 14:24
Co-authored-by: Rebecca Hum <16962021+rebeccahum@users.noreply.github.com>
Co-authored-by: Rebecca Hum <16962021+rebeccahum@users.noreply.github.com>
@pschoffer
Copy link
Author

Hopefully we can get the upstream change in 10up#2862 and then pull that in instead.

@pschoffer
Copy link
Author

I checked with 10up and they plan to merge the upstream PR next week. The code has quite diverged there so it can't be cleanly cherry-picked, but I try to sync it as much as I could. This is now ready I think.

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.

3 participants