WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 5 weeks ago

#43844 reviewing defect (bug)

PHP list language construct changed behaviour in PHP 7

Reported by: javorszky Owned by: SergeyBiryukov
Milestone: 4.9.8 Priority: normal
Severity: major Version:
Component: Administration Keywords: dev-feedback
Focuses: docs, administration Cc:

Description

The inline docs for the method as of 24th April, 2018:

        /**
         * Get a list of sortable columns. The format is:
         * 'internal-name' => 'orderby'
         * or
         * 'internal-name' => array( 'orderby', true )
         *
         * The second format will make the initial sorting order be descending
         *
         * @since 3.1.0
         *
         * @return array
         */
        protected function get_sortable_columns() {
                return array();
        }

See https://github.com/WordPress/WordPress/blob/master/wp-admin/includes/class-wp-list-table.php

The problem is that if I define the sortable columns as the first one, with

class My_List_Table extends WP_List_Table {
    
    protected function get_sortable_columns() {
        return array(
            'my-column' => 'my-column',
        );
    }
}

Then the list call when building the table list header / footer will not have the orderby variable defined. See https://github.com/WordPress/WordPress/blob/master/wp-admin/includes/class-wp-list-table.php#L1117

If however I use

class My_List_Table extends WP_List_Table {
    
    protected function get_sortable_columns() {
        return array(
            'my-column' => array( 'my-column' ),
        );
    }
}

everything works as expected.

Diff incoming shortly.

Attachments (1)

43844.diff (504 bytes) - added by javorszky 2 months ago.
Inline documentation fix for WP_List_Table::get_sortable_columns

Download all attachments as: .zip

Change History (10)

@javorszky
2 months ago

Inline documentation fix for WP_List_Table::get_sortable_columns

#1 @javorszky
2 months ago

  • Keywords has-patch added

#2 @javorszky
2 months ago

  • Keywords dev-feedback added; has-patch removed

Oh, hold on, this adds a different problem. Because $sortable[ $column_key ] is now an array, if the default sort direction is not defined, the list call will try to access the array offset 1, which doesn't exist, throwing an undefined offset Notice.

Need feedback on how best to solve this.

tldr:

  1. either we leave the sortable column definition as a string, in which case the orderby isn't getting added, so the column is useless, or
  2. we move to an array, but then leaving out offset 1 of the array (default direction), which will make sorting work, but will throw a notice in php

The solutions are

  1. we can fall back to dealing with orderby as a string, or
  2. sanitize the array to make sure there's always a default order present

#3 @javorszky
2 months ago

  • Severity changed from minor to major

Further digging unearthed changed behaviour between PHP 5 and PHP 7, see the following:

Strings can no longer be unpacked

http://uk1.php.net/manual/en/migration70.incompatible.php#migration70.incompatible.variable-handling.list.string

The order changes from right-to-left to left-to-right

http://uk1.php.net/manual/en/migration70.incompatible.php#migration70.incompatible.variable-handling.list.order

Which makes me think that all usage depending on list should be abandoned.

#4 @javorszky
2 months ago

  • Summary changed from Inline documentation for WP_List_Table::get_sortable_columns is wrong to PHP list language construct changed behaviour in PHP 7

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


2 months ago

#6 @SergeyBiryukov
2 months ago

  • Milestone changed from Awaiting Review to 4.9.6
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

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


8 weeks ago

#8 @SergeyBiryukov
7 weeks ago

  • Milestone changed from 4.9.6 to 4.9.7

#9 @desrosj
5 weeks ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

Note: See TracTickets for help on using tickets.