Skip to:
Content

BuddyPress.org

Opened 4 years ago

Last modified 13 days ago

#8378 assigned defect (bug)

Calls to `xprofile_get_field()` can result in unnecessary database queries

Reported by: boonebgorges's profile boonebgorges Owned by: espellcaste's profile espellcaste
Milestone: Under Consideration Priority: normal
Severity: normal Version:
Component: Extended Profile Keywords: has-patch
Cc:

Description

When calling xprofile_get_field(), the $get_data parameter defaults to true. When users are logged out, this doesn't have an effect. But when users are logged in, this means that calls to xprofile_get_field() will pull up the data for that field for the logged-in user. See https://buddypress.trac.wordpress.org/browser/tags/6.3.0/src/bp-xprofile/classes/class-bp-xprofile-field.php?marks=202-204#L189.

In the vast majority of cases, this feels inappropriate. A function named xprofile_get_field() should pull up the *field*. It should not load other data unless specifically asked to do so.

This happens in at least one place in BP itself. When you fetch user data for a specific field using xprofile_get_field_data(), the value is passed through the xprofile_filter_format_field_value_by_field_id() filter. This function needs to fetch the field's type, so it pulls up the field using xprofile_get_field(). See https://buddypress.trac.wordpress.org/browser/tags/6.3.0/src/bp-xprofile/bp-xprofile-filters.php?marks=313#L302.

This can result in large numbers of unnecessary database hits. Even in cases where persistent caching is present, the idiosyncratic cache behavior of BP_XProfile_ProfileData means that users who don't have a value set for the field in question will always result in a cache miss. See https://buddypress.trac.wordpress.org/browser/tags/6.3.0/src/bp-xprofile/classes/class-bp-xprofile-profiledata.php?marks=94-96#L82.

There are a few things worth discussing:

  1. The $userdata fallback in BP_XProfile_Field::populate() goes back to the very origins of BuddyPress. See https://buddypress.trac.wordpress.org/browser/trunk/bp-xprofile/bp-xprofile-classes.php?annotate=blame&rev=2786&marks=240-242#L231 - I didn't trace it back any further. As noted above, I think it's an error to have this behavior here. But it might be too hard to change at this point. But we could probably make it somewhat easier to override, for example by checking null === $user_id; this would allow you to pass 0 to explicitly override the behavior.
  1. We should audit use of xprofile_get_field() in BP to ensure that we're explicitly calling $get_data = false when appropriate. The xprofile_filter_format_field_value_by_field_id() is one example.
  1. When querying for a user's field data, null query results should be cached.

Some of these changes are harder than others and may warrant separate tickets. I'll start with a patch for item 2 and see what others think.

Attachments (1)

8378.diff (6.3 KB) - added by boonebgorges 4 years ago.

Download all attachments as: .zip

Change History (13)

@boonebgorges
4 years ago

#1 @boonebgorges
4 years ago

8378.diff proposes changes to the way that xprofile_get_field() is called throughout BP. Note that I have changed all but one calls to xprofile_get_field(). The only one I didn't change is here https://buddypress.trac.wordpress.org/browser/tags/6.3.0/src/bp-xprofile/bp-xprofile-functions.php?marks=415#L403. I only excluded this one because the filters may expect the data to be populated.

But I think that this patch shows how the current behavior of xprofile_get_field() is a little crazy. IMHO we could safely switch the default value of $get_data to false. It's technically possible for this to break functionality for someone, somewhere, but I would be very surprised if this were the case.

#2 @imath
4 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 7.0.0

Hi @boonebgorges

Thanks for this analysis. I agree with it. Let’s start with your first patch for 7.0.0 and improve it during next release 👌

#3 @boonebgorges
4 years ago

In 12768:

Declare $get_data = false when calling xprofile_get_field().

The default value of $get_data is true, which means that when calling
xprofile_get_field() with no explicit $user_id, but with a logged-in user,
BP_XProfile_Field::populate() will attempt to fetch the user's data. This
is usually neither intended nor needed, and results in unnecessary database
overhead.

See #8378.

#4 @boonebgorges
4 years ago

Thanks for reviewing, @imath !

#5 @imath
4 years ago

  • Milestone changed from 7.0.0 to Up Next

You're welcome 😉. I'm moving the ticket to the next milestone so that we can do the next steps.

#6 @imath
23 months ago

  • Milestone changed from Up Next to 12.0.0

#7 @imath
16 months ago

  • Milestone changed from 12.0.0 to Up Next

#8 @imath
11 months ago

  • Milestone changed from Up Next to 14.0.0

#9 @espellcaste
9 months ago

  • Milestone changed from 14.0.0 to Up Next

#10 @imath
4 months ago

  • Milestone changed from Up Next to 15.0.0

#11 @espellcaste
3 months ago

  • Owner set to espellcaste
  • Status changed from new to assigned

I'll look into it.

#12 @espellcaste
13 days ago

  • Milestone changed from 15.0.0 to Under Consideration

I'll punt this ticket so that I can find a solution that works best with #9147 too.

Note: See TracTickets for help on using tickets.