update_network_option changes stored option value if sanitize_option detects error

[thomaswm]

If you make a call like

update_network_option( null, "admin_email", "this.is.not.an.email.address" );

then the value of the admin_email network option will be changed to the value of the admin_email site option in the database. Expected behaviour would be that the option value in the database is not changed.

The reason is that update_network_option calls

sanitize_option( "admin_email", "this.is.not.an.email.address" );

and writes its return value to the database. However, sanitize_option will return the value of get_option( "admin_email" ), instead of get_network_option( null, "admin_email" ). See lines 3863ff. in formatting.php.

[jeremyfelt]

Confirmed.

It looks like this was introduced in [14231] not too long after update_site_option() was made multisite aware. It originally affected admin_email and siteurl. In [18346], WPLANG got similar treatment. And as of [32791], some of the fallback logic was consolidated and it now appears to affect illegal_names as well. This applies to several other single site options, but not all of them share names with network options.

I guess our two options are to add a sanitize_network_option() or add context to sanitize_option(). I'm inclined to go the route of sanitize_network_option() so that we can also account for the other network options that do not match the names of site options.

[codex-m]

I manage a multisite and came across this issue. My attempt to solve this in my local multisite is passing a new parameter for sanitize_option. This parameter is the network ID which can be an integer when using for a specific network in a multisite. Or it can be TRUE to use the current network. By default, it's false for non-multisite implementations. See attached 35379.diff.

I wrote some unit for test (see attached 35379_unit_test.diff). Covering three network affected options (admin_email, siteurl and WPLANG). This test also detects the issue (if the patch is not applied and confirms the fix when patch is applied.

In addition, I ran the overall unit test before and after the patch and I don't see any side effects on this. Feel free to review ;) Thanks a lot!

[codex-m]

Patch

[codex-m]

Unit test

[codex-m]

Refreshed version of the patch with latest changes on trunk

[codex-m]

Refreshed version of the unit test.

[codex-m]

I refreshed the version of the patch with latest changes on trunk/unit tests (includes code cleanup. :)

[chriscct7]

Punting per discussion in Slack

[jeremyfelt]

Thank you for your patches on this ticket, @codex-m. Can you take a look at the work in progress on #35698? That has a slightly wider focus, but should help to resolve this one as well.

[flixos90]

When merging #35698 into this ticket, we should probably have sanitize_option() use the $network_id as third parameter (as it provides more value and is needed for this ticket) and then automatically detect the $context variable inside the function like

$context = $network_id ? 'network' : 'site';

Alternatively we could add both as a parameter, $context as third parameter and then $network_id as fourth. We might even name it $context_id then as this would provide flexibility for possible other contexts in the future, also for the regular option context.

Examples: A function like get_option() could then call it as

sanitize_option( $option, $value, 'site', get_current_blog_id() )

while get_network_option() would call it as

sanitize_option( $option, $value, 'network', $network_id )

[codex-m]

Updated patch -merged version ($context and $context_id illustrated)

[codex-m]

Thanks @flixos90 / @jeremyfelt for the update. I merged the final patch from this ticket #35698. Attached is the final patch (35379.3.diff) which is now a combination of the two patches. Basically it now illustrates the concept of adding $context which will have a value of 'network' for multisite implementation. And also the $context_id which will have the value of network ID when used with multisite. As suggested, I illustrated $context_id instead of $network_id for a more flexible implementation.

I re-tested this manually and also using our automated unit testing (including the attached unit tests previously). This is working great and fixes the original issue being reported in this ticket. The unit test to cover these changes is 35379_unit_test.2.diff (previously attached).

Let me know what you think :)

[flixos90]

@codex-m The patch looks great I think. One thing that could be simplified in my opinion are lines 3654-3668 (initial check for $context_id in network mode). It's probably not needed to add a new variable $network_id there, we should be able to use the original $context_id throughout the entire function. Maybe something like this:

<?php
if ( 'network' === $context ) {
    if ( ! is_multisite() ) {
        $context = 'site'; // or we could throw a _doing_it_wrong() here
    } elseif ( ! $context_id ) {
        global $current_site;
        
        $context_id = $current_site->id;
    }
}

[codex-m]

