Allow to access network and site properties using the current naming conventions

[flixos90]

In multisite we have the issue that we used to call a site a blog and a network a site. Therefore the WP_Site and WP_Network classes use weird naming conventions for their properties. It has to be like that for backwards compatibility, but since the classes itself now use the new conventions, it can be extremely confusing to interact with them ($site->site_id will give me the network ID for example).

That's why I'm proposing to add magic getters to both these classes, allowing to access these properties in the following way:

• in both classes, id simply refers to the objects ID (so in WP_Site it will be the site ID, and in WP_Network it will be the network ID)
• to get the network ID of a site, we can use $site->network_id
• to get the main site ID of a network, we can use $network->site_id

While this doesn't provide a functional benefit, it would make interacting with these classes a lot more logical. And it's also backwards compatible since we're only adding these new properties virtually.

[flixos90]

patch with magic methods

[flixos90]

36717.diff​ is a patch that adds the necessary magic methods to WP_Site and WP_Network.

[flixos90]

If we include this in Core, we might also think about the following:

Maybe we could make the existing properties ($blog_id and $site_id in WP_Site and $blog_id in WP_Network) private and only make them accessible through magic methods. Then we could add the new virtual properties into a class docblock as @property. This would allow IDEs with autocompletion to show the correct property names while still supporting the old naming conventions.

Just an idea on top though...

[jeremyfelt]

This is on the right track. Let's move it to the 4.6 milestone.

[jeremyfelt]

36717.2.diff​ makes a couple changes:

• Use ID rather than id for both to follow precedent set by WP_User and WP_Post.
• Use if vs switch in WP_Network since we're only checking one key.
• Small changes to docs.

To recap, we'll get:

• $network->ID
• $network->site_id
• $site->ID
• $site->network_id

[jeremyfelt]

Replying to jeremyfelt:

To recap, we'll get:

• $network->ID
• $network->site_id
• $site->ID
• $site->network_id

id vs ID was covered in core chat today and everyone seems to agree that id is best.

So now:

• $network->id
• $network->site_id
• $site->id
• $site->network_id

[jeremyfelt]

36717.3.diff​ adjusts things so that the "new" properties are more visible to IDEs as @flixos90 described earlier:

• Make $blog_id and $site_id in WP_Site private
• Make $site_id in WP_Network private
• Add @property docs for the class
• Add setters so that the now private properties can still be set by legacy code.

I started trying to convert the new properties to int, but I think that gets too messy.

[flixos90]

Replying to jeremyfelt:

I started trying to convert the new properties to int, but I think that gets too messy.

I just thought about the same while looking at the patch before even reading your comment. Couldn't we, only for the new properties, ensure that when they're read they return an integer? And then, when setting the property, ensure it's a string? That would give the new properties a matching type while preserving backwards-compatibility with the old properties. Or am I missing something here?

[jeremyfelt]

Replying to flixos90:

Couldn't we, only for the new properties, ensure that when they're read they return an integer? And then, when setting the property, ensure it's a string? That would give the new properties a matching type while preserving backwards-compatibility with the old properties. Or am I missing something here?

My first thought was that the juggling seems a little out of place for how much strict typing we don't do in core. However, future endpoints will likely treat id as int and the DB schema already does, so it probably won't hurt.

[jeremyfelt]

36717.4.diff​ casts the new properties to int on get and all ID properties to string on set.

This seems nice for WP_Site because we'll have id and network_id and both will be int. It seems less nice for WP_Network because id will still be a string and only site_id would be int.

[flixos90]

Replying to jeremyfelt:

36717.4.diff​ casts the new properties to int on get and all ID properties to string on set.

This seems nice for WP_Site because we'll have id and network_id and both will be int. It seems less nice for WP_Network because id will still be a string and only site_id would be int.

Hmm, that's really unfortunate. We should probably still stick with using int for the other properties. If we only had decided for using ID instead of id for the new standards... ;) But yeah, I guess it has to remain a string then.

Another note, do we really need the default clause in the switch statement in the setter? I included it in my initial patch, but looking at it now, I don't think we need to set something there.

[jeremyfelt]

Replying to flixos90:

Another note, do we really need the default clause in the switch statement in the setter? I included it in my initial patch, but looking at it now, I don't think we need to set something there.

I thought not too, but we have a few dynamic properties for WP_Site that we don't yet include as part of the "official" object. get_blog_details() assigns blogname, siteurl, post_count, and home. Because these are there, we need some kind of default to fall back on.

WP_Network doesn't have this restriction, but I'm wary of being strict due to the possible existing uses of $current_site out in the wild.

[jeremyfelt]

In 37657:

Multisite: Allow access to network and site properties using current naming conventions

• Add magic __get(), __set(), and __isset() methods to WP_Site and `WP_Network.
• Provide (int) $network->site_id for (string) $network->blog_id
• Provide (int) $site->id for (string) $site->blog_id
• Provide (int) $site->network_id for (string) $site->site_id

Props flixos90, jeremyfelt.
Fixes #36717.

[jeremyfelt]

Reopening for some follow-up.

[jeremyfelt]

In 37658:

Multisite: Fix switch( spacing after [37657]

Fixes #36717.

[jeremyfelt]

This broke things on w.org last night, and needs some more exploration.

From @dd32 ​in Slack:

"When you serialize() a object which has public properties, and unserialize() that same string later when the object has a private property, the public property gets set which overrides any __get() methods."

Full WP_Site objects with existing public properties were stored in cache via get_blog_details(), which was being used along with the pre_get_site_path filter. There will be other sites doing something similar.

[flixos90]

Replying to jeremyfelt:

Full WP_Site objects with existing public properties were stored in cache via get_blog_details(), which was being used along with the pre_get_site_path filter. There will be other sites doing something similar.

This is closely related to #36935. The patch on that ticket should fix that problem since it explicitly stores a raw site object in cache. It only needs a small update to work properly: ensure that $blog_id and $site_id are cached as well (since these are now private and wouldn't be caught by get_object_vars()).

[flixos90]

I have updated the patch on #36935. Please apply that patch to test. As far as I can see, the bug related to get_blog_details() gets fixed by it.

[dd32]

Replying to jeremyfelt:

This broke things on w.org last night, and needs some more exploration.

From @dd32 ​in Slack:

"When you serialize() a object which has public properties, and unserialize() that same string later when the object has a private property, the public property gets set which overrides any __get() methods."

Full WP_Site objects with existing public properties were stored in cache via get_blog_details(), which was being used along with the pre_get_site_path filter. There will be other sites doing something similar.

FWIW it turned out that also broke a bunch of other things, such as editing network sites, and anything else that performed comparisons on blog_id's.

It looks like #36935 will indeed fix the underlying cache issue, although existing cached data will not be invalidated, which will be an issue (If this change had invalidated that cache, it would've been fine).

[jeremyfelt]

Replying to dd32:

FWIW it turned out that also broke a bunch of other things, such as editing network sites, and anything else that performed comparisons on blog_id's.

It also broke the deprecated version of wp_get_sites(), which casts site objects to array and in the process loses the private properties (which become prefixed). 36717.5.diff​ adds a shim and a test for this scenario.

It looks like #36935 will indeed fix the underlying cache issue, although existing cached data will not be invalidated, which will be an issue (If this change had invalidated that cache, it would've been fine).

Do we have options for invalidating the cache on update?

[dd32]

Replying to jeremyfelt:

It looks like #36935 will indeed fix the underlying cache issue, although existing cached data will not be invalidated, which will be an issue (If this change had invalidated that cache, it would've been fine).

Do we have options for invalidating the cache on update?

I was hoping that the $last_modified cache prefix was in use for that group, but it's not (nor does it need to be really). We don't really have any ability to flush a cache group upon update either (We request that the object cache flushes upon upgrade, but most caches don't perform that operation).

[flixos90]

Minor update: 36717.6.diff​ uses the to_array() method which will ensure that the keys blog_id and site_id are part of the array.

[jeremyfelt]

Replying to flixos90:

Minor update: 36717.6.diff​ uses the to_array() method which will ensure that the keys blog_id and site_id are part of the array.

Good call. :)

[jeremyfelt]

In 37667:

Multisite: Use to_array() method on WP_Site objects in wp_get_sites()

When an object with private properties is cast directly to an array, those properties are no longer available with their original keys.

Props @flixos90.
See #36717.

[jeremyfelt]

Some thoughts after pondering and discussing the cache conundrum over the last couple days...

• It seems the right approach for 4.6 is to make blog_id and site_id public again. Cached public properties via get_blog_details() is one scenario, and we can probably do a bunch of work to get around that, but it seems possible that others have serialized versions of WP_Site that would break.

Separately:

• That we're storing WP_Site objects in cache is an issue in waiting anyway. We should decide if it's worth upgrading (downgrading?) these to stdClass when storing in cache. If so, that's a new ticket.
• It may be that we'll be ready to deprecate get_blog_details() in the near future or at least stop relying on it. If we treat #36935 as something entirely different—maybe with new cache keys—then get_sites() / get_site() could be used instead. Maybe a last modified needs to be added to the key to help with future invalidation.

[flixos90]

Replying to jeremyfelt:

Some thoughts after pondering and discussing the cache conundrum over the last couple days...

• It seems the right approach for 4.6 is to make blog_id and site_id public again. Cached public properties via get_blog_details() is one scenario, and we can probably do a bunch of work to get around that, but it seems possible that others have serialized versions of WP_Site that would break.

Okay that may be the best solution for now. We only made these private to be able to ensure they are passed a string right? So we wouldn't really make something worse. In fact it would just work like before (you can pass anything).

Separately:

• That we're storing WP_Site objects in cache is an issue in waiting anyway. We should decide if it's worth upgrading (downgrading?) these to stdClass when storing in cache. If so, that's a new ticket.
• It may be that we'll be ready to deprecate get_blog_details() in the near future or at least stop relying on it. If we treat #36935 as something entirely different—maybe with new cache keys—then get_sites() / get_site() could be used instead. Maybe a last modified needs to be added to the key to help with future invalidation.

Where exactly do we store WP_Site in cache? Are there other locations than get_blog_details()? I generally think we should switch to storing plain stdClass everywhere to prevent issues like the above in the future. I agree that new cache keys for the get_details() method in #36935 would totally make sense as a first step. Actually, if we use new cache keys, we wouldn't need to do the changes in your first bullet point, do I get that right?

[flixos90]

36717.7.diff​ fixes the problem that came up after 37657. Since the properties were public before, there is an unserialization issue when getting cached objects as the properties are now private. This patch checks that the two properties have a valid value and otherwise queries the object again to update cache.

This current fix makes the code of that function even more confusing to read, but as we will get rid of most of that code after #36935 is done anyway, I think it's acceptable.

[jeremyfelt]

In 37871:

Multisite: Set WP_Network blog_id property default to string as expected.

The blog_id property is always returned (and expected) as a string, so we should set it as one by default.

Props flixos90.
See #36717.

[jeremyfelt]

I used these steps to test:

• Revert to r37656
• Use get_blog_details( 2 ) to populate cache with public WP_Site properties
• Update to r37657 (at least)
• Confirm unavailable data in get_blog_details( 2 ) return via object cache.
• Apply patch
• Confirm object is returned as expected and now available in cache properly.

[jeremyfelt]

In 37874:

Multisite: Clear incomplete objects from cache in get_blog_details() when found.

In [37657], the blog_id and site_id properties were changed to private. Any WP_Site objects previously stored in cache with public properties should now be considered invalid. We can detect this by checking for these missing properties and clearing the dirty cache if found.

Props flixos90.
Fixes #36717.

[westi]

The changes in this ticket feel pretty unnecessary and fragile and likely to break a number of requests on any site using a caching backend because we are going to cache things the old code can't uncache.... in a nasty way - you end up with WP_Site instances that re-initialise out of the cache without a blog_id.

I'm not really sure why it was really important to make this backwards incompatible change which makes reverting _hard_ on large installs and breaks things in imaginative ways when it goes wrong.

The fact that this broke the WP.org install should have been a hint to revert it rather than patch over the cracks IMHO.

[DrewAPicture]

In 37917:

Docs: Add changelog entries to the DocBlocks for the $blog_id and $site_id properties in WP_Site.

See [37657] for where access was explicitly changed from public to private.

See #36717.

[DrewAPicture]

In 37919:

Docs: Supplement a changelog entry in the DocBlock for the $id property in WP_Network.

See [37657] for where access was changed from public to private.
See [37870] for where the type was changed from string to int.

See #36717.

[jeremyfelt]

Replying to westi:

The changes in this ticket feel pretty unnecessary and fragile and likely to break a number of requests on any site using a caching backend because we are going to cache things the old code can't uncache.... in a nasty way - you end up with WP_Site instances that re-initialise out of the cache without a blog_id.

As of [37874], get_blog_details() should be in a position to repair itself if it encounters a WP_Site object without blog_id. Is that not working or part of what feels fragile?

I'm not really sure why it was really important to make this backwards incompatible change which makes reverting _hard_ on large installs and breaks things in imaginative ways when it goes wrong.

The fact that this broke the WP.org install should have been a hint to revert it rather than patch over the cracks IMHO.

I hear you. It was a hint originally, and we definitely discussed it several times. But I decided we could be in position to catch the error and resolve it rather than revert. If it's too fragile, I have no problem making those properties public again.

[jeremyfelt]

Replying to westi:
breaks things in imaginative ways when it goes wrong.

This is the part I'm most worried about. Taking a look at which parts to revert now. :)

[jeremyfelt]

36717.10.diff​ is a partial revert of [37657] and full revert of [37870]. Properties that were public and moved to private are now public again.

[jeremyfelt]

Hmm. We did talk about the back-compat of WP_Network's ID at WCEU. It is not used the same way as WP_Site right now, so I'm comfortable leaving it. Let's revert the WP_Site changes and go from there.

[jeremyfelt]

In 37923:

Multisite: Revert property type changes in WP_Site.

Partial revert of [37657]. Moving existing properties in WP_Site from public to private broke backwards compatibility in a pretty severe way. We made an initial attempt to work around this, but due to the variety of possible issues, moving forward does not seem wise.

Fixes #36717.

[westi]

Replying to jeremyfelt:

Replying to westi:

The changes in this ticket feel pretty unnecessary and fragile and likely to break a number of requests on any site using a caching backend because we are going to cache things the old code can't uncache.... in a nasty way - you end up with WP_Site instances that re-initialise out of the cache without a blog_id.

As of [37874], get_blog_details() should be in a position to repair itself if it encounters a WP_Site object without blog_id. Is that not working or part of what feels fragile?

The most obviously fragile bit is this makes it hard to test the new code, in a multiserver environment where production runs the old code ... the newer objects get cached and break production.

Which is going to bite anyone who runs a test upgrade of the new WP version on their (large) multisite install with object caching before deploying it.

[westi]

Replying to jeremyfelt:

In 37923:

Multisite: Revert property type changes in WP_Site.

Partial revert of [37657]. Moving existing properties in WP_Site from public to private broke backwards compatibility in a pretty severe way. We made an initial attempt to work around this, but due to the variety of possible issues, moving forward does not seem wise.

Fixes #36717.

Should we also revert the "back-compat" code added to get_blog_details?

[jeremyfelt]

Replying to westi:

Should we also revert the "back-compat" code added to get_blog_details?

Yes, I think this is the right idea. It now just adds some extra fluff.

[jeremyfelt]

In 37929:

Multisite: Revert [37874].

After [37923], get_blog_details() contained a now unnecessary attempt at back-compat for objects stored in cache.

See #36717.