WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#18785 closed task (blessed) (fixed)

Modernize screen functions

Reported by: nacin Owned by: nacin
Milestone: 3.3 Priority: normal
Severity: normal Version: 3.3
Component: Administration Keywords: dev-feedback
Focuses: Cc:

Description

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.

Attachments (26)

18785.patch (611 bytes) - added by jorbin 3 years ago.
Document two undocumented functions
18785.since.patch (906 bytes) - added by SergeyBiryukov 3 years ago.
18785.options-update.1.diff (6.5 KB) - added by mbijon 3 years ago.
Options > Reading & Privacy screen info
18785-default-rows.patch (1.5 KB) - added by azaozz 3 years ago.
18785.embeds-test.diff (1.5 KB) - added by mbijon 3 years ago.
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)
18785.widgets-screen-options.patch (1.3 KB) - added by SergeyBiryukov 3 years ago.
18785.andrew-2nd.1.diff (849 bytes) - added by mbijon 3 years ago.
For Andrew's comment #38, 2nd item
18785.andrew-2nd.2.diff (7.4 KB) - added by mbijon 3 years ago.
Updated use of existing add_help_tab() instances
18785.diff (9.8 KB) - added by nacin 3 years ago.
two-top-level-menus-18943.php (789 bytes) - added by mdawaffe 3 years ago.
18785.gary.diff (4.8 KB) - added by garyc40 3 years ago.
18785.gary.2.diff (4.7 KB) - added by garyc40 3 years ago.
remove debugging traces
18785.2.diff (24.0 KB) - added by nacin 3 years ago.
18785.3.diff (23.2 KB) - added by nacin 3 years ago.
Proper refresh after [19049]. Incorporates chaining in set_current_screen().
18785.4.diff (23.3 KB) - added by nacin 3 years ago.
Slight improvement in get_screen_icon()
18785.5.diff (24.0 KB) - added by nacin 3 years ago.
Sets screen->post_type for current edit-tags.php screens. Restores [15664].
18785.6.diff (14.4 KB) - added by nacin 3 years ago.
Refresh after [19050], [19051].
screen-ut.diff (4.2 KB) - added by ryan 3 years ago.
First pass at screen unit tests
18785.7.diff (511 bytes) - added by ryan 3 years ago.
post_type must be registered for the taxonomy.
18785.8.diff (1.0 KB) - added by ryan 2 years ago.
Use set_current_screen() in the wp-fullscreen-save-post ajax handler
18785.9.diff (933 bytes) - added by ryan 2 years ago.
Remove screen setup
18785.10.diff (2.3 KB) - added by nacin 2 years ago.
revert-help-so.patch (38.0 KB) - added by azaozz 2 years ago.
Move Screen Options and Help toggles to the H2s
revert-help-so-2.patch (8.5 KB) - added by azaozz 2 years ago.
18785.11.diff (581 bytes) - added by ryan 2 years ago.
Avoid fatals for plugins that erroneously include admin-header.php from an admin_init handler.
18785.12.diff (779 bytes) - added by ryan 2 years ago.
Remove dead overview code. Use prepend old- to back compat ID.

Download all attachments as: .zip

Change History (128)

comment:1 ocean903 years ago

  • Cc ocean90 added
  • Keywords needs-patch added
  • Type changed from defect (bug) to enhancement
  • Version set to 3.3

comment:2 dougwrites3 years ago

  • Cc heymrpro@… added

comment:3 nacin3 years ago

In [18784]:

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

comment:4 koopersmith3 years ago

In [18853]:

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

comment:5 mbijon3 years ago

  • Cc mbijon added

comment:6 nacin3 years ago

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.

comment:7 nacin3 years ago

In [18858]:

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

comment:8 nacin3 years ago

In [18859]:

Deprecate screen_meta(). see #18785.

comment:9 follow-up: scribu3 years ago

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.

Last edited 3 years ago by scribu (previous) (diff)

comment:10 follow-ups: jorbin3 years ago

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

comment:11 in reply to: ↑ 10 nacin3 years ago

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.

jorbin3 years ago

Document two undocumented functions

comment:12 jorbin3 years ago

Patch added to document two of the undocumented functions inside WP_screen

comment:13 nacin3 years ago

In [18863]:

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

comment:14 in reply to: ↑ 10 scribu3 years ago

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?

comment:15 scribu3 years ago

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.

Last edited 3 years ago by scribu (previous) (diff)

SergeyBiryukov3 years ago

comment:16 SergeyBiryukov3 years ago

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

comment:17 azaozz3 years ago

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.

mbijon3 years ago

Options > Reading & Privacy screen info

comment:18 follow-up: mbijon3 years ago

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)

comment:19 in reply to: ↑ 18 azaozz3 years ago

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.

comment:20 azaozz3 years ago

In [18880]:

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

comment:21 mbijon3 years ago

Much better plan to postpone those, thank you.

comment:22 in reply to: ↑ 9 mikeschinkel3 years ago

  • Cc mikeschinkel@… added

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.

comment:23 johnjamesjacoby3 years ago

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

comment:24 GaryJ3 years ago

  • Cc gary@… added

azaozz3 years ago

comment:25 azaozz3 years ago

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.

Last edited 3 years ago by azaozz (previous) (diff)

comment:26 mbijon3 years ago

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?

mbijon3 years ago

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)

comment:27 greenshady3 years ago

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.

comment:28 SergeyBiryukov3 years ago

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

comment:29 nacin3 years ago

In [18904]:

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

comment:30 nacin3 years ago

In [18910]:

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

comment:32 nacin3 years ago

In [18912]:

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

comment:33 nacin3 years ago

In [18913]:

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

comment:34 nacin3 years ago

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.

comment:35 nacin3 years ago

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.

comment:36 nacin3 years ago

  • Owner set to nacin
  • Status changed from new to assigned

comment:37 sbressler3 years ago

  • Cc sbressler@… added

comment:38 follow-up: azaozz3 years ago

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

comment:39 mtekk3 years ago

  • Cc mtekkmonkey@… added

mbijon3 years ago

For Andrew's comment #38, 2nd item

comment:40 mbijon3 years ago

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.

mbijon3 years ago

Updated use of existing add_help_tab() instances

comment:41 in reply to: ↑ 38 nacin3 years ago

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.

  1. 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?

comment:42 follow-up: GaryJ3 years ago

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

comment:43 mbijon3 years ago

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.

comment:44 in reply to: ↑ 42 azaozz3 years ago

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.

Last edited 3 years ago by azaozz (previous) (diff)

comment:45 nacin3 years ago

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.

comment:46 ryan3 years ago

We lost the default_contextual_help filter.

comment:47 nacin3 years ago

In [18941]:

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

nacin3 years ago

comment:48 nacin3 years ago

  • Milestone changed from Awaiting Review to 3.3
  • Type changed from enhancement to task (blessed)

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.

comment:49 ryan3 years ago

Looks good.

comment:50 follow-up: nacin3 years ago

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.

comment:51 nacin3 years ago

In [18945]:

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

comment:52 in reply to: ↑ 50 mdawaffe3 years ago

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.

comment:53 garyc403 years ago

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

garyc403 years ago

garyc403 years ago

remove debugging traces

comment:54 beaulebens3 years ago

  • Cc beau@… added

comment:55 nacin3 years ago

In [18949]:

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

comment:56 nacin3 years ago

In [18950]:

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

comment:57 follow-up: nacin3 years ago

  • Keywords has-patch commit added; needs-patch removed

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.

nacin3 years ago

comment:58 in reply to: ↑ 57 ; follow-up: scribu3 years ago

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.

Last edited 3 years ago by scribu (previous) (diff)

comment:59 in reply to: ↑ 58 ; follow-up: nacin3 years ago

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.

comment:60 nacin3 years ago

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

comment:61 in reply to: ↑ 59 ; follow-up: scribu3 years ago

Replying to nacin:

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.

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

Version 0, edited 3 years ago by scribu (next)

nacin3 years ago

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

nacin3 years ago

Slight improvement in get_screen_icon()

nacin3 years ago

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

comment:62 in reply to: ↑ 61 mikeschinkel3 years ago

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.
  1. 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":

FWIW.

comment:63 follow-up: scribu3 years ago

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.

comment:64 nacin3 years ago

In [19050]:

Deprecate favorite_actions(). see #18785.

comment:65 nacin3 years ago

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.

nacin3 years ago

Refresh after [19050], [19051].

comment:66 nacin3 years ago

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.
Last edited 3 years ago by nacin (previous) (diff)

ryan3 years ago

First pass at screen unit tests

comment:67 nacin3 years ago

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.

comment:68 ryan3 years ago

[UT463] to kick things off.

ryan3 years ago

post_type must be registered for the taxonomy.

comment:69 ryan3 years ago

In [19053]:

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

comment:70 nacin3 years ago

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.

comment:71 nacin3 years ago

In [19063]:

Make convert_to_screen() more resilient. see #18785.

comment:72 nacin3 years ago

In [19064]:

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

comment:73 follow-up: ocean902 years ago

  • Keywords dev-feedback added; has-patch commit removed

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.

Last edited 2 years ago by ocean90 (previous) (diff)

comment:74 in reply to: ↑ 73 nacin2 years ago

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.

comment:75 nacin2 years ago

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

comment:77 nacin2 years ago

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.

comment:20 nacin2 years ago

In [19098]:

Always run the old contextual_help filter. see #18785.

ryan2 years ago

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

comment:21 ryan2 years ago

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

comment:22 nacin2 years ago

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

ryan2 years ago

Remove screen setup

comment:23 ryan2 years ago

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

comment:24 ryan2 years ago

In [19106]:

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

comment:25 nacin2 years ago

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.

comment:26 nacin2 years ago

In [19108]:

Use more obvious variables in WP_Screen. see #18785.

nacin2 years ago

comment:27 nacin2 years ago

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.

comment:28 nacin2 years ago

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.

azaozz2 years ago

Move Screen Options and Help toggles to the H2s

azaozz2 years ago

comment:16 azaozz2 years ago

In [19130]:

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

comment:142 azaozz2 years ago

In [19131]:

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

comment:143 nacin2 years ago

In [19132]:

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

comment:144 nacin2 years ago

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

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

comment:145 nacin2 years ago

In [19154]:

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

comment:146 follow-up: nacin2 years ago

In [19155]:

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

comment:147 in reply to: ↑ 146 ; follow-up: scribu2 years ago

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?

comment:148 in reply to: ↑ 147 nacin2 years ago

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().

comment:149 in reply to: ↑ 63 mikeschinkel2 years ago

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.

ryan2 years ago

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

comment:150 follow-up: nacin2 years ago

In [19521]:

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

comment:151 nacin2 years ago

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.

comment:152 in reply to: ↑ 150 ; follow-up: nacin2 years ago

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.

comment:153 in reply to: ↑ 152 mikeschinkel2 years ago

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?

comment:154 F J Kaiser2 years ago

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.

comment:155 azizur2 years ago

  • Cc azizur added

ryan2 years ago

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

comment:156 ryan2 years ago

In [19563]:

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

Note: See TracTickets for help on using tickets.