Modernize screen functions

[nacin]

With WP_Screen introduced in #18690, there's some work to be done to merge in and/or clean up the 15 or so screen-related functions.

Everything is now in contained in screen.php, so it should be pretty easy to move some functionality into the class, kill some globals, shore up the code, and cut out the cruft.

WP_Screen also needs some documentation.

[nacin]

In [18784]:

Make WP_Screen final until we know which direction we want to go with it. Other keyword decorations. see #18785.

[koopersmith]

In [18853]:

Make screen options a help tab. Move screen option functions into WP_Screen. see #18690, #18785.

[nacin]

In [18857]:

Move all new WP_Screen properties to private. We should introduce getters. Correct legacy (and ideally read-only) properties to @access public. see #18785.

[nacin]

In [18858]:

Deprecate secreen_options(), screen_layout(). see #18785.

[nacin]

In [18859]:

Deprecate screen_meta(). see #18785.

[scribu]

Regarding [18857]:

Why:

private $_options = array(); 

instead of just:

private $options = array();

?

Adding a leading underscore to private properties is a PHP4 practice, when there were no actual access modifiers. It's not necessary anymore.

[jorbin]

While not required, it makes the code more readable imho. You immediately know that a variable is private just by looking at it.

[nacin]

Replying to jorbin:

While not required, it makes the code more readable imho. You immediately know that a variable is private just by looking at it.

Bingo. Same goes for private methods.

FWIW, this is part of PEAR's coding standard, even for PHP5.

[jorbin]

Document two undocumented functions

[jorbin]

Patch added to document two of the undocumented functions inside WP_screen

[nacin]

In [18863]:

Add phpdoc for two WP_Screen methods. props jorbin, see #18785.

[scribu]

Replying to jorbin:

While not required, it makes the code more readable imho. You immediately know that a variable is private just by looking at it.

Will we be adopting Hungarian notation next?

[scribu]

IMO, it makes the code more skimmable, but when you're actually trying to work on it, you get the feeling that you shouldn't be there.

[SergeyBiryukov]

18785.since.patch​ adds @since for screen_options(), screen_layout() and screen_meta().

[azaozz]

Following a discussion in #wordpress-dev it was decided to use the old functions and the new API (WP_Screen) in parallel in 3.3 and revisit it again in 3.4.

In that terms we can (should?) add get_column_headers(), get_hidden_columns() and get_hidden_meta_boxes() equivalents in WP_Screen so we can deprecate the stand-alone functions later.

Or perhaps we can "autoload" these three in __construct() as they are helper functions to get user options.

[mbijon]

Options > Reading & Privacy screen info

[mbijon]

I updated the Options > Reading & Privacy help in a different ticket. Figured I'd add here to start getting rid of the deprecation warnings in admin.

Doesn't seem like the right ticket though... Is there one just for these updates? (sorry, didn't find it just now)

[azaozz]

Replying to mbijon:

...Figured I'd add here to start getting rid of the deprecation warnings in admin.

Actually we should disable them, pending changes.

[azaozz]

In [18880]:

Don't trigger "deprecated" warnings before WP_Screen is finalized, see #18785

[mbijon]

Much better plan to postpone those, thank you.

[mikeschinkel]

Replying to scribu:

Why:

private $_options = array(); 

instead of just:

private $options = array();

?

One place where leading underscores are really helpful is when using the properties in a subclass where the declarations are not always directly or easily viewable.

[johnjamesjacoby]

Any reason not to name it WP_Admin_Screen, to leave WP_Screen open for non-admin uses?

[azaozz]

18785-default-rows.patch moves a bunch of filters to filtering the default table rows per screen, not what the user has selected to see. Not sure we need all of these filters too.

[mbijon]

add_help_tab() supports embedded media (images/video), but if there's no guidance to formatting then plugin devs are likely to create all sorts of formatting that could break the Help container.

Jane's ​latest post on WPdevel blog mentions users asked about this during testing:

"Several people suggested embedding images and/or video now that there felt like there was more room."

Is it worth adding a media "area" to WP_Screen, or can docs just recommend a best practice for including images & videos?

[mbijon]

