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 #2862

Merged

Conversation

pschoffer
Copy link
Contributor

Description of the Change

When a 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).

Alternate Designs

Possible Drawbacks

Verification Process

(sorry the steps are specific to our dev-env, but I guess they still make sense?)

  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'

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.

Changelog Entry

Fixed - Post reindex on meta deletion

Credits

@pschoffer

@pschoffer pschoffer force-pushed the update/allowed_meta_filter_upstream branch from 964292a to f689d29 Compare June 30, 2022 13:35
@pschoffer
Copy link
Contributor Author

👋 @felipeelia I added a test, change few typos and switched the version.

Please, let me know if that is what you meant in slack.

@felipeelia felipeelia added this to the 4.3.0 milestone Jul 10, 2022
@felipeelia felipeelia self-assigned this Jul 10, 2022
@felipeelia felipeelia modified the milestones: 4.3.0, 4.2.2 Jul 12, 2022
@felipeelia
Copy link
Member

Hey @pschoffer, thanks for the changes. The PHP Lint action detected a warning with a space alignment, do you mind fixing that before we merge it?

Also, is there any reason why you decided to not call maybe_unserialize on values in filter_allowed_metas()? I imagine that was because of how things were named but wanted to confirm.

@pschoffer
Copy link
Contributor Author

Hi @felipeelia ,
sorry I missed the ping.

My thinking around maybe_unserialize was that I was going for separate concerns. So filter_allowed_metas only does filtering, but doesn't mutate the values in any way.

maybe_unserialize is then still called later in prepare_meta, but not in is_meta_allowed where it is not needed.

@felipeelia felipeelia merged commit 2915993 into 10up:develop Aug 3, 2022
@rebeccahum rebeccahum deleted the update/allowed_meta_filter_upstream branch August 3, 2022 14:38
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.

3 participants