WordPress.org

Make WordPress Core

Opened 8 years ago

Last modified 2 years ago

#18088 reviewing defect (bug)

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

Reported by: kawauso Owned by: flixos90
Milestone: Future Release Priority: normal
Severity: normal Version: 3.2
Component: Networks and Sites Keywords: has-patch has-unit-tests early
Focuses: administration, multisite Cc:
PR Number:

Description

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.

Attachments (8)

18088.patch (496 bytes) - added by SergeyBiryukov 8 years ago.
18088.diff (1.7 KB) - added by flixos90 4 years ago.
fix for error notices not showing
18088.2.diff (2.2 KB) - added by flixos90 3 years ago.
18088.3.diff (2.2 KB) - added by flixos90 3 years ago.
18088.4.diff (2.3 KB) - added by iamfriendly 3 years ago.
Refreshed patch for 4.7.0
18088.5.diff (6.2 KB) - added by flixos90 3 years ago.
18088.6.diff (7.3 KB) - added by flixos90 3 years ago.
18088.7.diff (7.3 KB) - added by swissspidy 3 years ago.

Download all attachments as: .zip

Change History (51)

#1 @SergeyBiryukov
8 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.3

#2 @ryan
8 years ago

  • Milestone changed from 3.3 to Future Release

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.

#3 @DrewAPicture
8 years ago

  • Cc xoodrew@… added

#4 @jeremyfelt
6 years ago

  • Component changed from Network Admin to Networks and Sites
  • Focuses administration added

#5 @chriscct7
4 years ago

  • Keywords needs-refresh added

#6 @iamfriendly
4 years ago

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.

#7 @chriscct7
4 years ago

  • Keywords needs-refresh removed
  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from new to closed

#8 follow-up: @jeremyfelt
4 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone set to Future Release
  • Resolution worksforme deleted
  • Status changed from closed to reopened

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.

#9 @jeremyfelt
4 years ago

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.

#10 follow-up: @thomaswm
4 years ago

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.

#11 in reply to: ↑ 10 @flixos90
4 years ago

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
4 years ago

fix for error notices not showing

#12 in reply to: ↑ 8 @flixos90
4 years ago

  • Keywords has-patch added; needs-patch removed

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.

#13 @jeremyfelt
4 years ago

  • Milestone changed from Future Release to 4.6

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


3 years ago

@flixos90
3 years ago

#17 @flixos90
3 years ago

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

@flixos90
3 years ago

#18 @flixos90
3 years ago

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

#19 @flixos90
3 years ago

  • Keywords needs-testing added
  • Owner set to spacedmonkey
  • Status changed from reopened to reviewing

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


3 years ago

#21 @jeremyfelt
3 years ago

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.

#22 @jeremyfelt
3 years ago

  • Milestone changed from 4.6 to Future Release

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


3 years ago

#24 @flixos90
3 years ago

  • Milestone changed from Future Release to 4.7
  • Owner changed from spacedmonkey to jeremyfelt

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

@iamfriendly
3 years ago

Refreshed patch for 4.7.0

#25 @iamfriendly
3 years ago

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)

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


3 years ago

#27 @flixos90
3 years ago

  • Keywords needs-unit-tests added; needs-testing removed

@flixos90
3 years ago

#28 follow-up: @flixos90
3 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

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.

#29 in reply to: ↑ 28 ; follow-up: @jeremyfelt
3 years ago

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
3 years ago

#30 in reply to: ↑ 29 @flixos90
3 years ago

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.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 years ago

@swissspidy
3 years ago

#33 @swissspidy
3 years ago

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.

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by helen. View the logs.


3 years ago

#36 @jeremyfelt
3 years ago

  • Keywords early added
  • Milestone changed from 4.7 to Future Release

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 :)

#37 @flixos90
3 years ago

  • Keywords early removed
  • Milestone changed from Future Release to 4.8

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


3 years ago

#39 @flixos90
3 years ago

  • Keywords changed from has-patch, has-unit-tests to has-patch has-unit-tests
  • Owner changed from jeremyfelt to flixos90

#40 @jjcomack
3 years ago

Tested this and all seems well.

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


3 years ago

#42 @flixos90
3 years ago

  • Keywords early added
  • Milestone changed from 4.8 to Future Release

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

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


2 years ago

Note: See TracTickets for help on using tickets.