WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#19353 closed defect (bug) (fixed)

WP_Screen::get tramples screen ids

Reported by: batmoo Owned by: nacin
Milestone: 3.3 Priority: high
Severity: major Version: 3.3
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

There are several instances where get_current_screen() / WP_Screen::get are trampling the screen id, which breaks things like meta boxes.

Here's some that I've noticed:

  1. if your post_type slug contains "-new" (e.g. "breaking-news") or "-add"
  2. if your plugin page name contains hyphens with a post type or taxonomy slug after the first hyphen (e.g. "my-page")

In the example of "breaking-news", the screen id gets changed to "breakings", which causes the "Publish" meta box to disappear.

There are likely others.

Attachments (2)

19353-untrample-new-add.diff (623 bytes) - added by batmoo 4 years ago.
19353.diff (1.9 KB) - added by nacin 4 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 @nacin4 years ago

  • Component changed from General to Administration
  • Milestone changed from Awaiting Review to 3.3
  • Owner set to nacin
  • Priority changed from normal to high
  • Status changed from new to accepted

comment:2 @nacin4 years ago

Any example code would be very helpful.

comment:3 @batmoo4 years ago

Attached patch should fix the first case.

comment:4 @batmoo4 years ago

Example code:

add_action( 'init', function() {
	register_post_type( 'breaking-news', array(
		'label' => 'Breaking News',
		'public' => true,
		'has_archive' => true,
		'supports' => array('title', 'editor'),
	) );
});

comment:5 @batmoo4 years ago

Sorry, the issue is with convert_to_screen (which calls WP_Screen::get) and not get_current_screen.

comment:6 @nacin4 years ago

There are two primary and distinct fixes here.

One, if $hook_name is a post type, then we should instantly go with that. This is primarily for compatibility with the $page argument of add_meta_box().

Two, we can be a lot more specific about the whole hyphen explode thing.

Incoming patch.

@nacin4 years ago

comment:7 @nacin4 years ago

The only remaining thing that I can obviously see would be a taxonomy or post type that ends in -new or -add, or starts with edit-. Unusual, but possible (value-add, for example). Also probably not much of a regression given the existing code in 3.2. Re-branching or duplicative code will be needed, but it's solvable.

comment:8 @nacin4 years ago

  • Keywords has-patch added

comment:9 @johnbillion4 years ago

  • Cc johnbillion@… added

comment:10 @nacin4 years ago

In [19468]:

Use a scalpel to dissect names passed to convert_to_screen(). Carefully watch for post type names, suffixes we need to detect and remove, and when we need to remove 'edit-' from the start of a hookname. see #19353.

comment:11 @nacin4 years ago

  • Keywords needs-unit-tests added

If a post type starts with 'edit-', it will be caught by the initial meta boxes block. post_type_exists( $hookname ) is valid and accounted for.

If a taxonomy starts with 'edit-', it will end up needing to be passed to convert_to_screen() as edit-edit-*. taxonomy_exists( $hookname ) is not valid — taxonomy_exists( 'edit-' . $hookname ) is.

Post types that end with -add or -new will be caught with the initial check. A taxonomy that ends with -add or -new *will* break. I think this is fixable by also confirming that ( 'edit-' != substr( $id, 0, 5 ) || ! taxonomy_exists( substr( $id, 5 ) ) ). Remember, a taxonomy hook will need to start with 'edit-'.

Unit tests for these edge cases would be nice. These changes passed all existing tests.

comment:12 @nacin4 years ago

  • Keywords needs-unit-tests removed

comment:13 @nacin4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In [19471]:

WP_Screen: Whitelist -new and -add screens, and prevent edit-comments and edit-tags from being mashed on. fixes #19353.

Note: See TracTickets for help on using tickets.