Make WordPress Core

Opened 11 years ago

Closed 7 years ago

Last modified 5 years ago

#25344 closed enhancement (maybelater)

Introduce *_network_meta() functions

Reported by: jeremyfelt's profile jeremyfelt Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: Networks and Sites Keywords: reporter-feedback has-patch needs-testing has-unit-tests
Focuses: multisite Cc:

Description

The get_metadata(), update_metadata(), delete_metadata(), and add_metadata() functions are abstracted well enough for posts, comments, and users that adding in wrappers for network meta is pretty straight forward.

The attached patch introduces get_network_meta(), update_network_meta(), delete_network_meta(), and add_network_meta().

A few caveats:

  • The table we're pulling from is sitemeta, which is a bummer. While the functions are named *_network_meta(), we still end up passing site as a parameter to the wrapped function. As it stands, any filters available would still need to use site rather than network. There's a chance we can work around this with some logic on the underlying functions, but I didn't go that far yet.
  • Certain network meta likely has more consequence when removed. It may be worth protecting a few of the meta keys so that they are validated properly and are not deleted. See next point.
  • This may or may not belong as part of an overall Network strategy that includes better structure around what a WP_Network object looks like and requires. That thought probably belongs somewhere else. :)

Attachments (16)

25344.diff (2.7 KB) - added by jeremyfelt 11 years ago.
25344.2.diff (11.3 KB) - added by johnjamesjacoby 8 years ago.
25344.patch (17.7 KB) - added by spacedmonkey 8 years ago.
25344.2.patch (17.3 KB) - added by spacedmonkey 8 years ago.
25344.3.patch (17.1 KB) - added by spacedmonkey 8 years ago.
25344.4.patch (18.8 KB) - added by johnjamesjacoby 8 years ago.
Same as .3, but fully implements update_network_meta_cache into _prime_network_caches() and update_network_cache()
25344.5.patch (20.8 KB) - added by johnjamesjacoby 8 years ago.
25344.6.patch (22.8 KB) - added by johnjamesjacoby 8 years ago.
25344.7.patch (21.4 KB) - added by johnjamesjacoby 8 years ago.
25344.8.patch (21.6 KB) - added by spacedmonkey 8 years ago.
25344.9.patch (31.0 KB) - added by spacedmonkey 8 years ago.
Screen Shot 2017-04-17 at 22.08.07.png (86.7 KB) - added by spacedmonkey 8 years ago.
A table of data, profiling queries, time in db and page load time
25344.10.network-meta.diff (29.3 KB) - added by flixos90 8 years ago.
25344.10.network-options.diff (10.7 KB) - added by flixos90 8 years ago.
25344.10.patch (35.8 KB) - added by spacedmonkey 8 years ago.
25344.10.network-meta-without-query.diff (22.3 KB) - added by flixos90 8 years ago.

Download all attachments as: .zip

Change History (57)

@jeremyfelt
11 years ago

#1 follow-up: @nacin
11 years ago

sitemeta isn't really a meta table for key/value pairs, but is the network equivalent to "options". Indeed, sitemeta is currently handled by get/add/update/delete_site_option(). What's the purpose of these when we already have a more options-like API?

#2 in reply to: ↑ 1 @jeremyfelt
11 years ago

  • Keywords has-patch removed
  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

Replying to nacin:

sitemeta isn't really a meta table for key/value pairs, but is the network equivalent to "options". Indeed, sitemeta is currently handled by get/add/update/delete_site_option(). What's the purpose of these when we already have a more options-like API?

Well, when you put it that way... :) Chalk this one up to losing my mind when wrapping it around historical naming.

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


9 years ago

#4 @johnjamesjacoby
8 years ago

  • Component changed from Multisite to Networks and Sites
  • Focuses multisite added
  • Keywords reporter-feedback added
  • Milestone set to Future Release
  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Version set to 3.0

Related: #38025, #37181.

I think now is a good time to revisit this idea. WP_Network and WP_Network_Query are mature, and the metadata API is easier to understand & interact with than the alloptions cache approach of yore.

Reopening for future multisite discussion.

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


8 years ago

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


