Make WordPress Core

Opened 12 years ago

Closed 11 years ago

#18559 closed enhancement (duplicate)

Set_screen_options() ignores many page names. add_menu_page() and friends breaks others. Etc.

Reported by: mick-p's profile Mick P. Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: General Keywords: dev-feedback
Focuses: Cc:

Description (last modified by SergeyBiryukov)

wp-admin/includes/misc.php (rev18349)

function set_screen_options() 

 if(!preg_match('/^[a-z_-]+$/',$option)) return;	

This gem breaks screen option support for screen names with non-matching characters.

If your screen name is generated from add_menu/submenu_page() and you go the slug route, you can make ?page= style pages which have characters such as / ... even though that is not very slug-like. Ironically if you choose a slug with a dash (slug-like) your page will not be accessible.

However if you want set_screen_options to work, you are then limited to a-z and underscore. I thought underscores were not allowed in slugs, but I might be wrong.

So unless you want to use underscore in your page url (which looks real bad for some reason) you are limited to single word page names.

The problem with single word names is they often collide. So I don't know if my "revisions" page might clobber some other "revisions" page for example. So I might want to stick in a / to create a kind of namespace, but no that won't work (see above)

Final thought. It's pure folly to be generating the screen names from the menu. Admins/users have every right to want to customize their menus at some point, and in doing so they would totally break the screen framework from the plugin POV. Many pages in the codex suggest sniffing out your screen name and hard-coding it BTW.

Screen names should be synonymous with page permalinks. Admin pages should ideally follow the same slug guidelines as blog pages, so if you wanted to you could rewrite them. Dashes should definitely be allowed in an admin page slug/screen name. And probably slashes should also be respected indicating a hierarchy of slugs.

Screen names should be purely based upon the page name. They should not incorporate the top-level menu. Even if you have A) the internal menu, and B) the reworked menu that is written to the page, it's confusing. I realize the menu system needs a lot of work, so please think about.

Finally, as it stands, this makes it frightening to code a plugin with screen options support. And makes it impossible to add screen option support to many plugins with more exotic page names. If you do so, then you are limited to more or less to page names which will likely collide with names that other plugins would like to use, or even WP itself might use if WP intends to use this framework in the future.

Change History (6)

#1 @SergeyBiryukov
12 years ago

  • Description modified (diff)

#3 @Mick P.
12 years ago

Good point. Numbers won't fly either.

Last edited 12 years ago by SergeyBiryukov (previous) (diff)

#4 @SergeyBiryukov
12 years ago

  • Keywords dev-feedback added

So we need sanitize_key() here?

Last edited 12 years ago by SergeyBiryukov (previous) (diff)

#5 @nacin
12 years ago

It allows underscores and hyphens, then converts hyphens to underscores for the update_user_meta() call.

#6 @nacin
11 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #18323.

Note: See TracTickets for help on using tickets.