Allow `rest_validate_request_arg()` to validate arrays by JPry · Pull Request #2397 · WP-API/WP-API

JPry · 2016-03-21T00:50:58Z

This adds array validation to rest_validate_request_arg() as described in #2194. Unit tests have been adeed for the new type.

danielbachhuber · 2016-03-21T02:11:15Z

 		}

+		if ( 'array' === $args['type'] ) {
+			// A comma-separated array of IDs could be acceptable instead of an array.


This isn't the right abstraction.

JSON Schema permits us to specify two different types for a given property. In the case of anything using wp_parse_id_list(), we need to specify type => [ 'string', 'array' ], and then properly validate an array of types.

JSON Schema permits us to specify two different types for a given property. In the case of anything using wp_parse_id_list(), we need to specify type => [ 'string', 'array' ], and then properly validate an array of types.

I just want to make sure I'm understanding you correctly. In the case of type => [ 'string', 'array' ], we should ideally be checking for an array, and then ensuring that each element is a string? Or as another example, type => [ 'integer', 'array' ] would mean checking for an array of integers?

In JSON Schema, I believe the way to define an arrays type is like so

"options": { "type": "array", "minItems": 1, "items": { "type": "string" }, "uniqueItems": true },

See http://json-schema.org/example2.html

danielbachhuber · 2016-03-21T02:12:16Z

@JPry: @BE-Webdesign had already started a PR for this issue in #2361. Which of you wants to take this all of the way through?

BE-Webdesign · 2016-03-21T02:14:45Z

Doesn't matter to me, I'm happy to refine my PR or have somebody do a much better one 😃!

JPry · 2016-03-21T14:07:12Z

I don't mind if @BE-Webdesign's PR goes forward instead of mine. I just happened to have a couple of hours available and wanted to help out, so I picked a couple of issues in the Beta 13 milestone.

JPry · 2016-03-21T14:10:13Z

I do have an hour or 2 this morning to work on this, so maybe I'll see how far I can get 😄

joehoyle · 2016-10-11T16:27:19Z

@JPry is there any update that needs to be added here, or is this changed direction and needs to be closed out?

JPry · 2016-10-12T15:52:50Z

@joehoyle Give me a day or two to review this, and then we can close it out if nothing has changed.

@see

* develop: (200 commits) Fix extra space Fix code standards issues from our PHPCS rules Revert "Fixed styling issues. Maybe." Properly register "{taxonomy}_exclude" parameters in the schema Fixed styling issues. Maybe. Fix error when a single meta value attempts to update to the current value. Adds test. Correct @see url for WP_Query Revert 6e1f9ba Remove conditional check on the `unfiltered_html` cap for discussion Remove temporary fix for upstream bug 35614. Documentation and code standards for `WP_REST_Terms_Controller`. Update changelog with changes since beta14 Bump plugin version to 2.0beta15. Test theory that the problem is caused by and admin updating a comment as a different user Add back string type checking to content values Pass current filter into wp_kses Switch to `wp_filter_kses()` in an attempt to avoid having to pass global allowed tags Handle multiple duplicate values correctly Correct sanitization kses functions and re-add failing update content test I hate PHP 5.3 and lower ...

JPry · 2016-10-13T11:57:00Z

I made some updates here based on this comment. If we're following the JSON schema as @joehoyle mentioned, then the changes I've made seem to be the right way forward.

At this point I'm looking for some feedback on the approach I'm using. If what I've done here isn't the right way to go about this, then I would appreciate some input about what a better method might be.

I'm also curious about the failed tests from when I merged develop into my branch. I didn't have a chance to examine the tests in-depth yet, but a cursory examination makes it seem like it may not be my changes that caused the failures.

joehoyle · 2016-10-14T14:22:47Z

@JPry thanks for expanding on this, so I think this works, however i think it could be more elegant. If we have a function validate_value_by_schema( $object, $schema ), we could have this function recursively call it's self in the event of finding an array. This would also allow us to deep check json schema objects too, I think.

I have an idea of how this could work, I'm happy to adapt this to try that direction.

JPry · 2016-10-14T14:41:27Z

Thanks for the idea @joehoyle. I'd be willing to make some updates to this based on your idea and what @BE-Webdesign mentioned in slack today. For my own reference, here are the relevant links from slack:

https://wordpress.slack.com/archives/core-restapi/p1476455590007085
https://wordpress.slack.com/archives/core-restapi/p1476455898007096

BE-Webdesign · 2016-10-14T14:42:52Z

+			// Map values to the correct type, defaulting to string as the type.
+			$type = isset( $args['items']['type'] ) ? $args['items']['type'] : 'string';
+			foreach ( $value as &$_value ) {
+				settype( $_value, $type );


Rather than using settype we should use our own validation systems because settype can lead to the wrong information. We can get away with settype though as most of the endpoints only deal with arrays of strings or integers. If this PR plans to handle booleans then this will cause a problem when values like 'false' are passed into an array booleans.

BE-Webdesign · 2016-10-14T14:51:08Z

+				'somecsv'         => array(
+					'type'        => 'array',
+					'allow_csv'   => true,
+				),


We need to test against the actual JSON Schema here. So we need to have items added. In a PR I did that I ultimately did not like I had schema like this.

someIdList => array( 'type' => 'array', 'items' => array( 'type' => 'integer' ), 'minItems' => 1, 'uniqueItems' => true, ),

Then the same thing for string lists. Currently in the endpoints most of the arrays are id lists and only a few are string lists.

BE-Webdesign · 2016-10-14T14:59:00Z

+				case 'boolean':
+					$function = 'rest_is_boolean';
+					break;
+


I wouldn't worry about supporting boolean because there are no endpoint parameters that use an array of booleans. This PR won't handle booleans 100% properly so let's not check for them.

BE-Webdesign · 2016-10-14T15:01:39Z

+				case 'string':
+					$function = 'is_string';
+					break;
+			}


This should have a default case so there is no fall through, currently in the API we use super loose validation and most things will pass for true by default. I believe this PR is returning false for all of these values, that fall through.

BE-Webdesign · 2016-10-14T15:03:28Z

+			'default'           => array(),
+			'sanitize_callback' => 'wp_parse_id_list',
+			'validate_callback' => 'rest_validate_request_arg',
 		);


