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

[joehoyle]

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

[nacin]

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.

[joehoyle]

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.

[joehoyle]

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.

[wonderboymusic]

#25432 was marked as a duplicate.

[joehoyle]

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

[DrewAPicture]

Looks like the patch needs a refresh.

[wonderboymusic]

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

[DrewAPicture]

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.

[joehoyle]

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.

[wonderboymusic]

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

[wonderboymusic]

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.

[wonderboymusic]

In 32741:

After [32740], correct doc block typo.

See #29881.

[wonderboymusic]

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.

[joehoyle]

@wonderboymusic: my hero!

[wonderboymusic]

In 32752:

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

See #29881, [32740].

[wonderboymusic]

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.

[wonderboymusic]

WP_Comments_List_Table and WP_Post_Comments_List_Table already implement this properly

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

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.

[wonderboymusic]

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]

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

[helen]

In 32798:

Media list table: Restore row actions.

see #29881, #32657.

[helen]

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.

[wonderboymusic]

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

[obenland]

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?

[obenland]

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

[wonderboymusic]

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.