Skip to content

REST API: Split comma-separated methods given in array form. - #13136

Open
moonmeister wants to merge 1 commit into
WordPress:trunkfrom
moonmeister:fix/rest-array-methods-comma-split
Open

REST API: Split comma-separated methods given in array form.#13136
moonmeister wants to merge 1 commit into
WordPress:trunkfrom
moonmeister:fix/rest-array-methods-comma-split

Conversation

@moonmeister

Copy link
Copy Markdown

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

Problem

WP_REST_Server::register_route() accepts methods as a string or an array, but only the string form is split on commas. Any array element containing a comma becomes a single unmatchable method key.

register_rest_route( 'ns/v1', '/thing', array(
	'methods'             => array( WP_REST_Server::READABLE, WP_REST_Server::EDITABLE ),
	'callback'            => '__return_true',
	'permission_callback' => '__return_true',
) );

registers the keys 'GET' and 'POST, PUT, PATCH'. GET works; POST, PUT and PATCH all 404. The same route written as the string 'GET, POST, PUT, PATCH' works. Nothing warns at registration time.

Dates to the array support added in 4.4.

Fix

Split the array branch as well, merging each element's comma-separated parts.

A comma is a delimiter in the RFC 9110 token grammar, so no valid HTTP method name can contain one. Splitting can therefore only turn a permanently-dead method key into working ones — there is no case where the current behavior is the desired one.

The loop is deliberate rather than explode( ',', implode( ',', $handler['methods'] ) ): imploding an empty array yields '', which would register an empty-string method key. test_route_method_empty_array() guards that.

Tests

Four tests added to the existing test_route_method_* group, which covered an array of methods and a comma-separated string but not their intersection:

Test On trunk
test_route_method_array_with_comma_separated_values fails
test_route_method_array_with_multi_method_constant fails
test_route_method_array_with_multi_method_constant_dispatches fails (404 instead of 200)
test_route_method_empty_array passes — regression guard

--group restapi: 3554 tests / 16246 assertions, no new failures against a 3550 / 16239 baseline (identical pre-existing error and warning profile).

Impact

A plugin-directory regex sweep found 4 occurrences across 3 plugins (~2,000 installs) — all array( CREATABLE, EDITABLE ), where POST works and the intended PUT/PATCH silently 404. The array form itself is common (921 plugins), but only 87 of 7,024 array-form methods lines name a WP_REST_Server:: constant at all, which is why the bug has stayed quiet.

The larger reason to fix it is forward-looking. This was found while measuring what would happen if a method constant gained a second method — as it would if QUERY (RFC 10008) were added to READABLE or ALLMETHODS. Every array-form registration naming that constant would break at once, with no error. Landing this removes that constraint before the question comes up.

`WP_REST_Server::register_route()` accepts `methods` as either a string or an
array, but only the string form was split on commas. Any array element
containing one became a single unmatchable method key, so
`array( WP_REST_Server::READABLE, WP_REST_Server::EDITABLE )` registered the
literal key `'POST, PUT, PATCH'` and `POST`, `PUT` and `PATCH` all returned
404, with no notice at registration time. The same route written as a string
worked correctly.

Splitting the array branch is safe unconditionally: a comma is a delimiter in
the RFC 9110 `token` grammar, so no valid HTTP method name can contain one.
Splitting can only turn a permanently-dead method key into working ones. The
empty-array case is preserved by building up the list rather than imploding.

Adds four tests to the existing `test_route_method_*` group, which covered an
array of methods and a comma-separated string but not their intersection.
Three fail on trunk; the fourth guards the empty-array behavior.

Fixes #65905.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props moonmeister.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions

Copy link
Copy Markdown

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant