[wp-trac] [WordPress Trac] #28290: Changing _site_option functions for _network_option functions

[WordPress Trac]

#28290: Changing _site_option functions for _network_option functions
--------------------------------+---------------------------
 Reporter:  jmlapam             |       Owner:  spacedmonkey
     Type:  enhancement         |      Status:  assigned
 Priority:  normal              |   Milestone:  4.4
Component:  Networks and Sites  |     Version:  4.0
 Severity:  normal              |  Resolution:
 Keywords:  needs-patch         |     Focuses:  multisite
--------------------------------+---------------------------
Changes (by jeremyfelt):

 * owner:   => spacedmonkey
 * status:  reopened => assigned


Comment:

 Replying to [comment:14 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
 [https://mu.trac.wordpress.org/changeset/465 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. :)

--
Ticket URL: <https://core.trac.wordpress.org/ticket/28290#comment:15>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform