Improved recipient handling for clarity and added better inbox support #2210
Conversation
Refactored recipient extraction logic by introducing extract_recipients_from_activity_property() for improved modularity and clarity. Updated extract_recipients_from_activity() to use the new helper. Added comprehensive unit tests for both functions to ensure correct handling of various recipient formats and uniqueness.
Renamed the parameter and related documentation in extract_recipients_from_activity_property from 'attribute' to 'property' for clarity and consistency.
Simplifies extract_recipients_from_activity_property by streamlining recipient extraction and flattening logic. Updates object_to_uri to return false if 'href' or 'id' are not set, improving robustness when handling malformed or incomplete ActivityPub objects.
Updated extract_recipients_from_activity_property to remove empty values before returning unique recipients. Adjusted related test to reflect the change in behavior.
Updated object_to_uri to use 'url' as a fallback if 'id' is not present. Adjusted related unit tests to reflect this logic and removed unnecessary error handling in moderation tests.
Refactored recipient extraction functions to standardize argument order. Introduced an Audience trait to encapsulate recipient and visibility determination logic, and updated controllers to use this trait. Adjusted related tests to match the new function signatures.
Introduces a visibility parameter to the 'activitypub_inbox' and 'activitypub_inbox_{type}' actions, allowing handlers to determine the visibility of incoming activities. Refactors recipient determination to use 'determine_local_recipients' and updates the visibility logic for more accurate classification based on activity recipients.
Updated the handle_create method in Create handler to accept a visibility parameter instead of checking public status via is_activity_public. Adjusted related tests to pass the appropriate visibility constant, improving clarity and control over activity visibility handling.
Activities of types 'Follow', 'Accept', 'Reject', 'Undo', and 'Delete' now default to private visibility. This ensures these sensitive actions are not exposed publicly unless explicitly specified.
The array of activity types in the determine_visibility method was reordered for consistency. No functional changes were made.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors recipient extraction logic by introducing a new helper function extract_recipients_from_activity_property() to improve modularity and code reuse. The changes also add comprehensive unit tests for both the new helper and the existing extract_recipients_from_activity() function.
- Extracted recipient parsing logic into a reusable helper function
- Updated existing recipient extraction to use the new helper
- Added comprehensive unit tests covering various recipient formats and edge cases
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| includes/functions.php | Refactored recipient extraction by adding new helper function and updating existing logic |
| includes/rest/trait-audience.php | New trait providing audience handling methods using the refactored recipient extraction |
| includes/rest/class-inbox-controller.php | Updated to use the new Audience trait and pass visibility parameter |
| includes/rest/class-actors-inbox-controller.php | Updated to use the new Audience trait and pass visibility parameter |
| includes/handler/class-create.php | Updated to accept and use visibility parameter instead of checking activity publicity |
| tests/includes/class-test-functions.php | Added comprehensive unit tests for the new recipient extraction functions |
| tests/includes/handler/class-test-create.php | Updated test calls to include visibility parameter |
| tests/includes/class-test-moderation.php | Simplified test by removing error handling code |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Introduces a new test class covering the Audience trait's methods, including visibility determination and local recipient extraction. Tests various scenarios for public, private, and malformed activities to ensure correct behavior.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Prefixed array_merge and array_unique with backslashes in extract_recipients_from_activity to ensure use of PHP's global functions and avoid potential namespace issues.
pfefferle
changed the title
includes/functions.php
Outdated
| * @param array|string $data The ActivityPub object. | ||
| * | ||
| * @return string The URI of the ActivityPub object | ||
| * @return string|false The URI of the ActivityPub object or false if not found. |
There was a problem hiding this comment.
One of the strengths of object_to_uri() is that it reliably returns a string and we don't need to check whether it failed. That is rare(!) and I think we should do what we can to keep it that way.
I assume this change is meant to facilitate running array_filter() over the output? Could we use a specific callback in that instance instead?
There was a problem hiding this comment.
That is not completely true! We have way to often an issue, because of missing id fields! I am fine to change it to empty string, but simply not checking if an attribute is available is not "we don't need to check whether it failed"!
There was a problem hiding this comment.
From the spec:
All properties are optional (including the id and type).
| * | ||
| * Provides methods for handling ActivityPub audience and recipient extraction. | ||
| */ | ||
| trait Audience { |
There was a problem hiding this comment.
Could these be methods in Collections\Actors instead? They feel more like utility functions than something that's specific to rest api controllers. A trait doesn't feel like the best place for these.
There was a problem hiding this comment.
Even if we check the Audience, this is not a function I see in the Actors Collection!
I will see if I can move it to the Activity.
| * @param string $visibility The visibility of the activity. | ||
| */ | ||
| \do_action( 'activitypub_inbox', $data, $user_id, $type, $activity ); | ||
| \do_action( 'activitypub_inbox', $data, $user_id, $type, $activity, $visibility ); |
There was a problem hiding this comment.
It's not quite clear to me why the visibility stuff needs to be moved up a level, when (so far) it's only Create that's concerned about it. Can we not just expand is_activity_public() with any improvements this PR aims to make?
There was a problem hiding this comment.
Sure, we are currently only use it for Create, but the idea is that implementers do not have to care and simply can hook into the inbox to get all needed infos.
Aside from that, we are about to implement the reader, so we might want to have a more detailed visibility handling and not only a "Public or not".
Changed object_to_uri to return an empty string instead of false when a URI is not found. Updated the function's docblock and logic to reflect this behavior for consistency and clarity.
Refactored recipient extraction from the Audience trait into a new private get_recipients method in Inbox_Controller. Updated all usages and tests to reflect this change, and removed related tests and code from the Audience trait and its test class. This improves encapsulation and testability of recipient handling in the REST inbox controller.
Refactored recipient extraction logic by introducing
extract_recipients_from_activity_property()for improved modularity and clarity. Updatedextract_recipients_from_activity()to use the new helper. Added comprehensive unit tests for both functions to ensure correct handling of various recipient formats and uniqueness.Proposed changes:
Other information:
Testing instructions:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Improved recipient handling for clarity and added better inbox support.