Refactor actor blocking with unified API #2097
Conversation
- Add Actors::normalize_identifier() for consistent actor URI normalization - Simplify Moderation::is_actor_blocked() to accept normalized URIs - Refactor Following and Blocked Actors tables to use centralized logic - Change is_actor_blocked() default user_id from null to 0 for type consistency - Improve separation of concerns between normalization and blocking logic Addresses PR review feedback requesting unified blocking API.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the actor blocking functionality by introducing a unified API for actor identifier normalization. The refactoring consolidates duplicate normalization logic across the codebase and simplifies the blocking checks.
- Adds a new
Actors::normalize_identifier()method to centralize actor URI normalization logic - Introduces
Moderation::is_actor_blocked()method that checks both user and site-wide blocks in one call - Refactors Following and Blocked Actors table classes to use the new unified methods
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| includes/collection/class-actors.php | Adds normalize_identifier() method for centralizing actor URI normalization |
| includes/class-moderation.php | Adds is_actor_blocked() method for unified blocking checks |
| includes/wp-admin/table/class-following.php | Refactored to use new normalization and blocking methods |
| includes/wp-admin/table/class-blocked-actors.php | Refactored to use new normalization method |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Replaces the use of the normalized profile identifier with the original input in the error message when blocking an actor fails. This ensures the user sees the exact value they attempted to block.
…b.com/Automattic/wordpress-activitypub into refactor/unified-actor-blocking-check
| // Check if user has blocked the account. | ||
| if ( \in_array( $profile, Moderation::get_user_blocks( $this->user_id )['actors'], true ) ) { | ||
| $original = \sanitize_text_field( \wp_unslash( $_REQUEST['activitypub-profile'] ) ); | ||
| $profile = Actors::normalize_identifier( $original ); |
There was a problem hiding this comment.
I would move the normalizer into the is_actor_blocked function. It reduces the preconditions for the URL you want to check and simplifies the code.
There was a problem hiding this comment.
It includes prepending https for host-only queries, so the table functionality depends on it, too.
There was a problem hiding this comment.
How does it speak against calling it directly inside of the blocked check?
There was a problem hiding this comment.
If it was moved into is_actor_blocked we'd break the functionality that allows users to put in a host only and still follow the blog actor. Like activitypub.blog.
The normalized identifier is a service to both is_actor_blocked() and follow() and prevents a webfinger from having to be resolved twice.
There was a problem hiding this comment.
FYI: It will not be resolved twice, we cache the request. so yes, it might be a bit of overload, but might simplify the code. but I am fine with improving over time.
There was a problem hiding this comment.
Dang, that's impressive. I'm looking forward to having the same detailed grasp of the plugin as you do, I did not know that off the top of my head
This reverts commit e8fc9a8.
Simplifies the logic by ensuring we always work with a string after running object_to_uri() first, eliminating the need for separate object/string handling paths.
Added type check to Actors::normalize_identifier to handle non-string inputs gracefully. Introduced comprehensive unit tests for normalization logic and edge cases in blocked actors, followers, and following tables, ensuring robust handling of various identifier formats and invalid inputs. Also improved test cleanup by removing filters after each test.
Introduced a mock for HTTP requests to fetch actor data in both Test_Followers and Test_Following test classes. This ensures tests do not make real HTTP requests and improves test reliability and isolation.
Moved removal of 'pre_get_remote_metadata_by_actor' filter to tear_down() and removed redundant filter removals from the test method to ensure proper test cleanup.
… tests - Enhanced is_actor_blocked method to check both actor URIs and domains for site-wide and user-specific blocks - Added comprehensive unit tests covering: - Site-wide actor and domain blocking - User-specific actor and domain blocking - Hierarchical priority (site-wide takes precedence) - Mixed blocking scenarios - Edge cases and URL format handling - Performance testing with many blocks - Updated test mocks to properly support actor blocking functionality - Improved test cleanup to handle actor post metadata
Fixes #2095
Proposed changes:
Actors::normalize_identifier()method for centralizing actor identifier normalization logicModeration::is_actor_blocked()to accept pre-normalized URIs with consistent int parameterOther information:
Testing instructions:
@[email protected][email protected]https://example.com/@userexample.com/@userChangelog entry
Changelog Entry Details
Significance
Type
Message
Refactor actor blocking with unified API for better maintainability