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.

Sign up for GitHub

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

Jump to bottom

Host/Domain change support added for local avatar url #216

Merged
merged 16 commits into from
Sep 25, 2023
Merged

Host/Domain change support added for local avatar url #216

merged 16 commits into from
Sep 25, 2023

Conversation

jayedul
Copy link
Contributor

@jayedul jayedul commented Jun 26, 2023

Description of the Change

Current domain/host will be used for local avatar URLs even after site migration.

Closes #64

How to test the Change

  • Simply edit the image url host in simple_local_avatar user meta, maybe in database directly for easy test.
  • Reload the page wherever the avatar image is supposed to load
  • The image should be regenerated for that specific avatar size and the URL should get back to its original state based on current host/domain.

Changelog Entry

Fixed - Local avatar urls remain old after domain/host change

Credits

Props @jayedul

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.

@jayedul jayedul self-assigned this Jun 26, 2023
@jeffpaul jeffpaul added this to the 2.7.6 milestone Jul 10, 2023
@jeffpaul jeffpaul requested review from a team and ravinderk and removed request for a team July 10, 2023 21:04
@ravinderk
Copy link
Contributor

@jayedul Is this pull request ready for review?

@jeffpaul
Copy link
Member

@ravinderk let's go ahead and proceed with review on the PR

ravinderk
ravinderk suggested changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@ravinderk ravinderk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jayedul I left one suggestion. Let me know if you have questions.

includes/class-simple-local-avatars.php Show resolved Hide resolved
@jeffpaul jeffpaul marked this pull request as ready for review September 19, 2023 13:16
@github-actions github-actions bot added the needs:code-review This requires code review. label Sep 19, 2023
@ravinderk ravinderk self-requested a review September 19, 2023 13:25
@jeffpaul
Copy link
Member

@faisal-alvi can you help with the PHPUnit failures here as well as general testing/code review to help get this ready for merge/release?

@faisal-alvi
improve condition to fix cypress fatal error
7ce4d23 
Fatal error: Uncaught TypeError: array_key_exists(): Argument #2 ($array) must be of type array, string given in /var/www/html/wp-content/plugins/simple-local-avatars/includes/class-simple-local-avatars.php:342
@faisal-alvi
fixing phpcs issues
a6770c5
@faisal-alvi
Copy link
Member

The PHPUnit and Cypress test failures are fixed.

Also, I've tested this in a single setup site, and it's working perfectly. I'd like to move forward with testing it in a multisite network setup before we proceed with the merge. Hopefully, I'll have some time next week to conduct that testing.

faisal-alvi
faisal-alvi approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@faisal-alvi faisal-alvi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was tested in a multisite network, working as expected. Thanks for the work, everyone!

@faisal-alvi faisal-alvi removed the needs:code-review This requires code review. label Sep 25, 2023
@faisal-alvi
Copy link
Member

Note: Unable to merge, requires one more approval. cc @ravinderk

@ravinderk
Copy link
Contributor

@faisal-alvi pr approved 🤞

@faisal-alvi faisal-alvi merged commit 8188338 into develop Sep 25, 2023
11 checks passed
@faisal-alvi faisal-alvi deleted the fix/64 branch September 25, 2023 13:03
@leogermani leogermani mentioned this pull request Nov 30, 2023
Closed
1 task
@jakejackson1 jakejackson1 mentioned this pull request Dec 5, 2023
Closed
1 task
This was referenced Dec 7, 2023
Merged
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@faisal-alvi faisal-alvi faisal-alvi approved these changes

@ravinderk ravinderk ravinderk approved these changes

Assignees

@jayedul jayedul

Labels
None yet
Projects
None yet
Milestone
2.7.6
Development

Successfully merging this pull request may close these issues.

Avatar support when moving hosts / changing domains
4 participants
@jayedul @ravinderk @jeffpaul @faisal-alvi