Make WordPress Core

Opened 6 years ago

Closed 15 months ago

Last modified 14 months ago

#42066 closed enhancement (fixed)

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

Reported by: afercia's profile afercia Owned by: costdev's profile costdev
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch has-unit-tests has-testing-info commit has-dev-note
Focuses: Cc:

Description

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

Attachments (1)

42066.functions.php.diff (2.8 KB) - added by costdev 2 years ago.

Download all attachments as: .zip

Change History (35)

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


2 years ago

#2 @peterwilsoncc
2 years ago

  • Milestone changed from Awaiting Review to 5.9

#3 @costdev
2 years ago

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.

#4 @costdev
2 years ago

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.
Last edited 2 years ago by costdev (previous) (diff)

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


2 years ago

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


2 years ago

#7 @audrasjb
2 years ago

  • Keywords has-patch needs-testing added

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

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


2 years ago

#9 @audrasjb
2 years ago

  • Milestone changed from 5.9 to Future Release
  • Owner set to costdev
  • Status changed from new to assigned

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.

#10 @peterwilsoncc
20 months ago

Additional commit props: @garrett-eclipse


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

This ticket was mentioned in PR #2828 on WordPress/wordpress-develop by costdev.


18 months ago
#11

  • Keywords has-unit-tests added

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

#12 @costdev
18 months ago

  • Keywords dev-feedback added

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.

Last edited 18 months ago by costdev (previous) (diff)

#13 @davidbaumwald
18 months ago

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.

#14 @davidbaumwald
18 months ago

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

#15 @hellofromTonya
18 months ago

  • Milestone changed from Future Release to 6.1

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

#16 @afercia
17 months ago

@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.

#17 @costdev
17 months ago

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.

Last edited 17 months ago by costdev (previous) (diff)

#18 @costdev
15 months ago

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.

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


15 months ago

#20 @audrasjb
15 months ago

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!

#21 @costdev
15 months ago

  • Keywords dev-feedback removed

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 &#038;
    • This is done by esc_url(), &amp; 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.

#22 @costdev
15 months ago

  • Keywords has-testing-info added

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)
Last edited 15 months ago by costdev (previous) (diff)

This ticket was mentioned in Slack in #core-test by costdev. View the logs.


15 months ago

This ticket was mentioned in Slack in #core-test by costdev. View the logs.


15 months ago

#25 @Dharm1025
15 months ago

  • Keywords needs-testing removed

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

This ticket was mentioned in Slack in #core-test by dharmesh. View the logs.


15 months ago

#27 @costdev
15 months ago

  • Keywords commit added

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. 🙂

#28 @juhise
15 months ago

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

#29 @davidbaumwald
15 months ago

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

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.

#31 @davidbaumwald
15 months ago

  • Keywords needs-dev-note added

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.

#32 @SergeyBiryukov
15 months ago

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.

#33 @SergeyBiryukov
15 months ago

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.

Note: See TracTickets for help on using tickets.