Network Admin Email setting in wp-admin/network/settings.php fails silently

[kawauso]

Steps to reproduce:

1. Go to wp-admin/network/settings.php
2. Try setting your email to support@localhost
3. Page reports success but email remains the same

The check in wp-admin/network/edit.php consists of:

if ( is_email( $_POST['admin_email'] ) )
	update_site_option( 'admin_email', $_POST['admin_email'] );

which doesn't account for anything not passing is_email()

I know network admins should know what they're doing, but failing silently is a pretty sure-fire way to generate confusion.

[ryan]

settings.php needs quite a bit of work. Let's punt this from 3.3 and put in proper error notification as part of a general cleanup.

[iamfriendly]

This is now not the case as update_site_option() is called for all options without independent checks on the $_POSTed values. (The value is run through wp_unslash first).

Suggest we close this ticket.

[jeremyfelt]

I am very surprised, but this seems to still be valid. :)

In [21993], this chunk of code was moved so that it would be processed by sanitize_option(). While that changeset does not explicitly handle admin_email, code inside sanitize_option() does.

If an invalid email address is entered in the network admin email input box and the settings are saved, the network admin email does not change and the message at the top of the screen is green and says "Options saved." Lies!

sanitize_option() does store the error into add_settings_error(), but nothing on the network settings page does anything to present that to the user. I haven't dug any further than that.

[jeremyfelt]

See also #21546, aptly titled "Site Settings page is a hot mess".

I have no doubt all of this can be solved at once with a good looking hammer.

[thomaswm]

The current behaviour is even weirder than before: If you do not enter a valid email address, then the network admin email will be set to the email address of the administrator of the site with ID 1.

Steps to reproduce:

• Go to /wp-admin/network/settings.php
• Enter something in the email address field which is not an email address (for example "foo").
• Hit the save button.
• When the page has reloaded, this field will be filled with the value of admin_email from the site with ID 1.

The reason is:

• When the user saves the settings, WordPress will call update_site_option (see line 100 in settings.php) which then calls sanitize_option.
• When sanitize_option decides that the user did not provide a valid email address, it will call get_option( 'admin_email' ) and return that value (see line 3721 in formatting.php).
• This value will then be saved to the network options table by update_site_option.

[flixos90]

Replying to thomaswm:

• When the user saves the settings, WordPress will call update_site_option (see line 100 in settings.php) which then calls sanitize_option.
• When sanitize_option decides that the user did not provide a valid email address, it will call get_option( 'admin_email' ) and return that value (see line 3721 in formatting.php).
• This value will then be saved to the network options table by update_site_option.

This bug will be fixed with #35379 being completed.

[flixos90]

fix for error notices not showing

[flixos90]

Replying to jeremyfelt:

If an invalid email address is entered in the network admin email input box and the settings are saved, the network admin email does not change and the message at the top of the screen is green and says "Options saved." Lies!

sanitize_option() does store the error into add_settings_error(), but nothing on the network settings page does anything to present that to the user. I haven't dug any further than that.

The problem is that the settings errors are not stored in a transient and that the function to display settings errors is not called on wp-admin/network/settings.php. 18088.diff​ is a possible fix for this problem, basically making get_settings_errors() network-compatible and using it in the Network Settings screen.

[flixos90]

18088.2.diff​ is an updated patch that uses network-wide transients instead of regular ones (as appropriate here) and minor code tweaks.

[flixos90]

18088.3.diff​ is an updated patch which takes the changes from 37959 into account ("Options saved." is now "Settings saved.").

[jeremyfelt]

Thanks for the patches @flixos90. I'm going to push this to a future release. We should have a bigger conversation around network settings and how we're approaching this. Let's do that in 4.7.

[flixos90]

Moving to milestone for consideration. The patch should work well, but needs minor updates, discussion and review.

[iamfriendly]

Refreshed patch for 4.7.0

[iamfriendly]

Added minor updates to patch as needed for 4.7.0 inclusion. This is an iterative change as part of a larger discussion on site settings. (For my own benefit - this would be a good example for a Behat test proving useful where unit tests would prove tricky)

[flixos90]

18088.5.diff​ adds unit tests for get_settings_errors() (which didn't exist before). The new functionality is also included in one of the tests.

[jeremyfelt]

Replying to flixos90:

18088.5.diff​ adds unit tests for get_settings_errors() (which didn't exist before). The new functionality is also included in one of the tests.

Nice and creative, @flixos90. This is looking doable. :)

A couple notes:

Globals in tests are terrifying. If not managed properly they will bleed into other tests and cause strange bugs. In tests like these, make sure that the globals modified in the test are set back to their original value before the assertion is run. If the assertion was to fail, the remainder of the function would not run and the global would be stuck as is. This could cause false positives or negatives in the tests that run after the failed assertion.

function test_result() {
    $_old_global = $global_data;
    $result = get_result();
    $global_data = $_old_global;

    $this->assertEquals( 'expected', $result );
}

With that same concept in mind, it's also better to split these tests up so that only one (or as few as possible) assertions run in each. If the first assertion was to fail in test_get_settings_errors(), the next 2 would not run. This could cause other issues or could just mask a full picture of the result.

test_get_settings_errors() here could be split into 3 methods:

• test_get_settings_errors_retrieve_single_error_code()
• test_get_settings_errors_retrieve_all_error_codes()
• test_get_settings_errors_retrieve_invalid_error_code()

From time to time it probably doesn't make sense to split up the tests, but it's a good approach to start with. (IMO)

[flixos90]

Replying to jeremyfelt:

In tests like these, make sure that the globals modified in the test are set back to their original value before the assertion is run. If the assertion was to fail, the remainder of the function would not run and the global would be stuck as is. This could cause false positives or negatives in the tests that run after the failed assertion.

Good point, I didn't think about this! In 18088.6.diff​ I made sure that the globals and GET params get reset before running each assertion. I also split the one test method so that each test contains one assertion and used more descriptive method names.

[swissspidy]

18088.7.diff​ uses assertEqualSets() in the tests.

We should at least get the tests in for now, as they're separate from the actual fix.

[jeremyfelt]

I'm going to push this off once more. I think we can still get tests in as a start, but that can happen while the ticket is in Future Release. Early 4.8 :)

[jjcomack]

Tested this and all seems well.

[flixos90]

Let's come back to this early-4.9 cycle.