Skip to:
Content

BuddyPress.org

Opened 5 weeks ago

Last modified 4 weeks ago

#8378 new defect (bug)

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

Reported by: boonebgorges Owned by:
Milestone: Up Next 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 5 weeks ago.

Download all attachments as: .zip

Change History (6)

@boonebgorges
5 weeks ago

#1 @boonebgorges
5 weeks 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
5 weeks 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 weeks 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 weeks ago

Thanks for reviewing, @imath !

#5 @imath
4 weeks 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.

Note: See TracTickets for help on using tickets.