Skip to content
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

Fix Default Avatar Fallback (Param: default vs url) #281

Merged
merged 3 commits into from
May 16, 2024

Conversation

faisal-alvi
Copy link
Member

@faisal-alvi faisal-alvi commented May 15, 2024

Description of the Change

In #278, we fixed 2 issues as discussed on the slack but there is one more scenario we should handle.

Solution in this PR: The default parameter should be used only when the default avatar is set from the Media Library images.

Technically, we utilize the url parameter to establish the default (fallback) avatar when the "Local only" setting is enabled, and we use the default parameter when it's disabled. However, when it's disabled and, for instance, the RoboHash option is selected as the default avatar, WordPress core invariably checks for the url parameter and finds it empty, thus defaulting to the Gravatar logo as reported, bypassing the avatar set via the default parameter.

We have now realized that this is a complex flow because we introduced an option to select an actual image from the media library since #96. However, WordPress core is configured to utilize default avatars based solely on their respective names (e.g., mystery, blank, retro), taking these strings in the default parameter. Now, after #96, we must pass the full image URL in default, which has added complexity to this flow and requires careful handling. Ultimately, with this PR, we're appropriately (and hopefully) managing all scenarios with multiple conditions and utilizing the default versus url parameter according to the settings.

Closes https://wordpress.org/support/topic/gravatar-avatars-not-working/page/2/#post-17754560

How to test the Change

Changelog Entry

Fixed - Default Avatar (like RoboHash) always fallback to Gravatar Logo

Credits

Props @faisal-alvi

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@faisal-alvi faisal-alvi self-assigned this May 15, 2024
@github-actions github-actions bot added this to the 2.8.0 milestone May 15, 2024
@github-actions github-actions bot added the needs:code-review This requires code review. label May 15, 2024
@faisal-alvi faisal-alvi changed the title Fix Default Avatar Fallback (Param: default vs url) [Draft] Fix Default Avatar Fallback (Param: default vs url) May 15, 2024
@faisal-alvi faisal-alvi requested a review from dkotter May 15, 2024 11:56
@faisal-alvi faisal-alvi changed the title [Draft] Fix Default Avatar Fallback (Param: default vs url) Fix Default Avatar Fallback (Param: default vs url) May 15, 2024
@qasumitbagthariya
Copy link
Contributor

qasumitbagthariya commented May 15, 2024

QA Update ✅


Thanks for the PR @faisal-alvi

I have verified that the associated problem in the fix/default-avatar-issue branch has been fixed and is functioning as intended.

Testing Environment

  • WordPress: 6.5.3
  • Theme: Twenty Twenty-Four 1.1
  • PHP: 8.0.30
  • Web Server: Nginx 1.20.2
  • Browser: Chrome
  • OS: macOS Ventura 13.3
fix_default-avatar-issue.mp4

Cases related to this Issue/PR are added to the Critical Flow Wiki pages:

  • Yes
  • Not Required/Applicable for this PR

@faisal-alvi faisal-alvi merged commit c1a1f1e into develop May 16, 2024
17 checks passed
@faisal-alvi faisal-alvi deleted the fix/default-avatar-issue branch May 16, 2024 10:02
@dkotter dkotter modified the milestones: 2.8.0, 2.7.10 May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants