#33982 closed task (blessed) (fixed)
WP REST API Stage One: Infrastructure
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| 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)
Change History (42)
#3
@
5 years ago
New REST API component page added: https://make.wordpress.org/core/components/rest-api/
This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.
5 years ago
#5
follow-up:
↓ 6
@
5 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 inwp-includes/default-filters.phpor inrest-api/filters.phpor something src/wp-includes/rest-api.phpshould load files onlyextras.phpcan be calledfunctions.phporrest-functions.phpand contain the function defs fromrest-api.phpandextras.php- Is it necessary to have the
infrastructurefolder - for vanity and semantics' sake, can those files just all live inrest-api? or if in a folder, can we call itlib?
So close. so close. (so close.)
#6
in reply to:
↑ 5
;
follow-up:
↓ 19
@
5 years ago
Replying to wonderboymusic:
Awesome. Few notes:
- We no longer mix function definitions and top-level code.
add_filter|action()calls should go inwp-includes/default-filters.phpor inrest-api/filters.phpor something
Shucks! Forgot about that. Will fix.
src/wp-includes/rest-api.phpshould load files only
Will fix.
extras.phpcan be calledfunctions.phporrest-functions.phpand contain the function defs fromrest-api.phpandextras.php
Will fix.
- Is it necessary to have the
infrastructurefolder - for vanity and semantics' sake, can those files just all live inrest-api? or if in a folder, can we call itlib?
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.
#7
@
5 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.
#8
@
5 years ago
@wonderboymusic See 33982.2.patch, with changes based on your feedback and working unit tests.
#9
follow-up:
↓ 11
@
5 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?
#10
@
5 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
// @codingstandardsIgnorecomments need to be removed (as requested by @DrewAPicture)
#11
in reply to:
↑ 9
@
5 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_ResponseInterfacefrom 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.
5 years ago
This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.
5 years ago
@
5 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.
5 years ago
#18
@
5 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
#19
in reply to:
↑ 6
@
5 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
infrastructurefolder - for vanity and semantics' sake, can those files just all live inrest-api? or if in a folder, can we call itlib?I think the idea is to have the
infrastructuredirectory and eventually anendpointsdirectory. But I will name the directorymicrosoftif 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? (Maybeis_numeric_arrayor something.) WP_REST_Server::prepare_responseshould be rolled intowp_json_encodenow 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
@
5 years ago
The extra folder is unnecessary, just move them to rest-api. Make the function generic. Your pick how/when you commit what
#21
follow-up:
↓ 22
@
5 years ago
For 33982-jsonserialize.diff:
- What's
WP_JSON_SERIALIZE_COMPATIBLEfor? Do we need to be adding another constant? - What's the difference between this and normal
json_encode()behaviour?
#22
in reply to:
↑ 21
@
5 years ago
Replying to pento:
For 33982-jsonserialize.diff:
- What's
WP_JSON_SERIALIZE_COMPATIBLEfor? 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:
↓ 25
@
5 years ago
@rmccue
For 33982-jsonserialize.diff:
In _wp_json_prepare_data( $data )you are missing the null return type in your docblock.
#25
in reply to:
↑ 23
@
5 years ago
Replying to rachelbaker:
you are missing the
nullreturn type in your docblock.
Good catch, was missing a few others there too. Fixed in 33982-jsonserialize.2.diff.
#26
@
5 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.
None of the current components seemed like a great fit. Maybe an
External APIscomponent can contain aREST APIchild component and the currentXML-RPCcomponent?