Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#29881 closed task (blessed) (fixed)

Better abstraction for WP_*_List_Table so they are easier to subclass

Reported by: joehoyle's profile joehoyle Owned by: wonderboymusic's profile wonderboymusic
Milestone: 4.3 Priority: normal
Severity: normal Version: 3.1
Component: Administration Keywords: has-patch
Focuses: administration Cc:

Description

Currently WP_Posts_List_Table is a bit of a complicated mess as all columns are in a big switch statement in the single_row() method. This annoying for a couple of reasons.

  1. The code is a messy, huge method making it difficult to maintain and understand.
  2. Subclassing is a pain as you can't override a single column the way the magic method calling of column_$column_name should work.
  3. Subclasses don't automatically get the column_$column_name functionality as WP_Posts_List_Table (for whatever reason) closes out this functionality by overriding the single_row() method.

I'm not a fan of changing things for the sake of it, but I think this is an important cleanup. WP_Comment_List_Table is pretty well implemented in comparison, but I imagine WP_Posts_List_Table is the most often subclassed due to the heavy use of custom post types.

Apologies if this has been discussed before and/or rejected. I tried to use search... which I seem to kind of fail at finding tickets.

I attached a preliminary path that is functional but needs a couple of edge cases cleaned up. It basically splits out all the columns into their own method which are automatically called from single_row_columns().

Attachments (6)

wp-posts-list-table.diff (26.9 KB) - added by joehoyle 9 years ago.
wp-posts-list-table-2.diff (28.0 KB) - added by joehoyle 9 years ago.
29881.diff (20.6 KB) - added by wonderboymusic 9 years ago.
29881.2.diff (6.9 KB) - added by paulwilde 9 years ago.
29881.3.diff (6.5 KB) - added by paulwilde 9 years ago.
29881.4.diff (31.8 KB) - added by wonderboymusic 9 years ago.

Download all attachments as: .zip

Change History (43)

#1 @nacin
9 years ago

  • Milestone changed from Awaiting Review to 4.1

I absolutely love this.

Sadly the list tables were all implemented in slightly different ways, which is probably why we all hate them so much. But there *is* something to salvage here, and single_row_columns() can help with that. It's a nice abstraction we should be using everywhere.

column_taxonomy() should probably be protected. Also, it should probably have a name other than column_ because it could otherwise be invoked by single_row_columns().

I see no back compat concerns here, either.

#2 follow-up: @joehoyle
9 years ago

column_taxonomy() should probably be protected. Also, it should probably have a name other than column_ because it could otherwise be invoked by single_row_columns().

Agreed, I did originally have this as private, but then moved to public for other to use it - though I think realistically it's only subclasses that will use it so protected it good. As for naming, it's a workaround for avoiding accidental calling via single_row_columns so though as much as I hate underscored methods, protected function _column_taxonomy might make the most sense here. get_taxonomy_column doesn't follow as it outputs data, output_taxonomy_column -- perhaps.

#3 @joehoyle
9 years ago

Refreshed patch against trunk (some fun that was!) and fixed up the naming of the taxonomy helper function to protected output_taxonomy_column().

Also provided backwards compat for class names on the post title column; this required overriding single_row_columns() to set the post title classes, as there is no other way to inject those old classes unless we changed wp-post-list-table.php to somehow allow filtering / changing of the <td> class names.

Also fixed up remaining todos in the patch.

#4 @johnbillion
9 years ago

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

#5 @johnbillion
9 years ago

  • Keywords needs-testing 4.2-early added
  • Milestone changed from 4.1 to Future Release

#6 @wonderboymusic
9 years ago

#25432 was marked as a duplicate.

#7 @joehoyle
9 years ago

Anything needed from me here? Would like to get this in 4.2 is possible.

#8 @SergeyBiryukov
9 years ago

  • Keywords has-patch added
  • Milestone changed from Future Release to 4.2

#9 @DrewAPicture
9 years ago

  • Keywords needs-refresh added

Looks like the patch needs a refresh.

#10 @wonderboymusic
9 years ago

I tried to refresh this quickly to help out, but it's a bit if a mess at the moment due to churn

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


9 years ago

#12 @DrewAPicture
9 years ago

  • Keywords 4.3-early added; 4.2-early removed
  • Milestone changed from 4.2 to Future Release

Sounds like the patch still needs a refresh, and it's definitely too late for this in 4.2 as we head into beta. Let's try 4.3-early.

#13 @joehoyle
9 years ago

To try get this ticket on track, or closed out as invalid I'd like to get some discussion going here if this is a favored patch broadly speaking. It appeared initially that it was favored (@nacin chimed in here) - however based off the Slack discussion:

Drew:
Patch is borked and I’m not sure we want to do this right before beta.

azaoz:
uh, the best abstraction for that is... change to UL LI

helen:
help was offered, need was expressed, help was not given. punt.

[in relation to shipping in 4.2)

Some good advice from Mark, Helen and John at WC London: get the go-head and decide on a solution before writing any code. Clearly I made the wrong choice by starting with the code here! I really don't want to refresh this patch (and take a chunk of time) without there being a consensus that this is a good idea.

