Make WordPress Core

Opened 12 years ago

Closed 8 months ago

Last modified 5 months ago

#18449 closed defect (bug) (fixed)

List Table Factory Needs to be Pluggable

Reported by: miqrogroove's profile miqrogroove Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.1 Priority: normal
Severity: normal Version: 3.1
Component: Administration Keywords: has-patch needs-dev-note
Focuses: Cc:

Description

Better design planning should have been considered for http://core.trac.wordpress.org/browser/trunk/wp-admin/includes/list-table.php

function _get_list_table() is identical to an object factory pattern, which would make more sense as a static member of WP_List_Table. In other words...

$wp_list_table = _get_list_table('WP_Terms_List_Table');

... which looks simple, is actually confusing because it would be more intuitive if written as ...

$wp_list_table = WP_List_Table::factory('Terms');

Further, the factory hasn't offered any extensibility. Locking edit-tags.php against WP_Terms_List_Table makes it unusable for custom taxonomies that need a custom UI. Despite all of the great API improvements for taxonomies, this case still necessitates duplicating the entire category editing UI within a plugin just to tweak a table column or a link path.

Please adjust the ticket Type and Component as necessary. It could be a taxonomy design flaw or an administration enhancement, depending how you look at it.

Attachments (2)

18449.diff (1.0 KB) - added by Faison 8 years ago.
Adding a filter to _get_list_table() to allow developers to return an object of their own class in place of core classes
18449.2.diff (7.2 KB) - added by peterwilsoncc 6 years ago.

Download all attachments as: .zip

Change History (43)

#1 @nacin
12 years ago

_get_list_table() used to be get_list_table(), and it had a filter to allow for switching out tables.

When we pulled out XHR bits and made the entire API private near the end of 3.1, we renamed it to the current, private version.

When someone finally takes ownership of and rewrites the list table API, I think this would indeed be a good approach.

#2 @scribu
12 years ago

  • Milestone changed from Awaiting Review to Future Release

Somewhere in Space this may all be happening right now.

#4 @sirzooro
12 years ago

  • Cc sirzooro added

#5 @kurtpayne
12 years ago

  • Cc kpayne@… added

#6 @johnbillion
11 years ago

  • Cc johnbillion added
  • Component changed from Taxonomy to Administration

#7 @SergeyBiryukov
10 years ago

#24938 was marked as a duplicate.

#9 @jrbeilke
9 years ago

  • Cc jrbeilke@… added

@Faison
8 years ago

Adding a filter to _get_list_table() to allow developers to return an object of their own class in place of core classes

#10 @Faison
8 years ago

  • Keywords has-patch added

I recently ran into a situation where I needed to make some extensive customizations to the WP_Media_List_Table class and the filters it provided was not enough. So I'm hoping I can bring life back to this ticket with a patch that would help us all out.

It adds the filter 'pre_get_list_table' to _get_list_table(). It allows a developer to check what class was requested, and return an object of a different class if needed.

#11 @wonderboymusic
8 years ago

  • Milestone changed from Future Release to 4.4
  • Owner set to wonderboymusic
  • Status changed from new to assigned

I've ripped apart List Tables the past 2-3 releases. I think they are downright usable now.

#12 @wonderboymusic
8 years ago

#16987 was marked as a duplicate.

This ticket was mentioned in Slack in #core by sergey. View the logs.


8 years ago

#14 @SergeyBiryukov
8 years ago

The 'pre_get_list_table' filter should probably be 'wp_list_table_class', for consistency with 'wp_admin_bar_class'.

#15 @wonderboymusic
8 years ago

  • Milestone changed from 4.4 to Future Release

I have severe reservations about this endorsement now - I still consider WP_Screen , the random functions that interact with it, and WP_List_Table to be super weird

This ticket was mentioned in Slack in #core by sergey. View the logs.


6 years ago

#17 @peterwilsoncc
6 years ago

  • Milestone changed from Future Release to 4.9

In 18449.2.diff:

  • add the filter following @SergeyBiryukov's advice above
  • removes @access private from all the list table classes (ref #24938, labeled dupe of this ticket)

After six years I think the opportunity for someone to take ownership of the list table API has passed and WordPress is stuck with what's in there already.

Changing the API & breaking backward compatibility in any substantive way will break too many plugins.

I think the future release has come so am putting on the milestone to encourage discussion.

#18 @westonruter
6 years ago

  • Milestone changed from 4.9 to Future Release

Discussion doesn't seem to be happening and 4.9-beta3 is in a few hours, so I'm punting.

This ticket was mentioned in Slack in #core-php by sergey. View the logs.


5 years ago

#20 @birgire
5 years ago

#44813 was marked as a duplicate.

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


14 months ago

#22 @peterwilsoncc
14 months ago

  • Owner wonderboymusic deleted

I can not foresee a contributor brave enough to change the API of WP_List_Table. Nor can I imagine the extraordinarily brave committer deciding that changing the API is a good opportunity to maintain their SVN skills.

https://wpdirectory.net/search/01FZY3TDNVDGYZ0PX5131BF11H

I'll spend a few days deciding if 6.0 is the future release in which this gets done. YOLO.

#24 follow-up: @peterwilsoncc
10 months ago

  • Milestone changed from Future Release to 6.1

I've refreshed the patch in the linked pull request.

  • new filter per @SergeyBiryukov's suggestion above
  • removes the private access from all the list tables to reflect reality

About half the plugins in the most popular list include the extends WP_List_Table. The chance of the API being substantially changed in a backward incompatible manner is basically zero.

#25 @milana_cap
10 months ago

  • Keywords needs-dev-note add-to-field-guide added

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


9 months ago

#27 @audrasjb
9 months ago

  • Keywords commit added

This ticket was mentioned during today's bug scrub.
It looks pretty straightforward @peterwilsoncc I can't see any reason for not shipping your patch :)

Marking for commit.

#28 @peterwilsoncc
9 months ago

I've made a change to the PR since the commit keyword was added: @costdev suggested putting an is_string() check on the class name, so I've done that.

The filter is named wp_list_table_class_name.

Is everyone still good for a commit?

#29 @costdev
9 months ago

LGTM 👍

#30 @peterwilsoncc
9 months ago

  • Owner set to peterwilsoncc

#31 in reply to: ↑ 24 @desrosj
9 months ago

Replying to peterwilsoncc:

About half the plugins in the most popular list include the extends WP_List_Table. The chance of the API being substantially changed in a backward incompatible manner is basically zero.

Just wanted to chime in with support for and agreement with this statement.

I did add one inline comment on the PR with a question. But I don't consider it a blocker for committing this.

peterwilsoncc commented on PR #3091:


9 months ago
#32

@desrosj @costdev @SergeyBiryukov Should _get_list_table() have the private delegation removed while making these changes?

With the new filter, it appears WordPress is encouraging developers to use it so it seem odd to maintain it as a private API.

costdev commented on PR #3091:


9 months ago
#33

@peterwilsoncc I'm inclined to agree. Certainly it's plugin usage suggests it's also already used publicly, including by some very widely-used plugins.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


8 months ago

#35 @audrasjb
8 months ago

  • Keywords changes-requested added; commit removed

This ticket was discussed during today's bug scrub, and looking at the discussion in the above PR, I tend to agree with removing the private delegation as well.

#36 @peterwilsoncc
8 months ago

  • Keywords changes-requested removed

I've updated the PR to merge in trunk and remove the private delegation from _get_list_table().

#37 @peterwilsoncc
8 months ago

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

In 54378:

Administration: Remove private delegation from list tables.

Remove the private delegation from the following classes and function:

  • WP_List_Table
  • WP_Application_Passwords_List_Table
  • WP_Comments_List_Table
  • WP_Links_List_Table
  • WP_Media_List_Table
  • WP_MS_Sites_List_Table
  • WP_MS_Themes_List_Table
  • WP_MS_Users_List_Table
  • WP_Plugin_Install_List_Table
  • WP_Plugins_List_Table
  • WP_Post_Comments_List_Table
  • WP_Posts_List_Table
  • WP_Terms_List_Table
  • WP_Theme_Install_List_Table
  • WP_Themes_List_Table
  • WP_Users_List_Table
  • _get_list_table()

This change is to reflect the reality that list tables are very, very, very widely used by extenders and backward compatibility therefore needs to be maintained.

Introduces the filter wp_list_table_class_name within _get_list_table() to allow extenders to modify the list table returned for custom screens.

Props audrasjb, birgire, costdev, desrosj, faison, johnbillion, jrbeilke, kurtpayne, milana_cap, miqrogroove, nacin, peterwilsoncc, scribu, sergeybiryukov, sirzooro, westonruter, wonderboymusic.
Fixes #18449.

peterwilsoncc commented on PR #3091:


8 months ago
#38

Merged in https://core.trac.wordpress.org/changeset/54378 / 09d9a3d7ba97552f822d8ab33cf76722c82fa85c

#39 @milana_cap
8 months ago

  • Keywords add-to-field-guide removed

#40 @peterwilsoncc
8 months ago

As shared via slack, here's my suggestion for the miscellaneous developer focused changes dev note.

Administration
List tables are no longer considered private to WordPress Core and can now be freely used by themes and plugins. The private delegation has been removed from the _get_list_table() function and WP_List_Table and related classes. See #18449.

WP_List_Table is used 17,505 times in the plugin repo so I don't think a full dev note explaining their use is needed.

#41 @Faison
5 months ago

Thank you @peterwilsoncc for bringing this to a close. I'm no longer in the WordPress industry, but it is great to see this change make it to core!

Note: See TracTickets for help on using tickets.