Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#33982 closed task (blessed) (fixed)

WP REST API Stage One: Infrastructure

Reported by: rachelbaker's profile rachelbaker Owned by: rmccue's profile rmccue
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch
Focuses: Cc:

Description

This will be used as the master tracking ticket for Stage One of the WP REST API feature plugin, which has been proposed for merge in the 4.4 release.

Stage One: Infrastructure: The infrastructure is the code responsible for routing requests and handling the “meta” layer of the API, including JSON serialisation/deserialisation, linking, embedding, and REST best practices. This adds a simplified routing layer outside of WP’s rewrites, allowing non-query-var rewrites easily, acting as a base for building APIs inside WordPress.

GitHub Repo: https://github.com/WP-API/api-core
Merge Proposal: https://make.wordpress.org/core/2015/09/21/wp-rest-api-merge-proposal/
Commiters Review Request: https://make.wordpress.org/core/2015/09/23/committer-reviews-of-the-rest-api/

Attachments (10)

33982.patch (137.7 KB) - added by rachelbaker 8 years ago.
33982.1.patch (97.8 KB) - added by rachelbaker 8 years ago.
Refreshed patch with structure changes suggested by @wonderboymusic.
33982.2.patch (138.0 KB) - added by rachelbaker 8 years ago.
the structure changes from the .1 patch with working unit tests
33982.3.patch (138.2 KB) - added by welcher 8 years ago.
Adding missing unit test for test_rest_url_generation
33982.diff (130.6 KB) - added by wonderboymusic 8 years ago.
33982.3.2.patch (126.8 KB) - added by rachelbaker 8 years ago.
Refreshed @wonderboymusic's patch to remove some legacy @todo comments, add in Weston's changes from his review, and minor formatting fixes
33982-jsonserialize.diff (1.7 KB) - added by rmccue 8 years ago.
Add JsonSerializable support to wp_json_encode
33982-jsonserialize.2.diff (1.7 KB) - added by rmccue 8 years ago.
Correct return type of prepare
33982-jsonserialize.3.diff (1.8 KB) - added by rmccue 8 years ago.
Ensure we don't touch incomplete objects
33982-numeric.diff (739 bytes) - added by rmccue 8 years ago.
Add helper function for detecting numeric-indexed arrays

Download all attachments as: .zip

Change History (42)

#1 @rachelbaker
8 years ago

None of the current components seemed like a great fit. Maybe an External APIs component can contain a REST API child component and the current XML-RPC component?

Last edited 8 years ago by rachelbaker (previous) (diff)

#2 @pento
8 years ago

  • Component changed from General to REST API

New REST API component added.

@rachelbaker
8 years ago

This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.


8 years ago

#5 follow-up: @wonderboymusic
8 years ago

  • Keywords has-patch added
  • Owner set to rachelbaker
  • Status changed from new to assigned

Awesome. Few notes:

  • We no longer mix function definitions and top-level code. add_filter|action() calls should go in wp-includes/default-filters.php or in rest-api/filters.php or something
  • src/wp-includes/rest-api.php should load files only
  • extras.php can be called functions.php or rest-functions.php and contain the function defs from rest-api.php and extras.php
  • Is it necessary to have the infrastructure folder - for vanity and semantics' sake, can those files just all live in rest-api? or if in a folder, can we call it lib?

So close. so close. (so close.)

#6 in reply to: ↑ 5 ; follow-up: @rachelbaker
8 years ago

Replying to wonderboymusic:

Awesome. Few notes:

  • We no longer mix function definitions and top-level code. add_filter|action() calls should go in wp-includes/default-filters.php or in rest-api/filters.php or something

Shucks! Forgot about that. Will fix.

  • src/wp-includes/rest-api.php should load files only

Will fix.

  • extras.php can be called functions.php or rest-functions.php and contain the function defs from rest-api.php and extras.php

Will fix.

  • Is it necessary to have the infrastructure folder - for vanity and semantics' sake, can those files just all live in rest-api? or if in a folder, can we call it lib?

I think the idea is to have the infrastructure directory and eventually an endpoints directory. But I will name the directory microsoft if it that is your desire.

@rachelbaker
8 years ago

Refreshed patch with structure changes suggested by @wonderboymusic.

#7 @rachelbaker
8 years ago

In 33982.1.patch I pulled out the unit tests. The tests were all passing when run as the group phpunit --group restapi but had some failures when run with all core tests.
I wasn't able to get the tests to pass before my eyes started closing. Hopefully, @rmccue or @pento can see what I couldn't and refresh the patch with the tests.

@rachelbaker
8 years ago

the structure changes from the .1 patch with working unit tests

#8 @rachelbaker
8 years ago

@wonderboymusic See 33982.2.patch, with changes based on your feedback and working unit tests.

#9 follow-up: @DrewAPicture
8 years ago

@rachelbaker Let's remove the // @codingStandardsIgnore* and // @codingStandardsIgnore* comments while we're at it.

Also, was @ocean90's question about WP_HTTP_ResponseInterface from in the committer review post addressed somewhere?

@welcher
8 years ago

Adding missing unit test for test_rest_url_generation

#10 @rachelbaker
8 years ago

There are still a few known code changes needed here:

  • Our team needs to cherry pick Weston's PR into our main repo and include the changes in a refreshed patch.
  • @rmccue is going to confirm that we can remove the WP_HTTP_ResonseInterface class, and provide a refreshed patch.
  • the // @codingstandardsIgnore comments need to be removed (as requested by @DrewAPicture)

#11 in reply to: ↑ 9 @rachelbaker
8 years ago

Replying to DrewAPicture:

@rachelbaker Let's remove the // @codingStandardsIgnore* and // @codingStandardsIgnore* comments while we're at it.

Noted in comment:10

Also, was @ocean90's question about WP_HTTP_ResponseInterface from in the committer review post addressed somewhere?

I just left a reply for @ocean90 this morning. Also noted in comment:10.

This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.


8 years ago

This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.


8 years ago

#14 @wonderboymusic
8 years ago

In 34844:

REST API: add json_last_error_msg() compatibility function for PHP <5.5 to compat.php

Props rmmcue.
See #33982.

#15 @wonderboymusic
8 years ago

In 34845:

REST API: add JsonSerializable() compatibility interface for PHP <5.4 to compat.php

Props rmmcue.
See #33982.

#16 @wonderboymusic
8 years ago

In 34846:

REST API: add a utility function, mysql_to_rfc3339() to functions.php

Background:
https://github.com/WP-API/WP-API/commit/6d0ad766ca525c7a3aee921b66554ddf21a5c023

Props rmmcue.
See #33982.

@wonderboymusic
8 years ago

@rachelbaker
8 years ago

Refreshed @wonderboymusic's patch to remove some legacy @todo comments, add in Weston's changes from his review, and minor formatting fixes

This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.


8 years ago

#18 @rachelbaker
8 years ago

Props list:

rmccue
rachelbaker
danielbachhuber
joehoyle
drewapicture
adamsilverstein
netweb
tlovett1
shelob9
kadamwhite
pento
westonruter
nikv
tobych
redsweater
alecuf
pollyplummer
hurtige
bpetty
oso96_2000
ericlewis
wonderboymusic
joshkadis
mordauk
jdgrimes
johnbillion
jeremyfelt
thiago-negri
jdolan
pkevan
iseulde
thenbrent
maxcutler
kwight
markoheijnen
phh
natewr
jjeaton
shprink
mattheu
quasel
jmusal
codebykat
hubdotcom
tapsboy
QWp6t
pushred
jaredcobb
justinsainton
japh
matrixik
jorbin
frozzare
codfish
michael-arestad
kellbot
ironpaperweight
simonlampen
alisspers
eliorivero
davidbhayes
JohnDittmar
dimadin
traversal
cmmarslender
Toddses
kokarn
welcher
ericpedia

Last edited 8 years ago by rachelbaker (previous) (diff)

#19 in reply to: ↑ 6 @rmccue
8 years ago

Been working on making sure we have a smooth transition on the plugin side, looks like all is well there now.

(Apologies for not having commented here previously, being sick is no fun.)

Replying to rachelbaker:

  • Is it necessary to have the infrastructure folder - for vanity and semantics' sake, can those files just all live in rest-api? or if in a folder, can we call it lib?

I think the idea is to have the infrastructure directory and eventually an endpoints directory. But I will name the directory microsoft if it that is your desire.

I'd like to keep this one as infrastructure so that when we eventually get to endpoints, the two are separated. If you'd definitely prefer we have lib, I'd suggest we instead move it all up into rest-api, otherwise it's a bit redundant.


Questions for y'all:

  • We have rest_is_list, which tells us whether an array is a numeric array (aka list). Along the lines of the previous functions, should we turn this into a generic WP function? (Maybe is_numeric_array or something.)
  • WP_REST_Server::prepare_response should be rolled into wp_json_encode now that we have r34845 (since it's only there to be a JsonSerializable shim). Should I do this in the main patch, or a separate one?

#20 @wonderboymusic
8 years ago

The extra folder is unnecessary, just move them to rest-api. Make the function generic. Your pick how/when you commit what

@rmccue
8 years ago

Add JsonSerializable support to wp_json_encode

#21 follow-up: @pento
8 years ago

For 33982-jsonserialize.diff:

  • What's WP_JSON_SERIALIZE_COMPATIBLE for? Do we need to be adding another constant?
  • What's the difference between this and normal json_encode() behaviour?

#22 in reply to: ↑ 21 @rmccue
8 years ago

Replying to pento:

For 33982-jsonserialize.diff:

  • What's WP_JSON_SERIALIZE_COMPATIBLE for? Do we need to be adding another constant?
  • What's the difference between this and normal json_encode() behaviour?

The JsonSerializable interface (added in r34845) has two parts: first, your object implements JsonSerializable; second, json_encode calls $obj->jsonSerialize() on the object before encoding. We added the interface for compatibility, but we haven't added the other side yet. This handles it inside wp_json_encode, which ties it together nicely.

#23 follow-up: @rachelbaker
8 years ago

@rmccue
For 33982-jsonserialize.diff:
In _wp_json_prepare_data( $data )you are missing the null return type in your docblock.

Last edited 8 years ago by rachelbaker (previous) (diff)

#24 @rachelbaker
8 years ago

  • Owner changed from rachelbaker to rmccue

@rmccue
8 years ago

Correct return type of prepare

#25 in reply to: ↑ 23 @rmccue
8 years ago

Replying to rachelbaker:

you are missing the null return type in your docblock.

Good catch, was missing a few others there too. Fixed in 33982-jsonserialize.2.diff.

#26 @pento
8 years ago

Aha, I see. This is what happens when I'm too focussed on other projects. :-)

33982-jsonserialize.2.diff looks good for commit.

@rmccue
8 years ago

Ensure we don't touch incomplete objects

#27 @rmccue
8 years ago

Updated patch based on @chriscct7's note that incomplete objects (unserialized objects with a missing class) give object from gettype() despite being unusable. Adds an is_object check as a backup. (Avoided using switch ( true ) since it'd have a bit more overhead for non-objects.)

#28 @rmccue
8 years ago

In 34926:

REST API: Add JsonSerializable compatibility to wp_json_encode

Following on from r34845, the JsonSerializable shim needs support
on the encoding side too. _wp_json_prepare_data handles this when
we've loaded the shim.

Props chriscct7.

See #33982.

@rmccue
8 years ago

Add helper function for detecting numeric-indexed arrays

#29 @rmccue
8 years ago

In 34927:

REST API: Add wp_is_numeric_array helper function

The API uses this to do special operations on list responses (used
for collections), so we need to detect whether an array is
associative or numeric-indexed.

After much discussion, the bikeshed is to be painted green and gold.

See #33982.

#30 @rmccue
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 34928:

REST API: Introduce baby API to the world.

Baby API was born at 2.8KLOC on October 8th at 2:30 UTC. API has lots
of growing to do, so wish it the best of luck.

Thanks to everyone who helped along the way:

Props rmccue, rachelbaker, danielbachhuber, joehoyle, drewapicture,
adamsilverstein, netweb, tlovett1, shelob9, kadamwhite, pento,
westonruter, nikv, tobych, redsweater, alecuf, pollyplummer, hurtige,
bpetty, oso96_2000, ericlewis, wonderboymusic, joshkadis, mordauk,
jdgrimes, johnbillion, jeremyfelt, thiago-negri, jdolan, pkevan,
iseulde, thenbrent, maxcutler, kwight, markoheijnen, phh, natewr,
jjeaton, shprink, mattheu, quasel, jmusal, codebykat, hubdotcom,
tapsboy, QWp6t, pushred, jaredcobb, justinsainton, japh, matrixik,
jorbin, frozzare, codfish, michael-arestad, kellbot, ironpaperweight,
simonlampen, alisspers, eliorivero, davidbhayes, JohnDittmar, dimadin,
traversal, cmmarslender, Toddses, kokarn, welcher, and ericpedia.

Fixes #33982.

#31 @rmccue
8 years ago

In 34929:

REST API: Unbreak everything.

Obviously, it wouldn't have been a good commit unless I botched it.

See #33982.

#32 @rmccue
8 years ago

In 34930:

REST API: Add missing reference to WP_HTTP_Response

See #33982

Note: See TracTickets for help on using tickets.