WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 years ago

#35305 closed defect (bug) (fixed)

do_meta_boxes() does not display correctly with screen names that contain charaters outside sanatize_key()'s limits

Reported by: GregRoss Owned by: DrewAPicture
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.4
Component: Options, Meta APIs Keywords: has-patch
Focuses: docs Cc:

Description

Specifically the "Screen Options" tab is not displayed.

This is because WP_Screen sanitizes the slug where as add_meta_box() does not, resulting in each looking for a different key in the $wp_meta_boxes global.

When WP_Screen->show_screen_options() checks the list of meta boxes for the screen it does not find any and does not display the "Screen Options" tab as it looks for the sanitized slug but the unsanitized slug was to populate the global.

To make matters worse, add_menu_page()'s default behaviour is to return a screen name in the format of "toplevel_page_" + plugin_base(), which will include a / between the directory name and the file name of the plugin's main php.

There are a few options on what to do to resolve this:

  1. Change the call to sanitize_key() to something else to support more characters in the WP_Screen->id field.
  1. Change add_menu_page() (and probably add_submenu_page as well) to use the same logic and call sanitize_key() on the slug before it is returned.
  1. Update the documentation and note the incompatibility but leave it be.

Option 1 and 2 both have significant impact on existing code and plugins.

In option 1 any code that relies on the slug being safe to use in SQL or other places would have to be reviewed.

In option 2 any code the has hard coded a menu slug based on the current behaviour would have to be updated to use the new slug that is returned by the code.

Option 3 is probably the safest :)

Attachments (1)

35305.patch (4.0 KB) - added by GregRoss 4 years ago.
Update doc blocks.

Download all attachments as: .zip

Change History (7)

#1 @MikeHansenMe
4 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

#2 @MikeHansenMe
4 years ago

@GregRoss do you want to see if there is a way to write a patch that takes back-compat into consideration?

#3 @GregRoss
4 years ago

@MikeHansenMe I doubt the effort to try and resolve this through code is worth it.

Option 1 is easy enough to do, just remove the sanitize_key() call in WP_Screen object and it works, however I'm sure there must be a reason for it to be there so likely other things will break. Replacing sanitize_key() with something more permissive would only be a partial solution so not a very good option.

Option 2 could break a lot of plugins for a corner case with do_meta_boxes(). That seems like a bad tradeoff.

I think the best solution is to update the docs for both add_menu_page()/add_submenu_page() and do_meta_boxes()/add_meta_box() to indicate the proper format for the menu_slug/screen_id.

I've attached a patch which updates the doc blocks to reflect this.

@GregRoss
4 years ago

Update doc blocks.

#4 @DrewAPicture
4 years ago

  • Focuses docs added
  • Keywords has-patch added; needs-patch needs-unit-tests removed

#5 @DrewAPicture
3 years ago

  • Milestone changed from Future Release to 4.9

#6 @DrewAPicture
3 years ago

  • Owner set to DrewAPicture
  • Resolution set to fixed
  • Status changed from new to closed

In 40967:

Docs: Provide best practice guidance for achieving parity between $menu_slug values supplied when adding menu and submenu pages, and later trying to compare those initial values against sanitized screen IDs derived from $menu_slug.

At the heart of the matter, the $menu_slug parameter in add_menu_page() and add_submenu_page() is not sanitized with sanitize_key(). When the screen object is later built for the admin page, the screen ID is derived from that $menu_slug value, though passed through sanitize_key(), which can produce unexpected results in comparison check.

Changing the sanitization code to provide actual parity is out of the question at this juncture, so updating the docs to describe how to avoid this edge case is the next best option.

Props GregRoss.
Fixes #35305.

Note: See TracTickets for help on using tickets.