Opened 11 months ago
Closed 2 months ago
#29881 closed task (blessed) (fixed)
Better abstraction for WP_*_List_Table so they are easier to subclass
Reported by: | joehoyle | Owned by: | 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.
- The code is a messy, huge method making it difficult to maintain and understand.
- Subclassing is a pain as you can't override a single column the way the magic method calling of column_$column_name should work.
- 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)
Change History (43)
comment:2 follow-up: ↓ 22 @joehoyle — 11 months 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.
comment:3 @joehoyle — 10 months 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.
comment:4 @johnbillion — 10 months ago
- Owner set to nacin
- Status changed from new to assigned
comment:5 @johnbillion — 10 months ago
- Keywords needs-testing 4.2-early added
- Milestone changed from 4.1 to Future Release
comment:6 @wonderboymusic — 8 months ago
#25432 was marked as a duplicate.
comment:7 @joehoyle — 8 months ago
Anything needed from me here? Would like to get this in 4.2 is possible.
comment:8 @SergeyBiryukov — 8 months ago
- Keywords has-patch added
- Milestone changed from Future Release to 4.2
comment:9 @DrewAPicture — 7 months ago
- Keywords needs-refresh added
Looks like the patch needs a refresh.
comment:10 @wonderboymusic — 6 months ago
I tried to refresh this quickly to help out, but it's a bit if a mess at the moment due to churn
comment:11 @slackbot — 6 months ago
This ticket was mentioned in Slack in #core by drew. View the logs.
comment:12 @DrewAPicture — 6 months 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.
comment:13 @joehoyle — 6 months 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.
comment:14 @slackbot — 5 months ago
This ticket was mentioned in Slack in #core by drew. View the logs.
comment:15 @obenland — 4 months ago
- Keywords 4.3-early removed
- Milestone changed from Future Release to 4.3
@wonderboymusic — 3 months ago
comment:16 @wonderboymusic — 3 months ago
- Keywords needs-testing needs-refresh removed
29881.diff refreshes all of this and makes single_row() about 1,000,000 times cleaner.
comment:17 @wonderboymusic — 3 months ago
In 32740:
comment:18 @wonderboymusic — 3 months ago
In 32741:
comment:19 @wonderboymusic — 3 months ago
In 32742:
comment:20 @joehoyle — 3 months ago
@wonderboymusic: my hero!
comment:21 @wonderboymusic — 3 months ago
In 32752:
comment:22 in reply to: ↑ 2 @wonderboymusic — 3 months 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
comment:23 @wonderboymusic — 3 months ago
In 32753:
comment:24 @wonderboymusic — 3 months ago
WP_Comments_List_Table and WP_Post_Comments_List_Table already implement this properly
comment:25 @wonderboymusic — 3 months ago
In 32754:
comment:26 @wonderboymusic — 3 months ago
In 32755:
comment:27 @wonderboymusic — 3 months ago
In 32756:
comment:28 @wonderboymusic — 3 months ago
In 32757:
@paulwilde — 3 months ago
@paulwilde — 3 months ago
comment:29 @paulwilde — 3 months 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.
comment:30 @helen — 3 months ago
In 32798:
comment:31 @helen — 3 months ago
In 32805:
comment:32 @slackbot — 3 months ago
This ticket was mentioned in Slack in #core by obenland. View the logs.
comment:33 follow-up: ↓ 34 @wonderboymusic — 3 months 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
comment:34 in reply to: ↑ 33 @obenland — 2 months 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?
comment:35 @slackbot — 2 months ago
This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.
comment:36 @obenland — 2 months ago
@wonderboymusic, could you take a final look at this?
@wonderboymusic — 2 months ago
comment:37 @wonderboymusic — 2 months ago
- Resolution set to fixed
- Status changed from assigned to closed
In 33270:
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.