Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 4 years ago

#6091 closed defect (bug)

Duplicate xprofile fields when using object cache

Reported by: lakrisgubben Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Extended Profile Keywords: needs-unit-tests needs-patch close
Cc: alex@…, m_uysl@…

Description

I'm having the same problem as described here: https://buddypress.org/support/topic/duplicated-xprofile-fields/ and after searching for the problem it definitely has to do with using an object cache.

The problem is in bp-xprofile-classes.php and has to with the fact that on row 299 to 303 the list of group fields are just added to without checking if the field already exists.

My solution, which seems to work is replacing the current code with

			$populated_fields = wp_list_pluck( $groups[$index]->fields, 'id' );

			foreach( (array) $fields as $field ) {
				if ( $group->id == $field->group_id && ! in_array( $field->id, $populated_fields ) ) {
					$groups[$index]->fields[] = $field;
				}
			}

Attachments (1)

6091.diff (756 bytes) - added by lakrisgubben 6 years ago.

Download all attachments as: .zip

Change History (25)

This ticket was mentioned in Slack in #buddypress by lakrisgubben. View the logs.


6 years ago

#2 @r-a-y
6 years ago

  • Keywords reporter-feedback added

So I've looked into the code that lakrisgubben mentioned and am trying to figure out how $groups[$index]->fields would be populated before line 301:
https://buddypress.trac.wordpress.org/browser/tags/2.1.1/src/bp-xprofile/bp-xprofile-classes.php?marks=299-303#L292

$groups before line 301 should not have the fields property set yet.

$groups should only contain the data from the wp_bp_xprofile_groups table:
https://buddypress.trac.wordpress.org/browser/tags/2.1.1/src/bp-xprofile/bp-xprofile-classes.php#L175
https://buddypress.trac.wordpress.org/browser/tags/2.1.1/src/bp-xprofile/bp-xprofile-classes.php?marks=358-359#L318

lakrisgubben - Do you have any steps to replicate the duplicate profile fields issue? Are you using any custom snippets or code to grab / show the profile data on other pages?

Last edited 6 years ago by r-a-y (previous) (diff)

#3 @lakrisgubben
6 years ago

Sure, thanks for the quick reply. I've tested this on a clean install with latest wp and bp. Everything was working good so I thought I might have been mistaken, but then I added one call to bp_member_profile_data() and then the issue turns up again. But I realised now that maybe you should use the bp_profile_field_data() function instead, which makes the problem go away. So this might be a false report, anyways the problem seems to lie in when get_group_data() in class BP_XProfile_Group is called twice and already has a cache populated with profile fields.

#4 @r-a-y
6 years ago

Where are you adding your call to bp_member_profile_data()?

Can you list the exact steps to duplicate?

#5 @lakrisgubben
6 years ago

Steps to reproduce...

  1. clean install of latest wp+bp
  2. add a new xprofile field
  3. turn on object cache (w3tc+apc is what I use)
  4. add a call to bp_member_profile_data() in /members/single/member-header.php

#6 @boonebgorges
6 years ago

lakrisgubben - Can you please give some more specific steps? Presumably, we have to have some number of users (more than one, I assume?) and more than one xprofile field. I assume that the user in question should not have any value entered for that field? And I assume that we should be looking to see this on example.com/members/boone - or does it have to be a subpage?

It's possible that this is related to #5570. See esp https://buddypress.trac.wordpress.org/ticket/5570#comment:3

My solution, which seems to work is replacing the current code with

Could you be more specific about this? I'm not sure which 299-303 you're referring to. What version of BP? Can you provide a diff?

#7 @lakrisgubben
6 years ago

It doesn't seem to matter the number of users, or the number of fields. The problem is there even with no xprofile fields except for the default name, as long as I call <?php bp_member_profile_data( 'name' ); ?> in /members/single/member-header.php.

If the user hasn't entered a value for the field, it won't show up twice, that's only happening when the field has a value. And I've only seen it showing up on example.com/members/admin/profile/ and example.com/members/admin/profile/edit/ i.e. those places where the profile field is displayed twice (first time in the member header which I've added and then also on the list/edit view from BP).

Seems that it could definitely be related to #5570, I'll upload a patch that fixes it for me so you can take a look. Thanks for the help! :)

@lakrisgubben
6 years ago

#8 @johnjamesjacoby
6 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 2.3

#9 @boonebgorges
5 years ago

lakrisgubben - Thanks for the patch, and sorry for not reviewing sooner. I have banged my head against the computer a bit more here, and I still don't understand what's going on. As far as I can see, there is no way that the 'fields' property in the $groups array being touched by your patch could possibly be populated from the persistent cache. As such, I can't manage to make the $populated_fields variable be anything but an empty array.

The $populated_fields check, or something like it, is pretty harmless. But I'm very wary of putting the fix in without better understanding the circumstances under which the problem is being exhibited. lakrisgubben or anyone else, is there any chance you can debug further how the errant 'fields' data is being set in the cache?

#10 @lakrisgubben
5 years ago

Hey boone, thanks for the reply. I've tried to dig through this a bit more with the latest trunk. The problem still persists for me and the patch (even though it has to be refreshed a bit, but the logic is still valid) still takes care of the problem. But I can't find how/why the fields data is being printed twice, so unfortunately it seems like I can't be of further help here. :/ And seeing as how comment 3 in this thread fixes the issue (i.e. changing 'bp_member_profile_data' to 'bp_profile_field_data') it's not a big problem. But of course a bit annoying that I can't seem to track the cause down. :)

#11 @boonebgorges
5 years ago

  • Keywords needs-patch added; reporter-feedback removed
  • Milestone changed from 2.3 to Future Release

Thanks for reporting back, lakrisgubben. I'm going to leave this ticket open but move it out of the milestone. The fix you've proposed in the patch is very much a band-aid over some other problem, and I don't want to resort to this kind of workaround without understanding the cause. If you're seeing cache bleed in one spot, it's likely that there are other places where the symptoms would manifest themselves, and I want to be able to fix it at the source.

#12 @ethanfsmith
4 years ago

Has this been addressed? I'm experiencing the exact same problem and traced it to Memcached Redux, an object caching plugin that's part of WPEngine's default configuration.
Thanks!
Ethan

#13 @boonebgorges
4 years ago

@ethanfsmith No, it hasn't been addressed, because I think that we don't have reliable steps to reproduce yet. But this additional info may be enough. Do you know whether Memcached Redux in use on WP Engine is an unmodified version of the latest item here? https://wordpress.org/plugins/memcached-redux/ If not, are you able to provide the latest for testing?

#14 @boogah
4 years ago

@ethanfsmith Just a heads up, we're working on fixing the object caching issues on our (WP Engine's) platform. I don't have a timeline as to when it'll be fixed, but I do know the dev working on it and it's very much under active development.

#15 @boonebgorges
4 years ago

@boogah Does that mean that this is a WP Engine bug and not a BuddyPress one? :)

#16 @boogah
4 years ago

@boonebgorges Well, @ethanfsmith's issue is likely a WPE bug. Can't speak for anyone else though. ;)

#18 @m_uysl
4 years ago

  • Cc m_uysl@… added

I got similar problem with http://wordpress.org/plugins/memcached/. It happened in production and repeated after flushing cache, somehow I couldn't reproduce in development.

Commenting here just in case.

Last edited 4 years ago by m_uysl (previous) (diff)

#19 @boonebgorges
4 years ago

@m_uysl - Are you hosted on WP Engine by any chance?

#20 @m_uysl
4 years ago

@boonebgorges no, we are not hosted on WP Engine, I found the bug and reproduced steps in development. Please take a look #7401 (I created new ticket because, it might not fit this ticket.)

#21 @boonebgorges
4 years ago

In 11316:

XProfile: More consistent cache behavior when fetching user data.

  • Inside of a profile group loop (BP_XProfile_Group::get()), don't fetch user data when pulling up BP_XProfile_Field objects. In the absence of finer-grained information about users, fetching a field object grabs the data associated with the logged-in user. But in many cases, the logged-in user is irrelevant to the fields being looped over, so there's no benefit to pulling up this data. (When necessary - fetch_data - the data is queried separately, later in the get() method.)
  • When caching database misses for a data query (because the specifed user doesn't have anything filled in for the given field), store the field_id and user_id properties on the cached object. This ensures that values are properly associated with their fields when being displayed.

These changes resolve an issue where cached data for the logged-in user
can be shown erroneously on another user's profile, when the other user
doesn't have a value for a given field.

Props m_uysl, r-a-y.
See #6091. Fixes #7401.

#22 @boonebgorges
4 years ago

  • Keywords reporter-feedback added

We (me and @m_uysl) have a feeling that [11316] will address the problem originally reported here. @lakrisgubben or anyone else who was experiencing the issue - can you reproduce after that changeset?

#23 @lakrisgubben
4 years ago

  • Keywords close added; reporter-feedback removed

I can't reproduce the original problem reported in this ticket after the changeset, thanks @m_uysl @boonebgorges.

I think this ticket can be closed as fixed. But leaving that to you @boonebgorges.

#24 @boonebgorges
4 years ago

  • Milestone Future Release deleted
  • Status changed from new to closed

Thanks, @lakrisgubben !

Note: See TracTickets for help on using tickets.