Opened 10 years ago
Closed 9 years ago
#28290 closed task (blessed) (fixed)
Changing _site_option functions for _network_option functions
Reported by: | anonymized_10765487 | Owned by: | jeremyfelt |
---|---|---|---|
Milestone: | 4.4 | Priority: | normal |
Severity: | normal | Version: | 4.0 |
Component: | Options, Meta APIs | Keywords: | has-patch dev-feedback |
Focuses: | multisite | Cc: |
Description
Hi,
I'm pretty sure this has been already discussed but functions such as get_site_option()
, update_site_option()
, add_site_option()
have very confusing names.
Why not change that for get_network_option()
, update_network_option()
, add_network_option()
so there could be no doubt this options are network-wide options and do not belong to the option table?
Attachments (7)
Change History (62)
#2
in reply to:
↑ 1
@
10 years ago
Replying to jdgrimes:
So yes, let's change these to
*_network_option()
, but let's not just do it to change the names. Find everything wrong with the old functions that we couldn't fix without breaking back-compat, and let's fix it in the new functions.
While I can understand this need of backward compat this is not the philosophy of WP :
Decisions, not Options
When making decisions these are the users we consider first. A great example of this consideration is software options. Every time you give a user an option, you are asking them to make a decision. When a user doesn't care or understand the option this ultimately leads to frustration. As developers we sometimes feel that providing options for everything is a good thing, you can never have too many choices, right? Ultimately these choices end up being technical ones, choices that the average end user has no interest in. It's our duty as developers to make smart design decisions and avoid putting the weight of technical choices on our end users.
[Source](https://wordpress.org/about/philosophy/)
In addition to this, the latest version of WP should always be the best version, it often fix bugs and improves security. I doubt core committers think about backward compat first and I agree with them.
#3
follow-up:
↓ 4
@
10 years ago
The "Decisions, not Options" paragraph is about UI options, not underlying core functions.
I agree with jdgrimes. We could probably add aliases for these functions, but better naming is not a strong reason to deprecate them (and it would not help much in terms of consistency).
See #24164 for a similar discussion.
#4
in reply to:
↑ 3
@
10 years ago
Replying to SergeyBiryukov:
The "Decisions, not Options" paragraph is about UI options, not underlying core functions.
I agree with jdgrimes. We could probably add aliases for these functions, but better naming is not a strong reason to deprecate them (and it would not help much in terms of consistency).
See #24164 for a similar discussion.
Hi yeah great if you add aliases. To me it's part of that, I mean developers are users too. No problem with the fact I could be totally wrong, would not be the first/last time. Still find the "site" very confusing when it's about network-wide functions but you know I don't make core.
#5
@
10 years ago
To add more confusion, the _site_option()
functions are pulling data from the wp_sitemeta
table. If we were to add an alias at some point, it may make sense as _network_meta()
instead, or not.
The annoyance of the naming is probably worth dealing with for the near future until there is a larger reason for the change.
#6
@
10 years ago
I'd probably still go with _network_option(), but table names are a good point.
Since it hasn't been linked from here yet, see "On API and Naming" in http://make.wordpress.org/core/2013/10/06/potential-roadmap-for-multisite/.
#7
@
10 years ago
- Milestone Awaiting Review deleted
- Resolution set to maybelater
- Status changed from new to closed
#9
@
9 years ago
- Keywords needs-patch added
- Milestone set to 4.4
- Resolution maybelater deleted
- Status changed from closed to reopened
#10
@
9 years ago
I would be interested in writing the patch for this. Couple of questions then.
- Should the patch leverage and wrap, the functions I changed in #33814 ?
- Do I also need to write unit tests for these new functions?
Now for my ideas on what should be in this patch.
Was thinking about the ordering of the params and think that network id should be last. It makes the functions much more usable, as most users of multisite only have one network. Here are a list of the functions and parms.
get_network_option( $option, $default = false, $network_id = false )
add_network_option( $option, $value, $network_id = false )
update_network_option( $option, $value, $network_id = false )
delete_network_option( $option, $network_id = false )
We should also add get_current_network_id() function. It can be used inside above functions if network_id is false / 0. It is also confusing why this function doesn't already exists as get_current_blog_id exists.
#11
@
9 years ago
Having a look into this, I am starting think that we are looking at this all wrong. Here is what I think should be done. Create the following functions that leverage the metadata api.
add_network_meta
get_network_meta
set_network_meta
delete_network_meta
This would work as standard meta functions, giving the nice standard filter and action the metadata api gives.
Then create wrapper functions, as above.
get_network_option( $option, $default = false, $network_id = false )
add_network_option( $option, $value, $network_id = false )
update_network_option( $option, $value, $network_id = false )
delete_network_option( $option, $network_id = false )
These would serve 3 purposes.
- Add in filters / action found in the *_site_option functions, for backwards compatibility
- Do check to see if is_multisite is true and return options instead
- Be context aware and know what network you are on, so network is not a required field.
After a version or two, I would deprecated the *_site_option functions and point the internals of those functions to *_network_option.
This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.
9 years ago
#13
@
9 years ago
Thanks for the thoughts @spacedmonkey. I went down the *_network_meta()
route almost 2 years to the day in #25344. :)
There's too much different between what happens for site options now and what happens in standard meta handling. As you've mentioned, we would inherit a new way of dealing with the data, which would be a big change, and we would need to continue accounting for the current filters and logic. Let's stick with _network_option()
.
Parameter order could probably go as such:
get_network_option( $option, $network_id = false, $default = false, $use_cache = true ) add_network_option( $option, $value, $network_id = false ) update_network_option( $option, $value, $network_id = false ) delete_network_option( $option, $network_id = false )
get_site_option()
can use get_current_site()
, as can get_network_option()
by default. I agree that we should be able to deprecate get_site_option()
, maybe even in 4.5.
Any unit tests along the way would be excellent. The tests for *_site_option()
should continue passing and from a brief glance, it looks like there's plenty we can do to improve those.
#14
follow-up:
↓ 15
@
9 years ago
@jeremyfelt I think the differences can be dealt with the using wrapper functions. See this gist. https://gist.github.com/spacedmonkey/57135e6546dcf54fa527
You will notice that 90% of the code remains the same. The only thing of note that I have change is the SQL / cache call to call the meta function. If this was implemented, the *_network_meta function, would not be used by developers, but instead would be told to only use the *_network_option, as these are designed for public use. But using the metadata api, we just get caching, filters and hooks. It also standardises all the meta data calls. With the current push to term meta, I thought this was the direction all meta data was going towards. I understand if this maybe out of scope this ticket.
As for the functions, I am happy everything apart from the get network meta.
get_network_option( $option, $network_id = false, $default = false, $use_cache = true )
Having network id as the second param is a mistake. Consider this line in core
if ( $file_size > ( 1024 * get_site_option( 'fileupload_maxk', 1500 ) ) )
If the function name changes this line becomes this
if ( $file_size > ( 1024 * get_network_option( 'fileupload_maxk', false, 1500 ) ) )
This is ugly. Force developer to put an unnecessary param in the middle of a function call just to get current network is a pain. It also breaks the pattern the get_option setup of value, default. As I have stated above I believe the function should be
get_network_option( $option, $default = false, $network_id = false )
Which bring me to my second point, the use_cache param. Is this really required? No other call that I know of has this flag. What is used for? Should we really be encouraging uncached calls like this? I think with this change, we should do some clearing of house and get rid of this flag all together.
#15
in reply to:
↑ 14
@
9 years ago
- Owner set to spacedmonkey
- Status changed from reopened to assigned
Replying to spacedmonkey:
@jeremyfelt I think the differences can be dealt with the using wrapper functions. See this gist. https://gist.github.com/spacedmonkey/57135e6546dcf54fa527
You will notice that 90% of the code remains the same. The only thing of note that I have change is the SQL / cache call to call the meta function. If this was implemented, the *_network_meta function, would not be used by developers, but instead would be told to only use the *_network_option, as these are designed for public use. But using the metadata api, we just get caching, filters and hooks. It also standardises all the meta data calls. With the current push to term meta, I thought this was the direction all meta data was going towards. I understand if this maybe out of scope this ticket.
Yeah, let's save that as a future possibility. I'm happy to keep discussing, though I'm personally wary of stacking the two. It does need some more thought.
As for the functions, I am happy everything apart from the get network meta.
get_network_option( $option, $network_id = false, $default = false, $use_cache = true )
Having network id as the second param is a mistake. Consider this line in core
if ( $file_size > ( 1024 * get_site_option( 'fileupload_maxk', 1500 ) ) )
If the function name changes this line becomes this
if ( $file_size > ( 1024 * get_network_option( 'fileupload_maxk', false, 1500 ) ) )
This is ugly. Force developer to put an unnecessary param in the middle of a function call just to get current network is a pain. It also breaks the pattern the get_option setup of value, default. As I have stated above I believe the function should be
get_network_option( $option, $default = false, $network_id = false )
Hrm, good point. I can definitely see it both ways. Which would be more frequently used—with a network ID, but no default value or with a default value and no network ID? I would probably lean toward a default value, and no network ID, as multiple networks are still a rarity. Let's go with your suggestion there and see if it feels right.
Which bring me to my second point, the use_cache param. Is this really required? No other call that I know of has this flag. What is used for? Should we really be encouraging uncached calls like this? I think with this change, we should do some clearing of house and get rid of this flag all together.
Good question. It was introduced in WordPress core in [10593] and unused until the WPMU merge in [12615], at which point it was turned into its current form, a way to flush a cache value when retrieving it. It looks like the current form appeared in MU:465. I'm not able to find a spot where it was ever used to flush the cache while getting an option.
Let's try get_network_option()
without it and see where it takes us. :)
This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.
9 years ago
#18
@
9 years ago
Here is my first pass at the functions. Things to point out.
- I haven't done any testing on it yet. Either functional or unit.
- I have changed all references to *_site_option, throughout the core, which explains the large files size / name of files changed.
- I have to deprecated use_cache flag in get_site_options.
- I have added new filters / action with the networking naming and the network id passed to them. Old filters are still in place.
- All these functions allow for 0 to be sent as param, as this is a valid arg. 0 values are used to stored truly global network options.
The only bit I am not really happy with is the following lines.
/** If network ID not set, get current network. **/ if( false === $network_id && is_multisite() ){ $current_network = get_current_site(); $network_id = $current_network->id; } /** Make sure network id is an int */ $network_id = (int) $network_id;
We need something that will detect if the network is false, but allow 0 to be passed.
This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.
9 years ago
#20
follow-up:
↓ 22
@
9 years ago
- Keywords has-patch dev-feedback added; needs-patch removed
@spacedmonkey: Really nice work on a first pass. Some notes:
- It would be good to patch the function changes separately from the changeovers from
*_site_option
to*_network_option
, especially since they'll probably be committed separately anyway. Also, easier to grok changes during iteration - I'm not 100 percent on board with introducing new hooks in the name of vernacular, that is to say, new hooks that do exactly the same thing but are named differently. This is one area of core where we're starting to see a lot of technical debt (duplicate hooks with different names)
- We should add changelog entries to the DocBlocks for the
*_site_option
functions indicating that they were converted to wrappers for*_network_option
- Looks like there's copy pasta in the hook doc
@since
forpre_delete_network_option_' . $option
- Always use spaces in phpDocs, never tabs :)
#22
in reply to:
↑ 20
@
9 years ago
Replying to DrewAPicture:
- It would be good to patch the function changes separately from the changeovers from
*_site_option
to*_network_option
, especially since they'll probably be committed separately anyway. Also, easier to grok changes during iteration
Cool. Let me know when you want to do that and I will submit another patch.
- I'm not 100 percent on board with introducing new hooks in the name of vernacular, that is to say, new hooks that do exactly the same thing but are named differently. This is one area of core where we're starting to see a lot of technical debt (duplicate hooks with different names)
I personally wasn't sure about them either. But I thought without them, anyone coming to these functions posts 4.4, might be confused by the none guessable filter name. Also those filters and actions all have network id passed to them, so in a way they do do something different. I have left them in in my new patch.
- We should add changelog entries to the DocBlocks for the
*_site_option
functions indicating that they were converted to wrappers for*_network_option
Added changelog. There is now a since. There was also a uses in there as well.
- Looks like there's copy pasta in the hook doc
@since
forpre_delete_network_option_' . $option
Done
- Always use spaces in phpDocs, never tabs :)
Should be done.
#23
follow-up:
↓ 24
@
9 years ago
@spacedmonkey: On the duplicate hooks point, I like the idea of introducing the new hooks and deprecating the old ones. I'm reviving #10441 to tackle that angle.
I'd say go ahead and split the patches now as discussed.
#24
in reply to:
↑ 23
;
follow-up:
↓ 25
@
9 years ago
Replying to DrewAPicture:
@spacedmonkey: On the duplicate hooks point, I like the idea of introducing the new hooks and deprecating the old ones. I'm reviving #10441 to tackle that angle.
I like the idea of having the ability to deprecate filters and actions. But from a code stand point, there is nothing for me to do? Wouldn't it be better to merge this and refractively give this code. I am sure they will be lots of places where hooks should be deprecated.
I'd say go ahead and split the patches now as discussed.
Confused by this. What do I need to do from a patch stand point? Isn't it better to merge this and then I will create another ticket / patch to change this over the function name.
#25
in reply to:
↑ 24
@
9 years ago
Replying to spacedmonkey:
Replying to DrewAPicture:
@spacedmonkey: On the duplicate hooks point, I like the idea of introducing the new hooks and deprecating the old ones. I'm reviving #10441 to tackle that angle.
I like the idea of having the ability to deprecate filters and actions. But from a code stand point, there is nothing for me to do? Wouldn't it be better to merge this and refractively give this code. I am sure they will be lots of places where hooks should be deprecated.
For now, let's just skip adding new hooks altogether and relying on the old ones.
I'd say go ahead and split the patches now as discussed.
Confused by this. What do I need to do from a patch stand point? Isn't it better to merge this and then I will create another ticket / patch to change this over the function name.
It would be helpful to handle it all here, just in separate patches. The main reason for separate patches is to keep the commits cleaner. The second reason is that in patches where we touch a lot of code, they often fall out of sync and have to be refreshed. It's a lot easier to commit the base functionality, then generate a patch for the replacements (if any) after that.
#27
@
9 years ago
- Owner changed from spacedmonkey to jeremyfelt
- Status changed from assigned to reviewing
This is looking great, thanks! I'm going to take a closer look and poke around.
#28
follow-up:
↓ 31
@
9 years ago
FYI: A new filter on the expiration for "site" transients is being proposed in #21330. I'm tempted to recommend holding up introducing that new filter until we have a resolution here, hopefully in 4.4.
#30
@
9 years ago
Replying to spacedmonkey:
- All these functions allow for 0 to be sent as param, as this is a valid arg. 0 values are used to stored truly global network options.
We need something that will detect if the network is false, but allow 0 to be passed.
I missed this on one of my read throughs. Let's hold of on worrying about this immediately and focus on supporting valid networks. I like the thought though and we may be able to attach it to this ticket once other things are in.
28290.diff is a new riff on 28290.2a.patch. I'm going to hold on part b until this first one is in.
Everything is looking great, I modified the following:
- Some documentation, spacing tweaks.
- Now that we're using
$network_id
as part of the cache key, we don't need to rely on{
and}
wrapping it. - An
! isset( $value )
check was removed in the patch, which could change expected behavior. I've added that back. $network_id
was set totrue
by default inget_network_option()
.
I'm going to sit on things for a minute or few, but we're looking good. Thanks for all the great work @spacedmonkey.
#31
in reply to:
↑ 28
@
9 years ago
28290.2.diff uses global $current_site
instead of get_current_site()
as ms-settings.php
calls get_current_site()
before ms-functions.php
has been loaded. I'll need to document the new global use pre-commit as well. Multisite tests are passing fine locally.
Replying to DrewAPicture:
FYI: A new filter on the expiration for "site" transients is being proposed in #21330. I'm tempted to recommend holding up introducing that new filter until we have a resolution here, hopefully in 4.4.
My initial reaction is that we'll probably want to leave the naming of the transient functions alone as they don't have a great way to change the cache key to match the network yet. That will probably need to be part of a future ticket, so you may be okay to add the filter as needed now.
#35
@
9 years ago
28290.3.diff also introduces a few basic tests for network options. The existing site option tests are pretty thorough already for how core is currently using the functions. We'll probably want to transition those to _network_option()
tests once this is in.
#37
@
9 years ago
This is good.
Small issue: get_site_option()
needs a @since
entry for its deprecated $use_cache
parameter.
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.
9 years ago
#44
in reply to:
↑ 43
;
follow-up:
↓ 45
@
9 years ago
Replying to jeremyfelt:
In 34912:
Can someone explain why this was reverted? The commit doesn't explain the reasoning...
#45
in reply to:
↑ 44
;
follow-up:
↓ 47
@
9 years ago
Replying to spacedmonkey:
Can someone explain why this was reverted? The commit doesn't explain the reasoning...
Yep, sorry! I reversed the order of commenting in places. :)
We had a good discussion in #core-multisite a couple days ago and then another one yesterday and decided to change the parameter order in the _network_option()
functions so that $network_id
is first.
My summary:
- The parameter order feels nicer when retrieving an option for another network.
_blog_option()
accepts an ID as the first parameter. It would be strange to have another similar function with a different order.- WP Multi Network has provided
_network_option()
with$network_id
as the first parameter for quite a while. We eliminate some edge case fatals by doing this. _site_option()
functions will remain as the method for retrieving data about the current network without having to provide a network ID.
#46
@
9 years ago
The $network_id
parameter will default to current network if NULL is passed, right?
#47
in reply to:
↑ 45
;
follow-up:
↓ 49
@
9 years ago
I was not involved in that chat as I didn't know it was going on. I would have fought the corner of the current implementation but whatever. I think before writing any patch for this, this should have been decided. Feels like a lot of wasted work for something that ended up being binned.
So are _site_option() still going to be wrapper functions? You can easily just pass false / null as network_id and would still work. If this isn't the case, then we should just crib what JJJ did in WPMN and do a switch network.
This ticket was mentioned in Slack in #core by jeremyfelt. View the logs.
9 years ago
#49
in reply to:
↑ 47
@
9 years ago
Replying to knutsp:
The
$network_id
parameter will default to current network if NULL is passed, right?
There is a fallback right now to look for the current network. We'll require the parameter, but if it is false or null, we can look at the current network.
Replying to spacedmonkey:
I think before writing any patch for this, this should have been decided. Feels like a lot of wasted work for something that ended up being binned.
Your contributed work is very much appreciated. Iterations will always continue as part of the process, especially as things become more visible and more eyes provide feedback.
So are _site_option() still going to be wrapper functions? You can easily just pass false / null as network_id and would still work. If this isn't the case, then we should just crib what JJJ did in WPMN and do a switch network.
Yes, _site_option()
would be a wrapper around _network_option()
for the current network. While we've talked about network switching a bit, there's quite a bit involved in fleshing that one out.
I agree that this is confusing at first. But it's not just these functions. There are quite a few other places where
network
would now be more accurate butsite
is still used. I have the same sentiment toward changing these, but if we change these option functions, it really won't go very far toward improving core's consistency. I'd love to seesite
replaced withnetwork
everywhere in core, but it's almost impossible to do that and maintain backward compatibility. I think folks might favor this as part of a long-term project to improve core's consistency here, but it should really be the result of more significant changes. E.g., #20651So yes, let's change these to
*_network_option()
, but let's not just do it to change the names. Find everything wrong with the old functions that we couldn't fix without breaking back-compat, and let's fix it in the new functions.