Ensure a network's `notoptions` cache is an array to reduce lookup overhead

[flixos90]

In every call to get_network_option() the global {$network_id}:notoptions cache is requested. However the function only sets a value for that cache if both the individual network option's cache lookup and its database lookup failed failed, which in most cases will (and should) not be the case.

This isn't a significant problem, but when using an external object cache, this causes that cache to be called every time get_network_option() is called. If instead get_network_option() ensured an empty array is set on a cache miss, that would only happen once per request, as the local cache would afterwards have that value stored.

It's admittedly a very minor performance improvement, but I think it's worth it, as there should be no downside to it.

[flixos90]

Related: #31147

[flixos90]

43506.diff​ fixes this by always setting the {$network_id}:notoptions cache key in get_network_option() in case it is not an array. This ensures that on subsequent requests the value is stored in the (local) cache.

Two tests have been added to verify the correct behavior.

[spacedmonkey]

I don’t think this is the way we should go with this. At WordCamp US we agreed to go ahead with using the meta api for these functions. See #37181 . Funny enough, the main reason for me to move ahead with using the meta api, is performance. This ticket is no longer an issue once we switch to the meta api as there is no use of the notoption cache key.

Lets close this ticket out, and go ahead with #37181 as agreed.

[flixos90]

@spacedmonkey There's still not a 100% agreement if we should move over to using the meta API, so this fix here should go in regardless IMO, since it might take a bit longer for a decision regarding the meta API. It doesn't do any harm, it just fixes a minor issue.

[spacedmonkey]

@flixos90 At WordCamp US, the meeting of the multisite, team we all agreed that moving to the moving the meta api for network options was a good idea. What we didn't agree to was having the meta function, like get_network_meta in core. This doesn't stop us as from going ahead with #37181 , which does not functionality change anything and can be seen as a simple refactor of the code.

If you spend your time on this ticket, you will have to write unit tests which then will be removed once #37181 goes in. It feels like a massive waste of time. Why not use that time to work on #37181 , write a unit test, that proves that if issue is not a there in that.

If this goes in, then it will delay #37181 , blocking it's release in 5.0.0.

[flixos90]

Replying to spacedmonkey:

If you spend your time on this ticket, you will have to write unit tests which then will be removed once #37181 goes in. It feels like a massive waste of time. Why not use that time to work on #37181 , write a unit test, that proves that if issue is not a there in that.

There are two tests already that should be sufficient for this, and it wasn't much work at all to write them.

If this goes in, then it will delay #37181 , blocking it's release in 5.0.0.

Why is it blocking that? It just fixes a small issue with what we have now. Migrating to the meta API is not affected by this change at all.

[flixos90]

In 42833:

Multisite: Ensure the {$network_id}:notoptions array is set in cache in get_network_option().

Prior to this change, the {$network_id}:notoptions cache would only be fetched, but not set, unless the actual database lookup would be unsuccessful. This enhancement slightly improves performance by preventing unnecessary external object cache lookups if one is used.

Fixes #43506.

[ocean90]

[42833] doesn't seem to work as expected, see ​https://travis-ci.org/WordPress/wordpress-develop/builds/352899721.

[flixos90]

In 42834:

Multisite: Add missing group annotations to tests included in [42833].

This ensures tests are skipped correctly when not using multisite.

See #43506.

[flixos90]

Fixed via the above commit.