WP_Network class

[johnjamesjacoby]

At WordCamp Seattle, Jeremy Felt and I talked a bit about how to work towards a few different multisite goals without conflating them together. I recommended that we spend some time defining what we think our main WP_Network and WP_Site classes should look like.

I used the plane ride back to Wisconsin to hack together a patch I'll be attaching to this ticket imminently.

[johnjamesjacoby]

First pass at WP_Network class

[jeremyfelt]

WP_Network will be easy to nail down some basics for as $current_site isn't too complex and we don't do much with networks inside core. It will be hard to nail down because the pieces it touches are all seemingly tough to change.

I think this is a great start.

A few thoughts (some in reaction, some in general):

• I think we'll benefit from planning on a WP_Network_Query immediately and moving some of the (currently static) methods there. (JJJ planned ahead)
• I don't doubt we can do this in concert with WP_Site. This will make things like "how do we populate $current_site->blog_id somewhat easier to grasp.
• add() is empty right now, we could probably use what's done in populate_network() as a base?
• Is there a reason to separate ms_set_switched_stacks() from ms_set_current_globals()?

It would also be worth really staring at what test coverage is missing in advance so that adding WP_Network is as painless as possible.

[jeremyfelt]

31985.2.diff cuts down on @johnjamesjacoby's original patch to only satisfy the parts setup in ms-settings.php and to provide a base object. We can decide which parts should be attached to this and which belong in a WP_Network_Query or elsewhere.

All current multisite test are passing. :)

[johnjamesjacoby]

Replying to jeremyfelt:

31985.2.diff cuts down on @johnjamesjacoby's original patch to only satisfy the parts setup in ms-settings.php and to provide a base object. We can decide which parts should be attached to this and which belong in a WP_Network_Query or elsewhere.

All current multisite test are passing. :)

2.diff looks fine, but the original patch is farther along than this. Was my patch causing tests to fail for you?

I understand the unwinding process, so I can appreciate wanting to see the meaty middle and present it for others to digest. I think, though, that the original patch (despite missing the inevitable WP_Network_Query class) makes for a better foundation to iterate from since it already brings parity to the single-site wp-settings.php file, and isolates many of the existing multisite gotchas.

What's missing between the two diffs represents a few hours of drilling down deep into multisite spaghetti and consolidating it into 1 convenient location. I think, then, it will be much easier to scrutinize rather than going through the cherry-picking process again.

[johnjamesjacoby]

We could cut the WP_Network class down even more if we want to keep it true to the wp_site table schema. I don't think this is necessary, but it would make the best foundation to build from, if we do want to start over.

[jeremyfelt]

Replying to johnjamesjacoby:

2.diff looks fine, but the original patch is farther along than this. Was my patch causing tests to fail for you?

I understand the unwinding process, so I can appreciate wanting to see the meaty middle and present it for others to digest. I think, though, that the original patch (despite missing the inevitable WP_Network_Query class) makes for a better foundation to iterate from since it already brings parity to the single-site wp-settings.php file, and isolates many of the existing multisite gotchas.

I think I left out half of what I originally wanted to say in my comment. :) Definitely digesting and figuring out where things lay. I agree that we can do a more comprehensive patch and the original is in a good place. Part of this was me trying to understand what belongs elsewhere and what's absolutely required here. Part of it was that multisite tests weren't passing and I dug from the opposite end.

A riff directly off the 31985.01.patch, 31985.diff makes these changes:

• Includes class-wp-network.php in ms-settings.php so that WP_Network is available.
• get_core_options() was returning the original options rather than the rebuilt array.
• set_site_name() had an opposite empty() check for site name.
• Revert the use of get_active_network_plugins() and continue handling wp_get_active_network_plugins() as is. I'm not really understanding why this isn't working yet, but the tests for network activated plugins are failing and an empty array is coming back every time.

It's almost terrifying how clean ms-settings.php is. :)

Other thoughts:

It may be nice if get_site_option() (and friends) supported a network ID argument. Could that route lead to nicer things rather than maintaining a get_core_options() method?

• The network ID is already stored as part of the cache key. See #25883, [26304]
• site-options is already a global cache group.
• $wpdb->site_id is only used in the query. This could easily be replaced with a network ID if passed.

Would it be crazy to wrap a WP_Network()->get_option()? It would go a ways toward solving a naming annoyance.

[jeremyfelt]

31985.3.diff is a result of another read through.

• Updated to apply cleanly after several doc changes in [32611].
• Several docs cleaned up somewhat to match the core docs guide.

I've created #32504 to track progress on WP_Network_Query. While the patches for the two will likely overlap, there will be a time near commit where logistical separation will make sense.

[jacobsantos]

Programmers who use static methods quickly realize how much of a mistake it is to do so.

What reason are you defining class methods instead of instance methods?

Are you attempting to create namespaced functions? If so, then what benefit do you achieve outside of just creating a function?

Are you mixing class methods and instance methods? Do the class methods amount match or exceed the instance methods count? Would you be better served creating a separate class for the static methods?

Do you wish to break extensiblity? If so, then why not just use 'final'? If you decide to use 'final', then what problem are you truly attempting to solve? Would your solution be better if it used functions instead?

[johnjamesjacoby]

Replying to jacobsantos:

Programmers who use static methods quickly realize how much of a mistake it is to do so.

Not all programmers, and not all methods.

What reason are you defining class methods instead of instance methods?

For identifying and encapsulating existing procedural code, so we can work backwards from what currently exists.

Are you attempting to create namespaced functions? If so, then what benefit do you achieve outside of just creating a function?

None yet, other than developer sanity by identifying the logic flow (which currently spans several files) and isolating it to one convenient location to iterate from further.

Are you mixing class methods and instance methods? Do the class methods amount match or exceed the instance methods count? Would you be better served creating a separate class for the static methods?

Yes. CRUD actions will be broken out into a WP_Network_Query class.

Do you wish to break extensiblity? If so, then why not just use 'final'? If you decide to use 'final', then what problem are you truly attempting to solve? Would your solution be better if it used functions instead?

No? Not sure why this is mentioned here in this way. The current core code is really only extensible because core developers needed it to be for WordPress.org. No flexibility has been added or removed in any patches presented here. It's entirely likely there will be 10 or more iterations before we agree on a final solution.

The patches presented so far are not prestine candidates for core, but rather two people unwinding a messy portion of the codebase that's gone untouched for several years, and several years again before that.

I, for one, appreciate your feedback, and also look forward to your helping apply that feedback towards future patch iterations working towards the final result, if you're willing and able.

[jacobsantos]

Replying to johnjamesjacoby:

Replying to jacobsantos:

Programmers who use static methods quickly realize how much of a mistake it is to do so.

Not all programmers, and not all methods.

I find that those who don't, realize sooner or later that they made a huge mistake. In truth the amount of patterns where class methods are acceptable are minimal. Singletons, Registry, and languages where functions don't exist.

The realization that static methods bar inheritance, make testing more difficult, among other problems shine a light into why class methods should be avoided at all costs unless absolutely necessary and with good reason. I find that whenever I attempt to make a method static, what I'm really looking for is a singleton. Then I realize that what I'm really looking for is breaking the class out into smaller classes or creating a separate function.

If you are merely attempting to refactor existing code, then that is an acceptable reason. Provided that at a later date, they are sorted into instance methods.

What reason are you defining class methods instead of instance methods?

For identifying and encapsulating existing procedural code, so we can work backwards from what currently exists.

Great! Right. I read that in a book once a while back. I'll help.

Are you attempting to create namespaced functions? If so, then what benefit do you achieve outside of just creating a function?

None yet, other than developer sanity by identifying the logic flow (which currently spans several files) and isolating it to one convenient location to iterate from further.

I will suggest interfaces. Interfaces are awesome with process flow.

Are you mixing class methods and instance methods? Do the class methods amount match or exceed the instance methods count? Would you be better served creating a separate class for the static methods?

Yes. CRUD actions will be broken out into a WP_Network_Query class.

Do you wish to break extensiblity? If so, then why not just use 'final'? If you decide to use 'final', then what problem are you truly attempting to solve? Would your solution be better if it used functions instead?

No? Not sure why this is mentioned here in this way. The current core code is really only extensible because core developers needed it to be for WordPress.org. No flexibility has been added or removed in any patches presented here. It's entirely likely there will be 10 or more iterations before we agree on a final solution.

My ultimate goal is to attempt to show how WordPress could be more flexible and testable. One way of doing that is by using interfaces and creating code that is composable. I should have an example at some point in the near future. I am busy with another short project.

Actually, the presentation might be able to help describe what I mean. I should have a video tonight and a better one Friday.

The patches presented so far are not prestine candidates for core, but rather two people unwinding a messy portion of the codebase that's gone untouched for several years, and several years again before that.