There's a couple of comments on this ticket mentioning the patch needs a refresh, but based off the initial patch and reception - what this doesn't need is a refreshed patch, it needs a decision if this will be adopted for r+ or decided against. I think the attached patch sufficiently expresses what I wanted to do with this abstraction - we should be talking about whether it's a good or bad idea, not requiring a clean merge-able patch before we can talk about if the idea is a good or bad one.

So, I'd like to open up discussion if anyone wants to weigh in here. Once we have direction, I'm happy to handle a clean patch if that's decided.

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


9 years ago

#15 @obenland
9 years ago

  • Keywords 4.3-early removed
  • Milestone changed from Future Release to 4.3

@wonderboymusic
9 years ago

#16 @wonderboymusic
9 years ago

  • Keywords needs-testing needs-refresh removed

29881.diff refreshes all of this and makes single_row() about 1,000,000 times cleaner.

#17 @wonderboymusic
9 years ago

In 32740:

Override ->single_row_columns() in WP_Posts_List_Table.
Break apart the giant switch statement in ->single_row() into column_{$column_name} methods.
To maintain the ->single_row_columns( $item ) interface, add a property, $current_level, to allow access to $level.

This list table class is now easier to subclass.

Props joehoyle, wonderboymusic.
See #29881.

#18 @wonderboymusic
9 years ago

In 32741:

After [32740], correct doc block typo.

See #29881.

#19 @wonderboymusic
9 years ago

In 32742:

After [32740], in WP_Posts_List_Table::single_row() - $lock_holder is checked but unused, so the call to get_userdata() is unnecessary.

See #29881.

#20 @joehoyle
9 years ago

@wonderboymusic: my hero!

#21 @wonderboymusic
9 years ago

In 32752:

In WP_Posts_List_Table, move the <th> markup out of ->column_cb().

See #29881, [32740].

#22 in reply to: ↑ 2 @wonderboymusic
9 years ago

  • Summary changed from Better abstraction for WP_Posts_List_Table so it's easier to subclass to Better abstraction for WP_*_List_Table so they are easier to subclass

#23 @wonderboymusic
9 years ago

In 32753:

In WP_Links_List_Table::display_rows():

  • Move the giant switch statement into methods
  • Call -single_row_columns(), which it inherits from WP_List_Table

See #29881.

#24 @wonderboymusic
9 years ago

WP_Comments_List_Table and WP_Post_Comments_List_Table already implement this properly

#25 @wonderboymusic
9 years ago

In 32754:

In WP_Media_List_Table::display_rows():

  • Move the giant switch statement into methods
  • Call -single_row_columns(), which it inherits from WP_List_Table

See #29881.

#26 @wonderboymusic
9 years ago

In 32755:

In WP_MS_Sites_List_Table::display_rows():

  • Move the giant switch statement into methods
  • Call ->single_row_columns(), which we now override - it could inherit from WP_List_Table if we can find a way to handle the id column. When that happens, we can ditch this method.

See #29881.

#27 @wonderboymusic
9 years ago

In 32756:

In WP_MS_Themes_List_Table::display_rows():

  • Move the giant switch statement into methods
  • Call ->single_row_columns(), which we now override

See #29881.

#28 @wonderboymusic
9 years ago

In 32757:

In WP_MS_Users_List_Table::display_rows():

  • Move the giant switch statement into methods
  • Call ->single_row_columns(), which we now override - there is a small amount of logic that requires overriding, otherwise it is largely the same as the parent method

See #29881.

@paulwilde
9 years ago

@paulwilde
9 years ago

#29 @paulwilde
9 years ago

I've attached a patch which is a potential solution to repeating all of single_row_columns()'s code several times.

handle_row_actions() is hard-coded into the overrides, so maybe a single_row_column() method could exist which contains all the column logic without the <td> container.

Last edited 9 years ago by paulwilde (previous) (diff)

#30 @helen
9 years ago

In 32798:

Media list table: Restore row actions.

see #29881, #32657.

#31 @helen
9 years ago

In 32805:

Media list table: Avoid PHP notices by using the proper variables.

Also visually tightens up the row actions.

props tyxla.
fixes #32657. see #29881.

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


9 years ago

#33 follow-up: @wonderboymusic
9 years ago

  • Owner changed from nacin to wonderboymusic
  • Type changed from enhancement to task (blessed)

I'm gonna finish this - just want to make sure the method invocation for special fields isn't weird

#34 in reply to: ↑ 33 @obenland
9 years ago

Replying to wonderboymusic:

I'm gonna finish this - just want to make sure the method invocation for special fields isn't weird

What is left to do to get this finished?

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


9 years ago

#36 @obenland
9 years ago

@wonderboymusic, could you take a final look at this?

#37 @wonderboymusic
9 years ago

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

In 33270:

List Tables:

  • In ->handle_row_actions(), bail immediately if $primary and $column_name do not match. Saves us a nesting level and avoids declaring code that is unusable.
  • In WP_List_Table::single_row_columns(), allow _column_{$name} to be called dynamically by core to avoid having to override the entirety of ->single_row_columns() in WP_MS_Users_List_Table and WP_Posts_List_Table
  • In WP_MS_Sites_List_Table, id is not a column.

Props wonderboymusic, paulwilde.
Fixes #29881.

Note: See TracTickets for help on using tickets.