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

[jnylen0]

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.

[vagios]

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.

[mnelson4]

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.

[TimothyBlynJacobs]

Seems like the patch no longer applies.

[vagios]

In 40838.1.patch​:

• refresh against trunk
• unit tests are still successful

[DrewAPicture]

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

[TimothyBlynJacobs]

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.

[mnelson4]

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

<!--
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:

• FAQs for New Contributors: ​https://make.wordpress.org/core/handbook/tutorials/faq-for-new-contributors/
• Contributing with Code Guide: ​https://make.wordpress.org/core/handbook/contribute/
• WordPress Coding Standards: ​https://make.wordpress.org/core/handbook/best-practices/coding-standards/
• Inline Documentation Standards: ​https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/
• Browser Support Policies: ​https://make.wordpress.org/core/handbook/best-practices/browser-support/
• Proper spelling and grammar related best practices: ​https://make.wordpress.org/core/handbook/best-practices/spelling/

-->

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

[mnelson4]

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.

[mnelson4]

@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 😃

[mnelson4]

Ok PR updated. Ready for a re-review

[mnelson4]

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.

[kadamwhite]

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.

[prbot]

​kadamwhite commented on ​PR #181:

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