BP options get/update wrapper functions

[boonebgorges]

See #3219.

Just like we had with #2952, in order for multiblog and multinetwork to work correctly, we'll need to be able to filter where global settings are stored.

wpmuguru has suggested bp_get_option() and bp_update_option type wrappers, with the logic set internally. ​https://buddypress.trac.wordpress.org/ticket/3219#comment:10 I wish there were a way to do this that was more in line with what we did with usermeta #2952, where only the meta keys are filtered. But because on multisite the default behavior is to store things in site options, that's not possible.

To be honest, I don't really like that BP stores settings using update_site_option(). I don't see what benefit we get from having this additional logic. Unfortunately, we probably have to keep it that way for reasons of backward compatibility.

jjj and DJPaul, what's your view on introducing bp_get_option() and bp_update_option() wrappers?

[wpmuguru]

I like the filter idea.

[DJPaul]

I thought you? moved alot of the settings into site_options to make certain types of upgrade and setting storage easier.

Would these new options be required for BP_ENABLE_MULTIBLOG or the new multinetwork? If the former, does this mean BP 1.2.8 has a incomplete implementation, or has another change broken it?

[wpmuguru]

Replying to DJPaul:

I thought you? moved alot of the settings into site_options to make certain types of upgrade and setting storage easier.

Would these new options be required for BP_ENABLE_MULTIBLOG or the new multinetwork? If the former, does this mean BP 1.2.8 has a incomplete implementation, or has another change broken it?

For BP_ENABLE_MULTIBLOG because the page Ids for each BP page is currently stored in the site options. Those page IDs will be different on different sites.

[boonebgorges]

bp-pages data is stored differently from other site options. It should be in a sitemeta called bp-pages, which is a multi-d array that should look something like:
[2] => array( 'members' => 56, 'groups' => 58 ),
[3] => array( 'members' => 6, 'groups' => 8 ),
etc, where 2 and 3 are blog ids. If this is not working properly, it's a bug in my implementation.

Other settings are stored in regular sitemeta or options tables.

This should be regularized, so that all settings are done the same way. If we want to filter keys only, we'll have to move all settings out of sitemeta. This will break backward compatibility, but only in cases where people were accessing BP sitewide settings. This should *not* happen very frequently, because they are loaded into the $bp global at loadtime. In the name of consistency with usermeta, it's probably worth examining this course further, which I will do when I get back from vacation, unless someone beats me to it.

[boonebgorges]

PS I'm talking here about trunk. It's possible (probable?) that stuff on the Settings panel is not being differentiated on BP_ENABLE_MULTIBLOG for 1.2 branch. But I'd argue that on BP_ENABLE_MULTIBLOG it might make sense for settings like those to be sitewide. Obviously, this is not the case for bp-pages, or for multinetwork setups.

[wpmuguru]

Replying to boonebgorges:

PS I'm talking here about trunk. It's possible (probable?) that stuff on the Settings panel is not being differentiated on BP_ENABLE_MULTIBLOG for 1.2 branch.

Yeah, it's not differentiating. What you would need to accomplish that is get_site_option, update the [blog_id][component] element(s) and update_site_option.

[DJPaul]

What about wrapping the existing site meta that we use in another array, keyed by network ID?

[boonebgorges]

I'd like our approach to be consistent with different kinds of metadata/options. So if we are going to use a wrapper function for user_meta keys (which we are currently doing in the trunk with bp_get_user_meta_key()), then we should use a similar strategy for options, eg

update_site_option( bp_get_site_option_key( 'bp-active-components' ), $active_components );

The problem with this kind of approach is that it would (probably) require prefixing the key with the site id, eg bp_3-active-components or something like that. And this would mean that we were storing a bunch of clearly site-specific content in sitemeta. That seems wrong - that's the purpose of update_option(). Yet moving to update_option() means that we have to figure out something for backpat.

If the conceptual mismatch of storing blog-specific meta in sitemeta using update_site_option() doesn't bother anyone, then we can just go along with this solution. (Ron can then filter this in the same way he does bp_get_user_meta_key() for multinetwork, and we can filter it internally when BP_ENABLE_MULTIBLOG is set.) This is something that can be done quickly and with full backpat. Thoughts/objections?

[wpmuguru]

Since the pages are the only issue at present and there isn't a backward compatibility issue a simple solution would be to store the pages in the options table instead of sitemeta. Then the admin screen could have a bit of logic to only show the pages fields on subsites.

[boonebgorges]

What about the other site options? (Activated components, plus the stuff on the BuddyPress > Settings panel)

[wpmuguru]

Replying to boonebgorges:

What about the other site options? (Activated components, plus the stuff on the BuddyPress > Settings panel)

I think it would create lots of forum threads if those could be turned on and off on a per site basis. Currently those settings are network wide when BP_ENABLE_MULTIBLOG is set. For example, every time a new site is created the network settings currently apply. Changing it to per site settings would mean the new site has default settings.