Refreshed version of patch simplified

[codex-m]

@flixos90 Great, I agree :) I attached the refreshed and simplified version of the patch (35379.4.diff). Re-tested with 35379_unit_test.2.diff​ as well as some manual testing. Everything works good and the original issue is resolved. Thanks for the feedback. Cheers.

[jeremyfelt]

I gave another read through sanitize_option() and still find myself falling in favor of a new sanitize_network_option(). We would still need to add the old filter, but we could drop all of the site specific option switching and just focus on the network. This would also eliminate the need to pass both $context and $context_id.

Thoughts?

[flixos90]

Replying to jeremyfelt:

I gave another read through sanitize_option() and still find myself falling in favor of a new sanitize_network_option(). We would still need to add the old filter, but we could drop all of the site specific option switching and just focus on the network. This would also eliminate the need to pass both $context and $context_id.

Thoughts?

I'd prefer to use one function for this cause. Especially with a look at #15691, a Settings API for the network admin should work in a similar manner like the regular one. There is too much parity between site vs network options to create separate functions. I don't see the point in writing an additional function for so little change in functionality. I think we should generally go with an additional $context (and when needed $context_id) parameter to support existing functionality for networks.

What we could of course do is create a sanitize_network_option() function which would call the other function, automatically passing the network context. It would make it a little more intuitive to use, but not sure if this is needed either.

[codex-m]

Replying to jeremyfelt:

I gave another read through sanitize_option() and still find myself falling in favor of a new sanitize_network_option(). We would still need to add the old filter, but we could drop all of the site specific option switching and just focus on the network. This would also eliminate the need to pass both $context and $context_id.

Thoughts?

Like @flixos90 I prefer to use only one function instead of using sanitize_network_option. It is because options in WordPress are dual-purpose and can be used both (single-site or multisite). Options name in single-site is the same as in the network and validated similarly. Using one function in this case would be much more user-friendly to me.

By using only one function and passing appropriate values in $context and $context_id, we can easily process these options in a network mode (even though these options are still usable on a single site mode and validated the same.).

Cheers.

[flixos90]

#35698 was marked as a duplicate.

[codex-m]

sanitize_network_option function

[codex-m]

@jeremyfelt Here is my patch illustrating the use sanitize_network_option. It's now possible to sanitize network-specific options by putting them on their own function.

Since several of the WordPress default core options are used to both single-site and multisite-mode. I placed them in a new validating function called wp_validate_default_options which is called by both sanitize_option' and sanitize_network_option`. I implemented similar filters for consistency between single-site mode and multisite usage.

I tested this using 35379_unit_test.2.diff test to cover this issue as well as standard WordPress unit tests. Works fine. Let me know what you think ;) Cheers.

[flixos90]

Replying to codex-m:

@jeremyfelt Here is my patch illustrating the use sanitize_network_option. It's now possible to sanitize network-specific options by putting them on their own function.

I'm personally still in favor of the approach with one function that handles it. If we need sanitize_network_option(), we could make it a wrapper for that function. But it's great to get an example of what the alternate approach would look like, so now we have a better base to discuss.

I think that, even if we go with the approach of using separate functions, we should start with the initial idea. Your new patch introduces completely new behavior which would definitely take some additional time to review.

My proposal would be to follow the approach of having one flexible function for sanitization and aim for a 4.6 merge with that - and then possibly, in a follow-up ticket, continue to think about ways to split up these function into separate. I don't see any backwards-compatibility issues in coming up with outsourcing parts of the one function-approach / wrapping it in a sanitize_network_option() at a later point.

A minor note to your patch: I think we would still need the original sanitize_option filter to be present in the sanitize_network_option() function (for backwards compatibility, see https://core.trac.wordpress.org/ticket/35698#comment:2).

[flixos90]

We need to think further on what the implementation of this ticket will look like. Especially given a possible #37181, we could improve the way we handle network options a lot (think register_meta() here) - although it's a pain in terms of naming conventions.

[flixos90]

Moving to milestone for consideration. It has a solid patch that would only require minor updates now, but we still need to find out whether this is the route to take.

[flixos90]

Punting to a future release. As this is a blocker for a possible network settings API, I hope we can get to it in 4.8.