-
Notifications
You must be signed in to change notification settings - Fork 82
Avatars: Update filters, fallbacks, and API use #1812
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
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
This PR centralizes avatar URL retrieval by switching from direct comment meta access to the core get_avatar_url()
function, and adds filter and fallback support for comment types.
- Replaced all direct calls to
get_comment_meta(..., 'avatar_url', true)
withget_avatar_url()
. - Added and documented a
get_avatar_comment_types
filter, with a fallback for undefined comment types. - Removed a redundant
get_avatar_data
filter call and applied theget_avatar_url
filter in ActivityPub integration.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/reactions/render.php | Swap get_comment_meta for get_avatar_url |
includes/rest/class-post-controller.php | Use get_avatar_url instead of direct comment meta access |
includes/class-activitypub.php | Introduce get_avatar_comment_types filter, fallback logic, remove double-filtering, and apply get_avatar_url |
build/reactions/render.php | Mirror the render.php update in the built artifact |
Comments suppressed due to low confidence (2)
includes/class-activitypub.php:338
- The docblock for
get_avatar_comment_types
filter is missing a@return array
annotation to describe the expected return value.
* @param array $allowed_comment_types Array of allowed comment types.
includes/class-activitypub.php:341
- Introduce or update unit tests to cover the new fallback behavior when
comment_type
is empty or not in the allowed list, ensuring no unintended avatar URLs are returned.
if ( ! \in_array( $id_or_email->comment_type ?: 'comment', $allowed_comment_types, true ) ) { // phpcs:ignore Universal.Operators.DisallowShortTernary
includes/class-activitypub.php
Outdated
if ( ! \in_array( $id_or_email->comment_type ?: 'comment', $allowed_comment_types, true ) ) { // phpcs:ignore Universal.Operators.DisallowShortTernary | ||
$args['url'] = false; |
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.
@pfefferle I think I'm missing historic context, hoping for you to be able to fill in my blanks.
My understanding is that setting the url
will short-circuit get_avatar_data()
for all cases where the comment type is not part of the $allowed_comment_types
array, so things like pings and trackbacks and such. Is that on purpose?
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.
exactly this
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.
Could you help me unpack that a bit more? What's the purpose for disabling avatars for all other comment types?
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.
they simply do not have avatars!?
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.
but I have no strong feelings about that... happy to remove it!
true | ||
) | ||
) { | ||
if ( ! \in_array( $id_or_email->comment_type ?: 'comment', $allowed_comment_types, true ) ) { // phpcs:ignore Universal.Operators.DisallowShortTernary |
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.
For context: Previously this conditional failed for an empty comment_type
, which used to be the default for comments before WP 5.5. We no longer support WP versions that old, but I wonder if we should still just fallback to comment
, instead of bailing and disabling avatars.
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.
happy to remove the fallback!
I was looking for a way to photonize avatar urls on WP.com for the Reactions block and realized that there's no way to filter the avatar url. This PR makes it possible to filter the URL and updates all other direct calls to comment meta to get the avatar url.
Proposed changes:
get_avatar_url()
instead of gettingavatar_url
comment meta directly.get_avatar_comment_types
filter.get_avatar_data
filter call to avoid double-filtering the value.get_avatar_url
filter toavatar_url
comment meta.Other information:
Testing instructions:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Unified retrieval of comment avatars and re-used core filters to give access to third-part plugins.