WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#39933 closed defect (bug) (fixed)

WP_REST_Request::get_param() doesn't work for getting body params on DELETE requests

Reported by: mnelson4 Owned by: jnylen0
Milestone: 4.7.3 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch has-unit-tests fixed-major
Focuses: Cc:

Description

You cannot pass a request body on DELETE requests, although it seems to be admissible (see http://stackoverflow.com/questions/299628/is-an-entity-body-allowed-for-an-http-delete-request), and when deleting posts it should be possible (eg you can provide a "force" argument, but it gets ignored unless it's included in the querystring.)

The problem seems to be that WP_REST_Request::get_parameter_order() doesn't list DELETE as one of the methods that can pass a body in its variable $accepts_body_data.

Is the idea to just always provide the DELETE's parameters in the querystring? (eg that's how I saw it done here: https://github.com/WP-API/WP-API/issues/789#issuecomment-73986884). If so I guess that's ok although I couldn't find it documented. ...But also, just adding DELETE to the $accepts_body_data array is an easy change too, right? From my own testing that change got it to work how I had hoped.

Attachments (6)

0001-for-39933.-Allows-for-passing-a-request-body-in-DELE.patch (3.6 KB) - added by mnelson4 2 years ago.
allows you to pass request body on DELETE requests over the REST API
0002-use-WP-spacing-instead-of-PSR-spacing.patch (3.4 KB) - added by mnelson4 2 years ago.
fixes spacing issue from previous commit
0003-use-a-data-provider-to-make-rest-unit-test-code-a-bi.patch (2.0 KB) - added by mnelson4 2 years ago.
Use a data provider for rest request unit test
0004-blegh-add-a-phpdoc-comment-over-data-provider-just-e.patch (1.1 KB) - added by mnelson4 2 years ago.
adds a PHPdoc to the data provider method which I forgot in the previous commit
39933.5.diff (2.3 KB) - added by jnylen0 2 years ago.
Unify previous patch files; further tests cleanup
39933.6.diff (2.1 KB) - added by jnylen0 2 years ago.
A little more test cleanup

Download all attachments as: .zip

Change History (20)

#1 @jnylen0
2 years ago

  • Keywords needs-patch needs-unit-tests added

Nice find, @mnelson4 - I think it's just an oversight, and I'd like to get it fixed. Can you put up a patch and unit tests?

#2 follow-up: @swissspidy
2 years ago

According to the documentation, "Body parameters can and should be used for POST, PUT, and DELETE requests."

However, I recall some discussion where it was said it's not supported because many servers don't support it, or something like that. The only thing I could find right now was https://wordpress.slack.com/archives/core-restapi/p1455121876002422 though.

#3 @mnelson4
2 years ago

Can you put up a patch and unit tests?

sure @jnylen0 

I recall some discussion where it was said it's not supported because many servers don't support it, or something like that

@swissspidy I dunno. Do you think I should hold off making a patch until there is some more discussion?

#4 in reply to: ↑ 2 @jnylen0
2 years ago

Replying to swissspidy:

it's not supported because many servers don't support it, or something like that. The only thing I could find right now was https://wordpress.slack.com/archives/core-restapi/p1455121876002422 though.

This isn't a good argument not to support something, IMO. We support plenty of functionality that some servers may not allow, usually with appropriate fallbacks and documentation. One example is the _method parameter documented here.

Replying to mnelson4:

Do you think I should hold off making a patch until there is some more discussion?

I would go ahead and submit the patch, that will help us get it fixed more quickly.

#5 @swissspidy
2 years ago

@jnylen0 My comment wasn't an argument against this. I wanted to add some context/background so people know why something is that way.

@mnelson4
2 years ago

allows you to pass request body on DELETE requests over the REST API

#6 @mnelson4
2 years ago

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

#7 @jnylen0
2 years ago

  • Milestone changed from Awaiting Review to 4.7.3
  • Owner set to jnylen0
  • Status changed from new to accepted

Whitespace in the tests looks off, and we can use a PHPUnit data provider to make the tests cleaner.

Neither of those are difficult to fix. I'm not sure yet whether we can do it, but I'm going to look at getting this into 4.7.3.

#8 @mnelson4
2 years ago

Whitespace in the tests looks off,

Oh right, I think I used spaces instead of tabs. I can fix

we can use a PHPUnit data provider to make the tests cleaner.

Oh I actually wasn't even aware of data providers. Makes a lot of sense. This is one reason I'm glad to participate in core!

@mnelson4
2 years ago

fixes spacing issue from previous commit

@mnelson4
2 years ago

Use a data provider for rest request unit test

@mnelson4
2 years ago

adds a PHPdoc to the data provider method which I forgot in the previous commit

@jnylen0
2 years ago

Unify previous patch files; further tests cleanup

#9 @jnylen0
2 years ago

  • Keywords commit added

39933.5.diff looks good and tests out well for me. I merged the previous patch files into one (easier to deal with this way), did a bit more cleanup, and skipped the PHPDoc on the data provider method as I don't think it's necessary.

@jnylen0
2 years ago

A little more test cleanup

#10 @jnylen0
2 years ago

39933.6.diff is a slightly cleaner patch which avoids modifying the assert statement to include a message. This isn't really necessary because PHPUnit will still provide the failing request method for us:

1) Tests_REST_Request::test_non_post_body_parameters with data set #2 ('DELETE')
Failed asserting that null matches expected 'bar'.

#11 @jnylen0
2 years ago

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

In 40105:

REST API: Correctly parse body parameters for DELETE requests.

DELETE was inadvertently omitted from the list of non-POST HTTP methods that should be able to accept body parameters. Parameters passed to DELETE requests as JSON are already parsed correctly; this commit fixes application/x-www-form-urlencoded parameters as well.

Props mnelson4.
Fixes #39933.

#12 @jnylen0
2 years ago

  • Keywords fixed-major added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport to 4.7 branch.

#13 @mnelson4
2 years ago

39933.6.diff​ is a slightly cleaner patch which avoids modifying the assert statement to include a message. This isn't really necessary because PHPUnit will still provide the failing request method for us:

ah, cool. Also wasn't aware data sets provided that. Great.

#14 @SergeyBiryukov
2 years ago

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

In 40113:

REST API: Correctly parse body parameters for DELETE requests.

DELETE was inadvertently omitted from the list of non-POST HTTP methods that should be able to accept body parameters. Parameters passed to DELETE requests as JSON are already parsed correctly; this commit fixes application/x-www-form-urlencoded parameters as well.

Props mnelson4.
Merges [40105] to the 4.7 branch.
Fixes #39933.

Note: See TracTickets for help on using tickets.