The items schema needs to be added into all of the array parameters so that your validation is actually working. This is why all of the tests are failing because the schema is not set yet. Every include/exclude author_include/exclude parameter needs it and I believe user roles. There are some other ones that I can't remember.

JPry added 4 commits March 20, 2016 20:46

Add array/csv validation

11cc3a0

Add validate_callback params where needed

4c64be1

Add array callback unit tests

cd8b0fe

Add sanitization for arrays

e736101

danielbachhuber reviewed Mar 21, 2016
View reviewed changes

kadamwhite mentioned this pull request Sep 13, 2016

Combine Validation & Sanitization callbacks #2695

Open

joehoyle added the Reporter Feedback label Oct 11, 2016

JPry added 2 commits October 12, 2016 18:19

Include additional processing for array types

40ab1b6

BE-Webdesign reviewed Oct 14, 2016

View reviewed changes

JPry closed this May 26, 2017

Conversation

JPry commented Mar 21, 2016

Uh oh!

danielbachhuber Mar 21, 2016

Choose a reason for hiding this comment

Uh oh!

JPry Mar 21, 2016

Choose a reason for hiding this comment

Uh oh!

joehoyle Aug 17, 2016

Choose a reason for hiding this comment

Uh oh!

danielbachhuber commented Mar 21, 2016

Uh oh!

BE-Webdesign commented Mar 21, 2016

Uh oh!

JPry commented Mar 21, 2016

Uh oh!

JPry commented Mar 21, 2016

Uh oh!

joehoyle commented Oct 11, 2016

Uh oh!

JPry commented Oct 12, 2016

Uh oh!

JPry commented Oct 13, 2016

Uh oh!

joehoyle commented Oct 14, 2016

Uh oh!

JPry commented Oct 14, 2016

Uh oh!

BE-Webdesign Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BE-Webdesign Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BE-Webdesign Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BE-Webdesign Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

BE-Webdesign Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

BE-Webdesign Oct 14, 2016 •

edited

Loading

BE-Webdesign Oct 14, 2016 •

edited

Loading

BE-Webdesign Oct 14, 2016 •

edited

Loading