WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 months ago

Last modified 2 months ago

#40838 closed defect (bug) (fixed)

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

Reported by: jnylen0 Owned by: vagios
Milestone: 5.5 Priority: normal
Severity: minor Version: 4.4
Component: REST API Keywords: good-first-bug
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 3 years ago.
40838.1.patch (2.8 KB) - added by vagios 3 years ago.

Download all attachments as: .zip

Change History (26)

#1 @jnylen0
3 years ago

  • Keywords good-first-bug added

@vagios
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.
Version 0, edited 3 years ago by vagios (next)

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


3 years ago

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

  • Keywords needs-refresh added; has-patch removed

Seems like the patch no longer applies.

@vagios
3 years ago

#6 @vagios
3 years ago

  • Keywords needs-refresh removed

In 40838.1.patch:

  • refresh against trunk
  • unit tests are still successful

#7 @vagios
3 years ago

  • Keywords has-patch added

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


3 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
6 months 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.

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


3 months ago

#12 @mnelson4
3 months ago

I will try to make an updated patch for this before March 18th

This ticket was mentioned in PR #181 on WordPress/wordpress-develop by mnelson4.


3 months ago

<!--
Hi there! Thanks for contributing to WordPress!

Pull Requests in this GitHub repository must be linked to a ticket in the WordPress Core Trac instance (https://core.trac.wordpress.org), and are only used for code review. No pull requests will be merged on GitHub.

See the WordPress Handbook page on using PRs for Code Review more information: https://make.wordpress.org/core/handbook/contribute/git/github-pull-requests-for-code-review/

If this is your first time contributing, you may also find reviewing these guides first to be helpful:

-->

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

#14 @mnelson4
3 months ago

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.

It only overwrites types where it was set. set_param is vague as to which type you're setting, so I think it's appropriate to overwrite it for each that's previously set it.

Eg, if foo is set in the body and GET parameters, calling $request->set_param('foo','new_value'); will only overwrite it in the body and GET parameters, but none of the others.

I also wonder if we need to skip checking the defaults parameter type.

Agreed, the PR https://github.com/WordPress/wordpress-develop/pull/181 does that.

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


3 months ago

#16 @kadamwhite
3 months ago

  • Milestone changed from Future Release to 5.5

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


2 months ago

#18 @mnelson4
2 months ago

@TimothyBlynJacobs has provided some feedback on the pull request, all of which sounds good to me, so I'll be updating the pull request hopefully Monday or Tuesday 😃

#19 @mnelson4
2 months ago

  • Keywords needs-unit-tests needs-patch removed

Ok PR updated. Ready for a re-review

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


2 months ago

#21 @mnelson4
2 months ago

d'oh! There was a failing test that I missed... because I had fixed it locally but forgot to push 😖

Anyways, @TimothyBlynJacobs I think this is good-to-go.

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


2 months ago

#23 @kadamwhite
2 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 47559:

REST API: Handle parameter types consistently within set_param().

A request has multiple parameter types, including "query" and "json." Updating a parameter could previously modify a key's value in the wrong parameter type, leading to confusing and self-contradictory response objects.

Props mnelson4, TimothyBlynJacobs, vagios, jnylen0.
Fixes #40838.

#24 @prbot
2 months ago

kadamwhite commented on PR #181:

Committed in [changeset 47559](https://core.trac.wordpress.org/changeset/47559), thank you @mnelson4

Note: See TracTickets for help on using tickets.