WordPress.org

Make WordPress Core

Opened 22 months ago

Last modified 18 months ago

#43844 reviewing defect (bug)

PHP list language construct changed behaviour in PHP 7

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

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 22 months ago.
Inline documentation fix for WP_List_Table::get_sortable_columns

Download all attachments as: .zip

Change History (13)

@javorszky
22 months ago

Inline documentation fix for WP_List_Table::get_sortable_columns

#1 @javorszky
22 months ago

  • Keywords has-patch added

#2 @javorszky
22 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
22 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
22 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.


22 months ago

#6 @SergeyBiryukov
22 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.


22 months ago

#8 @SergeyBiryukov
22 months ago

  • Milestone changed from 4.9.6 to 4.9.7

#9 @desrosj
21 months ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#10 @pento
19 months ago

@SergeyBiryukov: Are you planning on landing this in 4.9.8? It's been punted a couple of times, it should probably be moved to Awaiting Review.

#11 @SergeyBiryukov
19 months ago

  • Milestone changed from 4.9.8 to Awaiting Review

#12 @jrf
18 months ago

@javorszky Please help me out. I've been looking at the code related to this and I cannot find the code using the PHP list construct which is related to this behaviour which you refer to.

The class calls the get_sortable_columns() method on line 1019, filters it and then walks through it and correctly takes both string as well as array input into account as is attested by the $data = (array) $data on line 1038 and will set array key 1 to false if it's not found, so it's quite unclear to me how what you refer to is even possible (error-wise).

See: https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-wp-list-table.php#L1019-L1044

Other than that, neither of the two PHP changes you refer to are relevant in any case, not even when $sortable is used in the print_column_headers() method (line 1117).

The first one "Strings can no longer be unpacked" is irrelevant as, as per the above, it will always be an array.
What is meant by that PHP native change is that you can no longer do this:

<?php
list( $char1, $char2, $char3 ) = 'cat';

The second one "List order change" is irrelevant as well, as that only applies to lists assigning the variables into an array without setting the key or to the same variable twice, i.e.:

<?php
list( $a[], $a[] ) = array( 'cat', 'dog' );
list( $animal, $animal ) = array( 'cat', 'dog' );

are both affected, but

<?php
list( $miauw, $woof ) = array( 'cat', 'dog' );

would not be affected.

Last edited 18 months ago by jrf (previous) (diff)
Note: See TracTickets for help on using tickets.