Gary Pendergast 12:45 am on September 23, 2015
Tags: feature plugins ( 120 ), json-api ( 39 ), rest-api ( 107 )

Committer Reviews of the REST API

With the proposed merge of the REST APIREST API The REST API is an acronym for the RESTful Application Program Interface (API) that uses HTTP requests to GET, PUT, POST and DELETE data. It is how the front end of an application (think “phone app” or “website”) can communicate with the data store (think “database” or “file system”) https://developer.wordpress.org/rest-api/. base code into CoreCore Core is the set of software required to run WordPress. The Core Development Team builds WordPress., I’d like to try something a little bit different. The REST API is potentially going to cause one of the biggest shifts in workflow that WordPress has seen, so it’s important that all committers know how it works, and how it affects the parts of Core that they focus on.

And so, here’s the plan. Before the REST API is merged into Core, it needs a code review from all active committers. The code being proposed for merge has been separated out into its own repo, for your viewing convenience.

There are five areas I’d like to see covered:

Docs: Are the inline docsinline docs (phpdoc, docblock, xref) up to standard? What needs to be done before they’re ready? Official tutorials will be helpful, can they be fit into Devhub?
Security: Is it secure?
Performance: Are there any obvious performance issues in the base code? Are we encouraging pluginPlugin A plugin is a piece of software containing a group of functions that can be added to a WordPress website. They can extend functionality or add new features to your WordPress websites. WordPress plugins are written in the PHP programming language and integrate seamlessly with WordPress. These can be free in the WordPress.org Plugin Directory https://wordpress.org/plugins/ or can be cost-based plugin from a third-party developers to write performant custom end points?
Ease of Use: Is it easy to write custom endpoints? Do we encourage quality code?
Forwards Compatibility: This is a little more nebulous, but can you envisage scenarios where we might need to break backwards compatibility in the future?

Choose one or more of these focusses for your review, and tackle it from that perspective.

You can also have a look at the proposed endpoints in the main repo, (scheduled for a later WordPress version), for inspiration on how it may interact with the areas of WordPress Core that you work on.

Post your review as a comment here, and link to any relevant bugbug A bug is an error or unexpected result. Performance improvements, code optimization, and are considered enhancements, not defects. After feature freeze, only bugs are dealt with, with regressions (adverse changes from the previous version) being the highest priority. reports or pull requests you submit to the plugin repo.

And finally, please have a think about how this process worked for you. I’d like this to be a model for future feature pluginFeature Plugin A plugin that was created with the intention of eventually being proposed for inclusion in WordPress Core. See Features as Plugins. merges, particularly those that touch many different areas of WordPress Core.

PS: I’m not kidding about all active committers. I won’t hesitate to publicly shame you for holding up the REST API merge. 🙂

#feature-plugins, #json-api, #rest-api

Ben Tremblay 2:37 am on September 23, 2015

For completeness?
http://v2.wp-api.org/
Boone Gorges 2:57 am on September 23, 2015
Just spent a while reading through the api-coreCore Core is the set of software required to run WordPress. The Core Development Team builds WordPress. repository, and cross-referencing against some of the existing endpoints. This is my first time looking at the internals of the APIAPI An API or Application Programming Interface is a software intermediary that allows programs to interact with each other and share data in limited, clearly defined ways.; my previous experience is limited to writing a couple of custom endpoints for v1.

Overall (and unsurprisingly) it seems like things have been nicely thought out. The syntax and conventions for registering endpoints seem easy to grok (they’re very WordPressy), without sacrificing flexibility and power.

A few notes I jotted down while reading. I’ve opted not to open issues or PRs, as some of these are more questions than anything else – both for the core team and for the API team.
- Why are we enabling JSONP by default? Is this for compatibility with browsers that don’t support CORS?
- Is CSRF protection left to the endpoints? What will this look like without an HTMLHTML HyperText Markup Language. The semantic scripting language primarily used for outputting content in web browsers. page to transport nonces?
- I’d argue we should consider shipping WP_Rest_Controller as part of the “infrastructure” round. It’s hard to imagine building a very useful endpoint without it.
- The API sends custom headers for deprecated functions and arguments. Should there be a corresponding headerHeader The header of your site is typically the first thing people will experience. The masthead or header art located across the top of your page is part of the look and feel of your website. It can influence a visitor’s opinion about your content and you/ your organization’s brand. It may also look different on different screen sizes. for _doing_it_wrong() notices?
- Your first stop for fetching request data is $HTTP_RAW_POST_DATA, with a fallback to php://input. It should probably be the other way around – the global is deprecated in PHPPHP The web scripting language in which WordPress is primarily architected. WordPress requires PHP 5.6.20 or higher 5.6 and removed in PHP 7.0.
- Should error messages in the json_last_error_msg() shim be localizable?
- Drew will probably have more to say about this, but docs formatting is inconsistent throughout, and will need a once- or twice-over.
Thanks to the API team for their work so far!
- Ryan McCue 6:44 am on September 23, 2015
  
  Thanks for the review @boonebgorges. 🙂
  
  Why are we enabling JSONP by default? Is this for compatibility with browsers that don’t support CORS?
  
  Correct, and compatibility with servers that don’t allow sending CORS headers, or old client libraries, or etc.
  
  Is CSRF protection left to the endpoints? What will this look like without an HTMLHTML HyperText Markup Language. The semantic scripting language primarily used for outputting content in web browsers. page to transport nonces?
  
  Authentication and authorisation is handled in the infrastructure. CSRF only applies to browser-based applications using cookie authentication, and you need the wp_rest nonce for that. (This is automatically output as WP_API_Settings.nonce and handled automatically when you use the JSJS JavaScript, a web scripting language typically executed in the browser. Often used for advanced user interfaces and behaviors. client.)
  
  I’d argue we should consider shipping WP_Rest_Controller as part of the “infrastructure” round.
  
  The base controller is half way between infrastructure and endpoints, but it’s possible we’ll continue changing things in it, and it’s not as stable as the infrastructure proper. Realistically, it’s just a set of best practices, so it’s definitely possible to build useful endpoints without it; I have a tonne of basic endpoints written that just use straight up functions or closures, which you don’t need a controller class for.
  
  The APIAPI An API or Application Programming Interface is a software intermediary that allows programs to interact with each other and share data in limited, clearly defined ways. sends custom headers for deprecated functions and arguments. Should there be a corresponding headerHeader The header of your site is typically the first thing people will experience. The masthead or header art located across the top of your page is part of the look and feel of your website. It can influence a visitor’s opinion about your content and you/ your organization’s brand. It may also look different on different screen sizes. for _doing_it_wrong() notices?
  
  Potentially. The deprecated stuff can be trigger from request data, whereas _doing_it_wrong() is likely only triggered by pluginPlugin A plugin is a piece of software containing a group of functions that can be added to a WordPress website. They can extend functionality or add new features to your WordPress websites. WordPress plugins are written in the PHP programming language and integrate seamlessly with WordPress. These can be free in the WordPress.org Plugin Directory https://wordpress.org/plugins/ or can be cost-based plugin from a third-party code, which is why we don’t handle it currently. I’d not be against adding it though.
  
  Your first stop for fetching request data is $HTTP_RAW_POST_DATA, with a fallback to php://input. It should probably be the other way around – the global is deprecated in PHPPHP The web scripting language in which WordPress is primarily architected. WordPress requires PHP 5.6.20 or higher 5.6 and removed in PHP 7.0.
  
  php://input suffers from a fatal flaw: it can only be read from once. In case other code wants to use this, or we need to reuse the data ourselves, we load it into the variable ourselves. I don’t think we can change this, alas.
  
  Should error messages in the json_last_error_msg() shim be localizable?
  
  I’d argue no, for parity with the PHP built-in/extension.
  
  docs formatting is inconsistent throughout
  
  Definitely, we need a bunch of work on this.
  
  Thanks again!
  - Ashworth Creative 2:01 pm on September 23, 2015
    
    php://input can be read multiple times as of PHP 5.6 Maybe flip the behavior in 5.6 or greater?
Drew Jaynes 8:15 pm on September 23, 2015

For posterity: I’ve opened a PR for a coreCore Core is the set of software required to run WordPress. The Core Development Team builds WordPress. docs standards audit here: https://github.com/WP-API/WP-API/pull/1590
Aaron Jorbin 8:13 pm on September 30, 2015
I haven’t finished a full review, but here are some initial things:

WP_HTTP_ResponseInterface

Does there really need to be an WP_HTTP_ResponseInterface interface? It feels like over abstraction for the sake of abstraction.

— WP_REST_Server —

Many of the constants in WP_REST_Server don’t seem to serve a purpose without endpoints. I would like to see some more inline documentation around how they are expected to be used. For example, why is ACCEPT_JSON set as 128?

serve_request feels like a very long method compared to many of the others. It seems to handle two things, serving the request, but before that, routing the request. Could this become two different methods? One that does the setting/routing and the other that does the serving?

A couple of other small changes I would look at:
- Add a set_default_headers method that could be called and encompass lines 248 -255. There could also be a filterFilter Filters are one of the two types of Hooks https://codex.wordpress.org/Plugin_API/Hooks. They provide a way for functions to modify data of other functions. They are the counterpart to Actions. Unlike Actions, filters are meant to work in an isolated manner, and should never have side effects such as affecting global variables and output. on this list to easily enable other default headers being added to every request.
- rest_ensure_response is called on the $result on both lines 321 and 337 (in the rest_post_dispatch filter). The only change between those is that if it is an error, it goes through error_to_response which returns WP_REST_Response which doesn’t need to be ensured is a response.
Should envelope_response be public? It seems like it only is needed in once one place and sdoesn’t need to be used outside of it.

More to come
- Jeremy Felt 4:44 am on October 2, 2015
  
  rest_ensure_response is called on the $result on both lines 321 and 337 (in the rest_post_dispatch filterFilter Filters are one of the two types of Hooks https://codex.wordpress.org/Plugin_API/Hooks. They provide a way for functions to modify data of other functions. They are the counterpart to Actions. Unlike Actions, filters are meant to work in an isolated manner, and should never have side effects such as affecting global variables and output.). The only change between those is that if it is an error, it goes through error_to_response which returns WP_REST_Response which doesn’t need to be ensured is a response.
  
  This caught me up as well when reading through `WP_Rest_Server`. I’m not sure if there’s a better way to handle it, but it is tricky when reading.
- Rachel Baker 1:01 pm on October 2, 2015
  
  @jorbin Regarding WP_HTTP_ResponseInterface see my comment above to Dominik: https://make.wordpress.org/core/2015/09/23/committer-reviews-of-the-rest-api/comment-page-1/#comment-28047
- Ryan McCue 6:40 am on October 7, 2015
  
  Many of the constants in WP_REST_Server don’t seem to serve a purpose without endpoints.
  
  These constants were leftovers from way back when we had bitwise flags. I’ve killed those now. The only constants left now are RESTful verbs (`READABLE`, `CREATABLE`, `EDITABLE`, `DELETABLE` and `ALLMETHODS`).
  
  Should envelope_response be public? It seems like it only is needed in once one place and sdoesn’t need to be used outside of it.
  
  Enveloping is super useful in third-party code, and there’s no real reason to make it protected. I’d like to keep it as part of the class’ public APIAPI An API or Application Programming Interface is a software intermediary that allows programs to interact with each other and share data in limited, clearly defined ways.. 🙂
  
  Working on the rest of the feedback.
Drew Jaynes 8:15 pm on September 30, 2015

I noted this to Rachel separately, but good to have it mentioned here:

The wp-api.js file should be renamed to wp-rest-api.js, and the handle changed to ‘wp-rest-api’ or ‘rest-api’. In the broader scheme of WordPress, this is another of many APIs, not the WordPress APIAPI An API or Application Programming Interface is a software intermediary that allows programs to interact with each other and share data in limited, clearly defined ways. any longer 🙂
Dominik Schilling (ocean90) 8:16 pm on September 30, 2015

WP_HTTP_ResponseInterface – Do we need it? If it gets released once there is no way to extend it without potentially breaking plugins which are using this class.
- Rachel Baker 12:59 pm on October 2, 2015
  
  I chatted with @rmccue, who first introduced WP_HTTP_ResonseInterface (or as previously named WP_JSON_ResponseInterface) on Feb 17, 2014, and he **believes** it can be removed. He has to do some testing to confirm this theory.
  If we can remove it, we will. Otherwise @rmccue will provide the reasoning behind needing to keep it.
Weston Ruter 10:46 pm on September 30, 2015

My review: https://github.com/WP-API/api-core/pull/1
- Rachel Baker 2:09 pm on October 2, 2015
  
  @westonruter Thank you for the PR. We will cherry pick it over to our main repo, and refresh our patchpatch A special text file that describes changes to code, by identifying the files and lines which are added, removed, and altered. It may also be referred to as a diff. A patch can be applied to a codebase for testing. on https://core.trac.wordpress.org/ticket/33982
Rachel Baker 2:10 pm on October 2, 2015

We have a patchpatch A special text file that describes changes to code, by identifying the files and lines which are added, removed, and altered. It may also be referred to as a diff. A patch can be applied to a codebase for testing. up in https://core.trac.wordpress.org/ticket/33982 that can be refreshed, if committers have any desired code changes.

Comments are closed.

Welcome!

Communication

Committer Reviews of the REST API

Welcome!

Communication

Share this: