Make WordPress Core

Opened 13 years ago

Last modified 5 years ago

#18954 new enhancement

Automatically whitelist screen options added via add_screen_option()

Reported by: mdawaffe's profile mdawaffe Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Plugins Keywords: has-patch
Focuses: administration Cc:


set_screen_options() currently maintains a hardcoded whitelist of screens that can set options.

The list is extendable via the filter:

apply_filters('set-screen-option', false, $option, $value);

but that's sort of a pain. If a plugin calls add_screen_option(), it should be added to the whitelist automatically.

Also note that set_screen_options() is called so early, there's no convenient place for plugins to add to the set-screen-option filter. The best workaround I've been able to come up with is to manually call set_screen_options() again inside the page's load-$hook.

Example naive plugin code that does not work in <=3.3.

# Inside a menu page's load-$hook
// Display per-page settings in screen options
add_screen_option( 'per_page', array( 'label' => __( 'Things' ) ) );

Attachments (1)

18954.patch (3.5 KB) - added by A5hleyRich 9 years ago.

Download all attachments as: .zip

Change History (12)

#1 @nacin
13 years ago

  • Keywords 3.4-early added
  • Milestone changed from Awaiting Review to Future Release

We may be able to get away with killing this whitelist come 3.4.

#2 @WraithKenny
13 years ago

  • Cc Ken@… added

#3 @nacin
10 years ago

  • Component changed from General to Plugins

#4 @nacin
10 years ago

  • Component changed from Plugins to Admin APIs
  • Keywords needs-patch added; 3.4-early removed

#5 @nacin
10 years ago

  • Component changed from Admin APIs to Plugins
  • Focuses administration added

Sorry for the noise.

9 years ago

#6 @A5hleyRich
9 years ago

  • Keywords has-patch added; needs-patch removed

What's the purpose of the whitelist? Patch submitted with whitelist removed.

#7 @GunGeekATX
9 years ago

Would love to see this as well. Spent the last couple hours digging through core to implement custom screen options for a plugin. I had to bypass add_screen_option() entirely, use the screen_settings filter to create the HTML form fields, and use the check_admin_referer action to hook into saving the custom form fields since. The first call in set_screen_options() is check_admin_referer(), so hooking into the action called at the end of that function seemed to be the only way to get access to my fields before the redirect happened.

#8 @GunGeekATX
9 years ago

Also, we should be able to call add_screen_option() and have it either rendered by the render_per_page_options() function, or be able to pass a callback to render the option ourselves (probably a better solution).

Maybe a more encompassing solution would be similar to the settings API, with functions for add_screen_group(), register_screen_option(), etc.

#9 @kitchin
9 years ago

The filter for updating unknown options in misc.php,
$value = apply_filters( 'set-screen-option', false, $option, $value );
has a major flaw. It implies plugins should return false by default, rather than $value. If more than one plugin is hooking in, they will wipe out each other's options if they return false on any $option they don't recognize, which I've seen in several tutorials online.
The filter is followed by:

if ( false === $value ) return; // ...
update_user_meta($user->ID, $option, $value);

Because it's an early hook, plugins have to hook in universally, unless they parse $_GET['page'].

Conflicting goals:

  • Do not save unconfirmed data to the db.
  • Plugins should pass through data that does not belong to them.

I can write a patch to add a hook if people think it would be accepted:
$value = apply_filters( "set-screen-option_$option", false, $value );

Then maybe deprecate the old filter (can we do that before a redirect?), but in any case let the new filter override it.

#10 @kitchin
9 years ago

Also, I have a working sample plugin that explains the somewhat crazy hook stuff needed to work with Screen Options, here: Sorry for the bugspam.

#11 @kitchin
9 years ago

One more comment - I reconsidered and plugins are correct to send back $status (which is false) for unknown options. The correct way to prevent conflicts is for plugins to do

if ( ! empty( $_GET[ 'page' ] ) && $menu_slug === $_GET[ 'page' ] ) {
	add_filter( 'set-screen-option', ..., 10, 3);

And use the same $menu_slug in add_menu_page().

Still think my new filter would be the best solution, since it would prevent plugins going nuts on the input.

Note: See TracTickets for help on using tickets.