Refactor avatar handling to use remote actor meta #2373
Conversation
Moved avatar logic from Activitypub class to new Avatar class and updated hooks. Avatars are now stored and retrieved via remote actor post meta instead of comment meta, with migration logic added to transfer existing avatar URLs. Interactions and remote actor creation now reference and update avatar URLs in remote actor meta, maintaining backward compatibility during migration.
Deprecated storing avatar_url in comment meta for backward compatibility during migration. Updated migration logic to remove offset parameter and clarify batch processing, as processed comments are filtered out by the query.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors avatar handling by migrating from comment meta storage to remote actor post meta storage. The change introduces a dedicated Avatar class to handle avatar display logic, with backward compatibility maintained during the migration period.
Key changes:
- Introduced new
Avatarclass to centralize avatar display logic previously in theActivitypubclass - Avatar URLs now stored as
_activitypub_avatar_urlin remote actor post meta instead ofavatar_urlin comment meta - Added migration function to transfer existing avatar URLs from comments to remote actors while adding actor references to comments
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| includes/class-avatar.php | New class created to handle avatar retrieval and display, checking remote actor meta first then falling back to comment meta |
| includes/class-activitypub.php | Removed avatar handling logic that was moved to the new Avatar class |
| includes/collection/class-remote-actors.php | Added avatar URL storage during actor creation and new get_avatar_url() method with fallback to JSON content |
| includes/collection/class-interactions.php | Updated to store remote actor post ID reference instead of avatar URL directly in comment meta |
| includes/class-migration.php | Added batch migration function to transfer avatar URLs from comment meta to remote actor meta |
| activitypub.php | Registered Avatar class initialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Updated Remote_Actors to handle avatar URLs as arrays or strings. Added and enhanced PHPUnit tests for avatar migration and remote actor interactions, including batching and remote actor reference in comments.
Simplifies the logic for storing the actor's avatar URL by using object_to_uri() and removing explicit empty string assignment when the icon is missing. This improves code clarity and consistency in avatar URL processing.
Refactored the query to use conditional aggregation, reducing the number of JOINs from three to one. This improves performance when fetching comments with specific meta values for migration.
Introduces Interactions::get_by_remote_actor_id for efficient comment retrieval by remote actor post ID and updates delete handler logic to use remote actor IDs. Adds comprehensive PHPUnit tests for remote actor comment queries and avatar URL retrieval, including meta and JSON fallbacks, array handling, and empty cases.
Refactored deletion logic for interactions and posts to use remote actor IDs instead of actor URLs. Updated Posts and Delete classes to add and use get_by_remote_actor_id methods. Adjusted related tests to use actor IDs and improved test setup and cleanup for remote actors.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Updated the docblock for delete_remote_actor to specify it returns bool or WP_Error, improving code documentation and clarity.
|
#2382 (comment) |
Co-authored-by: Konstantin Obenland <[email protected]>
Co-authored-by: Konstantin Obenland <[email protected]>
Co-authored-by: Konstantin Obenland <[email protected]>
Co-authored-by: Konstantin Obenland <[email protected]>
The type check for $remote_actor being an instance of WP_Post was removed, now only checking for WP_Error. This allows for more flexible handling of remote actor objects.
Replaces manual comment creation loop with factory's create_many method in the migration test. This simplifies the code and ensures comment meta is set during creation.
Deleted manual cleanup steps (wp_delete_comment and wp_delete_post) from test methods in class-test-migration.php. This streamlines the tests, likely relying on test framework teardown for cleanup.
Improves the performance of the avatar migration by filtering meta_key before aggregation in the SQL query. Also updates the check for storing remote actor references to allow any non-error result, not just WP_Post instances.
Co-authored-by: Konstantin Obenland <[email protected]>
|
@Jiwoon-Kim this PR is about avatars of remote Actors, not local ones. |
|
@obenland I have no idea why the |
Removed the logic that stored an empty string to clear the avatar URL. Now, the avatar URL is only stored if available, simplifying the metadata handling.
|
Anyway, I think it would be better to unify the caching logic for images in a remote actor's icon or image properties, as well as images in a remote object's attachment property when there is no ID. And I’ve realized that when anonymity isn’t a major concern, or when storage space is more important, local caching might not be meaningful. In such cases, it’s sufficient to be able to simply fetch the data on demand. In other words, I think media file caching should be optional unless it’s an actor icon likely to be used repeatedly or content that has been explicitly bookmarked. Even so, if local caching is chosen, having automatic resizing and WebP conversion would help optimize storage. Conditional Media Handling Architecture: Differentiating Upload Paths and Managing Non-Attached ActivityStreams Objects |
Renamed the Avatar class and its file to Avatars for consistency. Updated all references in activitypub.php to use the new Avatars class.
Yeah me neither. I think it's safe to ignore for now. I can take a look and see if there's something to debug |
|
Oh, have you had a chance to review #2373 (comment)?
|
|
Oh, commented on the wrong comment. I am fine adding a default avatar. |
If the actor data does not contain an icon, set and return a default avatar URL, and update the post meta with this value. This ensures that a valid avatar is always available.
Modified the test to assert that get_avatar_url returns the default avatar URL ('assets/img/mp.jpg') instead of an empty string when no avatar is set. This ensures the function's behavior matches the expected default handling.
|
@obenland ready for another review |
Moved avatar logic from Activitypub class to new Avatar class and updated hooks. Avatars are now stored and retrieved via remote actor post meta instead of comment meta, with migration logic added to transfer existing avatar URLs. Interactions and remote actor creation now reference and update avatar URLs in remote actor meta, maintaining backward compatibility during migration.
First step to fix: https://wordpress.org/support/topic/refresh-avatar-images/
If we have merged the Attachment caching, we can use the same concept for Avatars #926
Proposed changes:
Avatarclass to centralize avatar display logic previously in theActivitypubclass_activitypub_avatar_urlin remote actor post meta instead ofavatar_urlin comment metaOther information:
Testing instructions:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Refactored avatar handling into a new system that stores and manages avatars per remote actor, improving reliability and preparing for future caching support.