8 years ago

#7 @johnjamesjacoby
8 years ago

25344.2.diff does the following:

  • Registers the global site_meta cache group
  • Introduces *_network_meta() functions
  • Replace $wpdb queries with calls to the above functions
  • Remove network level alloptions and nooptions cache implementation 💜

A few things to note:

  • Transients still call the old _network_options() functions to keep filters intact
  • Transients still use the same old cache group
  • The site-options cache group is in use by a bunch of things that aren't actually options, so it remains untouched and unaltered for this patch
  • Unit tests pass for me, but failed the first time I ran them. I'm not sure why, because the restapi test group is causing out-of-memory issues on my MacBook out-of-nowhere, and I can't complete the entire suite. (I can run this on my Pro later and figure it out.)

#8 @spacedmonkey
8 years ago

My patch is very similar and can be seen here
https://core.trac.wordpress.org/attachment/ticket/37181/37181.1.diff

With all options, I just used the update_meta_cache function to prime caches. This will load all options and get rid of the all the all options mess.

We should also add an option in WP_network_query to prime the meta cache, like other meta.

#9 @flixos90
8 years ago

+1

Patch looks good regarding the Meta API. I found only two issues with the network option implementation:

  • In get_network_option(), the call should be get_network_meta( $network_id, $option ) instead of get_network_meta( $network_id, $option, $default ). The resulting array should then be checked for its first value. If that exists, we have a match, otherwise the option is not set and we need to return the default. It would be nice if we could use get_network_meta( $network_id, $option, true ), but that's not possible since that function returns an empty string if no value is found which does not allow us to differentiate between "empty value set" vs "no value found". See @spacedmonkey's patch on #37181. We can also move the $default_site_option_{$option} filter below that to only run it if we actually need to return the default.
  • In add_network_option() let's use add_network_meta( $network_id, $option, $value, true ) to make sure these values are unique.

I also agree with @spacedmonkey that we'll also need a meta query implementation for WP_Network_Query including _prime_network_caches() in order to bring this into Core. See https://core.trac.wordpress.org/attachment/ticket/37923/37923.diff, which includes that functionality for blogmeta.

@spacedmonkey
8 years ago

#10 @spacedmonkey
8 years ago

25344.3.patch Adds a number of fixes and changes

  • Added update_networkmeta_cache function.
  • _prime_network_caches now uses update_networkmeta_cache
  • WP_Network_Query uses update_networkmeta_cache
  • wp_load_core_site_options logic replaced with update_networkmeta_cache. This means that all meta (options) are loaded on page load. This now have some performance effects..
  • get_network_option - default option fix. Meta api doesnt support defaults.
  • Added cache group to tests.

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


8 years ago

@johnjamesjacoby
8 years ago

Same as .3, but fully implements update_network_meta_cache into _prime_network_caches() and update_network_cache()

#12 @johnjamesjacoby
8 years ago

25344.5.patch is the same as .4, but...

  • ms-functions.php needs to be included earlier, because the new _network_meta() functions live there
  • meta.php needs to be included earlier, so get_network_option() can get the site_name data from ms-settings.php
  • formatting.php needs to included earlier, because the meta-data API uses sanitize_key()

This looks scarier than it actually is.

  • None of the functions in these (or the surrounding) files are pluggable
  • The include-order in wp-settings.php is a mine-field anyways
  • It's a Christmas miracle we haven't had any problems including ms-blogs.php before ms-functions.php
  • Since meta is a thing that primary objects interact with, I'm OK making it available sooner, and if this turns out to be a mistake for some reason, there's no way for plugins or themes to get in this early anyways

#13 @johnjamesjacoby
8 years ago

25344.6.patch is the same as .5, but...

  • Visiting the Network Plugins screen will cause the recently_activated meta key to be re-added 1 time for every refresh of the page
  • Broken logic in the plugins list table class causes the above bug, and using the meta-data API (instead of relying on the alloptions cache) exposes it
  • Fixing the above broken logic is easy, and maybe should be its own separate ticket, but it's pretty critical that get fixed along with this, so I've lumped it in, which I think is safe to do since it's documented accordingly here and in the code

