Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#6045 closed enhancement (fixed)

$bp->site_options should not be referenced directly

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 2.3 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch
Cc:

Description

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.

Attachments (1)

6045.patch (10.7 KB) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (13)

#1 @boonebgorges
5 years ago

In 9219:

Add tests for bp_blog_signup_enabled().

See #6045.

#2 @boonebgorges
5 years ago

In 9220:

An attempt to appease PHP 5.2 regarding string offsets.

As introduced in [9219].

See #6045.

@boonebgorges
5 years ago

#3 @boonebgorges
5 years ago

  • Keywords has-patch added; good-first-bug needs-patch removed

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

#4 @johnjamesjacoby
5 years ago

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.

Version 0, edited 5 years ago by johnjamesjacoby (next)

#5 @boonebgorges
5 years ago

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.

#6 @johnjamesjacoby
5 years ago

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.

#7 @boonebgorges
5 years ago

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.

#8 @DJPaul
5 years ago

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

#9 @johnjamesjacoby
5 years ago

  • Milestone changed from 2.2 to 2.3
  • Type changed from defect (bug) to task

Open, but moving to 2.3.

#10 @boonebgorges
5 years ago

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.

#11 @boonebgorges
5 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

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.

#12 @DJPaul
3 years ago

  • Type changed from task to enhancement
Note: See TracTickets for help on using tickets.