XProfile fields should have cache support

[boonebgorges]

We have some cache support for xprofile groups and for xprofile data, but there is currently pretty much nothing for the fields themselves. At a glance, it looks like the following should happen:

• xprofile_get_field() should have a caching layer, so that calling xprofile_get_field( $field_id ) many times only hits the database once.
• BP_XProfile_Groups::get() currently does a SELECT a bunch of fields FROM $bp->profile->table_name_fields. Instead, we should split the query: one IDs only query, then update unprimed field caches, then fill in the field objects.
• Invalidation as necessary.

WordPress generally combines this sort of cache implementation with the process of mapping to proper objects: $this->posts = array_map( 'get_post', $posts ). See #6358. Maybe this is a good place to start that process too.

I think this ticket is a blocker for #5625.

[boonebgorges]

In 10164:

Lazy-load visibility properties in BP_XProfile_Field.

Previously, populating a new BP_XProfile_Field object would result in two
queries: one to fetch the field row from the database, and one to populate the
field meta cache in order to set the 'allow_custom_visibility' and
'default_visibility' properties. It's not often the case that we need the
latter properties, which means that the cache-priming query is wasted.

This changeset addresses the shortcoming by loading the properties only when
they're requested:

• $allow_custom_visibility and $default_visibility are now protected properties of the object. This directs requests for them to the magic methods.
• Introduce __isset() and __get() to handle backward compatibility.
• Introduce public getter functions for each property.

See #6638.

[boonebgorges]

6638.diff​ does a few main things:

• Splits the field query in BP_XProfile_Group::get(), and adds cache priming support there
• Turns xprofile_get_field() into a wrapper that can handle lots of different types of input, and convert to BP_XProfile_Field objects. This is similar to how get_post() and friends work.
• Adds cache support to xprofile_get_field().
• Using these tools, BP_XProfile_Group::get() now returns real field objects (array_map( 'xprofile_get_field', $field_ids )) instead of stdClass. See #6358.

What do people think of the approach? The awkward bit is fill_data() - our __construct() method expects an integer, and I could have used some parameter overloading, but thought that this was a bit more transparent.

If we like the approach, we can apply the return-real-objects approach elsewhere in BP.

[DJPaul]

Patch looks good. I haven't any concerns with fill_data. I think it's fine.

[r-a-y]

Yeoman's work here, boonebgorges!

Like DJPaul, I have no qualms with the fill_data() method. I did make a slight change in this method to use stripslashes() on the field and description properties so the properties are the same as in ​BP 2.3.x.

My only concern is in BP_XProfile_Group::get().

On line 386 of class-bp-xprofile-group.php, in the foreach ( $uncached_fields as $uncached_field ) block, you are caching $uncached_field in the 'bp_xprofile_fields' cachegroup. At this stage, $uncached_field is not a complete row from the $bp->profile->table_name_fields table as it should be (see BP_XProfile_Field::get_instance() where the initial cache is set).

Should the entire block from line 381-390 be removed?

The reason I say this is further down on line 394, you grab the fields again via field ID, which should trigger the cache to be set if it isn't already via xprofile_get_field( $field_id ) => BP_XProfile_Field::get_instance().

[boonebgorges]

Replying to r-a-y:

My only concern is in BP_XProfile_Group::get().

On line 386 of class-bp-xprofile-group.php, in the foreach ( $uncached_fields as $uncached_field ) block, you are caching $uncached_field in the 'bp_xprofile_fields' cachegroup. At this stage, $uncached_field is not a complete row from the $bp->profile->table_name_fields table as it should be (see BP_XProfile_Field::get_instance() where the initial cache is set).

Should the entire block from line 381-390 be removed?

The reason I say this is further down on line 394, you grab the fields again via field ID, which should trigger the cache to be set if it isn't already via xprofile_get_field( $field_id ) => BP_XProfile_Field::get_instance().

Ah, good catch. The reason for lines 381-390 is to fetch data for all uncached fields in a single database query. If we wait until xprofile_get_field( $field_id ), it'll be done one field at a time - 10 possible queries. But you're right that there's an error: it should probably be SELECT * FROM ... instead of SELECT id, name, description.... Then the rows will be complete.

Stripslashes changes look good. Thanks for noticing that.

I'm going to sleep on this and probably go for it tomorrow. Thanks to you and to DJPaul for having a look!

[boonebgorges]

In 10198:

Introduce cache support for xprofile fields.

xprofile_get_field() will now look in the cache before returning a
BP_XProfile_Field object (via the new BP_XProfile_Field::get_instance()),
and will populate the cache if necessary. It's strongly recommended to use
xprofile_get_field() when fetching individual existing fields, instead of
creating new BP_XProfile_Field objects directly.

BP_XProfile_Group::get() now leverages the field cache too. Instead of a
SELECT * query, it fetches a list of matching field IDs SELECT id, and
populates the full field objects from the cache, if available. The fields
property of BP_XProfile_Group objects is now an array of BP_XProfile_Field
objects, not stdClass. See #6358.

Props boonebgorges, r-a-y.
See #6638.

[boonebgorges]

I was about to start looking into improving the treatment of $field->data by lazyloading it similar to what I did in [10164], but it looks like that change will require non-trivial modifications to the way BP_XProfile_ProfileData works in order to maintain backward compatibility. A project for another day :)

[boonebgorges]

In 10200:

Use xprofile_get_field() throughout bp-xprofile.

It's properly cached after [10198], so let's use it.

See #6638.