Make WordPress Core

Opened 10 years ago

Closed 9 years ago

#28672 closed enhancement (wontfix)

add_settings_field & co don't return values

Reported by: tjnowell's profile TJNowell Owned by: obenland's profile obenland
Milestone: Priority: normal
Severity: normal Version: 2.7
Component: Options, Meta APIs Keywords: needs-unit-tests has-patch
Focuses: administration Cc:

Description

When working with the current settings API it's easy to muddle up the arguments or miss something out.

When this happens, there are no fields on the frontend, but aside from that, there is no error, no warning, or indication of why the fields aren't showing.

Currently the function add_settings_field and similar functions don't return a false or error value if they've been misused, e.g. adding a setting field to a section that doesn't exist. If it returned a WP_Error saying so, it would be a lot easier to debug the API

Attachments (4)

28672.diff (2.0 KB) - added by GunGeekATX 10 years ago.
Proposed patch
ticket-28672-samples.php (3.4 KB) - added by GunGeekATX 10 years ago.
Sample plugin showing functionality of patch
28672.2.diff (2.0 KB) - added by GunGeekATX 9 years ago.
Refreshed patch
28672.3.diff (1.2 KB) - added by DrewAPicture 9 years ago.
_doing_it_wrong()

Download all attachments as: .zip

Change History (29)

#1 @nacin
10 years ago

  • Version changed from trunk to 2.7

#2 @boonebgorges
10 years ago

  • Keywords needs-unit-tests good-first-bug added

#3 @GunGeekATX
10 years ago

Something like this in add_settings_field()?

global $wp_settings_sections;

if ( ! isset( $wp_settings_sections[$page] ) )
	$settings_page_check = new WP_Error( 'invalid_settings_page', sprintf( __('Settings page %s does not exist.'), $page ) );
else
	$settings_page_check = true;

$wp_settings_fields[$page][$section][$id] = array('id' => $id, 'title' => $title, 'callback' => $callback, 'args' => $args);

return $settings_page_check;

Last edited 10 years ago by GunGeekATX (previous) (diff)

#4 @McGuive7
10 years ago

In addition to checking if the settings section exists, what else might we want to check for? Missing required parameters? Anything else?

Seems like it would be good to knock this out for all of the following:

add_settings_section()
add_settings_field()
do_settings_sections()
do_settings_fields()

Any others that belong in this group?

#5 @GunGeekATX
10 years ago

I'm not sure it would specifically work for add_settings_section(). It looks like you can add sections to a page even if the page isn't already defined.

do_settings_sections() and do_settings_fields() return nothing if the page or section hasn't been defined, so it might not hurt to have those return WP_Error as well.

This is what do_settings_sections() returns now.

if ( ! isset( $wp_settings_sections[$page] ) )
	return;

If you're working with the settings API, I can see where a typo in your code would make debugging such an issue a bit harder. This would be something you could check for:

// updated do_settings_sections()
if ( ! isset( $wp_settings_sections[$page] ) )
	return new WP_Error( 'undefined_settings_page', sprintf( __('Settings page %s has not been defined.'), $page ) );

