#25344 closed enhancement (maybelater)
Introduce *_network_meta() functions
Reported by: |
|
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 passingsite
as a parameter to the wrapped function. As it stands, any filters available would still need to usesite
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)
Change History (57)
#2
in reply to:
↑ 1
@
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
@
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
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
@
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
andnooptions
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
@
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
@
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 beget_network_meta( $network_id, $option )
instead ofget_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 useget_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 useadd_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
.
#10
@
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
@
8 years ago
Same as .3, but fully implements update_network_meta_cache
into _prime_network_caches()
and update_network_cache()
#12
@
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 theremeta.php
needs to be included earlier, soget_network_option()
can get thesite_name
data fromms-settings.php
formatting.php
needs to included earlier, because the meta-data API usessanitize_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
beforems-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
@
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
#15
@
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
@
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
@
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.
#19
@
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 inwp-includes/ms-functions.php
orwp-includes/ms-blogs.php
? - Indentation issues in
get_network_option()
implementation. - In
add_network_option()
the functionadd_network_meta()
should be called with the$unique
parameter set to true. - Was
class-wp-list-table.php
moved inwp-settings.php
accidentally? It should remain beforefunctions.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
@
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
@
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
#25
@
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
@
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 insunrise.php
at the moment. We can revisit after figuring out loading order of files (see below). - Move the
*_network_meta()
functions intosrc/wp-includes/ms-blogs.php
. The names of the two files (ms-blogs.php
andms-functions.php
) are really confusing, but all basic site and network functionality resides inms-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 thatmeta.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:
↓ 29
@
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
@
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
@
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
@
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
#37
@
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
@
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).
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?