Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#38586 closed task (blessed) (fixed)

REST API: Allow arrays to be sent as CSVs

Reported by: pento Owned by: pento
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: REST API Keywords: needs-docs
Focuses: Cc:


After #38531, it'd be helpful if arrays could be sent as CSVs.

Related: #38553, #38577, #38579.

Change History (13)

#1 @pento
5 years ago

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

#2 @pento
5 years ago

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

In 39048:

REST API: Allow parameters defined as array to be sent as CSVs.

This allows parameters that are often handled as CSVs to be properly parsed.

Fixes #38586.

#3 @rmccue
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This needs to be documented further, and we should check whether this is the best way to implement it. Right now, there's no hint to the client that "array" means you can send CSV as well. Potentially, using JSON Schema's anyOf would be better here.

#4 @pento
5 years ago

I'm cool with whatever option. Everywhere endpoint parameters have the type array, we also want to allow CSV.

Actually, I'd be fine with just documenting that, and leaving it, particularly as changing the type in the endpoint docs will be confusing. Apps that care about JSON Schema can send actual arrays, apps that don't can send either.

#5 @rmccue
5 years ago

I'm OK with that too, so long as it's documented. If we can come up with a standard, self-documenting way though, probably best to do that.

#6 @TimothyBlynJacobs
5 years ago

I would really like to see anyOf or oneOf, etc. being used. Currently I'm going to need to add my own validation callbacks for handling those more complex schema, but it'd be great if something like that is handled in core.

#7 @pento
5 years ago

  • Keywords needs-docs added

Updating schema.html to say "array or CSV" will do the job, but I don't know what the plan is for merging the API docs to devhub.

#8 @joehoyle
5 years ago

Hmm I'm not too keen on this approach, and I think this was committed without too much consideration to different types etc. It appears this would also need a modification to the sanitizer also, as right now, it doesn't look like this is doing both validation and sanitization.

Just because the value isn't an array doesn't mean we can pass it to preg_split, what if it's other types?

#9 @joehoyle
5 years ago

In 39061:

REST API: Sanitize arrays being sent as CSVs.

In #38586 the ability to parse arrays as csv was introduced, however it didn't add any support for validating csv arrays. This adds such sanitization, and also a good amount of unit tests for all sanitization baed off schema.

See #38586.

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

5 years ago

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

5 years ago

#12 @joehoyle
5 years ago

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

In #38617 I audited all the endpoints to make sure we are using array / type combos wherever we can. These all support CSVs by default now, thanks to this ticket - given we haven't run into any issues in all those instances I think it's safe to say this was a good thing to do so I'm closing it out.

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

#13 @SergeyBiryukov
3 years ago

In 44703:

Build/Test Tools: Correct WP_Test_REST_Schema_Sanitization::test_type_string() to check for both 1.1 float and '1.10' string explicitly.

Previously, the test only passed due to a bug in PHPUnit 7.1.x and older versions.

Fixes #43218. See #38586.

Note: See TracTickets for help on using tickets.