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: user meta retrieval #302

Merged
merged 5 commits into from
Oct 14, 2024
Merged

fix: user meta retrieval #302

merged 5 commits into from
Oct 14, 2024

Conversation

adekbadek
Copy link
Contributor

@adekbadek adekbadek commented Aug 28, 2024

Description of the Change

Add sanity checks to the user meta values read from the DB. We've seen this user data malformatted – maybe it was in migration from another WP instance.

How to test the Change

  1. Directly in the DB, update the simple_local_avatar meta of a user to an empty string
  2. Update the simple_local_avatar meta of another user to an object (e.g. update_user_meta(<id>, 'simple_local_avatar', (object) ['value' => 42]))
  3. Observe that these changes crash the site on develop branch
  4. Switch to this branch, observe the site is not crashing

Changelog Entry

Fixed - Handling of malformed simple_local_avatar user meta.

Checklist:

@jeffpaul jeffpaul requested review from faisal-alvi and removed request for jeffpaul August 28, 2024 13:11
@jeffpaul jeffpaul added this to the Future Release milestone Aug 28, 2024
@dkotter dkotter modified the milestones: Future Release, 2.8.0 Aug 28, 2024
Copy link
Collaborator

@dkotter dkotter left a comment

Choose a reason for hiding this comment

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

In addition to the requested changes, I see a few other places that we are doing a get_user_meta call that probably make sense to update to this new get_user_local_avatar method:

* @param int $user_id User ID.
* @return array Array with avatar data.
*/
public static function get_user_local_avatar( $user ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We use the variable $user_id but the only variable passed in is $user. I think that just needs updated:

Suggested change
public static function get_user_local_avatar( $user ) {
public static function get_user_local_avatar( $user_id ) {

* @param int $user_id User ID.
* @return array Array with avatar data.
*/
public static function get_user_local_avatar( $user ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because this method is marked as static, we get an error when trying to use $this later on

@@ -343,7 +357,7 @@ public function get_simple_local_avatar_url( $id_or_email, $size ) {
}

// Fetch local avatar from meta and make sure it's properly set.
$local_avatars = get_user_meta( $user_id, $this->user_key, true );
$local_avatars = self::get_user_local_avatar( $user_id );
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we change the above method to not be static, this will need updated:

Suggested change
$local_avatars = self::get_user_local_avatar( $user_id );
$local_avatars = $this->get_user_local_avatar( $user_id );

@@ -1181,7 +1195,7 @@ public function ajax_assign_simple_local_avatar_media() {
* @param int $user_id User ID.
*/
public function avatar_delete( $user_id ) {
$old_avatars = (array) get_user_meta( $user_id, $this->user_key, true );
$old_avatars = self::get_user_local_avatar( $user_id );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$old_avatars = self::get_user_local_avatar( $user_id );
$old_avatars = $this->get_user_local_avatar( $user_id );

@adekbadek
Copy link
Contributor Author

Hi @dkotter , thanks for the review! I've added some changes in d9ff4bb

@adekbadek adekbadek requested a review from dkotter September 2, 2024 10:44
@@ -343,7 +357,7 @@ public function get_simple_local_avatar_url( $id_or_email, $size ) {
}

// Fetch local avatar from meta and make sure it's properly set.
$local_avatars = get_user_meta( $user_id, $this->user_key, true );
$local_avatars = $this->get_user_local_avatar( $user_id );
if ( empty( $local_avatars['full'] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

This condition might cause a PHP notice or warning if the get_user_local_avatar() method returns an array that doesn't have the full key or if an empty array is returned. To avoid the notice, we can check if the key exists before accessing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a check in eeb66bc

@jeffpaul
Copy link
Member

@adekbadek thanks for the PR; are you able to respond to the code review items here?

@adekbadek
Copy link
Contributor Author

@jeffpaul – sorry for the late reply – I've addressed the review now.

includes/class-simple-local-avatars.php Outdated Show resolved Hide resolved
includes/class-simple-local-avatars.php Outdated Show resolved Hide resolved
@faisal-alvi faisal-alvi merged commit 53b5f53 into 10up:develop Oct 14, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants