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 | Owned by: | 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:
- The
$userdata
fallback inBP_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 checkingnull === $user_id
; this would allow you to pass0
to explicitly override the behavior.
- We should audit use of
xprofile_get_field()
in BP to ensure that we're explicitly calling$get_data = false
when appropriate. Thexprofile_filter_format_field_value_by_field_id()
is one example.
- 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)
Change History (13)
#2
@
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 👌
8378.diff proposes changes to the way that
xprofile_get_field()
is called throughout BP. Note that I have changed all but one calls toxprofile_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
tofalse
. It's technically possible for this to break functionality for someone, somewhere, but I would be very surprised if this were the case.