WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 7 weeks ago

#37181 reopened enhancement

Use metadata api in *_network_options

Reported by: spacedmonkey Owned by:
Milestone: 5.0 Priority: normal
Severity: normal Version: 4.4
Component: Networks and Sites Keywords: has-patch ms-roadmap dev-feedback has-unit-tests
Focuses: multisite Cc:

Description

The network (site) options are stored in the database as sitemeta. The table is formatted as a meta table. However the CRUD of this data in get_network_option, update_network_option, add_network_option and delete_network_option doesn't use the metadata api. Using the metadata api has many advantages, such as filters and a more consistent caching api.

Attachments (11)

37181.diff (9.9 KB) - added by spacedmonkey 2 years ago.
37181.1.diff (9.9 KB) - added by spacedmonkey 2 years ago.
37181.2.diff (14.0 KB) - added by spacedmonkey 11 months ago.
37181.3.diff (14.2 KB) - added by spacedmonkey 11 months ago.
37181.4.diff (18.1 KB) - added by spacedmonkey 11 months ago.
37181.5.diff (17.9 KB) - added by spacedmonkey 10 months ago.
37181.6.diff (23.8 KB) - added by spacedmonkey 10 months ago.
37181.7.diff (23.6 KB) - added by spacedmonkey 7 months ago.
37181.8.diff (23.6 KB) - added by jeremyfelt 6 months ago.
37181.9.diff (20.1 KB) - added by spacedmonkey 3 months ago.
37181.10.diff (22.2 KB) - added by spacedmonkey 3 months ago.

Download all attachments as: .zip

Change History (62)

@spacedmonkey
2 years ago

@spacedmonkey
2 years ago

#1 @swissspidy
2 years ago

  • Keywords has-patch needs-unit-tests added

#2 @swissspidy
2 years ago

Note that the default_site_option_ filter needs to be kept in order to not break BC. This includes the documentation for this hook.

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by spacedmonkey. View the logs.


2 years ago

#5 @sc0ttkclark
2 years ago

This could utilize the stuff in WP 4.6 for register_meta() now too:

register_meta( 'site', 'my_site_option_name', array( … ) );

For testing, you'd also need to add 'site' to the array in the new wp_object_types filter. However, in the patch, you'd add 'site' in the wp_object_types array within wp_object_type_exists() in the wp-includes/functions.php file.

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


22 months ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


17 months ago

This ticket was mentioned in Slack in #core-multisite by richardtape. View the logs.


17 months ago

#12 @spacedmonkey
17 months ago

I have been looking at this ticket again. There seems to be existing unit tests for the *_network_option functions. I believe me and @jeremyfelt worked on those original tests. The tests seem good to me. We will need some more test, but I would like some advise on what those tests look like.

Related: #28290

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


15 months ago

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


15 months ago

#15 @flixos90
15 months ago

Briefly summarizing today's office-hours proposed goals:

  • Introduce *_network_meta() functions using the Metadata API for direct interaction with the sitemeta table.
  • Add support for meta_query in WP_Network_Query.
  • Use the new functions in the *_network_option() functions. This gives performance benefits and allows us getting rid of the alloptions mess. The *_network_option() functions will continue to maintain their current hooks for BC, while the *_network_meta() functions will be clean as other object metadata functions.
  • Encourage using the new functions and use *_network_meta() in Core going forward. The *_network_option() functions will stay around for BC though, and should not be deprecated.
  • This will also benefit a future /networks REST API endpoint.

See #37923 for related plans of introducing a blogmeta table and surrounding *_site_meta() functions.

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


15 months ago

#17 follow-up: @johnjamesjacoby
15 months ago

I'd like to make a motion to close this ticket in favor of recent activity on #25344.

I believe doing so will concentrate our efforts and be a bit simpler for everyone to pay attention to.

#18 in reply to: ↑ 17 @flixos90
15 months ago

  • Keywords close added

Replying to johnjamesjacoby:

I'd like to make a motion to close this ticket in favor of recent activity on #25344.

I believe doing so will concentrate our efforts and be a bit simpler for everyone to pay attention to.

+1

#19 @spacedmonkey
15 months ago

  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #25344.

#20 @swissspidy
15 months ago

  • Milestone Awaiting Review deleted

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


11 months ago

#22 @spacedmonkey
11 months ago

  • Keywords ms-roadmap added; close removed
  • Resolution duplicate deleted
  • Status changed from closed to reopened

#23 @spacedmonkey
11 months ago

  • Milestone set to Future Release

#24 @spacedmonkey
11 months ago

In 37181.2.diff are the following changes

  • wp_load_core_site_options now uses get_current_network_id to bring into line with _network_option functions.
  • serialize functions returned to _network_option functions which was stopping tests from passing.
  • delete_network_option uses delete_metadata and passes 4 param as an empty string, as per meta api.
  • meta.php and formatting.php moved to earlier in bootstrap process because network options loaded ealier in bootstrap.
  • Test added to prove that first value in meta store is honored.
  • New update_network_meta_cache param added to wp_network_query. Network options are primed in cache, like other query classes.
  • Removed test_site_notoptions test, as no longer valid.
  • Documentation. Poke @DrewAPicture for feedback.

