Make WordPress Core

Opened 7 years ago

Last modified 6 years ago

#43844 reviewing defect (bug)

PHP list language construct changed behaviour in PHP 7

Reported by: javorszky's profile javorszky Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Awaiting Review 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 7 years ago.
Inline documentation fix for WP_List_Table::get_sortable_columns

Download all attachments as: .zip

Change History (13)

@javorszky
7 years ago

Inline documentation fix for WP_List_Table::get_sortable_columns

#1 @javorszky
7 years ago

  • Keywords has-patch added

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


7 years ago

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


7 years ago

#8 @SergeyBiryukov
7 years ago

  • Milestone changed from 4.9.6 to 4.9.7

#9 @desrosj
7 years ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#10 @pento
6 years 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
6 years ago

  • Milestone changed from 4.9.8 to Awaiting Review

#12 @jrf
6 years 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 6 years ago by jrf (previous) (diff)
Note: See TracTickets for help on using tickets.