If someone wants to do per blog/site it can easily be done with pre_site_option_ filters using similar logic to what I used in the bp_multi-network plugin.

[boonebgorges]

Replying to wpmuguru:

I think it would create lots of forum threads if those could be turned on and off on a per site basis. Currently those settings are network wide when BP_ENABLE_MULTIBLOG is set.

Right. This seems like it's probably the correct behavior for BP_ENABLE_MULTIBLOG. But it's definitely not the right behavior for multinetwork. I guess I was thinking that it would make sense to be consistent across data types, but since pre_site_option_ is available, it's probably better not to overengineer.

If the only thing that needs to be site-specific is bp-pages, then this is already done. bp-pages data is stored in a site_option, which is an array keyed by blog_id. You'll need to apply the patch 3219.2.patch from #3219 to see how it works (that patch puts BP options on the blog Dashboard, rather than Network Admin, when BP_ENABLE_MULTIBLOG is enabled, so that you get the 'you have components with no WP pages' admin notice on secondary blogs). That means that, once #3219 is fixed, and the bp-multi-network patch has filtered 'bp_core_do_network_admin' to return false, then the bp-pages part should just work automatically.

If those two things are true (that bp-pages works on a per-blog basis when #3219 is fixed, and that pre_site_option_ is an acceptable way to filter other site options if desired), then we can probably close this ticket as rejected.

[wpmuguru]

Replying to boonebgorges:

Replying to wpmuguru:

I think it would create lots of forum threads if those could be turned on and off on a per site basis. Currently those settings are network wide when BP_ENABLE_MULTIBLOG is set.

Right. This seems like it's probably the correct behavior for BP_ENABLE_MULTIBLOG. But it's definitely not the right behavior for multinetwork. I guess I was thinking that it would make sense to be consistent across data types, but since pre_site_option_ is available, it's probably better not to overengineer.

Site option/meta is already separated by network using the site_id field.

[boonebgorges]

In this week's dev chat we decided on the following strategy:

• All settings (bp-pages and otherwise) will be moved to _options rather than sitemeta
• Introduce bp_get_option() and bp_update_option(), which will be wrappers for get_blog_option( $blog_id, 'option' ) where $blog_id defaults to BP_ROOT_BLOG on standard installs. (For non-MS installs, we define get_blog_option() in bp-core-wpabstraction.php. update_blog_option() will have to be added to the abstraction file)

working on a patch right now.

[boonebgorges]

Whew, this one was a doozy. Backpat, the installation wizard, and BP_ENABLE_MULTIBLOG meant that there was a lot to account for. A few notes:

• The hard work of bp_get_option() and bp_update_option() is done by a new function bp_get_option_blog_id(). It takes the option name as an argument, and checks that argument against a filterable list of options that should be stored site-specifically. Essentially, this function always returns BP_ROOT_BLOG, except in the following scenario: BP_ENABLE_MULTIBLOG is true and the option is 'bp-pages'. The logic here: on _MULTIBLOG, all settings should be the same, except for bp-pages, which must necessarily be different from blog to blog. A multinetwork BP plugin will need to filter this value, so that other BP options are stored in a site-specific way. I'd like to know what you think about this approach.

• I combed through the codebase and found all the places (I think) where get_blog_option or get_site_option etc needed to be changed to the new bp_x_option() functions. Some things are left in sitemeta (like bp-version, and certain WP things like registration settings).

• The patch should automatically detect the old structure of your data and port it over. Let me know if that doesn't happen smoothly.

Feedback greatly appreciated on 3261.patch

[wpmuguru]

For reference [4482].

Is there a reason you left the bp-blogs-first-install as a site option L1130 of bp-core-update.php?

[DJPaul]

Just a heads-up that when #3153 goes in, there may be a couple more references to update.

[boonebgorges]

Is there a reason you left the bp-blogs-first-install as a site option L1130 of bp-core-update.php?

No, it was an oversight. I'm fixing it in my updated patch.

In an earlier post, I said:

A multinetwork BP plugin will need to filter this value, so that other BP options are stored in a site-specific way.

I realize now that this is incorrect. bp_get_option_blog_id() will fall back on BP_ROOT_BLOG in most cases, which should be inherited from filters already applied elsewhere in the multinetwork setup. That's a good thing, as it means that bp_get_option() and bp_update_option will work totally seamlessly.

See 3261.2.patch

[wpmuguru]

I didn't test, but that looks good.

[boonebgorges]

(In [4559]) Introduces bp_x_option() wrapper functions, along with bp_get_option_blog_id() to allow for fully filterable option storage. Refactors internal option storage to use new API functions. Fixes bp-pages for BP_ENABLE_MULTIBLOG. Fixes #3261