I didn't think the WordPress MU stuff that was that bad. Of course, I never truly looked at it. I might have burned it from my memory.

I, for one, appreciate your feedback, and also look forward to your helping apply that feedback towards future patch iterations working towards the final result, if you're willing and able.

I will be able to submit a patch Friday or Saturday at the earliest. I am guessing you might have a few more iterations available at that time. Or maybe not. I have also been reviewing WP_Widget that I also need to submit a patch for to show an example of what I mean for that. Not really relevant, other than I'm attempting to better show what I mean with code examples.

I think once I get to the unit and integration tests, it might make more sense. Or maybe not.

[jeremyfelt]

Leaving a note per a ​comment from @ocean90, wp_get_network() no longer returns false if a network is not found. __construct() should probably account for that.

[spacedmonkey]

Been looking at this work and looks great. Have some feedback, so here we go.

Been looking at this I feel like this class should be little more filterable. I know it might be something you want to worry about at the end, I thought I would bring it up now.

Firstly here

 public function __construct( $network = false ) { 
 	// Bail if not populating from an existing network 
 	if ( empty( $network ) ) { 
 	         return; 
 	} 

How about

 public function __construct( $network = false ) { 
        $network = apply_filters( 'pre_get_network', $network ); 
 	// Bail if not populating from an existing network 
 	if ( empty( $network ) ) { 
 	         return $network; 
 	} 

Just in case you want to highjack the getting network process for anything. Also returned the network variable, do you can do some type checking on the return. Empty will flag, null, zero and empty string values, maybe want to return that...

Get get_core_option_keys should return through apply_filters.

Add / edit / delete should clear / set cache values, as you don't to return an old version of the network object with incorrect data.

$network = wp_cache_get( $network_id, 'networks' )

get_core_options method should use get_site_option instead of raw sql query. I know this means more sql queries, but at the moment, it is bypassing all the filters on get_site_option, which many use to change these values out of the db. This would also mean that set_site_name also magically gets filterable.

[jacobsantos]

Replying to spacedmonkey:

Been looking at this work and looks great. Have some feedback, so here we go.

Been looking at this I feel like this class should be little more filterable. I know it might be something you want to worry about at the end, I thought I would bring it up now.

PHP5 objects __construct() can not return. What this means is that return does not do anything as $obj = new Whatever; will always give an instance of Whatever, even if all __construct() has is return null.

Furthermore, returning a new instance of an object, does not and will not replace the current instance of that object.

__construct() having return self::$instance does nothing.

What you are looking for is a factory.

public static function factory() {
        $network = apply_filters( 'pre_get_network', $network ); 
        // Bail if not populating from an existing network 
        if ( empty( $network ) ) { 
                return $network;
        }
        return new static; // static is a PHP5.3 feature, need to do __CLASS__(); or similar.
}

Firstly here

 public function __construct( $network = false ) { 
 	// Bail if not populating from an existing network 
 	if ( empty( $network ) ) { 
 	         return; 
 	} 

How about

 public function __construct( $network = false ) { 
        $network = apply_filters( 'pre_get_network', $network ); 
 	// Bail if not populating from an existing network 
 	if ( empty( $network ) ) { 
 	         return $network; 
 	} 

See above for why this code will not work. Also this: ​http://stackoverflow.com/questions/5622605/php-getting-a-reference-through-a-constructor and it does appear this behavior is not documented on the __construct and __destruct PHP.net page, but I don't believe any OOP language allows you to violate OO paradigm by returning a different instance. Well, there might be, but it would be undocumented and something that a programmer would not want to do.

[spacedmonkey]

Good spot to jacobsantos, don't know what I was thinking there. I stand up my other comments.

[jeremyfelt]

We've made quite a bit of progress here toward WP_Network. It would be nice if we can keep progress going throughout the remainder of the 4.3 cycle on this, #32450, and #32504 (among others) so that we can start landing these earlier in the 4.4 cycle.

[jeremyfelt]

Another riff

[jeremyfelt]

I spent a bunch more time staring at this today and ended up with 31985.4.diff. Here are some of my thoughts throughout and changes in comparison to previous patches.

• I scaled back on the reorganization of bootstrap code in ms-settings.php. IMO, it doesn't feel right to consolidate that as part of this ticket. There was a point when I realized we were no longer populating the $domain and $path globals and I decided to leave it all alone to help make a smaller patch. I think we should address this in a future ticket.
• Moved WP_Network::get_by_id() to WP_Network::get_instance() to mirror what WP_Post and WP_Comment do.
• Made WP_Network() expect a standard network object in __construct() that it could then use to populate the remainder of the object, also mirroring what WP_Post and WP_Comment do.
• Changed WP_Network::get_core_options() to WP_Network::load_core_options() to better match the spirit and functionality of wp_load_core_site_options(), which it replaces. At first it seemed like a great idea to load the options into a property and reuse them, but we then skip the filters normally used in get_site_option(). I'd still like to consider wrapping this at some point in the future, but we need to support different network IDs when looking at network options. I think this can be another ticket. In the meantime, we have the opportunity to load any network's main options into cache if a persistent object cache is not available.
• Removed WP_Network::get_active_network_plugins() and WP_Network:validate_network_plugin() because of the core options issue. I think we can reconsider this in the future as well.
• Removed the add, update, and delete methods as I don't think we want to support CRUD in a first version. I don't have any issues supporting it soon after though. This would really be a new feature.

I think we're getting close. :)

[DrewAPicture]

Docs

[DrewAPicture]

31985.5.diff​ takes a pass through the documentation. I added a file header to class-wp-network.php, and generally shored up some missing phpDoc tags in various methods and throughout the other changed files. Finally, sprinkled in a few changelog entries for functions that were significantly changed to leverage WP_Network instead.

On the private methods in WP_Network, I think we should probably underscore-prefix the function names just so it's very clear that they're for internal use only, seems to be core style for the most part.

[jeremyfelt]

Excellent, thanks @DrewAPicture. That's looking good.

• I had also considered "Multisite API" in the file header, but it's not really an API. Or is it? "WP_Network object class" seems to fit better with the subpackage as multisite.
• I'm okay with underscore prefixing on the private methods.
• It looks like one @access public snuck in on a private method in 31985.5.diff. No big deal to change that during commit.

[BinaryKitten]

I'd like to see the Private methods changed to protected. This would leave them in the same state as they are but extensions (I envision extending the class to create "Types" of network etc) would also be able to use the data.

[wonderboymusic]

31985.6.diff​ removes some SQL formatting quirks (that style doesn't exist anywhere else in core).

[spacedmonkey]

Big changes in my patch

• Allow get_instance to take WP_Network object as a param.
• Added pre_get_network filter in get_instance method, so you can short-circuit. Based on pre_get_network_by_path
• get_core_option_keys method also now has a filter.

[spacedmonkey]

I also think that the method load_core_options shouldn't use a raw SQL query like this. It should use the get_site_option function, as this has filters that should be applied. Maybe get_site_option should allow site id / network id to be passed to it?

[jeremyfelt]

Thanks for the patch @spacedmonkey!

Replying to spacedmonkey:

• Allow get_instance to take WP_Network object as a param.

Can you expand on why this might be necessary? My recent thought has been to follow the pattern laid out in WP_Post and WP_Comment which both have a get_instance() method that only accepts an ID. We could probably even change this to be more explicit and reject anything that isn't int in the first version.

• Added pre_get_network filter in get_instance method, so you can short-circuit. Based on pre_get_network_by_path

I'm not sure we should have this available just yet. First because we should hold on introducing any new filters for a future commit. Second, because I'm not sure if I see this as needing to be short-circuited. Definitely welcome varying opinions on this though.

• get_core_option_keys method also now has a filter.

Another to save for a future ticket or future patch/commit on this ticket once we get WP_Network in. get_core_option_keys and load_core_options are explicit in that they are processing the core defined option keys. If we do add a filter for this, we should wait until the class has been implemented.

I also think that the method load_core_options shouldn't use a raw SQL query like this. It should use the get_site_option function, as this has filters that should be applied. Maybe get_site_option should allow site id / network id to be passed to it?

The purpose of wp_load_core_options() is really only to prime non-persistent cache for a network's options when it is loaded. That it's there at all is somewhat strange to me. :) I definitely think there's room for a future ticket where we add better get_site_option() support and allow a network ID to be passed.

[jeremyfelt]

Another day, another trim. :)

31985.8.diff is what I'm looking at for the initial implementation right now. We can leave the ticket open as a task for a bit to make sure we're catching everything.

Differing from previous patches:

• I removed the load_core_options change and left the original as is. This would normally only fire during multisite's bootstrap and only do anything if a persistent object cache was not available. If we move it to WP_Network() without the object cache check, we end up setting cache values every time a WP_Network object is created. There's probably a way to prime persistent cache properly, but I'm not sure we need the existing load functionality every time a new object is created.
• Similar to that, I took out the set_site_name method temporarily as it currently relies on get_site_option(), which only works with the current network. This makes it the perfect time to introduce a get_network_option() that also takes a $network_id. We're ready to go on that in #28290. Once that is available, some of this starts making more sense.

[jeremyfelt]

In 34097:

Multisite: Introduce the WP_Network class.

A WP_Network object initially matches a row from wp_site and is populated with additional properties used by WordPress core. The first iteration is used to retrieve an existing network based on data passed to the class.

• A network can be retrieved by its ID through WP_Network::get_instance(), following in the steps of WP_Post and WP_Comment.
• A network object can be created or completed by passing initial properties in as a standard object to new WP_Network().

Using these methods, we are now able to populate the global $current_site during load via this class.

Props johnjamesjacoby, jeremyfelt, drewapicture, wonderboymusic.
See #31985.

[jeremyfelt]

In 34099:

Multisite: Implement the get_by_path method in WP_Network.

Move the internals of get_network_by_path() to WP_Network() and allow network objects to be retrieved by passing a requested domain and path.

Props johnjamesjacoby, jeremyfelt, drewapicture, wonderboymusic.
See #31985.

[jeremyfelt]

Now that we can retrieve options via network ID (#28290), we can _set_site_name() inside WP_Network().

31985.9.diff adds this.

[jeremyfelt]

In 34819:

MS: Populate site_name property in a new WP_Network.

This allows us to reduce some extra handling in ms-settings.php. Requires [34777].

Props johnjamesjacoby for the initial patch.
See #31985.

[toscho]

Can we please use proper getters for the currently public properties and make the properties private? If anybody can change those at any time, the whole object becomes completely unreliable. This is not helpful. Also, this needs an interface, so we can use the class in dependency injection without a static dependency.

[jeremyfelt]

Replying to toscho:

Can we please use proper getters for the currently public properties and make the properties private? If anybody can change those at any time, the whole object becomes completely unreliable. This is not helpful. Also, this needs an interface, so we can use the class in dependency injection without a static dependency.

We ​talked about public vs private a bit during yesterdays office hours. We're going to leave the properties as public. A couple reasons for this:

• Most related objects in core have public properties. It's an expectation of sorts.
• We're introducing this as the way to populate $current_site, which has assumed properties that have been stomped on for years. We risk breakage if we make those properties private all of a sudden.

[jeremyfelt]

Closing this out as fixed for 4.4. Any bugs or new enhancements can be opened as new tickets.

Thanks everyone!

[toscho]

Replying to jeremyfelt:

• Most related objects in core have public properties. It's an expectation of sorts.

This is one of the main causes for plugin incompatibilities. It is an extremely bad pattern.

• We're introducing this as the way to populate $current_site, which has assumed properties that have been stomped on for years. We risk breakage if we make those properties private all of a sudden.

Then just use __get( $name ). That solves the problem without direct write access. Just make sure that a property is always the same, no matter when I'm calling it.

[jeremyfelt]

Replying to toscho:

• We're introducing this as the way to populate $current_site, which has assumed properties that have been stomped on for years. We risk breakage if we make those properties private all of a sudden.

Then just use __get( $name ). That solves the problem without direct write access. Just make sure that a property is always the same, no matter when I'm calling it.

Some type of write access is needed to support existing uses of the $current_site global. We could add __set(), but it doesn't seem necessary until we have a property that needs validation.

// Custom via sunrise.php
$current_site = new stdClass();
$current_site->site_id = 1;
$current_site->domain = 'mydomain.com';
$current_site->path = '/';
$current_site->site_name = 'My Network';

// WP trunk, ms-settings.php
$current_site = new WP_Network( $current_site );

// Custom network switching via plugin
$current_site->site_id = 2;
$current_site->blog_id = 5;
$current_site->site_name = 'My other network';
// etc...

[ocean90]

In 35212:

Multisite: Remove the strictness for $using_paths in WP_Network::get_by_path().

The network lookup was broken when using an external object cache because $using_paths isn't always a boolean. Added in [34099].

See #31985, #31491.

[jeremyfelt]

In 35573:

Multisite: Clarify documentation for WP_Network::get_by_path().

See #31985.