Skip to content

Split publish/cleanup gates so allowlist changes don't orphan remote records#38

Open
kraftbj wants to merge 1 commit intotrunkfrom
fix/cleanup-gate-split
Open

Split publish/cleanup gates so allowlist changes don't orphan remote records#38
kraftbj wants to merge 1 commit intotrunkfrom
fix/cleanup-gate-split

Conversation

@kraftbj
Copy link
Copy Markdown
Contributor

@kraftbj kraftbj commented Apr 24, 2026

Fixes the rollback hazard where narrowing `atmosphere_syncable_post_types` (directly or via the `activitypub_support_post_types` option projected into that filter, as FOSSE's `Post_Types` projector does) orphans the remote AT Protocol records of posts that were already synced under the wider configuration.

Proposed changes:

  • `on_status_change()` — split the allowlist gate so it governs only the new-publish / update decisions. Unpublish now consults `_atmosphere_bsky_tid` / `_atmosphere_doc_tid` directly and schedules `atmosphere_delete_post` whenever either is present, regardless of whether the post type is currently in the allowlist. The `did_action/do_action` recursion guard moves with the scheduling block so non-syncable unpublishes without TIDs still early-return without firing it.
  • `on_before_delete()` — drop the allowlist check entirely. Permanent delete is a pure cleanup path: if the post has Atmosphere publication metadata it was synced at some point, and the remote records must be removed even if the post type has since been dropped from the allowlist. A comment on the method explains the omission.

Rationale

The bug is a gate-conflation issue. `Backfill::syncable_post_types()` answers "what post types is this site willing to publish to Bluesky?" — a publish-time eligibility question. Both cleanup paths were reusing it as a proxy for "was this post ever synced?", which only coincides when the allowlist never narrows. A site owner unchecking a post type on the AP side (where FOSSE's new projector surfaces that control in a way most sites already used) becomes a normal operation, not an edge case — at which point the conflation silently leaks orphaned records.

FOSSE PR Automattic/fosse#31 surfaces this path by making the allowlist editable via AP's existing checkbox UI, but the bug pre-exists in every standalone Atmosphere site that filters `atmosphere_syncable_post_types` in PHP. Fixing it here — not in a FOSSE-side workaround — is correct per Atmosphere's existing upstream-first positioning: post-type-agnostic correctness lives here.

Other information:

  • Have you written new tests for your changes, if applicable?

Three new PHPUnit cases in `tests/phpunit/tests/class-test-status-change.php`:

  1. `test_unpublish_of_previously_synced_non_syncable_post_schedules_delete` — publish→draft of a `page` with synthetic `META_TID` / `META_TID` (document) set; asserts `atmosphere_delete_post` is scheduled even though `page` isn't in the default allowlist.
  2. `test_before_delete_of_previously_synced_non_syncable_post_schedules_delete_records` — `on_before_delete` on the same kind of post; asserts `atmosphere_delete_records` is scheduled with the captured TIDs.
  3. `test_new_publish_respects_allowlist_even_when_filter_narrows` — regression guard: narrowing via `atmosphere_syncable_post_types` filter still blocks a new publish of a type the filter excludes, so the split gate doesn't accidentally loosen publish-time semantics.

Full suite runs clean (97/97).

Testing instructions:

  • Check out this branch, `composer install`, `npm install`, `npm run env-start`.
  • Run `npx wp-env run tests-cli --env-cwd="wp-content/plugins/wordpress-atmosphere" vendor/bin/phpunit --filter=Test_Status_Change`. Expect 14/14.
  • (Or `npm run env-test` for the full 97-case suite.)
  • Manual: connect a site, publish a `post`, then add `atmosphere_syncable_post_types` filter returning `[]`, trash the post; confirm `wp cron event list` shows an `atmosphere_delete_post` scheduled.

Changelog entry

  • Automatically create a changelog entry from the details below.
Changelog Entry Details

Significance

  • Patch

Type

  • Fixed - for any bug fixes

Message

Preserve remote cleanup of already-synced posts when their post type is removed from the syncable allowlist.


Companion references:

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a rollback hazard in Atmosphere’s post lifecycle hooks where narrowing the syncable post-type allowlist could prevent cleanup of already-synced remote AT Protocol records, leaving orphaned records behind.

Changes:

  • Split on_status_change() gating so the allowlist applies only to publish/update scheduling, while unpublish cleanup schedules deletion based on existing stored TIDs (regardless of current allowlist).
  • Remove the allowlist check from on_before_delete() so permanent deletes always capture TIDs and schedule remote record cleanup when publication metadata exists.
  • Add PHPUnit coverage for the new cleanup behavior and for the regression that new publishes still respect the allowlist.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
includes/class-atmosphere.php Separates publish/update allowlist gating from cleanup paths; ensures unpublish + permanent delete still clean up remote records based on stored TIDs.
tests/phpunit/tests/class-test-status-change.php Adds regression tests verifying cleanup bypasses allowlist for previously-synced posts, while new publish remains allowlist-gated.
.github/changelog/fix-cleanup-gate-split Adds changelog entry documenting the fix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if ( ! \in_array( $post->post_type, Backfill::syncable_post_types(), true ) ) {
$is_new_publish = 'publish' === $new_status && 'publish' !== $old_status;
$is_update = 'publish' === $new_status && 'publish' === $old_status;
$is_unpublish = 'publish' === $old_status && 'publish' !== $new_status;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to delete posts that were put into draft mode!? Is it possible to re-publish posts in Bluesky? Mastodon is very picky with that... once deleted, it can't be published (with the same ID) again.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Those are good questions. I would think that if someone made a post a draft, it should be deleted. But, the how should we act on re-publish is fair. Let me think about it a bit and checkout the AT docs a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Tests] Includes Tests PR includes test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants