Skip to content
This repository has been archived by the owner. It is now read-only.

Automatically validate params based of the Schema data #1128

Merged
merged 22 commits into from May 26, 2015

Conversation

@joehoyle
Copy link
Contributor

commented Apr 23, 2015

The basic idea here is to provide to "for free" validation based off the item schema. Looking at enums and data types to start with.

@joehoyle

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2015

@WP-API/amigos #reviewmerge

@danielbachhuber

This comment has been minimized.

Copy link
Member

commented Apr 23, 2015

Does it return all potential failures or just the first?

@joehoyle

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2015

@danielbachhuber The implementation of the validate_callback is to fail on the first, see https://github.com/WP-API/WP-API/pull/989/files#diff-887d62e7b2ec4bf35ee08d99831b5b4bR681

We could change that and bulk them all or multiple - though that should be a PR on top of https://github.com/WP-API/WP-API/pull/989/files#diff-887d62e7b2ec4bf35ee08d99831b5b4bR681 I think.

I think it could be a good idea - the only downside is slight performance hit I suppose.

@danielbachhuber

This comment has been minimized.

Copy link
Member

commented Apr 23, 2015

I think it makes sense to validate all at once. Consider submitting a form. If you did them one at a time, then the user would only get one error at a time and potentially hit 10+ error states before successfully submitting.

If it's too much of a lift, we can come back to it later. At the very least though, it would be nice if the response could support one or more error messages.

@joehoyle

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2015

As the response is a WP_Error I think multiple error is already supported from the server part.

@joehoyle joehoyle self-assigned this Apr 23, 2015
joehoyle added a commit that referenced this pull request Apr 23, 2015
This means clients can get lots of info at once, rather than one at a time, see #1128 (comment)
@joehoyle

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2015

Created #1131 to address multiple errors at once.

@joehoyle

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2015

I also added the same for sanitizing defaults for the schema to this PR

}
}
if ( $property['type'] === 'string' ) {

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Apr 24, 2015

Member

What about strings with HTML in them?

This comment has been minimized.

Copy link
@joehoyle

joehoyle Apr 24, 2015

Author Contributor

Good point, will remove this default.

This comment has been minimized.

Copy link
@joehoyle

joehoyle May 4, 2015

Author Contributor

Fixed in b344f5b

* @param string $parameter
* @return WP_Error|bool
*/
public function validate_schema_property( $value, $request, $parameter ) {

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber Apr 24, 2015

Member

It would be good to have a strong set of tests for this validate_schema_property() and the sanitize_schema_property() methods, independently of how they're used in our controllers.

This comment has been minimized.

Copy link
@joehoyle

joehoyle Apr 24, 2015

Author Contributor

agreed

This comment has been minimized.

Copy link
@joehoyle

joehoyle May 4, 2015

Author Contributor

Added in ca7d320

@rachelbaker

This comment has been minimized.

Copy link
Member

commented Apr 27, 2015

I like the approach here, but I don't think we can introduce another enhancement this late into Beta 1. Moving to Beta 2

@rachelbaker rachelbaker added this to the 2.0 Beta 2 milestone Apr 27, 2015
@joehoyle

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2015

@WP-API/amigos ready for a #reviewmerge again

@danielbachhuber

This comment has been minimized.

Copy link
Member

commented May 5, 2015

Looks good to me. I'll wait for an assist from Ryan or Rachel.

WP_REST_Controller is getting a bit hairy though. I think it's time for more abstraction.

@joehoyle

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2015

@rmccue @rachelbaker fancy merging / any other thoughts? :)

@rachelbaker

This comment has been minimized.

Copy link
Member

commented May 25, 2015

@joehoyle is going to resolve the merge conflicts so this can be merged.

joehoyle added 2 commits May 25, 2015
Conflicts:
	tests/test-rest-users-controller.php
rachelbaker added a commit that referenced this pull request May 26, 2015
Automatically validate params based of the Schema data
@rachelbaker rachelbaker merged commit 887a868 into develop May 26, 2015
3 checks passed
3 checks passed
Scrutinizer 18 new issues, 16 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on develop at 87.87%
Details
@rachelbaker rachelbaker deleted the validate-based-off-schema branch May 26, 2015
@rachelbaker

This comment has been minimized.

Copy link
Member

commented May 26, 2015

Merged #1128

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.