Skip to content

REST API: Add Field Controller#10600

Merged
mdawaffe merged 10 commits intomasterfrom
add/rest-api-field-controller
Nov 15, 2018
Merged

REST API: Add Field Controller#10600
mdawaffe merged 10 commits intomasterfrom
add/rest-api-field-controller

Conversation

@mdawaffe
Copy link
Copy Markdown
Member

Just as WP_REST_Controller is mostly just a fancy wrapper for
register_rest_route(), WPCOM_REST_API_V2_Field_Controller is a wrapper
for register_rest_field().

Changes proposed in this Pull Request:

The new class handles a few conveniences:

  • Returning the correct things (errors, default values, etc.) for
    permission check errors.
  • Ensuring output is always correctly typecast.
  • ?context=... filtering for nested structures, which core does not
    handle correctly.

Testing instructions:

yarn docker:phpunit --group rest-api

Proposed changelog entry for your changes:

None required.

Just as `WP_REST_Controller` is mostly just a fancy wrapper for
`register_rest_route()`, `WPCOM_REST_API_V2_Field_Controller` is a wrapper
for `register_rest_field()`. It handles a few conveniences:
* Returning the correct things (errors, default values, etc.) for
  permission check errors.
* Ensuring output is always correctly typecast.
* `?context=...` filtering for nested structures, which core does not
  handle correctly.
@mdawaffe mdawaffe added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review This PR is ready for review. Touches WP.com Files [Feature] WP REST API [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Nov 12, 2018
@mdawaffe mdawaffe added this to the 6.8 milestone Nov 12, 2018
@mdawaffe mdawaffe self-assigned this Nov 12, 2018
@mdawaffe mdawaffe requested review from a team, ockham and tyxla November 12, 2018 19:53
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Nov 12, 2018

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is automated check which relies on PULL_REQUEST_TEMPLATE.We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS

ockham
ockham previously approved these changes Nov 12, 2018
Copy link
Copy Markdown
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Left one minor note. LGTM otherwise! 🚢

tyxla
tyxla previously approved these changes Nov 13, 2018
Copy link
Copy Markdown
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

This is looking and working great for me 👍 💯

I've left some minor suggestions, nothing blocking though, so feel free to take or leave any of them.

Feel free to 🚢 at will!

@mdawaffe mdawaffe dismissed stale reviews from tyxla and ockham via 2fcd571 November 13, 2018 22:26
tyxla
tyxla previously approved these changes Nov 14, 2018
Copy link
Copy Markdown
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

This is looking great, tests are passing too 👍

🚢

@lezama lezama added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Nov 14, 2018
@lezama
Copy link
Copy Markdown
Contributor

lezama commented Nov 14, 2018

🚢 :shipit:

@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 14, 2018
lezama
lezama previously approved these changes Nov 14, 2018
Copy link
Copy Markdown
Contributor

@lezama lezama left a comment

Choose a reason for hiding this comment

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

After Linter fixes fixes let me approve this again

…alue

Return `WP_Error` from `update_from_request()` when `update_permission_check()` returns false.
@mdawaffe
Copy link
Copy Markdown
Member Author

WordPress.com code ready at D20160-code.

Copy link
Copy Markdown
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

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

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] WP REST API [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants