Make WordPress Core

Opened 3 years ago

Last modified 7 weeks 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 needs-unit-tests needs-patch
Focuses: rest-api Cc:
PR Number:


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 3 years ago.
40838.1.patch (2.8 KB) - added by vagios 2 years ago.

Download all attachments as: .zip

Change History (12)

#1 @jnylen0
3 years ago

  • Keywords good-first-bug added

3 years ago

#2 @vagios
3 years 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 3 years ago by vagios (previous) (diff)

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

2 years ago

#4 @mnelson4
2 years 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
2 years ago

  • Keywords needs-refresh added; has-patch removed

Seems like the patch no longer applies.

2 years ago

#6 @vagios
2 years ago

  • Keywords needs-refresh removed

In 40838.1.patch:

  • refresh against trunk
  • unit tests are still successful

#7 @vagios
2 years ago

  • Keywords has-patch added

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

2 years ago

#9 @DrewAPicture
2 years ago

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

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

#10 @TimothyBlynJacobs
7 weeks ago

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

Apologies @vagios for letting this slip away.

Looking at the patch again, I think we'd want to stop after updating the parameter for the first parameter type instead of overwriting it for all types. That way we retain as much of the original semantics of the request as possible.

I also wonder if we need to skip checking the defaults parameter type. I don't think we should override the default values if someone calls set_param() with a parameter that was initially provided in the request. In that case, I think we'd fall back to the first parameter type in the order.

Note: See TracTickets for help on using tickets.