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

WIP for Additional Fields in responses #927

Merged
merged 23 commits into from May 5, 2015

Conversation

@joehoyle
Copy link
Contributor

commented Feb 25, 2015

No description provided.

@rachelbaker rachelbaker added this to the 2.0 Beta 1 milestone Apr 23, 2015
@rachelbaker rachelbaker modified the milestone: 2.0 Beta 1 Apr 27, 2015
@rachelbaker rachelbaker modified the milestones: 2.0 Beta 1, 2.0 Beta 2 Apr 27, 2015
continue;
}
$result = call_user_func( $field_options['update_callback'], $request[$field_name], $object, $field_name, $request );

This comment has been minimized.

Copy link
@rmccue

rmccue Apr 29, 2015

Member

We don't actually do anything with this result. If it's a WP_Error, we should return it upwards and break the loop immediately.

This comment has been minimized.

Copy link
@joehoyle

joehoyle May 4, 2015

Author Contributor

This seems to contradict your comment below:

I'm fine with us treating each field as atomic by itself though, rather than the whole set of fields as atomic (i.e. one failure doesn't break immediately).

I think the fields should be treat in insolation, if a given field fails to update, we shouldn't discriminate against other addition fields by other plugins etc.

@@ -208,6 +208,9 @@ public function create_item( $request ) {
}
$user->ID = $user_id;
}
$this->update_additional_fields_for_object( $user, $request );

This comment has been minimized.

Copy link
@rmccue

rmccue Apr 29, 2015

Member

Make sure we add checks here when we add WP_Error returning

This comment has been minimized.

Copy link
@joehoyle

joehoyle Apr 29, 2015

Author Contributor

I don't see why we'd listen for a WP_Error, as the update to the "main" object has been done - we wouldn't want to return an error anyway presumably?

This comment has been minimized.

Copy link
@rmccue

rmccue May 3, 2015

Member

If an update_option or update_post_meta fails (DB is too busy, perhaps), it's important that we pass this data back up the entire stack. I'm fine with us treating each field as atomic by itself though, rather than the whole set of fields as atomic (i.e. one failure doesn't break immediately).

This comment has been minimized.

Copy link
@joehoyle

joehoyle May 4, 2015

Author Contributor

Does this mean we'd return a 500 or something on an update of a post? If not, how if this error returned?

joehoyle added 4 commits May 4, 2015
@joehoyle joehoyle removed the Needs Refresh label May 4, 2015
@@ -1150,7 +1156,7 @@ public function get_item_schema() {
$base = $this->get_post_type_base( $this->post_type );
$schema = array(
'$schema' => 'http://json-schema.org/draft-04/schema#',
'title' => $base,
'title' => $this->post_type,

This comment has been minimized.

Copy link
@joehoyle

joehoyle May 4, 2015

Author Contributor

@danielbachhuber this was using posts before for the "title", however all our other endpoints use singular names. I presumed this title refers to the object name, not plural form, so i changed this to just the post type. Do you see issues with that?

This comment has been minimized.

Copy link
@danielbachhuber

danielbachhuber May 5, 2015

Member

Should be fine for now. We'll eventually need to revisit these schemas to dot our i's and cross our t's

On May 4, 2015, at 16:53, Joe Hoyle notifications@github.com wrote:

In lib/endpoints/class-wp-rest-posts-controller.php:

@@ -1150,7 +1156,7 @@ public function get_item_schema() {
$base = $this->get_post_type_base( $this->post_type );
$schema = array(
'$schema' => 'http://json-schema.org/draft-04/schema#',

  •       'title'      => $base,
    
  •       'title'      => $this->post_type,
    
    @danielbachhuber this was using posts before for the "title", however all our other endpoints use singular names. I presumed this title refers to the object name, not plural form, so i changed this to just the post type. Do you see issues with that?


Reply to this email directly or view it on GitHub.

rachelbaker added a commit that referenced this pull request May 5, 2015
Introduce `register_api_field()` to add response fields to an object and it's schema.
@rachelbaker rachelbaker merged commit 9c11b0c into develop May 5, 2015
2 checks passed
2 checks passed
Scrutinizer 107 new issues, 180 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rachelbaker rachelbaker deleted the api-fields branch May 5, 2015
@rachelbaker

This comment has been minimized.

Copy link
Member

commented May 5, 2015

Merged #927

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