Deduplicate and optimize the collection classes#3527
Open
pfefferle wants to merge 11 commits into
Open
Conversation
Persist() attached an anonymous akismet_comment_nonce filter that was never removed, leaking a closure per federated comment and forcing Akismet's nonce check to 'inactive' for every later comment in the request. Use a named method and a try/finally so all four temporary filters are restored on every path. Claude-Session: https://claude.ai/code/session_01HDA9QR3bJoqM7NhnhT1whg
Extract the copy-pasted patterns across the CPT-backed collections into six composable traits (recipients, guid lookup, KSES-safe insert, object title, paginated query, purge) using late static binding, keeping the static call sites unchanged. Collapse Followers::get_inboxes into a single JOIN, replace a sloppy scheme regex with wp_parse_url, harden the actor serialization filter toggle to restore only the filters it removed, and drop deprecated meta_key query args. Claude-Session: https://claude.ai/code/session_01HDA9QR3bJoqM7NhnhT1whg
Split Interactions::activity_to_comment into prepare_local_comment_data, prepare_remote_comment_data, and prepare_remote_comment_meta helpers, and funnel Remote_Actors create/update through a single persist() method so the KSES-safe insert/update path lives in one place. Claude-Session: https://claude.ai/code/session_01HDA9QR3bJoqM7NhnhT1whg
Collapse activity_to_comment() so each prepare helper owns its own fields and the shared date/type keys are merged in once, instead of re-assembling every field by hand. Reuse Sanitize::webfinger() for the comment author email, drop a needless md5() around the interaction-counts cache key, and restore the note that serialize_for_storage()'s filter suspension is a workaround for a missing incoming/outgoing serialization context. Claude-Session: https://claude.ai/code/session_01HDA9QR3bJoqM7NhnhT1whg
There was a problem hiding this comment.
Pull request overview
This PR refactors and optimizes several includes/collection/ classes, focusing on removing duplicated logic, improving query efficiency for inbox lookups, and fixing global hook/filter leakage during inbound comment persistence.
Changes:
- Fix and harden inbound comment persistence (
Interactions) by preventing Akismet/kses/option overrides from leaking across later comments in the same request, and add regression coverage. - Optimize inbox resolution for
Remote_Actors::get_inboxes()andFollowers::get_inboxes()(single-query JOIN approach) and ensure empty results are cached. - Improve correctness/robustness in a few utility paths (e.g.,
Actors::get_id_by_resource()scheme parsing, outbox title derivation guardrails) with expanded PHPUnit coverage.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
includes/collection/class-interactions.php |
Refactors target resolution and comment persistence; adds cached grouped counts and named Akismet callback. |
includes/collection/class-remote-actors.php |
Optimizes inbox fetching query, centralizes persistence, and adds inbox-cache invalidation + safer filter suspension for storage serialization. |
includes/collection/class-followers.php |
Reworks follower inbox retrieval into a single JOIN query and improves cache handling for empty results. |
includes/collection/class-following.php |
Adjusts blocked-actor removal to resolve via Remote_Actors::get_by_uri() before unfollowing. |
includes/collection/class-actors.php |
Uses wp_parse_url() for scheme detection (vs regex) when resolving resources. |
includes/collection/class-outbox.php |
Prevents fatal errors by guarding get_object_title() when given an unexpected array. |
tests/phpunit/tests/includes/collection/class-test-interactions.php |
Adds regression tests ensuring global hooks/filters are restored and introduces query-count caching assertions. |
tests/phpunit/tests/includes/collection/class-test-remote-actors.php |
Adds tests for restoring only registered outbound filters and for inbox cache invalidation on actor lifecycle changes. |
tests/phpunit/tests/includes/collection/class-test-followers.php |
Extends follower inbox tests for single-query behavior and caching of empty results; adds blocked-actor removal test. |
tests/phpunit/tests/includes/collection/class-test-following.php |
Adds blocked-actor removal test for following collection. |
.github/changelog/restore-akismet-nonce-after-federated-comment |
Adds changelog entry for the Akismet nonce restoration bug fix. |
Comment on lines
+552
to
583
| $is_insert = self::INSERT === $action; | ||
| $flood_priority = \has_action( 'check_comment_flood', 'check_comment_flood_db' ); | ||
| $had_name_filter = false !== \has_filter( 'pre_option_require_name_email', '__return_false' ); | ||
| $akismet_callback = array( self::class, 'akismet_comment_nonce_inactive' ); | ||
| $had_akismet_filter = false !== \has_filter( 'akismet_comment_nonce', $akismet_callback ); | ||
| $kses_callback = array( self::class, 'allowed_comment_html' ); | ||
| $had_allowed_html = false !== \has_filter( 'wp_kses_allowed_html', $kses_callback ); | ||
|
|
||
| // Disable flood control. | ||
| \remove_action( 'check_comment_flood', 'check_comment_flood_db' ); | ||
| if ( false !== $flood_priority ) { | ||
| \remove_action( 'check_comment_flood', 'check_comment_flood_db', $flood_priority ); | ||
| } | ||
|
|
||
| // Do not require email for AP entries. | ||
| \add_filter( 'pre_option_require_name_email', '__return_false' ); | ||
| if ( ! $had_name_filter ) { | ||
| \add_filter( 'pre_option_require_name_email', '__return_false' ); | ||
| } | ||
|
|
||
| // No nonce possible for this submission route. | ||
| \add_filter( | ||
| 'akismet_comment_nonce', | ||
| static function () { | ||
| return 'inactive'; | ||
| } | ||
| ); | ||
| \add_filter( 'wp_kses_allowed_html', array( self::class, 'allowed_comment_html' ), 10, 2 ); | ||
| if ( ! $had_akismet_filter ) { | ||
| \add_filter( 'akismet_comment_nonce', $akismet_callback ); | ||
| } | ||
|
|
||
| if ( self::INSERT === $action ) { | ||
| if ( ! $had_allowed_html ) { | ||
| \add_filter( 'wp_kses_allowed_html', $kses_callback, 10, 2 ); | ||
| } | ||
|
|
||
| if ( $is_insert ) { | ||
| $state = \wp_new_comment( $comment_data, true ); | ||
| } else { | ||
| $state = \wp_update_comment( $comment_data, true ); | ||
| } |
Comment on lines
+164
to
178
| private static function persist( $args ) { | ||
| $is_update = ! empty( $args['ID'] ); | ||
|
|
||
| $has_kses = false !== \has_filter( 'content_save_pre', 'wp_filter_post_kses' ); | ||
| if ( $has_kses ) { | ||
| // Prevent KSES from corrupting JSON in post_content. | ||
| \kses_remove_filters(); | ||
| } | ||
|
|
||
| $post_id = \wp_update_post( $args ); | ||
| $post_id = $is_update ? \wp_update_post( $args ) : \wp_insert_post( $args ); | ||
|
|
||
| if ( $has_kses ) { | ||
| // Restore KSES filters. | ||
| \kses_init_filters(); | ||
| } |
Comment on lines
+657
to
+680
| private static function serialize_for_storage( $actor ) { | ||
| $callbacks = array( | ||
| array( 'Activitypub\Mention', 'filter_activity_object' ), | ||
| array( 'Activitypub\Hashtag', 'filter_activity_object' ), | ||
| array( 'Activitypub\Link', 'filter_activity_object' ), | ||
| ); | ||
| $registered = array(); | ||
|
|
||
| foreach ( $callbacks as $callback ) { | ||
| $priority = \has_filter( 'activitypub_activity_object_array', $callback ); | ||
| if ( false !== $priority ) { | ||
| $registered[] = array( $callback, $priority ); | ||
| \remove_filter( 'activitypub_activity_object_array', $callback, $priority ); | ||
| } | ||
| } | ||
|
|
||
| $serialized = array( $actor->to_json(), $actor->to_array() ); | ||
|
|
||
| foreach ( $registered as $filter ) { | ||
| \add_filter( 'activitypub_activity_object_array', $filter[0], $filter[1] ); | ||
| } | ||
|
|
||
| return $serialized; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes:
Refactors
includes/collection/to remove duplication and cut complexity, keeping every public call site identical (composable traits via late static binding, static shape retained).Interactions::persist()attached an anonymousakismet_comment_noncefilter it never removed, leaking a closure per federated comment and forcing Akismet's nonce check toinactivefor every later comment in the same request. Now a named method plustry/finallyrestores all four temporary filters on every path.query()+get_by_authority(), object-title derivation, guid lookup, KSES-safe insert, and the batched purge loop.Followers::get_inboxescollapses a two-step query into a single JOIN (runs on every federation send).Remote_Actorsactor serialization now suspends and restores only the mention/hashtag/link filters that were actually registered, at their original priority (fixes an unconditional re-add edge).Actors::get_id_by_resourcereplaces a sloppy scheme regex withwp_parse_url().Remote_Posts::get_by_remote_actor_idswaps deprecatedmeta_key/meta_valuefor ameta_query(drops two PHPCS suppressions).Net roughly −435 lines across the collection classes, plus six small trait files.
With_Guid_Lookupalso drops a latent double-escape inRemote_Actors::get_by_uri(it ranesc_sql()insideprepare()), standardizing on the sameesc_url_rawnormalization used at write time.Other information:
New regression test asserts the comment filters/actions are fully restored after
persist().Testing instructions:
npm run env-test(2760 tests green).composer lint(clean).trunk:Changelog entry
Changelog entry for the Akismet fix is included in the branch (
.github/changelog/restore-akismet-nonce-after-federated-comment); the rest is internal refactor.