WordPress.org

Make WordPress Core

Opened 12 months ago

Last modified 4 months ago

#40838 assigned defect (bug)

REST API: Inconsistent parameter type handling in `set_param()`

Reported by: jnylen0 Owned by: vagios
Milestone: Future Release Priority: normal
Severity: minor Version: 4.4
Component: REST API Keywords: good-first-bug has-unit-tests has-patch
Focuses: rest-api Cc:

Description

Calling set_param() on a REST API WP_REST_Request object may lead to unexpected results. As of #40344, calling get_param() after set_param() should always work correctly, but the following situation could be improved further:

  1. A JSON request comes in with a parameter set in the query string (for example, ?key=value).
  2. set_param( 'key', 'new_value' ) is called on this request object. Since JSON is first in get_parameter_order(), the request will now contain the following parameters:
  • get_json_params(): array( 'key' => 'new_value' )
  • get_query_params(): array( 'key' => 'value' )

Instead, I would expect that set_param() loops through the parameter order, first checking if the given parameter exists in any parameter type, and updating it if so. Otherwise, it should set the parameter using the first parameter type as a last resort.

Attachments (2)

40838.patch (3.1 KB) - added by vagios 11 months ago.
40838.1.patch (2.8 KB) - added by vagios 6 months ago.

Download all attachments as: .zip

Change History (11)

#1 @jnylen0
11 months ago

  • Keywords good-first-bug added

@vagios
11 months ago

#2 @vagios
11 months ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

In 40838.patch,

  • set_param() function now updates the value of the given key if it exists in any parameter type. It will create a new param in the first parameter type if the key doesn't exist anywhere.
  • Add unit tests.
  • Update function documentation to describe the new behaviour.
Last edited 11 months ago by vagios (previous) (diff)

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


6 months ago

#4 @mnelson4
6 months ago

In today's meeting, it was suggested that this could go into a point release, seeing how WP 5.0 may be a ways off, and this change is very unlikely to be a breaking change.

#5 @TimothyBlynJacobs
6 months ago

  • Keywords needs-refresh added; has-patch removed

Seems like the patch no longer applies.

@vagios
6 months ago

#6 @vagios
6 months ago

  • Keywords needs-refresh removed

In 40838.1.patch:

  • refresh against trunk
  • unit tests are still successful

#7 @vagios
6 months ago

  • Keywords has-patch added

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


6 months ago

#9 @DrewAPicture
4 months ago

  • Owner set to vagios
  • Status changed from new to assigned

Assigning to mark the good-first-bug as "claimed".

Note: See TracTickets for help on using tickets.