Make WordPress Core

Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#45089 closed enhancement (fixed)

Update WP_List_Table::get_sortable_columns() to support 'asc' and 'desc' arguments

Reported by: tkama's profile Tkama Owned by: birgire's profile birgire
Milestone: 5.5 Priority: normal
Severity: normal Version: 4.9.8
Component: Administration Keywords: has-patch needs-testing has-unit-tests commit
Focuses: administration Cc:

Description

When registering new sortable column we can set default first sort order, by setting false/true in the second parameter of array:

<?php
function get_sortable_columns(){
        return array(
                'register_date' => array( 'register_date', false ), // false = asc
                'login_date'    => array( 'login_date', true ),
                'rating'        => array( 'rating', false ),
        );
}

It is much more clear to write 'asc' / 'desc' then false/true.

To achieve such behavior and not lose current one, we need to change /wp-admin/includes/class-wp-list-table.php#L16

<?php
if ( $current_orderby === $orderby ) {
        $order = 'asc' === $current_order ? 'desc' : 'asc';
        $class[] = 'sorted';
        $class[] = $current_order;
} else {
        $order = $desc_first ? 'desc' : 'asc';
        $class[] = 'sortable';
        $class[] = $desc_first ? 'asc' : 'desc';
}

to

<?php
...
} else {
        if( in_array( strtolower($desc_first), array('desc', 'asc'), true ) )
                $order = strtolower( $desc_first );
        else
                $order = $desc_first ? 'desc' : 'asc';

        $class[] = 'sortable';
        $class[] = $order === 'desc' ? 'asc' : 'desc';
}

Attachments (5)

class-wp-list-table.php.patch (986 bytes) - added by Tkama 6 years ago.
45089.patch (923 bytes) - added by Shital Patel 6 years ago.
create patch
45089.diff (922 bytes) - added by Shital Patel 6 years ago.
45089.2.diff (1.4 KB) - added by Tkama 6 years ago.
45089-2.diff (5.5 KB) - added by birgire 5 years ago.

Download all attachments as: .zip

Change History (19)

#1 @SergeyBiryukov
6 years ago

  • Component changed from General to Administration
  • Keywords has-patch needs-docs added
  • Milestone changed from Awaiting Review to 5.1
  • Summary changed from WP_List_Table to Update WP_List_Table::get_sortable_columns() to support 'asc' and 'desc' arguments

Hi @Tkama, thanks for the patch!

The documentation for WP_List_Table::get_sortable_columns() would also need to be updated to reflect the new accepted value.

@Shital Patel
6 years ago

create patch

@Shital Patel
6 years ago

@Tkama
6 years ago

#2 @pento
6 years ago

  • Milestone changed from 5.1 to 5.2

Patch needs review.

#3 @Shital Patel
6 years ago

  • Keywords needs-testing added

#4 @desrosj
6 years ago

  • Keywords needs-docs removed
  • Milestone changed from 5.2 to 5.3

Patch still needs review. Punting to 5.3 as 5.2 beta is in less than 2 days. If a committer has time to properly review and commit this, it can be moved back.

@birgire
5 years ago

#5 @birgire
5 years ago

  • Focuses administration added
  • Keywords has-unit-tests added
  • Owner set to birgire
  • Status changed from new to assigned

This seems like a good improvement.

In 45089-2.diff I updated the patch with minor code standard fixes and test cases:

  • test_sortable_columns()
  • test_sortable_columns_with_current_ordering()

for the WP_Comments_List_Table class.

The WP_List_Table::get_sortable_columns() method is protected. I therefore considered adding new test classes (as new files) that extended an existing list table class, like WP_Comments_List_Table. That way we could modify the {{get_sortable_columns()}}} method. Anonymous classes might have made that more cleaner, but it's only available in PHP 7, so I ended with using the mock builder to override the method instead.

This will need an owner for the 5.3 milestone, so I add myself as an owner to help with that.

#6 @desrosj
5 years ago

  • Milestone changed from 5.3 to 5.4

This still needs some testing, and with 5.3 beta 1 in a few hours, we've run out of time.

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


5 years ago

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


5 years ago

#9 @davidbaumwald
5 years ago

  • Milestone changed from 5.4 to Future Release

This ticket still needs testing, and with 5.4 Beta 1 landing tomorrow, this is being moved to Future Release. If any maintainer or committer feels this can be included in 5.4 or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#10 @SergeyBiryukov
5 years ago

  • Milestone changed from Future Release to 5.5

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


4 years ago

#12 @SergeyBiryukov
4 years ago

  • Keywords commit added

#13 @whyisjake
4 years ago

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

In 48151:

Administration: Update WP_List_Table::get_sortable_columns() to support asc and desc arguments.

This makes the API a little more clear, whereas setting false used to mean asc and true meant desc, you can now use those directly, while maintaining back-compat.

Fixes #45089.

Props Tkama, SergeyBiryukov, shital-patel, desrosj, birgire, davidbaumwald.

#14 @SergeyBiryukov
4 years ago

In 48165:

Administration: Correct and simplify the logic for asc and desc arguments in WP_List_Table::get_sortable_columns().

Setting the initial order didn't work as expected due to reversed logic.

Follow-up to [48151].

See #45089.

Note: See TracTickets for help on using tickets.