fix: remove stale posts when publish status changes - #305
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new permanent-deletion path lacks direct integration coverage, and test credential setup bypasses the production storage API.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes incremental de-indexing when posts become unpublished, trashed, or permanently deleted.
Changes:
- Centralizes sanitized site/post identifiers.
- Adds permanent-deletion cleanup.
- Extends Algolia request capture and regression tests.
File summaries
| File | Description |
|---|---|
inc/Modules/Search/Watcher.php |
Uses consistent deletion filters and handles permanent deletion. |
inc/Modules/Search/Post_Record.php |
Centralizes site key and post identifier generation. |
tests/phpunit/TestCase.php |
Captures Algolia request payloads. |
tests/phpunit/Integration/Modules/Search/WatcherTest.php |
Tests transition-based record deletion. |
tests/phpunit/Integration/Modules/Search/PostRecordTest.php |
Tests identifier consistency and sanitization. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #305 +/- ##
============================================
- Coverage 85.95% 85.93% -0.03%
- Complexity 613 620 +7
============================================
Files 22 22
Lines 2158 2169 +11
============================================
+ Hits 1855 1864 +9
- Misses 303 305 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The newly added permanent-deletion hook lacks integration coverage.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
inc/Modules/Search/Watcher.php:27
- The new permanent-deletion path is not exercised by
WatcherTest; the added regression test only transitions a post todraft. Please add an integration test that registers the hooks, callswp_delete_post( $post_id, true ), and verifies the emitteddeleteByQueryfilter so the hook registration and callback arguments are covered.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The pre-deletion hook can remove search records even when WordPress subsequently fails to delete the post.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
| /** | ||
| * Gets the key used to namespace this site's records. | ||
| * | ||
| * Sanitized because it is embedded in the `objectID`, which Algolia addresses | ||
| * as a URL path segment. | ||
| */ | ||
| public static function get_site_key(): string { | ||
| return sanitize_key( Utils::normalize_url( get_site_url() ) ); | ||
| } |
There was a problem hiding this comment.
We're not using this anywhere else (yet at least). At minimum, lets make this private static and put it with the other private methods in this class (although if we decide that we don't need public static get_site_post_id(), then just private)
| * as a URL path segment. | ||
| */ | ||
| public static function get_site_key(): string { | ||
| return sanitize_key( Utils::normalize_url( get_site_url() ) ); |
There was a problem hiding this comment.
The internals also don't seem to make sense here. Note how they duplicate $this->site_url(). Usually a sign to rethink our approach and desired end result.
| * Runs on `deleted_post` rather than `before_delete_post`: deletion can still fail | ||
| * after the earlier hook, and attachments never fire it at all. |
There was a problem hiding this comment.
this is a decision log about how it's used, if you think it's worth retaining, then move it as a comment above add_action( 'deleted_post', ... ) not here.
| ] | ||
| ); | ||
|
|
||
| // A site without credentials is not a failure worth reporting. |
There was a problem hiding this comment.
At a glance, fixing this is out of scope of this PR, but we should address the underlying issue long term, not just assume the tech debt.
(If algolia is configured correctly, but we still get those errors, we def want to log them)
| // A site without credentials is not a failure worth reporting. | |
| // @todo this class shouldn't run if the Algolia config isn't good. |
| // First delete the old records, so a post that is no longer indexable leaves nothing behind. | ||
| if ( is_wp_error( $this->delete_post_records( $indexer, (int) $post->ID ) ) ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Should we always be deleting this? I havn't tried to replicate myself, but I'd assume we only want to delete if the old record if the currently status is NOT indexable and the previous status was indexable. (I'd also assume that the $indexer->save_records() cleans up old records for the same id when resaving). Otherwise, why slow the user down with the external API hit?
So something like:
// Check if the new status is allowed before reindexing.
if ( ! in_array( $new_status, Post_Record::get_allowed_statuses( [ $post->post_type ] ), true ) ) {
// Cleanup old indexed post.
if ( in_array ( $old_status, Post_Record::get_allowed_statuses( [ $post->post_type ] ) ) ) {
$this->delete_post_records( $indexer, (int) $post->ID )
}
return;
}
$records = ( new Post_Record() )->to_records( $post );
...There was a problem hiding this comment.
Good optimisation, but we have to handle drop cleanup for shrinking chunk counts.
There was a problem hiding this comment.
Added the suggested change for now. Once this is merged, I’ll put up another PR to handle the chunks that are left behind when shrinking the post_type.
That change will require storing metadata related to the index post’s chunk size in the DB so we can track these cases and clean up any leftover chunks.
| * The `site_post_id` written to records must come from get_site_post_id(). | ||
| * | ||
| * Watcher deletes a post's records by filtering on this value, so the two must never drift. | ||
| * | ||
| * @see https://github.com/rtCamp/OnePress/issues/84 |
There was a problem hiding this comment.
Our tests aren't a decision log.
| * The `site_post_id` written to records must come from get_site_post_id(). | |
| * | |
| * Watcher deletes a post's records by filtering on this value, so the two must never drift. | |
| * | |
| * @see https://github.com/rtCamp/OnePress/issues/84 | |
| * The `site_post_id` written to records must come from get_site_post_id(). |
| * | ||
| * @param string[] $entities The indexable post types. | ||
| */ | ||
| private function set_up_governing_site( array $entities = [ 'post' ] ): void { |
There was a problem hiding this comment.
Do we need to clear these at tearDown()?
| /** | ||
| * A post leaving the index must be deleted by the `site_post_id` its records carry. | ||
| * | ||
| * Regression test: the filter used to be built from the raw site URL while records store | ||
| * the sanitized site key, so Algolia matched nothing, reported success, and unpublished | ||
| * or trashed posts kept showing up until a full re-sync. The expected value is read back | ||
| * off the outgoing payload, so the write and delete paths are checked against each other | ||
| * instead of against a value the test rebuilds for itself. | ||
| * | ||
| * @see https://github.com/rtCamp/OnePress/issues/84 | ||
| */ | ||
| public function test_deletes_by_the_site_post_id_written_to_records(): void { |
There was a problem hiding this comment.
- Doc slop.
- We also want to test 'trash' status (different from calling wp_delete_*() manually), so update this to use a data provider. E.g.:
| /** | |
| * A post leaving the index must be deleted by the `site_post_id` its records carry. | |
| * | |
| * Regression test: the filter used to be built from the raw site URL while records store | |
| * the sanitized site key, so Algolia matched nothing, reported success, and unpublished | |
| * or trashed posts kept showing up until a full re-sync. The expected value is read back | |
| * off the outgoing payload, so the write and delete paths are checked against each other | |
| * instead of against a value the test rebuilds for itself. | |
| * | |
| * @see https://github.com/rtCamp/OnePress/issues/84 | |
| */ | |
| public function test_deletes_by_the_site_post_id_written_to_records(): void { | |
| /** | |
| * Tests a post that is set to draft is triggered for removal from Algolia. | |
| * | |
| * @param string $status The post status | |
| */ | |
| #[DataProvider( 'delete_record_provider' )] | |
| public function test_deletes_record_when_not_published( string $status ): void { |
| /** | ||
| * A post deleted without passing through the trash must take its records with it. | ||
| * | ||
| * Deleted straight from `publish` on purpose: trashing first would clear the records | ||
| * through `on_post_transition`, so the assertion would pass even with the deletion | ||
| * hook unregistered. | ||
| * | ||
| * @see https://github.com/rtCamp/OnePress/issues/84 | ||
| */ | ||
| public function test_deletes_records_when_a_post_is_permanently_deleted(): void { | ||
| $this->set_up_governing_site(); |
There was a problem hiding this comment.
You don't need to separate posts to test the same thing (wp_delete_post( $id, true) ), but you're also not testing that a regular wp_delete_post() also purges, just wp_update_post().
So instead, repurpose this one for the latter.
| /** | |
| * A post deleted without passing through the trash must take its records with it. | |
| * | |
| * Deleted straight from `publish` on purpose: trashing first would clear the records | |
| * through `on_post_transition`, so the assertion would pass even with the deletion | |
| * hook unregistered. | |
| * | |
| * @see https://github.com/rtCamp/OnePress/issues/84 | |
| */ | |
| public function test_deletes_records_when_a_post_is_permanently_deleted(): void { | |
| $this->set_up_governing_site(); | |
| /** | |
| * A post deleted via `wp_delete_post()` should have its records purged. | |
| */ | |
| public function test_deletes_records_when_wp_delete_post_is_called(): void { | |
| $this->set_up_governing_site(); |
| wp_delete_post( $post_id, true ); | ||
|
|
||
| $this->assertSame( | ||
| [ sprintf( 'site_post_id:"%s"', $stored_id ) ], | ||
| $this->get_delete_filters( $requests ), | ||
| 'Permanently deleting a post must delete its records by the stored site_post_id.' | ||
| ); |
There was a problem hiding this comment.
| wp_delete_post( $post_id, true ); | |
| $this->assertSame( | |
| [ sprintf( 'site_post_id:"%s"', $stored_id ) ], | |
| $this->get_delete_filters( $requests ), | |
| 'Permanently deleting a post must delete its records by the stored site_post_id.' | |
| ); | |
| wp_delete_post( $post_id ); | |
| $this->assertSame( | |
| [ sprintf( 'site_post_id:"%s"', $stored_id ) ], | |
| $this->get_delete_filters( $requests ), | |
| 'Trashing a post with wp_delete_post must delete its records by the stored site_post_id.' | |
| ); |
| /** | ||
| * The records must outlive the post until the deletion has actually succeeded. | ||
| * | ||
| * Deleting from `before_delete_post` would strip the index for a post that then | ||
| * survives a failed deletion. | ||
| */ | ||
| public function test_does_not_delete_records_before_the_post_is_gone(): void { |
There was a problem hiding this comment.
| /** | |
| * The records must outlive the post until the deletion has actually succeeded. | |
| * | |
| * Deleting from `before_delete_post` would strip the index for a post that then | |
| * survives a failed deletion. | |
| */ | |
| public function test_does_not_delete_records_before_the_post_is_gone(): void { | |
| /** | |
| * Ensures a record is purged when wp_delete_post() is used to permanently delete the post directly. | |
| */ | |
| public function test_deletes_record_when_a_post_is_permanently_deleted(): void { |
| $filters_while_the_post_still_exists = null; | ||
| add_action( | ||
| 'before_delete_post', | ||
| function () use ( &$requests, &$filters_while_the_post_still_exists ): void { | ||
| $filters_while_the_post_still_exists = $this->get_delete_filters( $requests ); | ||
| }, | ||
| PHP_INT_MAX | ||
| ); | ||
|
|
||
| wp_delete_post( $post_id, true ); | ||
|
|
||
| $this->assertSame( | ||
| [], | ||
| $filters_while_the_post_still_exists, | ||
| 'No records may be deleted while the post is still in the database.' | ||
| ); | ||
| $this->assertNotEmpty( |
There was a problem hiding this comment.
What exactly is this supposed to be testing? Seems like slop to me.
There was a problem hiding this comment.
removed it, I also think the same now.
Co-authored-by: Dovid Levine <david@axepress.dev>
Co-authored-by: Dovid Levine <david@axepress.dev>
There was a problem hiding this comment.
🟢 Approval recommended
The implementation consistently uses stored record identifiers and covers the affected deletion paths with focused regression tests.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Dovid Levine <david@axepress.dev>
Co-authored-by: Dovid Levine <david@axepress.dev>
Co-authored-by: Dovid Levine <david@axepress.dev>
Co-authored-by: Dovid Levine <david@axepress.dev>
There was a problem hiding this comment.
🟡 Changes recommended
The new watcher tests contain an invalid data provider and one test does not exercise permanent deletion as claimed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
…te post ID `get_site_post_id()` is now an instance method reading `$this->site_key`, and `get_site_key()` is removed: as a static it could not read `$this->site_url`, so it re-derived the site URL and duplicated the constructor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Also drops the `deleted_post` vs `before_delete_post` rationale from the `on_deleted_post()` docblock.
`test_deletes_record_when_not_published` was erroring with ArgumentCountError: the conversion to a data-provider test added the `#[DataProvider]` attribute but never added the provider, never imported the attribute class (so PHPUnit silently ignored it and called the test with no arguments), and left `$status` unused while the body hard-coded `draft`. Adds the import and the provider, and passes `$status` through, so the case actually covers draft, pending, private and trash. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What
Fixes the de-indexing path so unpublished, trashed and permanently deleted posts stop appearing in search results without a full re-sync.
Why
The delete filter and the stored record disagreed on the key, so
deleteBymatched nothing and Algolia reported success:sanitize_key()lowercases and strips:and/, so the two can never be equal. Algolia matches facet filters on exact attribute values, and deleting zero records is a legitimate outcome, not an error — sois_wp_error()passed and the failure was completely silent.Publishing kept working because
saveObjectsupserts byobjectIDand never consults the filter, which is why the incremental indexing path looked healthy while only the delete half was broken.Related Issue(s):
AI Disclosure
Written with Claude Code (Opus 5), used for diagnosis, implementation, and tests.
Testing Instructions
postan indexable entity.deleted_posthook.Screenshots
No UI changes.