Skip to content

Conversation

pfefferle
Copy link
Member

@pfefferle pfefferle commented Aug 8, 2025

This PR implements centralized tombstone handling by introducing a new Activitypub\Tombstone class to replace scattered tombstone detection logic. The changes deprecate existing is_tombstone() functions in favor of a unified approach that supports multiple data types and includes local URL tracking.

See https://www.w3.org/TR/activitypub/#delete-activity-outbox

Proposed changes:

  • Introduces a new Tombstone class with comprehensive check methods for different data types
  • Deprecates Http::is_tombstone() and is_tombstone() functions while maintaining backward compatibility
  • Adds local tombstone tracking with bury() and check_local_url() methods

Other information:

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

Testing instructions:

  • Delete a User
  • Query the User-ID-URL with the ActivityPub Accept header
  • Check for Tombstone JSON

Changelog entry

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

Significance

  • Patch
  • Minor
  • Major

Type

  • Added - for new features
  • Changed - for changes in existing functionality
  • Deprecated - for soon-to-be removed features
  • Removed - for now removed features
  • Fixed - for any bug fixes
  • Security - in case of vulnerabilities

Message

Improved handling of deleted content with a new unified system for better tracking and compatibility.

Moved all tombstone detection logic into a new Activitypub\Tombstone class, deprecating is_tombstone() and Http::is_tombstone(). Updated all usages to use Tombstone::check() and related methods. Added a tombstone JSON template and updated tests to reflect the new class. This centralizes and standardizes tombstone checks across the codebase.
Replaces inline array check in the is_tombstone() method with a call to self::check_array() for improved code reuse and maintainability.
Expanded the test suite in class-test-tombstone.php to cover Tombstone::check_wp_error, check_array, check_object, and check_local_url methods. Updated existing tests and data providers to reflect method renaming from is_tombstone to check_remote_url.
@pfefferle pfefferle changed the title Refactor tombstone handling into Tombstone class Implement a proper Tombstone handling Aug 8, 2025
@pfefferle pfefferle marked this pull request as ready for review August 8, 2025 18:05
@Copilot Copilot AI review requested due to automatic review settings August 8, 2025 18:05
@pfefferle pfefferle added this to the User Delete milestone Aug 8, 2025
Copy link

@Copilot 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 implements centralized tombstone handling by introducing a new Activitypub\Tombstone class to replace scattered tombstone detection logic. The changes deprecate existing is_tombstone() functions in favor of a unified approach that supports multiple data types and includes local URL tracking.

  • Introduces a new Tombstone class with comprehensive check methods for different data types
  • Deprecates Http::is_tombstone() and is_tombstone() functions while maintaining backward compatibility
  • Adds local tombstone tracking with bury() and check_local_url() methods

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
includes/class-tombstone.php New class implementing centralized tombstone detection with support for URLs, WP_Error, arrays, and objects
includes/class-http.php Deprecates is_tombstone() method, delegates to new Tombstone class
includes/functions.php Deprecates global is_tombstone() function, delegates to new Tombstone class
includes/handler/class-delete.php Updates calls from Http::is_tombstone() to Tombstone::check()
includes/collection/class-followers.php Updates import and function call to use new Tombstone class
includes/class-scheduler.php Updates function call to use new Tombstone class
includes/class-activitypub.php Adds local tombstone checking to template rendering
includes/class-query.php Changes get_request_url() visibility from protected to public
templates/tombstone-json.php New template for rendering tombstone JSON responses
tests/includes/class-test-tombstone.php Comprehensive test suite for new Tombstone class functionality
tests/includes/class-test-http.php Removes old HTTP tombstone tests (moved to new test file)

@pfefferle pfefferle requested a review from obenland August 8, 2025 18:10
matticbot and others added 6 commits August 8, 2025 20:10
Changed the docblock parameter type from WP_Error to \WP_Error for improved clarity and namespacing consistency.
pfefferle and others added 5 commits August 24, 2025 19:31
Co-authored-by: Konstantin Obenland <[email protected]>
Co-authored-by: Konstantin Obenland <[email protected]>
@obenland what about `is_gone`? because the tombstone check is for `410 gone`.
@pfefferle pfefferle requested review from Copilot and obenland August 25, 2025 08:11
Copy link

@Copilot 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@pfefferle pfefferle requested a review from Copilot August 25, 2025 09:10
Copilot

This comment was marked as outdated.

@obenland
Copy link
Member

@obenland what about is_gone? because the tombstone check is for 410 gone.

It's better! Probably good enough for our purposes. Tombstone::is_gone() could be misread as "the tombstone is no longer there", bit that's what docs and reading the code are for.

obenland
obenland previously approved these changes Aug 25, 2025
@pfefferle pfefferle requested review from obenland and Copilot August 25, 2025 22:16
Copy link

@Copilot 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 implements centralized tombstone handling by introducing a new Activitypub\Tombstone class to replace scattered tombstone detection logic throughout the codebase. The changes deprecate existing tombstone functions while maintaining backward compatibility and add support for user deletion with proper ActivityPub Delete activities.

  • Introduces unified Tombstone class with comprehensive detection methods for different data types
  • Adds Delete activity generation when users are deleted with local tombstone tracking
  • Deprecates scattered is_tombstone() functions in favor of centralized approach

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
includes/class-tombstone.php New centralized tombstone handling class with detection methods
includes/scheduler/class-actor.php User deletion handling with Delete activity generation
includes/handler/class-delete.php Updated to use new Tombstone class methods
includes/collection/class-followers.php Updated tombstone checks and deprecated old methods
includes/collection/class-actors.php Added inbox retrieval for all actors
includes/class-http.php Deprecated is_tombstone method with delegation
includes/functions.php Deprecated is_tombstone function with delegation
templates/tombstone-json.php New template for rendering tombstone JSON responses
tests/includes/class-test-tombstone.php Comprehensive test suite for new Tombstone class

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Eliminated a duplicate check for error codes in the Tombstone class. The logic now relies solely on the error data's status field for validation.
Renamed Tombstone methods from 'is_gone', 'is_remote_url_gone', 'is_local_url_gone', 'is_wp_error', 'is_array_gone', and 'is_object_gone' to 'exists', 'exists_remote', 'exists_local', 'exists_in_error', 'check_array', and 'check_object' for clarity and consistency. Updated all usages and related tests to match the new method names. Improves code readability and better reflects the purpose of the methods.
Replaces the deprecated function notice and call from Tombstone::is_wp_error to Tombstone::exists_in_error to reflect the updated method name.
@pfefferle
Copy link
Member Author

pfefferle commented Aug 26, 2025

@obenland the Delete PR was a PR to this branch, so this PR now also includes the Delete code. I am sorry about that, I no longer thought about it 🫣

Enhanced and clarified docblocks for all methods and the class itself in includes/class-tombstone.php. The new comments provide more detailed descriptions, parameter explanations, and context for tombstone detection logic, improving code readability and maintainability.
@pfefferle pfefferle merged commit fd037a6 into trunk Aug 26, 2025
13 checks passed
@pfefferle pfefferle deleted the add/tombstone branch August 26, 2025 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants