WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 19 months ago

Last modified 19 months ago

#21871 closed enhancement (fixed)

Reduce reliance on global variables in list tables

Reported by: ryan Owned by: ryan
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.4
Component: Administration Keywords: needs-patch
Focuses: Cc:

Description

The list tables rely on several global variables such as $post_type, $taxonomy, $post_type_object, and $tax. Instead of globals, list tables should consult the WP_Screen object. Further, to accommodate ajax handlers, the list table constructors should accept screen IDs as an arg and create and store a WP_Screen object internally.

Attachments (15)

21871.diff (38.5 KB) - added by ryan 19 months ago.
Refresh of nacin's patch
21871.2.diff (38.5 KB) - added by ryan 19 months ago.
21871.3.diff (29.5 KB) - added by nacin 19 months ago.
21871.4.diff (774 bytes) - added by nacin 19 months ago.
21871.5.diff (1.0 KB) - added by ryan 19 months ago.
Append post type to edit taxonomy screen ids. edit-post_tag-post
21871-guid.diff (2.9 KB) - added by ryan 19 months ago.
Incomplete, barely test PoC for screen guid
21871-guid.2.diff (3.1 KB) - added by ryan 19 months ago.
21871-guid-ut.diff (3.8 KB) - added by ryan 19 months ago.
21871-guid.3.diff (3.4 KB) - added by ryan 19 months ago.
Now with mess to guid translation
21871-guid-ut.2.diff (9.3 KB) - added by ryan 19 months ago.
21871-guid-ut.3.diff (9.3 KB) - added by ryan 19 months ago.
21871-guid.4.diff (3.5 KB) - added by ryan 19 months ago.
21871-guid.5.diff (4.7 KB) - added by ryan 19 months ago.
21871-guid-ut.4.diff (10.9 KB) - added by ryan 19 months ago.
21871-minimal.diff (646 bytes) - added by ryan 19 months ago.
Minimal fix for edit-tags

Download all attachments as: .zip

Change History (38)

ryan19 months ago

Refresh of nacin's patch

ryan19 months ago

comment:2 wonderboymusic19 months ago

  • Keywords has-patch added

nacin19 months ago

comment:3 ryan19 months ago

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

In [21914]:

Reduce reliance on global variables in the list tables. Allow passing a screen ID to the list tables so that ajax handlers can set the needed screen.

Props nacin
fixes #21871

comment:4 ocean9019 months ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

When adding a tag or category on edit-tags.php you will get a notice:

Notice: Trying to get property of non-object in /wp-admin/includes/class-wp-terms-list-table.php on line 290

$this->screen->post_type is null.

nacin19 months ago

comment:5 nacin19 months ago

I was afraid of that. I think this is actually fixable in WP_Screen.

When we aren't given a hook_name, we try to clean the post type and taxonomy from $_REQUEST. We also try to set $post_type to 'post' on edit-tags.php when we have no $post_type. But we should always try to set $post_type to 'post', whether or not we're given a hook name.

See 21871.4.diff.

comment:6 ocean9019 months ago

21871.4.diff works well for posts, but not for custom post types.

comment:7 ryan19 months ago

Time to finally do edit-tax-type?

ryan19 months ago

Append post type to edit taxonomy screen ids. edit-post_tag-post

comment:8 follow-up: nacin19 months ago

As hyphens are allowed in taxonomy and post type names, this could be more trouble than it is worth.

What about allowing a query string to be passed with as the hookname, from which we can then glean post type or taxonomy?

<?php
$hookname = 'edit-post_tag?post_type=foo';
$query_string = null;
if ( false !== strpos( $hookname, '?' ) )
        list( $hookname, $query_string ) = explode( '?', $hookname, 2 );
parse_str( $query_string, $query_args );
var_dump( $hookname, $query_args );

comment:9 nacin19 months ago

For the record, I feel really dirty suggesting that.

comment:10 ryan19 months ago

Or a different separator in the screen ID that taxonomies don't use? edit-post_tag:post, edit-post_tag|post, edit-post_tag/post?

comment:11 nacin19 months ago

My first thought was indeed to use a colon. But then I started thinking about parse_str() and it went downhill from there.

comment:12 ryan19 months ago

I like keeping it in the ID string because it allows set_current_screen( 'edit-post_tag:post' ) to set all of the needed bits. Back compat problems are possible though. Looking into that.

comment:13 ryan19 months ago

Hmm, stuff like this might kill the notion.

"manage_{$this->screen->id}_columns"

comment:14 ryan19 months ago

I think we need a guid property that is actually unique.

Last edited 19 months ago by ryan (previous) (diff)

ryan19 months ago

Incomplete, barely test PoC for screen guid

ryan19 months ago

ryan19 months ago

ryan19 months ago

Now with mess to guid translation

ryan19 months ago

ryan19 months ago

ryan19 months ago

ryan19 months ago

ryan19 months ago

comment:15 ocean9019 months ago

#21968 was marked as a duplicate.

comment:16 scribu19 months ago

  • Cc scribu added

comment:17 in reply to: ↑ 8 scribu19 months ago

Replying to nacin:

What about allowing a query string to be passed with as the hookname, from which we can then glean post type or taxonomy?

I think that's actually the most elegant solution, since that's the information that's originally used to generate the screen object.

That is, of course, assuming that all the logic is contained within the WP_Screen class, as opposed to scatered all over the place.

comment:18 scribu19 months ago

Oh, I was actually thinking of passing the original query string, not a hookname or whatever.

comment:19 scribu19 months ago

So, in the admin page, you'd do:

$screen = WP_Screen::get_instance( basename(__FILE__), $_GET );

and in an AJAX request, you'd do:

$screen = WP_Screen::get_instance( $_POST['admin_file'], $_POST['admin_file_args'] );

Or something along those lines.

comment:20 ryan19 months ago

The screen basically needs four things: the admin context ( user | network | site | front ), the particular admin page, post_type, and taxonomy. Currently, get() attempts to get these in several ways.

  • The global hook_suffix along with post_type and taxonomy from the request and the *_ADMIN constants
  • A hook name that is pretty much hook_suffix with the .php being optional plus post_type and taxonomy from request and the *_ADMIN constants
  • An "ID" that is a little more removed from the hook suffix that encodes the admin context and the post_type and taxonomy. It currently does not provide both the post_type and the taxonomy to edit-tags.php. Only taxonomy is provided.
  • The guid patches introduce a new id that provides everything needed in one string.

There are several methods used for passing screen info along to ajax handlers.

  • Inline JS in admin-header.php also attempts to provide some screen related information via pagenow and typenow variables.
  • WP_List_Table::_js_vars() passes along the screen id and base.
  • edit-tags.php passes the screen ID in a hidden input.

get() is already pretty much a query processor so passing query strings to it won't require much work. Once we parse that info we still need to create an ID of some sort to use for option names, filters, and so on and also to provide some insulation from file naming changes. The existing id property in the screen object seems sufficient for these purposes. I doubt anyone will want to target to guid specificity.

So, I guess we need to answer the question of how we want to pass screen info to ajax handlers and JS. We're currently all over the place. A guid is one simple string, but passing some sort of struct or query string is pretty simple too.

ryan19 months ago

Minimal fix for edit-tags

comment:21 nacin19 months ago

In [21974]:

Add back the space between class names removed in [21914]. see #21871.

comment:22 ryan19 months ago

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

In [21982]:

The edit-tags screen ID does not contain the post_type. Fallback to post_type in the REQUEST. Fixes AJAX term addtions from edit-tags.php. fixes #21871

comment:23 ryan19 months ago

Decided to do a minimum fix and discuss ID improvements for another release. We don't need yet another rushed screen ID system.

Note: See TracTickets for help on using tickets.