#25 @spacedmonkey
11 months ago

  • Keywords dev-feedback has-unit-tests added; needs-unit-tests removed

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


11 months ago

#27 @spacedmonkey
11 months ago

In 37181.4.diff

  • Introduce the cache group of site_meta as a global group
  • Include site_meta in clean_network_cache

#28 @spacedmonkey
10 months ago

37181.6.diff

Fixes some styling issues with the comments and the code. It now is WP code standards. Also there has been changed to variables since original patch.

One thing to note about this change how the meta api handles caching. The meta api loads all the meta for an object into one cache key. This means, when you request one option, it then loads all options in memory / object cache. Personally, I think this is a good thing, you are effectively autoloading all options, similar to how options works in core. But it may have effects on performance in different use cases. In WP_Network_Query, the network option (Meta) cache is primed. It is reasonable to guess, that if you are query networks that you will likely want the meta as well. As Network query is used in the bootstrap, the network option will be cache will be primed much easier.

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


10 months ago

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


10 months ago

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


8 months ago

#34 @spacedmonkey
7 months ago

  • Milestone changed from Future Release to 5.0

#35 @spacedmonkey
7 months ago

In the last patch, is a small performance improvement.

Unsetting the following update_network_cache update_network_meta_cache

Means cache key is unchanged on these query variable

#36 @spacedmonkey
7 months ago

  • Component changed from Options, Meta APIs to Networks and Sites

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


7 months ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


7 months ago

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


6 months ago

@jeremyfelt
6 months ago

#41 follow-up: @jeremyfelt
6 months ago

Thanks for your work on this, @spacedmonkey.

37181.8.diff is updated against current trunk to account for the changes in [42343].

A couple notes so far:

  • We're switching from site-options to site_meta as the cache group, but still using site-options for a handful of other things—current_network, networks_have_paths, etc... This is probably okay because we weren't ever storing those "options" in the DB.
  • We'll want to make sure once this is in that external object cache drop-ins are aware/updated with the new global cache group.
  • In general, I think our network option tests could use some work. For something like this we should write some more tests that rely on an external object cache.
  • Is it possible that anyone is relying on notoptions right now for some reason? May need to do some digging through plugins briefly to see if anything stands out.

I'll spend some more time with this and think through the testing a bit more.

#42 @jeremyfelt
6 months ago

One more - it'd also be helpful to know if anyone is relying on the site-options cache group for one of these values in their sunrise.php or elsewhere with a direct call to cache.

#43 in reply to: ↑ 41 @spacedmonkey
6 months ago

Replying to jeremyfelt:

  • We're switching from site-options to site_meta as the cache group, but still using site-options for a handful of other things—current_network, networks_have_paths, etc... This is probably okay because we weren't ever storing those "options" in the DB.

I think we need to support the site-options cache group forever more. It is used else where in core and I am personally use it for some plugin. I see no harm in leaving it.

  • We'll want to make sure once this is in that external object cache drop-ins are aware/updated with the new global cache group.

Most modern object cache drops implement the wp_cache_add_global_groups, which should do this for us. Examples, can found, here, here, here, here and here. These are the largest plugins that I know of. Older ones may not have it implement it, but that will break other things and is not a core problem.

  • In general, I think our network option tests could use some work. For something like this we should write some more tests that rely on an external object cache.

Any ideas of this @jeremyfelt ?

  • Is it possible that anyone is relying on notoptions right now for some reason? May need to do some digging through plugins briefly to see if anything stands out.

A quick look of github, shows all the refrences to it, are people checking core into GH. I havent found a plugin in the wild that uses

Last edited 6 months ago by spacedmonkey (previous) (diff)

#44 @spacedmonkey
6 months ago

Some things to not

  • All options will be loaded in one cache key. This may effect people with lots of network options, however, not many people use network options, it is unlike to be a problem.
  • Even through all options are loaded at once, it is not effected by the alloptions issue, detailed in #31245 . This because the meta api, does a delete instead of update. See here
  • Network options will now be primed in WP_Network_Query, meaning network options will be loaded

in the bootstrap, which I think is is a good thing.

  • In my patch, I run maybe_serialize before passing to the meta api. Howeve the meta api also serialize. I dont think there is a problem with double serializing something?
  • Many of the existing filters could be moved out of the filters / actions in these could be moved out to there own functions. Consider the following example.
    add_filter('get_site_metadata', function($value, $network_id, $option){
       $pre = apply_filters( "pre_site_option_{$option}", false, $option, $network_id );
       if ( false !== $pre ) {
            $value = $pre;
       }
       return $value;
    }, 5, 3);
    
    

This allows for backward compat, but also if to remove those filters if you don't wish to use them anymore. This may over complex things.

  • clean_network_cache now clears network options, which I think it should be doing already. This means that the delete_network function in wp-multi-network, it work better. ( Good work @johnjamesjacoby on that one ).

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


4 months ago

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


3 months ago

#48 @spacedmonkey
3 months ago

In 37181.10.diff I updated the patch after coding standard changes.

There are now, 3 tests that pass. If you could take a look, @jeremyfelt that would be great :D

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


2 months ago

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


2 months ago

#51 @spacedmonkey
7 weeks ago

As part of the slack chat on this ticket, I have added a related ticket #43941 for meta data default values.

Note: See TracTickets for help on using tickets.