Skip to content
This repository was archived by the owner on Sep 24, 2018. It is now read-only.

Conversation

@joehoyle
Copy link
Member

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
Copy link
Member Author

@WP-API/amigos #reviewmerge

@danielbachhuber
Copy link
Member

Does it return all potential failures or just the first?

@joehoyle
Copy link
Member Author

@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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

Created #1131 to address multiple errors at once.

@joehoyle
Copy link
Member Author

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about strings with HTML in them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will remove this default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b344f5b

@rachelbaker
Copy link
Member

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
Copy link
Member Author

joehoyle commented May 4, 2015

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

@danielbachhuber
Copy link
Member

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
Copy link
Member Author

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

@rachelbaker
Copy link
Member

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

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
@rachelbaker rachelbaker deleted the validate-based-off-schema branch May 26, 2015 00:03
@rachelbaker
Copy link
Member

Merged #1128

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants