CPT identifier class name missing from #icon-edit DIV

[fjarrett]

The #icon-edit DIV should identify the current Custom Post Type with a unique class name so the icon can be changed via CSS.

Example: icon32-posts-foobar

Where foobar is the CPT slug.

The unique class is working on edit.php but is missing from post.php and post-new.php views.

[nacin]

Cannot reproduce. I see icon32-posts-foobar on post.php, post-new.php, and edit.php for the foobar post type.

This is potentially a plugin conflict, whereby one is mucking with $current_screen->post_type. If that's the case, then the conflict was caused by my work in #18785.

[fjarrett]

No plugins activated in my original test environment.

Setup a second test environment running a separate CPT-capable theme, no plugins activated. Same issue.

Running 3.3 beta-2-19067 on both.

[dd32]

Can you please post a link to the theme(s) which you're using to test it with?

[fjarrett]

Have tested these themes, with the same result:

​http://www.woothemes.com/2011/07/empire

​http://churchthemes.net/themes/urban-city (free version)

Just tested this one and it seems to work:

​http://wordpress.org/extend/themes/star

The two that aren't working are using the menu_icon argument in register_post_type, Star is not. Could be the culprit?

[SergeyBiryukov]

Replying to fjarrett:

​http://churchthemes.net/themes/urban-city (free version)

I've just checked Urban City Free Edition, and I see icon32-posts-slide class on "Slides" (edit.php?post_type=slide) and "Slide Tags" (edit-tags.php?taxonomy=slide_tag&post_type=slide) screens, but not on "Add New Slide" (post-new.php?post_type=slide) and "Slide Settings" (edit.php?post_type=slide&page=slide-settings) screen.

The reason seems to be the same as in #19077 ($current_screen->post_type is empty), and the patch there makes the custom icon appear.

"Reorder Slides" screen is displayed by the theme itself, and only icon32 class is hardcoded there.

[fjarrett]

@sergey

What about post.php? Publish a new slide and then go back and edit it.

There is not a post_type query string in that view.

[SergeyBiryukov]

Replying to fjarrett:

What about post.php?

Indeed. In 3.2, $current_screen->post_type was set there from $post->post_type:
​http://core.trac.wordpress.org/browser/tags/3.2.1/wp-admin/post.php#L29

In 3.3, WP_Screen::get() is called twice (in this case). First, via add_meta_box() (without $post_type set). Second, via set_current_screen(). $post_type is set then, but the screen is already in the registry, so it returns just before setting $screen->post_type, leaving it empty again:
​http://core.trac.wordpress.org/browser/trunk/wp-admin/includes/screen.php#L501

[SergeyBiryukov]

WP_Screen::get() called from add_meta_box():

WP_Screen Object( [base] => slide, [id] => slide, [post_type] => , ... )

Second time, called from set_current_screen() (with premature return commented out):

WP_Screen Object( [base] => post, [id] => slide, [post_type] => slide, ... )

[SergeyBiryukov]

19080.patch​ is an attempt to store a screen in the registry using both $base and $id if they differ.

[nacin]

The issue is actually [19013]. We can't call convert_to_screen() for an arbitrary post type (or other screen that contains meta boxes), as it only handles hook names. A simple post type like 'slide' results in a terribly mangled WP_Screen.

I've thought about a hash for WP_Screen but I didn't find it necessary at the time. Though I do see some other reasons why this should happen now. In part because I assumed that set_current_screen() would always be the first WP_Screen, which obviously was an incorrect assumption.

If a meta box is a string, we may be able to get away with assuming it is going to be the id. It certainly works for post types and links, at the very least. I'll investigate further.

[nacin]

In [19089]:

Require show_ui rather than public for a taxonomy's parent post type. see #19080.

[nacin]

I've dug really deep into this issue at this point.

I noticed that the Slide post type is registered show_ui = true and public = false. The problem is that the taxonomy attached to it then results in $post_type = 'post', and then the menu gets all screwed up because $parent_file is 'edit.php' rather than 'edit.php?post_type=slide'. Thus [19089].

[nacin]

19080.diff​ takes a few different approaches here.

First, until a WP_Screen object is the current screen, it allows future WP_Screen::get() calls to augment the screen object properties that are already in the registry. If the current screen is what's in the registry, we can assume that as much as possible was gleaned from the global state of the page (hook_suffix, GET/POST/REQUEST).

I find it clever but I wouldn't be surprised if someone else can poke a hole in the approach.

Second, and separately, it introduces WP_Screen::for_meta_box(). This is then used in functions that previously could have accepted a post type. Better data is then passed to WP_Screen::get() to allow it to realize it is dealing with a post type. It should be noted that for other meta box "$page" values, "comment," "link," and "nav-menus," are all proper id *and* base values for those screens. We only need to trap the post type.

I think only the second approach fixes the bug here, but the first could mitigate future issues.

[fjarrett]

@nacin

Just confirming that this patch is working great for me.

[nacin]

19080.2.diff​ introduces two other changes. One, $typenow and $taxnow will only be set to the screen's post_type and taxonomy if the base is 'post', 'edit-tags', or 'edit' -- three situations where the screen object shines, and tries to set these values inclusively and accurately.

The problem here is that plugin pages (say one added as a submenu page to edit.php?post_type=page) then don't get the proper treatment in get_screen_icon(). Since $typenow will be set from the generic $_GET check in admin.php, we can fall back there.

I'm not a huge fan at the moment of how WP_Screen handles plugin pages. While it *may* be safe to set the screen's post_type and taxonomy off of $_GETpost_type? and $_GETtaxonomy?, that seems like it could pretty easily conflict with other things and poison some unrelated screen objects that happen to have a GET var on the page. On the other hand, with the menu code already relying so much on $typenow already, it's probably fine to just have the screen object reflect these on instantiation. In order to do that, we wouldn't apply the diff here at all, and we'd make another tweak to WP_Screen::get().

[nacin]

Per IRC, let's always set ->post_type and ->taxonomy. That renders 19080.2.diff moot.

We still like 19080.diff. There was some talk about the method name for for_meta_box(). Should it be for_post_type(), or maybe convert_post_type()? At first I thought it may be used for more than just post types, but the other core values are safe.

I was curious what other people choose for IDs to their own custom pages with meta boxes, but the API doesn't really work unless the $page for do_meta_boxes() is the current page. We kept this compatible with post types by making the screen IDs for post.php/post-new.php the name of the post type.

[nacin]

Per even more IRC conversations. Supplants 19080.diff and .2.diff.

[nacin]

Further chatter resulted in 19080.3.diff. We can assume with few or no side effects that if post_type_exists( $hook_name ), then it's post.php/post-new.php.

19080.4.diff​ fixes some copy/paste typos. Passes the existing unit tests. Works for meta boxes. Establishes a consistency where $taxnow and $typenow is set based on the existence of the tax and type. There's a chance that a post type won't be registered until admin_init, but set_current_screen() isn't much farther down in admin.php and it'll be corrected then. If this causes a problem then sanitize_key() can be restored.

[nacin]

In [19097]:

Consistently set taxnow/typenow and the current screen's post_type/taxnomy, whenever it can be detected. Allow WP_Screen::get() to accept a post type as a hook_name. Fixes issues with the meta box $page/$screen argument. fixes #19080. see #18785.

[chrisbliss18]

Replying to nacin:

In [19097]:

Consistently set taxnow/typenow and the current screen's post_type/taxnomy, whenever it can be detected. Allow WP_Screen::get() to accept a post type as a hook_name. Fixes issues with the meta box $page/$screen argument. fixes #19080. see #18785.

In addition to fixing the cited issues, this commit also fixed a problem caused by [19052]. The problem was that custom submenus (non-Taxonomy menus) added to custom post types would not work properly. With the custom submenu entry active, the custom post type menu would expand but the custom submenu entry would not be highlighted. In addition, the Post menu would be expanded.

After [19097], this is no longer an issue. Thanks.

[nacin]

Replying to chrisbliss18:

In addition to fixing the cited issues, this commit also fixed a problem caused by [19052]. The problem was that custom submenus (non-Taxonomy menus) added to custom post types would not work properly. With the custom submenu entry active, the custom post type menu would expand but the custom submenu entry would not be highlighted. In addition, the Post menu would be expanded.

Yep, thanks. I was chasing that around for a while.