#14 @johnjamesjacoby
8 years ago

#35773 was marked as a duplicate.

#15 @johnjamesjacoby
8 years ago

  • Keywords has-patch needs-testing added

See #38203 for a fix to the meta API.

The patch on that ticket prevents a regression when trying to save something to wp_sitemeta with a negative site_id.

Currently, that is allowed _and_ I do that for versioning all of my custom global database tables, so there are for sure cases of this in the wild blocking using the meta-data API for this.

#16 @johnjamesjacoby
8 years ago

The get_network_option - default option fix from the .3 patch is incorrect. It needs to strictly check against false or '' to avoid cache-misses with values that might be serialized arrays.

Patch imminent, that removes the plugins list-table bit from .6

#17 @spacedmonkey
8 years ago

Updated patch with a better solution for getting network meta.

See my solution.

        $meta = get_network_meta( $network_id, $option ); 
         if ( is_array( $meta ) && ! empty( $meta ) ) {  
                $value = array_shift( $meta );  
         } else { 
                $value = $default; 
         } 

The meta api current supports storing multiple rows in one key. However network options do not support it. There could be issues with using the meta functions do write multiple rows then never being able to get the out the db. This work around allows for all the items to be received but the first is used.

#18 @spacedmonkey
8 years ago

#37181 was marked as a duplicate.

#19 @flixos90
8 years ago

  • Keywords needs-unit-tests added

Some observations on 25344.8.patch:

  • Indentation of argument documentation and defaults array in WP_Network_Query needs to be adjusted.
  • I think we should leave out the adjustments in wp-includes/meta.php since they are part of #38203.
  • Should the *_network_meta() functions live in wp-includes/ms-functions.php or wp-includes/ms-blogs.php?
  • Indentation issues in get_network_option() implementation.
  • In add_network_option() the function add_network_meta() should be called with the $unique parameter set to true.
  • Was class-wp-list-table.php moved in wp-settings.php accidentally? It should remain before functions.php.

Further thoughts:

  • I think we should split the patch into two parts, one with the actual API and the other with the implementation within the network option API.
  • At some point we'll need unit tests as well. Those can pretty much be copy/paste (with a few name adjustments obviously) from https://core.trac.wordpress.org/attachment/ticket/37923/37923.diff.

#20 @spacedmonkey
8 years ago

  • Keywords needs-unit-tests removed

I did a little work on profiling this patch. The biggest change to performance is the function wp_load_core_site_options.
Instead of loading on a 9 of core options, it now loads all of network options on page load. On average the standard wordpress install has 31 rows in the network meta table. However, loading all the network options, doesn't mean now queries, it just means one query that gets more rows. Loading all network meta might be a problem if there are plugins that add lots of network options.

If object caching is enabled, there is a possible effect on performance. Before only the core keys were loaded on page load. Now, that all the keys are loaded, it means that the admin terminal is faster and there are less SQL queries.

There is no right answer here, I am just saying this change is going to have an effect, for some positive and for some negative.

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


8 years ago

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


8 years ago

#23 @spacedmonkey
8 years ago

  • Keywords has-unit-tests added

Notable changed in 25344.9.patch

  • Add delete_network_meta_by_key function
  • Use get_current_network_id in wp_load_core_site_options. Removed use of global $wpdb;
  • Updated docs for network_options to add changed in this patch.
  • clean_network_cache now deletes site meta
  • update_network_option now used $old_value param in update_network_meta
  • Added limited tests. Props @flixos90 as I copied his ones from wp_blogmeta patch.
  • Fixed Indentation issues

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


8 years ago

@spacedmonkey
8 years ago

A table of data, profiling queries, time in db and page load time

#25 @spacedmonkey
8 years ago

Reviewing the data for my profiling, I can conclude, the effects on performance are positive. The greatest performance wins come from installs without object caching.

More profilng could be done here. My profile was just on vvv using query monitor. We could do with more profiling with plugins enabled etc, different pages on the front end.

#26 @flixos90
8 years ago

