List tables: consider a new method to generate the views links markup

[afercia]

Splitting this out from #32399.

Most of the List Tables used in core comes with some "views links" to switch the table to a subset of items, say published, drafts, trashed posts or approved, pending comments or different user types.

While the logic to get the views differs depending on the type of items (post status, user role, active tab, etc.) the code that actually generates the links markup is basically always the same and could benefit from some abstraction in a general method. See also 32399#comment:32.

/cc @flixos90

[costdev]

This ticket was raised in a triage meeting on Slack and it was agreed that abstracting the link generation makes sense. The ticket has been set for the 5.9 milestone.

[costdev]

To get the ball rolling, I'm attaching 42066.functions.php.diff with a general function for discussion/consideration.

• Doing a search for the get_views() method, I've identified 11 files that generate admin status links.
• I have diffs that convert each of these to use the new function, with some clean up in the get_views() methods. I'll attach these if this is a goer.
• There will be some other files with a similar format but in a method not called get_views().
• wp-class-media-list-table.php generates <option> elements. A similar general function could be written for this as well.

I've called the function generate_links(), as it can be used in virtually any scenario.

1. Accepts an array of arrays containing the data for each link.
2. If the is_current (bool) key is true, the function will add class="content" aria-content="page".
3. Allows for an array of additional attributes to be specified in the attributes key.
4. Handles invalid/empty arguments - I'll add unit tests if this is a goer.
5. Doesn't perform esc_url() or esc_attr() and leaves that up to the caller.

---

Calling the function looks like so:

<?php

$post_link_data = array(
    'all' => array(
        'url'        => 'edit.php',
        'label'      => 'All',
        // `generate_links()` adds/appends `class="current" and aria-current="page"`.
        // You can specify your own by setting this to `false` and adding your own
        // `class` and `aria-current` key/value pairs to the `attributes` array below.
        'is_current' => true,
        'attributes' => array(
            'title'  => _( 'View All Posts' ),
            'id'     => 'all-of-the-posts',
            'class'  => 'button-primary',
        ),
    ),
    // ...
);

$post_links = generate_links( $post_link_data );
/*
$post_links = array(
   'all' => '<a href="edit.php"
                title="View All Posts"
                id="all-of-the-posts"
                class="button-primary current"
                aria-current="page">All</a>',
    // ...
);
*/

For links that have a similar set of attributes, we could have a wrapper function specifically for say, the admin status links, to apply attributes as needed.

e.g.

<?php

function generate_admin_status_links( $link_data ) {
    // Additional attributes
    $link_data = array_map(
        function( $link ) {
            $link['attributes']['admin-attribute-1'] = 'Howdy, admin!';
            $link['attributes']['admin-attribute-2'] = 'Howdy again, admin!';
            return $link;
        },
        $link_data
    );
    return generate_links( $link_data );
}

You could also do the same for a generate_clean_links() wrapper that performs esc_url(), esc_attr(), etc. on the data before calling generate_links().

N.B. The indentation on the diff is a little messed up on trac. It was run through WPCS before uploading. It's best to download and apply for readability.

Notes

• Can replace array_map() calls with foreach( $link_data as &$link ) for performance per ​this benchmark.

[audrasjb]

Reviewed during today's bug srcub.
Marking this as needs-testing to make sure it's peer-reviewed and works as expected :)

[audrasjb]

As per today's bug scrub, this ticket needs a general consensus on the proposed approach. Moving to Future release and adding @costdev as the ticket owner.

[peterwilsoncc]

Additional commit props: @garrett-eclipse

---

Garrett provided a patch on #46122​ which covered some of the code needed for this ticket.

This PR introduces WP_List_Table::get_admin_status_links() to abstract the link markup generation for list tables.

Includes PHPUnit tests with full line/branch coverage.

Trac ticket: https://core.trac.wordpress.org/ticket/42066

[costdev]

I've submitted ​PR 2828, which introduces WP_List_Table::get_admin_status_links() to abstract link markup generation in list tables.

The PR includes PHPUnit tests for the new method and also implements the use of this method in:

• WP_MS_Sites_List_Table
• WP_MS_Themes_List_Table
• WP_MS_Users_List_Table
• WP_Plugins_List_Table
• WP_Plugin_Install_List_Table
• WP_Theme_Install_List_Table
• WP_Users_List_Table

This is a more direct approach to the ticket's summary than using the proposed general function mentioned earlier.

@afercia @peterwilsoncc @audrasjb Let me know your thoughts on this method, and whether it should also cover the markup for class="current" aria-current="page" and instead change the "current" key to bool type.

Should a general function be preferred to cover link markup generation in most cases, I drafted ​PR 2380 with wp_create_links() for this, which also supports additional attributes (including boolean attributes) and which is easily applied to this use case.

[davidbaumwald]

Is there any reason that "status" is being used here, other than that's mostly what Core is using for different views?

Personally, I've long wished for a get_views method in the list table. I usually end up recreating the code in my final. These views could be used for many more things other than "status", so I don't think we should lock in on that.

Going back to the original request in the ticket, can we not just promote get_views to the parent class with a default output that outputs HTML for link data in an array for a new property views. If it's not set, get_views doesn't output anything. Extending classes could just add their links to the views array. The output would be default unless the extending class overrides get_views.

If I've missed the case against doing this, I apologize in advance.

[davidbaumwald]

Ignore my comment above. I was way off. So, I think the current patch is OK. /unprops davidbaumwald.

[hellofromTonya]

As there's a new patch and activity, moving this ticket into 6.1 for consideration.

[afercia]

@costdev thanks for looking into this.

whether it should also cover the markup for class="current" aria-current="page" and instead change the "current" key to bool type.

I'd say moving all the HTML to the new method would be cleaner.

Aside:
naming is hard. Other names in this context refer to 'views', 'get_views' so I'd propose to rename get_admin_status_links to get_views_links.

As per backwards compatibility with plugins that extend the list table classes and with existing filters e.g. apply_filters( "views_{$this->screen->id}", $views ) I definitely defer to your expertise.

[costdev]

Thanks for the feedback @davidbaumwald and @afercia, and thanks @hellofromTonya for milestoning this one!

I have updated ​PR 2828 to:

• Rename get_admin_status_links to get_views_links.
• Update all references within child classes in the PR.
• Abstract the current HTML markup as well. The current key is now bool.
• Update the tests.
• Rebase the PR branch on trunk.

I also tested some of the plugins in ​this result and ​this result, including:

• WordPress San-Thumbnail Posts Plugin
• BuddyPress
• Media Library Assistant
• Contact Form to Manage and respond to conversations with customers — Happyforms
• ATUM WooCommerce Inventory Management and Stock Tracking

All of the plugins' views links seem to appear and function as expected. The PR is ready for review.

[costdev]

I've updated the ​PR 2828 with additional tests for BC as requested by @peterwilsoncc. I haven't pushed fixes for ​the test failures so that these can be reviewed:

Types of test failure:

1. Some of the classes use href="", others use href=''. This could be fixed to href="" for all classes for consistency.
2. & is not escaped in the href, but is escaped in the PR. The links still work, so I think this is an acceptable change and I can update the tests to account for it.
3. For a multisite test, I mistakenly used a local user/superadmin count value, rather than getting the actual numbers, so locally it passes and on CI it fails. I have a fix for this ready to push once a decision is made about the two above types of failure.

[audrasjb]

Reviewed during today's bug scrub.

Thanks @costdev! I think your proposal for the test failures are all relevant.

1. Then let's make it consistent everywhere
2. Updating tests looks like the best solution here
3. Let's go with this fix then!

[costdev]

Thanks @audrasjb! I've updated ​PR 2828 with the following changes:

1. As agreed, the tests have been updated to reflect that the affected list tables that previously rendered href='' will now render href="".
2. As agreed, the tests have been updated to reflect that & will now be escaped to &
   • This is done by esc_url(), & fans should direct their attention there 😝
3. As agreed, the faulty test introduced in the PR has been fixed to use actual user counts rather than what happened to be my local user counts (my bad!)
4. After rebasing on trunk, the new tests for WP_Comments_List_Table and WP_Post_Comments_List_Table were faulty.
   • They would run as part of the whole test suite, but not in isolation. I'm not sure what commit may have affected these, but either way, the solution was to make sure that $this->table->prepare_items() was called in these two particular tests.
   • This fix has been applied to the original test commits for ​Comments and ​Post Comments.

The PR is ready for another review and testing.

---

• Removing dev-feedback.

[costdev]

Testing Instructions

Steps to test

1. Apply ​PR 2828.
2. Navigate to the following admin pages:
   • Posts
   • Comments
   • Pages
   • Plugins
   • Plugins > Add New
   • Users
   • Tools > Export Personal Data
   • If you have a multisite test site set up:
     • Sites
     • Users
   • Other list tables touched by the PR but I'm not sure where to navigate to 😝:
     • Plugin Install (this doesn't appear to be Plugins > Add New)
     • Themes/MS Themes (this doesn't appear to be Appearance > Themes)
     • Theme Install (this doesn't appear to be Appearance > Themes > Add New)
   • Additionally, you can install and activate some of the plugins in ​this result and ​this result, and check their respective list tables to ensure they continue to function as expected.
3. Click on all other statuses in the list table (e.g. "Draft", etc). You may need to create a few items (Posts, Pages, Users, etc).

Expected Results

• ✅ The first status (e.g. "All") should be selected before and after applying the PR.
• ✅ Clicking on another status should correctly filter the list table and highlight the selected status.
• (in short, everything should work as it used to)

[Dharm1025]

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/2828

Environment

• OS: Ubuntu 20.04.5 LTS
• Web Server: nginx/1.21.5
• PHP: 7.4.27
• WordPress: 6.1-alpha-53344-src
• Browser: Version 105.0.5195.102 (Official Build) (64-bit)
• Theme: Twenty Twenty-Two
• Active Plugins: -

Actual Results

✅ Works as expected with patch.

Hi @costdev, Thanks for testing instructions. I have tested patch with below List tables as per testing instructions and it works as expected.

• Posts
• Comments
• Pages
• Plugins
• Plugins > Add New
• Users
• Tools > Export Personal Data
• Tools > Erase Personal Data
• Plugins > Add New
• Appearance > Themes > Add New

Multisite:

• Sites
• Users
• Themes
• Themes > Add New
• Plugins > Add New

Additionally, I tested the list tables added by "BuddyPress" plugin and it seems working fine.

Thanks

[costdev]

Thanks for testing the PR so extensively @Dharm1025!

This one seems ready for commit consideration. Adding the keyword so a committer can take a look. 🙂

[juhise]

Hi

I have also tested this, sharing the report below

Environment

OS: macOS Monterey
PHP: 7.3.5
WordPress: 6.0.2
Browser: Chrome 105.0.5195.52 (Official Build) (x86_64)
Theme: Twenty Twenty-Two

Result

Tested with below settings, works as expected

• Posts
• Comments
• Pages
• Plugins
  • Plugins > Add New
• Users
• Tools > Export Personal Data
• Tools > Erase Personal Data
• Appearance > Themes > Add New

Also tested with below plugins, works as expected

• WordPress San-Thumbnail Posts Plugin
• Media Library Assistant
• BuddyPress

[davidbaumwald]

In 54215:

Administration: Add new get_views_links method to WP_List_Table.

Many WP_List_Table child classes in core use mostly the same code to create their "view" links markup. To DRY-up the code, a new WP_List_Table->get_view_links method is being introduced to consolidate the HTML link generation when provided an array of links.

This change also implements this new method in the relevant WP_List_Table_xxx child classes get_views methods. Finally, unit tests are being added to validate view links markup and test for some "unhappy paths".

Props afercia, costdev, garrett-eclipse, Dharm1025, juhise, peterwilsoncc.
Fixes #42066.

Merged into core in https://core.trac.wordpress.org/changeset/54215.

[davidbaumwald]

While this probably doesn't need a full, separate Dev Note, this new function deserves at least a call-out on the Misc Dev Note.

[SergeyBiryukov]

In 54222:

I18N: Remove <code> tags from translatable strings in WP_List_Table::get_views_links().

To simplify the strings and exclude any parts that don't require translation, the <code> tags wrapping a placeholder can be moved out of the string and added to the placeholder value.

Follow-up to [54215].

See #42066.

[SergeyBiryukov]

In 54223:

Coding Standards: Move WP_List_Table::get_views_links() to a more appropriate place.

This moves the newly introduced ::get_views_links() method to a more predictable location, next to the the ::get_views() and ::views() methods.

Follow-up to [54215].

See #42066.

[costdev]

Dev Note: ​https://make.wordpress.org/core/2022/10/09/introducing-wp_list_tableget_views_links-in-wordpress-6-1/