Added media embeds to Dashboard Help menus. Image on Help>Screen Options, and Video on and Help>Content (sorry about YouTube embed, just rushing, didn't mean to endorse that content)

[greenshady]

On a custom plugin page, the "screen options" tab isn't triggered if there are meta boxes. This should list the meta boxes to show/hide with checkboxes.

The show/hide meta box options do appear if screen layout column options are available.

I hope I explained that well enough. I can provide screenshots if needed. I'll dig into the code a bit and see what's happening too.

[SergeyBiryukov]

18785.widgets-screen-options.patch​ restores options tab for Widgets screen, missing since [18853].

[nacin]

In [18904]:

@since for screen_meta(), screen_options(), screen_layout(). props SergeyBiryukov, see #18785.

[nacin]

In [18910]:

Revert CSS that slipped into [18874]. see #18785.

[nacin]

In [18911]:

Revert [18861], [18862], [18864], [18874], [18879], and [18880]. see #18785.

[nacin]

In [18912]:

Compress and bump wp-admin.css. CSS in [18867] and [18874] will need to be explained and re-committed. see #18785.

[nacin]

In [18913]:

Remove redundant help tab title from the tab body. see #18785.

[nacin]

In [18914]:

Move the Screen Options tab to the top and introduce a new 'overview' screen option, which accepts content for the top of this tab. see #18785.

[nacin]

In [18915]:

Use local method when handling back compat screen layout column filter. The function serves as a wrapper for but one object. see #18785.

[azaozz]

Been reviewing WP_Screen in its current form and see some shortcomings:

1. It goes against the notion of OOP. Base classes like this one should be easily extendable and have the basic methods that handle common usage.

However WP_Screen is just a collection of private and semi-private functions that rely on filters and globals. It has no methods to handle the most common usage and has no methods that can be extended easily for handling more advanced usage. On top of that it can be re-initialized and loose all processed data.

In that terms we have three options:

• Forget about OOP and separate the functions. There's no benefits of them being in a class (and it will make the code shorter).

• Make WP_Screen a pseudo-singleton, mark it as private and discourage plugins of ever trying to use it directly.

• Add methods for the most common usage and make it easily extendable.

2. It is a (very) bad idea to derive HTML IDs from translatable strings. They end up being useless as they are unpredictable and change from translation to translation. Also they are often illegal. What IDs will be derived from 官方中文站点 and 为您提供最新最全的 or हिन्दी अनुवाद and यहां उपलब्ध, and wouldn't they be the same (i.e. illegal in HTML context)?

Looking at ​http://core.trac.wordpress.org/browser/trunk/wp-admin/includes/screen.php#L613 we should either require the ID or drop it completely.

[mbijon]

For Andrew's comment #38, 2nd item

[mbijon]

Andrew, your num2 started with some of my code in #18690. It's patched up in the simplest manner above.

I actually like this method better than the original. The original intent of generating the HTML ID (instead of assigning it in incremented ID or similar) was to give plugin authors the ability to add tabs in a predictable way so they could apply custom CSS.

• The slugs I originally used weren't HTML-safe, but didn't regex the title to A-Z0-9.
• The L613 you note used sanitize_html_class(). That may have made things HTML-safe, but used a heavy-handed regex and since we didn't support passing in a fallback string would have left the CSS class blank for the examples you gave.

My patch above will require that 6 of the 11 places where add_help_tab() to have an ID value added.

[mbijon]

Updated use of existing add_help_tab() instances

[nacin]

Replying to azaozz:

1. It goes against the notion of OOP. Base classes like this one should be easily extendable and have the basic methods that handle common usage.

No it doesn't. OOP does not enforce inheritance. It allows for it. This is not a base class. It is a class. The class is designed to leverage the concepts of modularity and particularly encapsulation, not inheritance.

However WP_Screen is just a collection of private and semi-private functions that rely on filters and globals. It has no methods to handle the most common usage and has no methods that can be extended easily for handling more advanced usage.

Most of these filters and globals exist for compatibility reasons. Over time, we will be able to remove them. As stated above, the point of this object is encapsulation, not inheritance.

On top of that it can be re-initialized and loose all processed data.

Currently, yes. It's also no different than how set_current_screen() worked in 3.2, which is why I reverted the code that prevented re-initialization. I agree that losing the processed data is lame, but this is an isolated issue we can fix, not a problem with the class as a whole.

In that terms we have three options:

• Forget about OOP and separate the functions. There's no benefits of them being in a class (and it will make the code shorter).

See above.

• Make WP_Screen a pseudo-singleton, mark it as private and discourage plugins of ever trying to use it directly.

Plugins won't be using it much directly at all. It already is a pseudo-singleton, as it is used but once on a page (in a normal situation).

• Add methods for the most common usage and make it easily extendable.

There is no reason to make it extendable at this time. It was marked final with this in mind.

2. It is a (very) bad idea to derive HTML IDs from translatable strings.

Correct, it is. Again, this is not an indictment of the entire class, but rather a single issue that does deserve a fix. I'll work in mbijon's patch.

Are there specific issues with the code beyond HTML IDs?

[GaryJ]

ID values in HTML5 can be just about anything - see ​http://mathiasbynens.be/notes/html5-id-class

[mbijon]

I thought on the OOP-ness of this last night. My views of OOP in PHP aren't typical, but maybe I can limit the options explored:

As a class or bucket of functions this code will always be tightly coupled to the front-end code required to display it. Generalizing for all-purpose use would require support for skinning and JS action hooks. That seems like reinventing the wheel, especially with jQueryUI built-in now. Then again, Koopersmith recommended against that at #wcpdx so it probably isn't a great idea to do either.

Personally (at the risk of casting myself as anti-OOP), I think a file that's just a bucket of functions is OK for PHP use. Facebook may have gone the other way in recent years but they used to rely on "bucket" files to act as pseudo-objects instead of building classes. So my recommendation would be to just dump these methods into screen.php and forget the class?

Since someone else did the work of building WP_Screen I think we should keep it. The encapsulation will prevent method-name collisions that my way won't. We could generalize it later with wrapper methods and not break compatibility, though I don't think that's the right route.

[azaozz]

Replying to mbijon:

My patch above will require that 6 of the 11 places where add_help_tab() to have an ID value added.

Yes, the proper way is to require the IDs, that was already in but was reverted :)

Replying to GaryJ:

ID values in HTML5 can be just about anything...

Right, sanitize_html_class() actually sanitizes for XHTML IDs. Class names and IDs in HTML 5 can contain almost any character, however that brings problems/complications when using them in CSS and JS. Don't see the point of having:

<div id="官方中文站点" class="为您提供">...

and hope that plugin and theme authors would be able to use them. Also don't even want to think of what would happen if the browser is not using UTF-8 encoding.

[nacin]

Valid or not, IDs built off of translations are not stable, so that's a no-go. We're not just talking about HTML IDs here. A remove_help_tab() would need an ID to be able to grab hold of the tab.

No one looks at WP and thinks we're an OOP shop. The major selling point behind WP_Screen was encapsulation. Not just namespacing. There are huge benefits to being able to hide internal components from plugins, including the avoidance of globals.

[ryan]

We lost the default_contextual_help filter.

[nacin]

In [18941]:

Enforce IDs for add_help_tab(). props mbijon. Restore default_contextual_help, for now. see #18785.

[nacin]

18785.diff​ reworks how WP_Screen stores data. Everything ends up in private and static variables. Keeps full compatibility and since everything is encapsulated, we can optionally create a formal factory later.

convert_to_screen() can return a new WP_Screen object now, even for the current screen's ID. Data does not die when a reference to an object is gone.

The global in get_column_headers() is now just a static. $_wp_contextual_help is gone now too, though the back compat filters remain. It is now WP_Screen::$_old_compat_help.

There's some more cleanup to be had here, but it's in good shape. If anyone else has something to propose, please provide a patch and comment. Thanks.

I intend to close this as fixed before beta 2.

[ryan]

Looks good.

[nacin]

In [18943]:

Store screen help and options as static data against WP_Screen. Individual screen objects no longer hold data it can't re-generate on construction or otherwise fetch. convert_to_screen() now returns a WP_Screen object. Various globals are gone. Introduces WP_Screen::get_option(). Allows for a formal factory to be introduced later. see #18785.

[nacin]

In [18945]:

Restore $screen argument for contextual_help* filters. see #18785.

[mdawaffe]

Replying to nacin:

In [18943]:

Store screen help and options as static data against WP_Screen. Individual screen objects no longer hold data it can't re-generate on construction or otherwise fetch. convert_to_screen() now returns a WP_Screen object. Various globals are gone. Introduces WP_Screen::get_option(). Allows for a formal factory to be introduced later. see #18785.

This breaks plugins that add menus and call convert_to_screen(). One such plugin is NextGEN Gallery 1.8.3 ​http://wordpress.org/extend/plugins/nextgen-gallery/

$GLOBALS['typenow'] can get inappropriately overwritten during WP_Screen::__construct() during a convert_to_screen() call which breaks $the_parent in wp-admin/admin.php.

Attached is a test case.

1. Disable all plugins.
2. Enable the test case plugin "Two Top Level Menus [18943]"
3. Try to access both new top level menus and each submenu of those two new top level menus.

Everything works in r18942, the submenus break in r18943.

[garyc40]

I got the same issue as mdawaffe described.

It turns out that whenever you create an instance of WP_Screen, it overwrites the $typenow and $taxnow global variables.

Attached below is a patch that fixes this. $typenow and $taxnow are supposed to be modified only when WP_Screen::set_current_screen() is called.

Also I refactored a bunch of spaghetti lines that deals with 'edit-xxx', 'edit-tags' and 'edit.php'.

[garyc40]

remove debugging traces

[nacin]

In [18949]:

Don't instantiate WP_Screen in convert_to_screen() for now. see #18785.

[nacin]

In [18950]:

Restore object handling in add_contextual_help(). see #18785.

[nacin]

18785.2.diff​ resolves this ticket.

It makes WP_Screen behave as a registry of singletons. convert_to_screen() calls WP_Screen::get($hook_name), and set_current_screen() calls WP_Screen::get() (and then ->set_current_screen() to ensure the globals are correct).

WP_Screen::get() runs through the screen ID building steps, and then uses that ID to either return a new WP_Screen, or to return a WP_Screen object already in the $_registry.

Aside from the rework, this patch does a lot of good things here.

• wp-admin/post.php no longer has a mess of ugly code to determine the selected post type. This code is cleaned and brought into WP_Screen::get() (when setting the current screen only), and then carried over to edit.php and edit-tags.php as well. This fixes #14886 for both post types and taxonomies, meaning that $current_screen holds proper data the moment it is created, requires no corrections in post.php, and no longer lies otherwise.

• Eliminates the use of static variables to hold state. Keeps the old compat help in a single static variable to ensure legacy filters can operate against it (specifically contextual_help_list).

• Has get_screen_icon() use the $current_screen object to make determinations, rather than a messy use of globals ($typenow) and comparisons ($screen == $current_screen).

• Clarifies the multiple interpretations of $id by trying to an argument called $hook_name when appropriate.

• Deprecates favorite_actions(). Removes unused globals like $wp_list_table. Streamlines some logic in show_screen_options().

• Locks up the constructor to ensure we have complete control over how the API is leveraged.

I have a few tests in progress for this as well.

[scribu]

Replying to nacin:

It makes WP_Screen behave as a registry of singletons.

​Singleton does not accept plural form. :)

What the patch does is store all the WP_Screen instances in a static property on the WP_Screen class.

Maybe this:

$screen = WP_Screen::get( $hook_name ); 
$screen->set_current_screen();

could be replaced with this:

WP_Screen::get( $hook_name )->set_as_current();

to keep with our coding standards.

+1 otherwise.

[nacin]

Replying to scribu:

Replying to nacin:

It makes WP_Screen behave as a registry of singletons.

​Singleton does not accept plural form. :)

Well, it's technically called a multiton, but also known as a basic registry pattern.

WP_Screen::get( $hook_name )->set_as_current();

I don't have a problem with chaining here (definitely makes sense for this usage), but I'm not sure what you mean by coding standards.

[nacin]

18785.3.diff​ is just a refresh after [19049].

[scribu]

Replying to nacin:

I don't have a problem with chaining here (definitely makes sense for this usage), but I'm not sure what you mean by coding standards.

"If you don't use a variable more than once, don't create it."

[nacin]

Proper refresh after [19049]. Incorporates chaining in set_current_screen().

[nacin]

Slight improvement in get_screen_icon()

[nacin]

Sets screen->post_type for current edit-tags.php screens. Restores [15664].