I uploaded a new set of patches - set as I have splitted the network meta API introduction (see 25344.10.network-meta.diff) from its implementation in the *_network_option() functions (see 25344.10.network-options.diff), to have a better overview and to be able to later commit them separately (that does not imply I'm wary of any of the two parts). Be aware that the network options patch relies on the network meta patch in order to work.

Some changes I made in 25344.10.network-meta.diff:

  • Remove the fix in src/wp-includes/meta.php as that does not belong into this patch, but into a patch on #38203.
  • Bust network query cache when network meta is added/updated/deleted.
  • Introduce meta query functionality for WP_Network_Query. Meta query functionality is only available if the class has already been loaded; that means that no meta queries are supported in sunrise.php at the moment. We can revisit after figuring out loading order of files (see below).
  • Move the *_network_meta() functions into src/wp-includes/ms-blogs.php. The names of the two files (ms-blogs.php and ms-functions.php) are really confusing, but all basic site and network functionality resides in ms-blogs.php for now, and questioning that belongs into another ticket IMO (although it's probably worth doing that at some point). This also makes the patch more clear and reduces chances for unexpected bugs, as we don't need to change the loading order of files. It's certainly sub-optimal that meta.php is loaded after some other stuff that relies on it, but this happens in a broader scope - again, that belongs into another ticket I think. I don't like to clutter this patch here with such things, so maybe we should open a ticket to rethink the multisite file structure in general, and then adjust this patch after we have figured that out.
  • Add tests for network meta functionality (@spacedmonkey, looks like you forgot to do svn add :) ).

I didn't change anything in 25344.10.network-options.diff. However, that patch currently does not work because of the loading order that we need to discuss (so here it is critical, see above). Functionality-wise, I don't see anything we need to change in the patch. Once we get it back to work, we of course need to verify through unit tests.

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


8 years ago

#28 follow-up: @spacedmonkey
8 years ago

I resubmitted my patch with tests. I did forget to add the file into my patch. I agree we should break this into smaller patches so make it easier to merge. I think we should keep on talk of a meta query in #38025 as adding a meta query is really unrelated to implementing the meta api.

#29 in reply to: ↑ 28 @flixos90
8 years ago

Replying to spacedmonkey:

I resubmitted my patch with tests. I did forget to add the file into my patch.

Okay, the tests look very similar to the one in 25344.10.network-meta.diff (as one may expect).

I think we should keep on talk of a meta query in #38025 as adding a meta query is really unrelated to implementing the meta api.

Good point. I'll split that functionality out as well later today. Since it's all related, I'll leave the patches on this ticket, although we can later commit the meta query stuff in regard of #38025.

#30 @flixos90
8 years ago

25344.10.network-meta-without-query.diff is exactly the same as 25344.10.network-meta.diff, however without the meta query functionality. This allows us to get a better overview about the most basic part of what we would need. The meta query implementation could even be committed separately from the main API functions.

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


8 years ago

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


8 years ago

#33 @flixos90
8 years ago

  • Keywords ms-roadmap added

These tickets belong to our planned roadmap (a few of them not per final decision), so flagging with a keyword for better overview.

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


8 years ago

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


8 years ago

#36 @spacedmonkey
7 years ago

#35773 was marked as a duplicate.

#37 @spacedmonkey
7 years ago

  • Keywords changed from reporter-feedback, has-patch, needs-testing, has-unit-tests, ms-roadmap to reporter-feedback has-patch needs-testing has-unit-tests ms-roadmap

After working on #37181 ticket for a while, I have spotted an issue with adding these functions. sanitize data. Currently the network options allow for sanitize data to be used in them. This could be a object you want to cached. But the meta api out of the box doesn't handle this. In #37181 I handle this inside the *_network_option functions, for compatiblity. But this may mean we may never to able to put these functions in core.

#38 @flixos90
7 years ago

  • Keywords ms-roadmap removed
  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from reopened to closed

As discussed at WordCamp US, we will not introduce *_network_meta() functions at this point, this should remain plugin territory. Our network meta efforts in core should be limited to using it internally in the *_network_option() functions (see #37181).

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


7 years ago

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


7 years ago

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


5 years ago

Note: See TracTickets for help on using tickets.