Make WordPress Core

Opened 9 months ago

Closed 11 days ago

Last modified 11 days ago

#59494 closed defect (bug) (fixed)

Passing orderby as array in URL results in Notice

Reported by: nomnom99's profile NomNom99 Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.6 Priority: normal
Severity: normal Version: 4.0
Component: Administration Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

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.

Change History (18)

#1 @peterwilsoncc
9 months ago

  • Version changed from 6.3.1 to 4.0

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?

#2 @NomNom99
9 months ago

Thanks for looking into this @peterwilsoncc

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

This ticket was mentioned in PR #5404 on WordPress/wordpress-develop by @LeonidasMilossis.


9 months ago
#3

  • Keywords has-patch added

#4 @LeonidasMilossis
9 months ago

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!

#5 @SergeyBiryukov
2 months ago

  • Milestone changed from Awaiting Review to 6.6

#6 @LeonidasMilossis
2 months ago

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.

#7 @rajinsharwar
2 months ago

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.

#8 @peterwilsoncc
2 months ago

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 ticket was mentioned in PR #6439 on WordPress/wordpress-develop by @rajinsharwar.


2 months ago
#9

  • Keywords has-unit-tests added

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


8 weeks ago

@swissspidy commented on PR #6439:


8 weeks ago
#11

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 );
        }

@rajinsharwar commented on PR #6439:


4 weeks ago
#12

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!

#13 @rajinsharwar
4 weeks ago

  • Keywords commit added

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.

@rajinsharwar commented on PR #6439:


4 weeks ago
#14

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

@rajinsharwar commented on PR #6439:


11 days ago
#15

@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. :)

#16 @peterwilsoncc
11 days ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.