// example from a plugin/theme
$section_check = do_settings_section( 'wordcampp' );
if ( is_wp_error( $section_check )
	wp_die( $section_check->get_error_message() );

I can tackle a patch for this if it sounds like something that should be in core.

#6 @GunGeekATX
10 years ago

I stand corrected. It looks like this would work in add_settings_section()

global $_registered_pages;
if ( !isset( $_registered_pages[$page] ) )
	// return wp_error here

#7 @mattshaw
10 years ago

Just wanted to mention in case this had not been considered, fixing this could break a LOT of existing plugins that are using fake pages for registering with settings sections (especially for tabbed settings pages).

I tested some of the fixes mentioned above, the most noticeable error was it broke with BuddyPress, but any other plugins that are using 'fake' settings pages will need to be updated.

#8 follow-up: @boonebgorges
10 years ago

mattshaw - Could you give additional information about the breakage that you're seeing in BuddyPress and elsewhere? Is it the WP_Error return value?

#9 in reply to: ↑ 8 @mattshaw
10 years ago

Replying to boonebgorges:

mattshaw - Could you give additional information about the breakage that you're seeing in BuddyPress and elsewhere? Is it the WP_Error return value?

Sure, I noticed the issue occurred when returning a WP_Error during add_settings_section for an unregistered page, using the fix above mentioned by GunGeekATX:

function add_settings_section($id, $title, $callback, $page) {
	global $wp_settings_sections, $_registered_pages;
	if ( !isset( $_registered_pages[$page] ) ) {
		return new WP_Error( 'undefined_settings_page', sprintf( __('Settings page %s has not been defined.'), $page ) );
	}
...

It removed the BuddyPress main settings tab ( admin.php?page=bp-settings ), along with the settings pages for a few other plugins as well.

#10 follow-up: @GunGeekATX
10 years ago

Yeah, my fix should only be returning a true or a WP_Error at the end of the function and not exiting sooner if it does not find the page. That will prevent it from breaking any existing plugins.

#11 in reply to: ↑ 10 @mattshaw
10 years ago

Replying to GunGeekATX:

Yeah, my fix should only be returning a true or a WP_Error at the end of the function and not exiting sooner if it does not find the page. That will prevent it from breaking any existing plugins.

My mistake, sorry for the confusion!

#12 @GunGeekATX
10 years ago

Been digging into this more and it's going to be a little more complex than just checking $_registered_pages.

Settings sections and fields can be added to existing pages (general, media, reading, etc) and those don't show up in $_registered_pages. New pages can be added via add_menu_page() and add_submenu_page(), and those do show up in $_registered_pages, but the format is not as simple as the page name that gets passed to do_settings_sections().

Some examples from $_registered_pages while testing locally; adding a top-level page, adding a sub-menu page to the top-level page, and adding a sub-menu page to the Dashboard menu:

array(6) {
  ["toplevel_page_new-menu-page"]=>
  bool(true)
  ["new-menu-page_page_new-submenu-page"]=>
  bool(true)
  ["dashboard_page_new-dashboard-page"]=>
  bool(true)
  ["appearance_page_custom-header"]=>
  bool(true)
  ["appearance_page_custom-background"]=>
  bool(true)
  ["appearance_page_theme-editor"]=>
  bool(true)
}

However, it may be as simple as checking to see if the page is one of the built-in pages (general, media, etc), or if any of the keys in $_registered_pages end with the page. So far, the following addition to add_settings_section() seems to work:

// check to see if the page is one of the built-in pages
$settings_page_check = in_array( $page, array( 'discussion', 'general', 'media', 'permalink', 'reading', 'writing' ) );

// if it's not a built-in page, check to see if the page matches the end of any of the keys for the registered pages
if ( ! $settings_page_check ) {
	foreach ( array_keys( $_registered_pages ) as $registerd_page_name ) {
		if ( substr_compare( $registerd_page_name, $page, strlen( $registerd_page_name ) - strlen( $page ), strlen( $page ) ) === 0 ) {
			$settings_page_check = true;
			break;
		}
	}
}

$wp_settings_sections[$page][$id] = array('id' => $id, 'title' => $title, 'callback' => $callback);

if ( ! $settings_page_check )
	return new WP_Error( 'invalid_settings_page', sprintf( __('Settings page %s does not exist.'), $page ) );
else
	return $settings_page_check;

Is this something we'd want to make new functions for, settings_page_exists() (and settings_section_exists())?

#13 @GunGeekATX
10 years ago

Thought about it some more and it may not be necessary to check for a registered page in add_settings_section(). The menu slug has no correspondence to the do_settings_sections() call.

However, add_settings_field() should do this check for a either a registered settings section or one of the built-in pages and sections (such as do_settings_fields('media', 'uploads')). I'll work on a patch for this.

@GunGeekATX
10 years ago

Proposed patch

@GunGeekATX
10 years ago

Sample plugin showing functionality of patch

#14 @GunGeekATX
10 years ago

  • Keywords has-patch added

Uploaded a patch for add_settings_field() this that checks the built-in pages and sections, i.e. do_settings_fields('general', 'default'); and then checks for sections added with add_settings_section(). Also included a sample plugin that has examples of using the new functionality.

#15 @DrewAPicture
10 years ago

  • Owner set to GunGeekATX
  • Status changed from new to assigned

#16 @wonderboymusic
9 years ago

  • Keywords needs-refresh added; has-patch removed

@GunGeekATX
9 years ago

Refreshed patch

#17 @GunGeekATX
9 years ago

  • Keywords has-patch added; needs-refresh removed

Patch refreshed, updated some of the code formatting as well

#18 @obenland
9 years ago

What about just throwing a _doing_it_wrong() notice if the section has not been defined?

#19 follow-up: @GunGeekATX
9 years ago

That would probably work, though anyone running with WP_DEBUG set to true would start to get notices.

#20 in reply to: ↑ 19 @obenland
9 years ago

Replying to GunGeekATX:

That would probably work, though anyone running with WP_DEBUG set to true would start to get notices.

Correct, that's the purpose of calling that function. It gives people the chance to correct their code.

#21 @obenland
9 years ago

  • Keywords good-first-bug removed

As @GunGeekATX pointed out earlier, add_settings_section() is designed to register both $page and $section. The way core just calls do_settings_section() without registering its sections first suggests that this is very much the way the API is designed, and it would also apply to add_settings_field().

If we decide to be stricter in that regard and require registering sections before being able to add fields to it, rather than accounting for core's oddity here we should register core's sections before we call do_settings_section() on them. In that case we'd also have to figure out how to deal with the 'default' section.

#22 follow-up: @obenland
9 years ago

The more I think about it the more I feel like we should leave it as is. This will probably cause more harm than good.

#23 in reply to: ↑ 22 @DrewAPicture
9 years ago

  • Milestone changed from Awaiting Review to 4.4
  • Owner changed from GunGeekATX to obenland
  • Status changed from assigned to reviewing

Replying to obenland:

The more I think about it the more I feel like we should leave it as is. This will probably cause more harm than good.

I agree, in some respects. I think failing silently is kind of a feature, not a bug.

At the same time, we can either continue to fail silently or throw a _doing_it_wrong() if the $page doesn't exist when do_settings_sections() is called and another one if $section isn't defined in do_settings_fields() would suffice.

As you said in comment:20, it gives developers the opportunity to fix their code while providing meaningful feedback as to why it isn't working.

This is done in 28672.3.diff.

@DrewAPicture
9 years ago

_doing_it_wrong()

#24 follow-up: @obenland
9 years ago

  • Keywords close added

The only time we fail silently is when do_settings_*() calls something different than what is registered. In all other cases add_settings_*() will register the page, section, and/or field and everything is peachy.

With 28672.3.diff we'd generate doing it wrong notices on the core implementation and every time a field relies on the default section, which is not good. We don't have a function to register a settings page, so we can't really throw a doing it wrong when a page is not registered. Pages are registered with the section or field that are registered to it.

Which is why I'm saying we should probably not do anything at all here. We wouldn't be making things better.

#25 in reply to: ↑ 24 @DrewAPicture
9 years ago

  • Keywords close removed
  • Milestone 4.4 deleted
  • Resolution set to wontfix
  • Status changed from reviewing to closed

Replying to obenland:

With 28672.3.diff we'd generate doing it wrong notices on the core implementation and every time a field relies on the default section, which is not good. We don't have a function to register a settings page, so we can't really throw a doing it wrong when a page is not registered. Pages are registered with the section or field that are registered to it.

Which is why I'm saying we should probably not do anything at all here. We wouldn't be making things better.

I see now what you're getting at now. Yeah, I'm with you on not doing anything here. Closing as wontfix.

Note: See TracTickets for help on using tickets.