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.99% +0.03%
- Complexity 613 621 +8
============================================
Files 22 22
Lines 2158 2170 +12
============================================
+ Hits 1855 1866 +11
- Misses 303 304 +1
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 );
...
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.