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

BUG: Removal from index on update may fail #3936

Open
1 task done
dtakken opened this issue Jun 12, 2024 · 1 comment · May be fixed by #3937
Open
1 task done

BUG: Removal from index on update may fail #3936

dtakken opened this issue Jun 12, 2024 · 1 comment · May be fixed by #3937
Labels
bug Something isn't working
Milestone

Comments

@dtakken
Copy link

dtakken commented Jun 12, 2024

Describe the bug

Setting a password on a post and updating it from the admin page does not always remove it from the ES index. It may require hitting the update button a second time for the post to be removed. The behavior depends on details in the implementation of wp_insert_post() in WordPress, which varies between versions.

Explanation of why this might happen follows. Some versions of WordPress trigger the set_post_terms hook before the post is updated in the database, while the wp_insert_post hook is triggered after the database update. You can check this in the implementation of wp_insert_post() of WordPress 6.4.3 for example. In other WordPress versions (6.5.2) both hooks are invoked after the database update.

ElasticPress uses both of these hooks. As a result, SyncManager::should_reindex_post() is called twice while updating a post. Because it fetches the post from the database, this method may first see the old post, then the new post.

Currently, when any of the filters adds the post to the SyncManager update queue, the post is indexed even when other filters determine that the post should not be indexed. I think this is not correct. Regardless of whether or not this is a WordPress bug, I think the last filter that runs should have the final say about indexing the post or not.

Steps to Reproduce

  1. Create and publish a post
  2. Check that the post is in de index
  3. Change visibility, set a password
  4. Update the post
  5. Check that the post is no longer in the index

Screenshots, screen recording, code snippet

No response

Environment information

Ubuntu 22.04 , Chromium 125.0.6422.141

WordPress and ElasticPress information

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@dtakken dtakken added the bug Something isn't working label Jun 12, 2024
@dtakken dtakken linked a pull request Jun 12, 2024 that will close this issue
4 tasks
@felipeelia felipeelia added this to the 5.2.0 milestone Jun 13, 2024
@dtakken
Copy link
Author

dtakken commented Jun 21, 2024

I just identified another scenario where updating a post fails to exclude it from the index, also on newer WordPress versions (I found the issue using WP 6.5.3).

The scenario is as follows. We use a custom ACF field to enable authors to flag posts for exclusion from the index. This predates the similar feature offered by newer ElasticPress versions. During a post update, WordPress calls
wp_set_post_terms() before the meta field is updated. The result is the same: ElasticPress has already added the post to the queue before the meta field which indicates exclusion is updated.

The linked pull requests will fix this scenario as well.

@felipeelia felipeelia modified the milestones: 5.2.0, 5.1.4 Dec 3, 2024
@felipeelia felipeelia modified the milestones: 5.1.4, 5.2.0 Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants