Passing orderby as array in URL results in Notice

[NomNom99]

Visiting /wp-admin/edit.php?post_type=page&orderby[menu_order]=ASC&orderby[title]=ASC
or
/wp-admin/edit.php?post_type=page&orderby%5Bmenu_order%5D=ASC&orderby%5Btitle%5D=ASC (URL encoded version) throws the following notice:

 Notice: Array to string conversion in /Users/siddharth/Sites/osp/app/public/wp-includes/formatting.php on line 1104

Since multiple values can be passed to orderby in get_posts() or WP_Query(), this behaviour should also be consistent via URL.

There error occurs at this line: ​https://github.com/WordPress/WordPress/blob/master/wp-admin/includes/class-wp-list-table.php#L391 because esc_attr() expects a string but $_REQUEST['orderby'] is an array.

[peterwilsoncc]

Hi @NomNom99 and welcome back to trac!

This looks like something that was missed in [29027] for #17065 so I've updated the version number accordingly.

Ordering the list table via an array needs to be considered when rendering the hidden fields in the search form.

@NomNom99 Is this a patch you'd feel comfortable working on?

[NomNom99]

Thanks for looking into this @peterwilsoncc

I'll spend some time next week to raise a patch.

Trac ticket: https://core.trac.wordpress.org/ticket/59494

[LeonidasMilossis]

Props to @pls78 as we paired on this.

We think that we have to cover multiple formats of arrays in query parameters. There are 2 types of formats we could think of:

• /wp-admin/edit.php?post_type=page&orderby[menu_order]=ASC&orderby[title]=ASC - the OP's format
• /wp-admin/edit.php?post_type=page&orderby[]=menu_order&orderby[]=title&order=asc - a different format we noticed that also works

The first one makes $_REQUEST['orderby'] into:

array(2) {
  ["menu_order"]=>
  string(3) "ASC"
  ["title"]=>
  string(3) "ASC"
}

and the second one makes it into:

array(2) {
  [0]=>
  string(10) "menu_order"
  [1]=>
  string(11) "menu_order2"
}

So, we created a PR that covers both.

Hope this helps!

[LeonidasMilossis]

Applied @peterwilsoncc's feedback in the PR and have it in more refined state (I hope).

The current functionality added can be described likeso:

Like said above, an orderby parameter can be passed as an array in two ways:

• /wp-admin/edit.php?post_type=page&orderby[menu_order]=ASC&orderby[title]=ASC - the "associative" way
• /wp-admin/edit.php?post_type=page&orderby[]=menu_order&orderby[]=title&order=asc - the "non-associative" way

The PR adds support for both.

To be more complete, for each case it adds hidden fields for each element of the array in the following way:

<input type="hidden" name="orderby[menu_order]" value="ASC">
<input type="hidden" name="orderby[title]" value="ASC">

for the "associative" way

and

<input type="hidden" name="orderby[0]" value="menu_order">
<input type="hidden" name="orderby[1]" value="title">
<input type="hidden" name="order" value="asc">

for the "non-associative" way.

[rajinsharwar]

I have checked on the PR, and it does look good to me. Wondering, if should we update or add new unit tests for this scenario.

[peterwilsoncc]

The source changes in the ​linked pull request look good to me.

I don't think any tests will need to be updated but adding some to make sure the search box doesn't through a notice would be helpful. The tests would need to set $_REQUEST values directly and prior to calling the search_box method.

This is a followup to the ​https://github.com/WordPress/wordpress-develop/pull/5404, with unit tests.

Trac ticket: https://core.trac.wordpress.org/ticket/59494

Using wp_remote_* is not really the right approach here. If you want to test the frontend output like this, you would write an e2e test. However, this change here does not really warrant an e2e test. A simple unit test that tests the method is good enough. Doing an HTTP call is not really a unit test.

Look at the other unit tests in tests/phpunit/tests/admin/wpListTable.php and how they're written.

You'll want a simple test that calls the search_box() method and verifies the output is as expected.

The structure could look like this:

/**
         * Tests that `WP_List_Table::search_box()` supports multiple orderby fields.
         *
         * @ticket 59494
         *
         * @covers WP_List_Table::search_box
         */
        public function test_search_box_supports_multiple_orderby() {
                $_REQUEST['s'] = 'search term';

                $output = get_echo( array( $this->list_table, 'search_box' ), array( 'foo', 'bar' ) );

                $this->assertStringContainsString( 'Write your expected string here', $output );
        }

Thank you so much @swissspidy. I was specifically looking for a function like get_echo(), which I was unaware of its existence. If you can take a look at my changed tests, please. 🙂. Thanks again!

[rajinsharwar]

Unit tests are now passing. Above I have tested this patch as well, so I guess it will be a fairly small change to be committed in this cycle. Adding 'commit' so that this can be merged before Beta 1 probably.

Thanks for your suggestion @peterwilsoncc. I have added another test for the single orderby. Let me know if it fits right.

@peterwilsoncc I see. I have committed to the changes. Also, I kept the test for one element array as well. Let me know if this looks good to you. :)

[peterwilsoncc]

In 58379:

Administration: Prevent an orderby array throwing a notice.

Prevent WP_List_Table::search_box() from throwing an array to string conversion notice when post list tables are loaded with an array of orderby parameters in the URL, eg: /wp-admin/edit.php?post_type=page&orderby[menu_order]=ASC&orderby[title]=ASC.

Follow up to [29027].

Props leonidasmilossis, rajinsharwar, swissspidy, NomNom99, pls78, SergeyBiryukov.
Fixes #59494.
See #17065.

Committed in r58379 / ​https://github.com/WordPress/wordpress-develop/commit/8cd24bc12c7ac3c588ccbf549b1df42884181fe0

Committed in r58379 / ​https://github.com/WordPress/wordpress-develop/commit/8cd24bc12c7ac3c588ccbf549b1df42884181fe0