[mikeschinkel]

Replying to scribu:

"If you don't use a variable more than once, don't create it."

As an aside, there are two (2) good reasons to create a variable even if you only use it once:

1. It is impossible to single-step through chained methods when using a PHP debugger like PhpStorm, NetBeans, Zend Studio, etc.

2. Using well-named variables documents complex code and is a practice advocated by Steve McConnell, author of "The single most influential book every programmer should read":

• ​http://stackoverflow.com/questions/1711/what-is-the-single-most-influential-book-every-programmer-should-read

FWIW.

[scribu]

It is impossible to single-step through chained methods when using a PHP debugger like PhpStorm, NetBeans, Zend Studio, etc.

You should be able to "step into" them though.

But I agree that there is such a thing as too much chaining. Not the case here, though.

[nacin]

In [19050]:

Deprecate favorite_actions(). see #18785.

[nacin]

In [19051]:

screen.php tidying. Doc fixes and improvements. Improve logic in ::show_screen_options(). Remove regex in ::set_parentage(). Better variable names. Streamline get_screen_icon(). Whitespace. see #18785.

[nacin]

Refresh after [19050], [19051].

[nacin]

18785.6.diff​ is much easier to examine.

• Replaces the innards of convert_to_screen() with WP_Screen::get( $hook_name )->set_current_screen().
• Removes the static-ness of help_tabs, options, help_sidebar. This required a simple find-replace which makes up for a good portion of the diff.
• Introduces WP_Screen::get() and WP_Screen::set_current_screen(), which is the original constructor split into two, as well as some fairy dust to correct post_type and taxonomy values.
• Replaces a mess of code in wp-admin/post.php with some elegance.

[ryan]

First pass at screen unit tests

[nacin]

In [19052]:

Move WP_Screen to a full registry. Have convert_to_screen() return a WP_Screen object. Improve and verify values for post_type and taxonomy. see #18785. also fixes #14886.

[ryan]

​[UT463] to kick things off.

[ryan]

post_type must be registered for the taxonomy.

[ryan]

In [19053]:

Fallback to the default post type only if it is registed for the taxonomy. Props nacin. see #18785

[nacin]

Added ​[UT467]. A convert_to_screen() argument can be equivalent to a page's $hook_suffix, so it requires some similar sanitization. Next commit rebranches the code a bit to keep things DRY.

[nacin]

In [19063]:

Make convert_to_screen() more resilient. see #18785.

[nacin]

In [19064]:

Add missing keyword. fixes #19046, see #18785.

[ocean90]

There is a problem with the contextual_help filter. It doesn't work for plugin pages/settings pages. This snippet doesn't return $screen, but it does in 3.2.

add_filter( 'contextual_help', 'ds_text_help', 10, 2 );
function ds_text_help( $text, $screen ) {
	var_dump( $screen );
}

See ​Filtering the contextual help text.

[nacin]

Replying to ocean90:

There is a problem with the contextual_help filter. It doesn't work for plugin pages/settings pages. This snippet doesn't return $screen, but it does in 3.2.

Works for me.

Note that the contextual_help filter will not always fire. It will fire if there is already old compatible help added to the page to form a "Screen Info" tab -- that help will then get filtered. It will also fire if there are no new-style tabs added, as it then serves as the basis for default help text.

But if add_contextual_help() has not been called and add_help_tabs() has been, that filter can only fire if someone uses the contextual_help_list filter to add something specific to the current screen. You'll see that your code does not fire on post-new.php/post.php and index.php, as those have help tabs.

[nacin]

Per a quick conversation with ocean90, the filter should probably be available even if it's empty, just in case someone adds to it.

[SergeyBiryukov]

Related: #19095

[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.

[nacin]

In [19098]:

Always run the old contextual_help filter. see #18785.

[ryan]

Use set_current_screen() in the wp-fullscreen-save-post ajax handler

[ryan]

18785.8.diff avoids an E_STRICT warning and creates a proper WP_Screen object.

[nacin]

set_current_screen() should probably occur before check_ajax_referer() as the post type is used as the key.

[ryan]

Remove screen setup

[ryan]

After talking to azaozz, I think we can get rid of the screen setup in wp-fullscreen-save-post entirely.

[ryan]

In [19106]:

No need to setup current screen in wp-fullscreen-save-post handler. see #18785

[nacin]

In [19107]:

Simplify logic in wp-fullscreen-save-post. Most of this came from post.php but that's been cut down as well. see #18785.

[nacin]

In [19108]:

Use more obvious variables in WP_Screen. see #18785.

[nacin]

18785.10.diff​ makes is_user/is_network checks work for $hook_name instantiations. Also kills currently dead code in fetch-list. We can leave the define's there (see #15903) but they're not needed.

[nacin]

In [19119]:

s/add_help_sidebar/set_help_sidebar/g and introduce screen->remove_help_tab($id) and screen->remove_help_tabs(). see #19020, #18785.

[azaozz]

Move Screen Options and Help toggles to the H2s

[azaozz]

In [19130]:

Revert placement of Help and Screen Options to under the admin bar on the right, see #18197, see #18785

[azaozz]

In [19131]:

Revert the look and functionality of Screen Options ans Help links/tabs, see #18197, see #18785

[nacin]

In [19132]:

Allow is_network and is_user to work for all screen instances. see #18785.

[nacin]

Calling this one fixed. New tickets for all new items.

[nacin]

In [19154]:

Pass $tab array to callback execution for help tabs. see #18785.

[nacin]

In [19155]:

Use call_user_func_array() as call_user_func() cannot pass by reference. see #18785.

[scribu]

Replying to nacin:

In [19155]:

Use call_user_func_array() as call_user_func() cannot pass by reference. see #18785.

Huh? You mean you want to allow callbacks to modify $tab?

[nacin]

Replying to scribu:

Huh? You mean you want to allow callbacks to modify $tab?

After testing, [19155] wasn't necessary, but it can stay.

The docs suggested that call_user_func() could not pass by reference. I took that to include objects -- we wouldn't want $this to get copied. Turns out objects in PHP (5, anyway) are still passed by reference with call_user_func().

[mikeschinkel]

Replying to scribu:

You should be able to "step into" them though.

In theory, yes. In practice, not really. Frankly, I'd love to see a lot less chaining than already happens because some code in core it just a total PITA to debug through. FWIW.

But I agree that there is such a thing as too much chaining. Not the case here, though.

I was commenting on the use of an absolute in your advice.

[ryan]

Avoid fatals for plugins that erroneously include admin-header.php from an admin_init handler.

[nacin]

In [19521]:

Make current_screen an action, not a filter. see #18785.

[nacin]

In [19522]:

Call set_current_screen() again in admin-header.php in case a plugin includes admin-header.php before admin.php completes. Rare and silly, but seen in the wild. props ryan, see #18785.

[nacin]

Replying to nacin:

In [19521]:

Make current_screen an action, not a filter. see #18785.

Since $current_screen is an object, making it an action doesn't prevent modifications to it. But it does make plugin authors think twice about using this to modify the object directly.

[mikeschinkel]

Replying to nacin:

Since $current_screen is an object, making it an action doesn't prevent modifications to it. But it does make plugin authors think twice about using this to modify the object directly.

Can you speak directly to this? Are you saying that plugins should not add properties to the $current_screen object, and if not, why not?

One of my projects, which includes code planned to be released for others to use, is a $context object which I started developing before you added $current_screen to WordPress core. I've been planning to consolidate the properties of my $context object into $current_screen once v3.3 releases.

Are you saying we should not modify $current_screen? If we should not add properties to it, why not, and why would it be better to force developers to look elsewhere for the current context added by a plugin? Also, $current_screen is missing context items which are often needed such as $current_screen->post_id (where applicable, of course) so it makes sense for plugins to add those if they are not already set so the plugin can depend on them being there.

Or am I just misunderstanding your comment above?

[F J Kaiser]

Note about admin menus: When inspecting $GLOBALS['menu'] and $GLOBALS['submenu'] then you'll find a lot of stuff that could (and IMHO should) be aligned to the (object) $current_screen. That would make it a lot easier to move around sub menu items, declare styling, etc, which currently is as messed up as the info you get from current_screen.

[ryan]

Remove dead overview code. Use prepend old- to back compat ID.

[ryan]

In [19563]:

Contextual help: Remove dead overview code. Prepend old- to back compat ID. Props nacin. see #18785