Skip to content

Conversation

@mi5t4n
Copy link
Member

@mi5t4n mi5t4n commented May 5, 2025

Description

This PR saves the google profile picture of the user during account creation.

Demo

  1. Before
    Screencast 2025-05-05 19:06:18.webm

  2. After
    Screencast 2025-05-05 19:07:38.webm

Closes - #249

@mi5t4n mi5t4n self-assigned this May 5, 2025
@mi5t4n mi5t4n requested a review from SH4LIN May 5, 2025 13:26
rtBot
rtBot previously requested changes May 5, 2025
Copy link
Contributor

@rtBot rtBot left a comment

Choose a reason for hiding this comment

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

Code analysis identified issues

action-phpcs-code-review has identified potential problems in this pull request during automated scanning. We recommend reviewing the issues noted and that they are resolved.

phpcs scanning turned up:

🚫 1 error


Powered by rtCamp's GitHub Actions Library

@rtBot rtBot dismissed their stale review May 5, 2025 13:30

Dismissing review as all inline comments are obsolete by now

@mi5t4n mi5t4n requested a review from rtBot May 5, 2025 13:32
Copy link
Contributor

@SH4LIN SH4LIN left a comment

Choose a reason for hiding this comment

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

image

"You can change your profile picture on Gravatar."

Wouldn't this be confusing? Currently, the feature only updates the image during registration. We should either periodically check for updates, add a button to re-sync the image, or enhance the feature to also check the profile image during login.

I'm testing the .tmp case for download_url() and will submit feedback after some time.

// Using larger image size. By default, profile picture has 96 width size with cropped.
$profile_picture_url = str_replace( '=s96-c', '', $user->picture );

$profile_picture_filename = download_url( $profile_picture_url );
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a check for WPVIP? and if the site is hosted on WPVIP we directly use wpcom_vip_download_image() this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

@SH4LIN Yes, it can be a bit unclear—I’ll look into it further to make it more straightforward. As for re-syncing the image at each login, should we also extend this to include the First Name and Last Name, since those fields can also be updated?

Copy link
Member Author

Choose a reason for hiding this comment

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

@SH4LIN Regarding vip check, that is a good catch. I will promptly implement this.

Copy link
Contributor

Choose a reason for hiding this comment

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

should we also extend this to include the First Name and Last Name, since those fields can also be updated?

In WordPress, we have the option to update the first name and last name. So, if they want to customize it, they can do so from the settings. However, there is no option in the WordPress dashboard to change the 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.

Can we add a check for WPVIP? and if the site is hosted on WPVIP we directly use wpcom_vip_download_image() this function.

I tried to use this function, it seems it won't work in our scenario. I got the following error while trying to use it .
2025-05-15_15-38

The function was expecting a POST request, but Google calls the redirect URL using the GET method instead. We could try to change the global variables to make it work, but that would be a hacky solution. So, I decided to remove the function for now.

Copy link
Contributor

@SH4LIN SH4LIN left a comment

Choose a reason for hiding this comment

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

Hi @mi5t4n,

From what I can see in the implementation of media_handle_sideload(), it appears to handle the .tmp file case as well. Is there a specific reason we’ve added this case explicitly?

Also, I noticed that wpcom_vip_download_image() doesn’t seem to account for this scenario—just wanted to highlight that for consistency.

Thanks!

@mi5t4n
Copy link
Member Author

mi5t4n commented May 7, 2025

@SH4LIN In my test cases, media_handle_sideload() wasn't handling the temporary files correctly. Even though the MIME types were valid, it rejected the files because they had a .tmp extension, resulting in the error: "You are not allowed to upload the given file type." When the correct extension was set to the file according it's mime type, it worked.

@SH4LIN
Copy link
Contributor

SH4LIN commented May 7, 2025

@SH4LIN In my test cases, media_handle_sideload() wasn't handling the temporary files correctly. Even though the MIME types were valid, it rejected the files because they had a .tmp extension, resulting in the error: "You are not allowed to upload the given file type." When the correct extension was set to the file according it's mime type, it worked.

If that's the case then another option is this, we identify mime, and ext from our end pass it to the file array with key type and ext. Can we check whether this solution works or not.

Copy link
Contributor

@SH4LIN SH4LIN left a comment

Choose a reason for hiding this comment

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

@mi5t4n We will also need to update the docs with the newly added hooks.

@mi5t4n
Copy link
Member Author

mi5t4n commented May 15, 2025

image

"You can change your profile picture on Gravatar."

Wouldn't this be confusing? Currently, the feature only updates the image during registration. We should either periodically check for updates, add a button to re-sync the image, or enhance the feature to also check the profile image during login.

At the moment, I can think of two ways to make it better.

  1. Override the defaule Profile Picture section to handle the removal and upload of the picture.

Before:
2025-05-15_20-58_1

After:
2025-05-15_20-58

  1. Create a new section called Google Profile Picture and a global setting to override the default avatar.

2025-05-15_21-11

LMK your thoughts on this.

cc @SH4LIN @joelabreo227

@mi5t4n
Copy link
Member Author

mi5t4n commented May 16, 2025

@SH4LIN In my test cases, media_handle_sideload() wasn't handling the temporary files correctly. Even though the MIME types were valid, it rejected the files because they had a .tmp extension, resulting in the error: "You are not allowed to upload the given file type." When the correct extension was set to the file according it's mime type, it worked.

If that's the case then another option is this, we identify mime, and ext from our end pass it to the file array with key type and ext. Can we check whether this solution works or not.

I'm not sure why, but during my second test, the .tmp files were no longer created. Now, every profile picture is saved with the correct file extension. Also, we can provide the MIME type, but not the file extension directly — the extension comes from the filename itself. The media_handle_sideload() function throws an error if the file extension doesn’t match the MIME type.

Copilot AI review requested due to automatic review settings November 6, 2025 07:59
Copy link

Copilot AI left a 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 adds functionality to save and use Google profile pictures for user avatars. When users register via Google login, their profile picture is downloaded and stored as a WordPress media attachment, then used as their avatar throughout the site.

  • Adds profile picture saving during user registration with a filter to bypass the process
  • Implements avatar URL override to use stored Google profile pictures instead of Gravatar
  • Extracts profile picture saving logic into a reusable private method

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

File Description
src/Utils/Authenticator.php Adds save_user_profile_picture() method to download and store Google profile pictures during registration, with filter hook to bypass
src/Plugin.php Adds return_avatar_url() method hooked to get_avatar_url filter to replace Gravatar with stored profile pictures

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

*
* @param boolean $save Whether to save profile picture or not.
* @param int $uid WP User ID.
* @param \stdClass User object return by Google.
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

Corrected spelling of 'return' to 'returned' in the docblock parameter description.

Suggested change
* @param \stdClass User object return by Google.
* @param \stdClass User object returned by Google.

Copilot uses AI. Check for mistakes.
global $wp_filesystem;

if ( is_null( $wp_filesystem ) ) {
require_once ABSPATH . '/wp-admin/includes/file.php';
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

Inconsistent path separator usage. Line 209 uses '/wp-admin' while lines 213-215 use 'wp-admin' (without leading slash). The leading slash should be removed for consistency, as ABSPATH already ends with a trailing slash.

Suggested change
require_once ABSPATH . '/wp-admin/includes/file.php';
require_once ABSPATH . 'wp-admin/includes/file.php';

Copilot uses AI. Check for mistakes.
$profile_picture_url = str_replace( '=s96-c', '', $user->picture );

$profile_picture_filename = download_url( $profile_picture_url );

Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

Missing error handling for download_url() which can return a WP_Error object on failure. The code should check if the result is a WP_Error before proceeding to use it as a filename.

Suggested change
if ( is_wp_error( $profile_picture_filename ) ) {
// Optionally log the error: error_log( $profile_picture_filename->get_error_message() );
return;
}

Copilot uses AI. Check for mistakes.
$mime_types = wp_get_mime_types();
foreach ( $mime_types as $ext => $mime_type ) {
if ( $profile_picture_mime_type === $mime_type ) {
$profile_picture_extension = current( explode( '|', $ext ) );
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

The variable $profile_picture_extension may not be defined if no matching MIME type is found in the foreach loop. Initialize this variable before the loop or add an else condition to handle the case when no match is found.

Copilot uses AI. Check for mistakes.
);

$attachment_id = media_handle_sideload( $file_array );

Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

Missing error handling for media_handle_sideload() which can return a WP_Error object on failure. The code only checks if result is an integer but doesn't handle potential WP_Error cases or clean up the temporary file on error.

Suggested change
if ( is_wp_error( $attachment_id ) ) {
// Clean up the temporary file if there was an error.
if ( file_exists( $profile_picture_filename ) ) {
@unlink( $profile_picture_filename );
}
// Optionally, log the error.
// error_log( 'Failed to sideload profile picture: ' . $attachment_id->get_error_message() );
return;
}

Copilot uses AI. Check for mistakes.
*
* @since n.e.x.t
*
* @param boolean $use_saved_profile_picture_for_avatar Whether to bypass the use the saved profile picture for avatar or not.
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

Redundant word 'the' in the docblock. Should be 'Whether to bypass the use of the saved profile picture' or 'Whether to use the saved profile picture'.

Suggested change
* @param boolean $use_saved_profile_picture_for_avatar Whether to bypass the use the saved profile picture for avatar or not.
* @param boolean $use_saved_profile_picture_for_avatar Whether to bypass the use of the saved profile picture for avatar or not.

Copilot uses AI. Check for mistakes.
$profile_picture_id = get_user_meta( $wp_user->ID, 'rtlwg_profile_picture_id', true );

if ( ! empty( $profile_picture_id ) ) {
$url = wp_get_attachment_image_url( $profile_picture_id, [ $width, $height ] );
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

wp_get_attachment_image_url() can return false if the attachment doesn't exist. The code should check the return value and only update $url if a valid URL is returned, otherwise the original URL will be lost.

Suggested change
$url = wp_get_attachment_image_url( $profile_picture_id, [ $width, $height ] );
$profile_picture_url = wp_get_attachment_image_url( $profile_picture_id, [ $width, $height ] );
if ( $profile_picture_url ) {
$url = $profile_picture_url;
}

Copilot uses AI. Check for mistakes.
require_once ABSPATH . 'wp-admin/includes/file.php';
require_once ABSPATH . 'wp-admin/includes/image.php';

// Using larger image size. By default, profile picture has 96 width size with cropped.
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

Missing property existence check for $user->picture. The code should verify that the picture property exists on the user object before attempting to access it to avoid potential PHP warnings or errors.

Suggested change
// Using larger image size. By default, profile picture has 96 width size with cropped.
// Using larger image size. By default, profile picture has 96 width size with cropped.
if ( ! isset( $user->picture ) ) {
// No profile picture available, so nothing to do.
return;
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants