[buddypress-trac] [BuddyPress Trac] #8378: Calls to `xprofile_get_field()` can result in unnecessary database queries

[buddypress-trac]

#8378: Calls to `xprofile_get_field()` can result in unnecessary database queries
------------------------------+-----------------------------
 Reporter:  boonebgorges      |      Owner:  (none)
     Type:  defect (bug)      |     Status:  new
 Priority:  normal            |  Milestone:  Awaiting Review
Component:  Extended Profile  |    Version:
 Severity:  normal            |   Keywords:
------------------------------+-----------------------------
 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.

 2. 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.

 3. 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.

-- 
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/8378>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac