-
Notifications
You must be signed in to change notification settings - Fork 85
Refactor notification system to use action hooks instead #2223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Deprecates the Notification class in favor of new 'activitypub_handled_{type}' action hooks for all activity types. Updates all handler classes to trigger these hooks with consistent parameters, removes Notification usage, and updates integration and tests accordingly. This change improves extensibility and standardizes activity handling throughout the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Refactor to deprecate the Notification class and standardize activity handling via new activitypub_handled_{type} hooks across handlers and integrations, improving extensibility and consistency.
- Replace Notification usage with action hooks carrying consistent parameters (activity, user_id, state, context)
- Update handlers (Accept, Reject, Follow, Move, Create, Delete, Like, Update, Undo) to fire new hooks; adjust signatures to accept $user_id
- Update Stream_Connector to listen to activitypub_handled_follow and log accordingly; adjust tests to new method signatures
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
includes/class-notification.php | Mark Notification as deprecated and forward old hooks via do_action_deprecated to activitypub_handled_{type}. |
includes/handler/class-follow.php | Emit activitypub_handled_follow and update queue_accept signature; remove Notification usage. |
includes/handler/class-accept.php | Emit activitypub_handled_accept; remove Notification usage. |
includes/handler/class-reject.php | Emit activitypub_handled_reject; remove Notification usage. |
includes/handler/class-move.php | Accept $user_id and emit activitypub_handled_move with status and context. |
includes/handler/class-update.php | Accept $user_id, split Update handling and emit activitypub_handled_update with state/reaction or actor context. |
includes/handler/class-delete.php | Accept $user_id, track state, and emit activitypub_handled_delete; return state from maybe_delete_follower. |
includes/handler/class-like.php | Normalize hook doc and use \do_action. |
includes/handler/class-announce.php | Normalize hook doc and use \do_action. |
includes/handler/class-undo.php | Add null context param to activitypub_handled_undo. |
includes/handler/class-inbox.php | Update handled_inbox hook doc to new parameter semantics. |
integration/class-stream-connector.php | Listen to activitypub_handled_follow; add new callback mapping and adapt logging payload. |
tests/includes/handler/class-test-*.php | Update tests to new handler signatures (additional user_id parameter, queue_accept changes). |
Comments suppressed due to low confidence (1)
includes/handler/class-delete.php:128
- Initialize $state to null at the beginning of the method to make control flow explicit and avoid relying on null coalescing with an undefined variable. For example: $state = null; … return $state;
$follower = Remote_Actors::get_by_uri( $activity['actor'] );
// Verify that Actor is deleted.
if ( ! is_wp_error( $follower ) && Tombstone::exists( $activity['actor'] ) ) {
$state = Remote_Actors::delete( $follower->ID );
self::maybe_delete_interactions( $activity );
}
return $state ?? null;
}
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Moved the assignment of the $success variable to occur before the do_action call in the Accept handler. This ensures the correct value is passed to the 'activitypub_handled_accept' action.
Adjusted the indentation of @param tags in the callback_activitypub_handled_follow docblock for improved readability and consistency.
Replaces the $success and $reaction parameters with $result and $actor_post in the 'activitypub_handled_accept' action. Updates the corresponding docblock to reflect these changes for improved clarity and consistency.
Refactors the Announce handler to use $result instead of $reaction for clarity and updates related documentation and do_action parameters accordingly.
Replaces the variable name 'reaction' with 'result' for clarity in the Create handler. Updates all relevant references and docblocks to reflect this change.
Changed the return value of maybe_delete_follower to return false instead of null when no state is set. Updated the docblock to clarify the return type and behavior.
Updated docblocks and variable names for ActivityPub handler action hooks to ensure consistent parameter types and naming. This improves code clarity and developer experience when using these hooks.
Replaces the duplicate $remote_actor parameter with $activity in the 'activitypub_handled_follow' action hook to match the updated docblock and intended usage.
Updated the default value of the $status variable in the Move handler from null to false for clarity and type consistency. Also updated the related docblock to specify the status as a boolean.
Added detailed @deprecated docblocks to legacy ActivityPub action hooks in Notification and Follow classes, guiding developers to use the new 'activitypub_handled_*' hooks instead. This clarifies the migration path and improves documentation for upcoming changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing I noticed was that we don't bail as much anymore and pass through WP_Errors to these hooks.
Hook naming (*_handled_*
) and the way they executed up until now suggest that there's a reasonable expectation of success when they run. Like, the only reason these hooks would receive null or WP_Errors would be because there was some internal failure while handling a request.
I don't think we need to make these too generally applicable, that's what I'd consider activitypub_inbox_{type}
hooks to be for. activitypub_handled_
hooks should probably be more something to use if you want to do something in response to something the plugin did.
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]>
Co-authored-by: Konstantin Obenland <[email protected]>
Co-authored-by: Konstantin Obenland <[email protected]>
Co-authored-by: Konstantin Obenland <[email protected]>
Standardizes the signature of ActivityPub handler actions to include a boolean success flag and a result parameter, replacing previous inconsistent usage of state/result variables. Updates all relevant handler classes, their documentation, and related tests to reflect this change, improving clarity and consistency in action hooks and callbacks.
Adjusted spacing and formatting in the docblock for the activitypub_handled_accept action to improve readability and consistency.
Update the PHPDoc comments for the $success parameter in ActivityPub handler classes to consistently state 'True on success, false otherwise.' This improves clarity and consistency in the documentation for action hooks across multiple handler files.
Updated PHPDoc parameter descriptions for clarity and consistency in handler classes. Refactored test variable from $was_success to $was_successful in Test_Inbox for improved readability.
Refactors the Undo handler to improve logic for handling 'Follow', 'Like', 'Create', and 'Announce' undo activities, and adds a new validate_object method to ensure incoming Undo activities have required attributes. This enhances robustness and input validation for ActivityPub Undo requests.
Updated the Followers::remove method to accept either a WP_Post object or a post ID as the first parameter. This improves flexibility and consistency with other methods that accept both types.
Introduces comprehensive unit tests for the Activitypub Undo handler, covering scenarios for undoing follow, like, create, and announce activities. Also tests the action hook firing and object validation logic with various input cases.
Refactors the undo handler tests for follow and comment-based activities to use data providers, reducing code duplication and improving test coverage. The new structure allows for easier addition of test cases and clearer test descriptions.
Refactors and expands tests for the follow handler, including a new data provider for multiple follow scenarios, tests for the queue_reject method, and verification that both deprecated and new action hooks fire as expected. Also adds improved test cleanup and ensures outbox posts are properly checked for expected responses.
Refactors and extends the Like handler test suite to use a data provider for multiple scenarios, adds tests for duplicate like handling, verifies action hook firing, and tests outbox_activity behavior for Like and non-Like activities. Improves coverage and robustness of Like-related functionality.
Refactors Accept, Reject, and Update handler tests to use data providers for broader scenario coverage and improved maintainability. Consolidates multiple test cases into parameterized tests, making it easier to add new scenarios and improving test clarity.
Consolidates multiple test cases for the get_type method into a single data-driven test using a data provider. This improves maintainability and readability by reducing code duplication and centralizing test scenarios.
Refactored the test_handle_inbox_requests method to use a data provider for multiple activity scenarios, improving test coverage and maintainability. Added inbox_requests_provider to supply various activity types and expected outcomes.
Deleted the unnecessary @Covers tag from the handle_inbox_requests test method in Test_Inbox, as it was either redundant or not required for the current test coverage setup.
Introduces a comprehensive integration test suite for the Activitypub Stream Connector, covering registration, property validation, action links, callback methods, data preparation, and class hierarchy. Includes mock classes for WP_Stream\Connector and WP_Stream\Record to ensure tests run in environments without the Stream plugin.
Introduces a new test to verify the behavior of prepare_outbox_data_for_response when handling a blog user URL. The test checks both the case where the URL is recognized as a blog user and the fallback behavior when it is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 27 out of 27 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.
Deprecates the Notification class in favor of new
activitypub_handled_{type}
action hooks for all activity types. Updates all handler classes to trigger these hooks with consistent parameters, removes Notification usage, and updates integration and tests accordingly. This change improves extensibility and standardizes activity handling throughout the codebase.For the context: #2216 (comment)
Proposed changes:
Accept
,Reject
,Follow
,Move
,Create
,Delete
,Like
,Update
,Undo
) to fire new hooks; adjust signatures to accept$user_id
Stream_Connector
to listen toactivitypub_handled_follow
and log accordingly; adjust tests to new method signaturesOther information:
Testing instructions:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Standardized notification handling with new hooks for better extensibility and consistency.