$bp->site_options should not be referenced directly

[boonebgorges]

Currently, we use $bp->site_options as storage for some networkwide values that are expensive to look up on the fly. We load these values in BP_Core_Component::setup_globals(). But there are cases where BuddyPress needs access to these values before they've been filled, such as bp_get_signup_allowed(). See #6043.

The solution is to have a wrapper function for accessing these site options, and then to use that wrapper whenever referencing them. Inside the function, load the site_options with bp_core_get_root_options() if they're not yet loaded into $bp->site_option. Then, be sure to use this wrapper whenever calling the options.

[boonebgorges]

In 9219:

Add tests for bp_blog_signup_enabled().

See #6045.

[boonebgorges]

In 9220:

An attempt to appease PHP 5.2 regarding string offsets.

As introduced in [9219].

See #6045.

[boonebgorges]

6045.patch​ introduces bp_core_get_root_option() and uses it throughout BP. Suggestions welcome.

[johnjamesjacoby]

Now that the _option() functions have caching on them at all levels, is our site_options cache even necessary anymore, other than for backwards compatibility? I believe much of this comes from really old MU days of these options not existing in a cache.

[boonebgorges]

is our site_options cache necessary anymore (other than for backwards compatibility?)

I'll have to look into this a little bit more. In the case of options fetched on the current site, they are cached persistently, and are also loaded into the cache with a single query ("alloptions") for sites that don't have a persistent cache. Site options don't work like this - they're not prefetched en masse, but are individually cached in the 'site-options' group. So removing this stuff would probably increase by a handful the number of database queries on sites without persistent caching. I'd have to study it a bit more to see what happens on different kinds of setups (like on a non-root blog).

If we could tear this stuff out, I'd be delighted to do so, but I don't want to place too harsh a penalty on sites with no persistent cache backend.

[johnjamesjacoby]

Note that only the first call to get_site_option() for each option results on a DB hit, similarly to what BuddyPress does now, then caches that option on its own via {$wpdb->siteid}:$option so subsequent calls won't hit the DB (even without a persistent object cache.)

I have a hunch the impact would be next to nil.

[boonebgorges]

Note that only the first call to get_site_option() for each option

Yes, *for each option*. We currently fetch all of these with a single DB query. So if we pull up 5 options, that'll be 4 extra queries.

This is different from the way get_option() works. That function loads *all* options (at least autoload=true options) into the cache on the first call.

Anyway, we should test it. I'd be OK with adding a DB hit or two if it let us simplify all of this.

[DJPaul]

Are we done here or do you two want this ticket left open?

[johnjamesjacoby]

Open, but moving to 2.3.

[boonebgorges]

It's worth considering johnjamesjacoby's suggestion that we stop doing a direct query for site options, but it seems to me that the current suggestion of a wrapper is an improvement on how things currently work, so let's discuss the further improvements in a separate ticket.

[boonebgorges]

In 9698:

Introduce bp_core_get_root_option() and use throughout BP.

Previously, we referenced the $bp->site_options array directly. This causes
problems in cases where these options may be referenced before the array is
initially populated. bp_core_get_root_option() will fetch the requested
value from the array if it's been populated, and will populate it if it has not.

Fixes #6045.