Opened 10 years ago
Closed 9 years ago
#28672 closed enhancement (wontfix)
add_settings_field & co don't return values
Reported by: | TJNowell | Owned by: | 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)
Change History (29)
#4
@
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
@
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
@
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
@
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:
↓ 9
@
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
@
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:
↓ 11
@
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
@
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
@
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
@
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.
#14
@
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.
#17
@
9 years ago
- Keywords has-patch added; needs-refresh removed
Patch refreshed, updated some of the code formatting as well
#18
@
9 years ago
What about just throwing a _doing_it_wrong()
notice if the section has not been defined?
#19
follow-up:
↓ 20
@
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
@
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
@
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:
↓ 23
@
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
@
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.
#24
follow-up:
↓ 25
@
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
@
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.
Something like this in add_settings_field()?