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

Add option to clear cache of user meta to remove image sizes that do not exist #90

Merged
merged 8 commits into from
Dec 10, 2021

Conversation

thrijith
Copy link
Member

Description of the Change

  • Adds a Clear Cache button in Settings -> Discussion, which updates the user meta to remove images that don't exist.

Alternate Designs

N/A

Benefits

  • Fixes issue with broken avatar links after media regeneration which may delete the images.

Possible Drawbacks

N/A

Verification Process

  • Set an avatar for your profile
  • Install Regenerate Thumbnails plugin
  • Use Tools -> Regenerate Thumbnails, select Delete thumbnail files for old unregistered sizes in order to free up server space. This may result in broken images in your posts and pages. checkbox and regenerate the thumbnails.
  • This will make the avatar images broken, go to Settings -> Discussion and use the Clear Cache to fix the issue.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

Fixs #31

Changelog Entry

  • Add Clear Cache button to fix broken avatar images after thumbnail regeneration.

@thrijith thrijith requested a review from dinhtungdu November 19, 2021 16:34
@thrijith thrijith self-assigned this Nov 19, 2021
@vikrampm1 vikrampm1 added the type:enhancement New feature or request. label Nov 20, 2021
Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

@thrijith In this PR, we're deleting cache for the current admin user, normal users can't delete the cache because they don't have access to the setting page. Besides, the main usage of this feature is deleting the cache after regenerating thumbnails, which only admins can perform.

I think the deleting cache function should delete the avatar cache for all users instead. We also need to care about the performance of large sites that have many users.

@jeffpaul jeffpaul linked an issue Nov 22, 2021 that may be closed by this pull request
@jeffpaul jeffpaul modified the milestones: Future Release, 2.3.0 Nov 22, 2021
@thrijith
Copy link
Member Author

@thrijith In this PR, we're deleting cache for the current admin user, normal users can't delete the cache because they don't have access to the setting page. Besides, the main usage of this feature is deleting the cache after regenerating thumbnails, which only admins can perform.

I think the deleting cache function should delete the avatar cache for all users instead. We also need to care about the performance of large sites that have many users.

@dinhtungdu I've made some changes, please check again when you get a chance, thanks!

@jeffpaul jeffpaul requested a review from dinhtungdu November 24, 2021 21:56
Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

@thrijith This is likely to have timeout issues IMO. For large sites with a lot of users, it would take time to get all users, check and update the meta for each user. We're doing everything in a page load, which is not reliable to me. I think Ajax can be the solution to this issue.

@thrijith
Copy link
Member Author

thrijith commented Dec 3, 2021

@thrijith This is likely to have timeout issues IMO. For large sites with a lot of users, it would take time to get all users, check and update the meta for each user. We're doing everything in a page load, which is not reliable to me. I think Ajax can be the solution to this issue.

@dinhtungdu please check again when you get a chance, thanks!

@jeffpaul jeffpaul requested a review from dinhtungdu December 6, 2021 19:52
@dinhtungdu dinhtungdu mentioned this pull request Dec 7, 2021
Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

@thrijith thanks for the update! I left some comments on your PR, please take a look. I also format the admin.js file in #91, please check it as well. Feel free to merge it if it makes sense to you.

includes/class-simple-local-avatars.php Outdated Show resolved Hide resolved
}
},
error : function () {
alert(slaAdmin.error);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of alert, can we print the error message below the clear cache button? By doing so we can display more detailed errors. We can even add the status there too, for example:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the message span below the button, demo https://cln.sh/u5NhwT

}
}

if ( ! empty( $rows ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of creating $row to hold only user IDs, I think we can use the $users variable above to check for the end of the progress.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dinhtungdu I've updated this, please check again when you get a chance, thanks!

dinhtungdu
dinhtungdu previously approved these changes Dec 10, 2021
Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

LGTM! Left an optional comment but this is ready to be merged!

includes/class-simple-local-avatars.php Outdated Show resolved Hide resolved
@jeffpaul jeffpaul merged commit 9499974 into develop Dec 10, 2021
@jeffpaul jeffpaul deleted the feature/add-clear-cache branch December 10, 2021 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

avatars are being deleted after thumbnail regenarting
4 participants