Add /taxonomies routes #211

Merged
merged 40 commits into from Jun 9, 2014

Conversation

Projects
None yet
5 participants
Contributor

kadamwhite commented May 23, 2014

As described in #198, It is frequently desirable to know about all taxonomies, regardless of the post type to which they are associated; it is also convenient to be able to retrieve taxonomies from a base-level endpoint.

This adds the following routes to the taxonomies class:

  • /taxonomies
  • /taxonomies/(?P<taxonomy>\w+)
  • /taxonomies/(?P<taxonomy>\w+)/terms
  • /taxonomies/(?P<taxonomy>\w+)/terms/(?P<term>\w+)

Details

I have implemented this by augmenting the existing get_taxonomies, get_taxonomy, get_terms and get_term methods, rather than creating new ones. I re-ordered the arguments of these methods and set a default for $type = '' in the arguments for all route handlers to allow the same function to be used with or without a <type> parameter.

If the type if '', then, get_taxonomies is used to retrieve all the registered taxonomies (for the base route), and the $base_url value is updated to reflect the new routes (for all endpoints accessed through /taxonomies/.

Note that at present this does not contain the types cross-linking described in #198.

Fixes #198, fixes #197.

Contributor

kadamwhite commented May 23, 2014

Here is a link to wordpress-dev chat logs where @rmccue gave me some direction on this

Member

tlovett1 commented May 23, 2014

Looks good to me. My impression is that we weren't doing this because of the potential taxonomy roadmap that would eventually prevent terms from being shared across taxonomies. Although this would still have some use even if terms aren't shared.

Contributor

kadamwhite commented May 23, 2014

I thought the links @rmmcue was referencing were the links from a taxonomy, to the post types with which it was associated. Understood that terms won't be shared, that is as it should be.

The work in this PR feels independent from any linking, inasmuch as it solves a problem I've had so far--I think the linking question deserves more conversation in #198

Owner

rmccue commented May 24, 2014

So, I'm +1 on /taxonomies and /taxonomies/<tax>, but not the terms, I think. Kind of need clarification on the plan in WP; maybe @nacin can weigh in on that.

Contributor

kadamwhite commented May 24, 2014

Personally, the utility of the root /taxonomies route feels significantly diminished without being able to enumerate the terms. The two cases, for the question "I need to print all terms for all taxonomies":

  1. Terms on taxonomy endpoint: I go to taxonomies, it returns a list of tax objects; I hit each endpoint and get a full list of taxonomies.
  2. Terms on post type endpoints: I go taxonomies, and get the tax objects; from there, I need to pick one of the potentially numerous other post-type-specific endpoints for the collections link; then I need to visit that to get the terms.

Particularly in light of the ongoing term cleanup work, I feel that having a canonical route to a taxonomy independent of the CPTs to which it applies is easier than having to keep track of several CPT-rooted endpoints. It also fits the mental model we create of WP data relationships ("there are taxonomies and types, and the former apply to one or more of the latter"), and unless I'm missing something it doesn't feel at odds with the actual data model either.

I'd be curious to know what @nacin thinks!

Contributor

kadamwhite commented May 26, 2014

@rmccue thoughts?

Owner

rachelbaker commented May 27, 2014

Needs addition to the post route docs and tests.

rachelbaker added this to the 1.1 milestone May 27, 2014

nacin commented May 27, 2014

I don't see how this is affected by the term cleanup. As in, looks good to me.

As long as you must specify a taxonomy in order to do anything, then it is fine. Technically that means a shared term will be available in two places (taxonomies/fruits/terms/apple and taxonomies/companies/terms/apple) but this would be the most forward-compatible way of doing things. I'm happy to address specific concerns.

kadamwhite added some commits May 23, 2014

@kadamwhite kadamwhite Add /taxonomies routes
It is frequently desirable to know about all taxonomies, regardless
of the post type to which they are associated; it is also convenient
to be able to retrieve taxonomies from a base-level endpoint.

This adds the following routes to the taxonomies class:

- /taxonomies
- /taxonomies/(?P<taxonomy>\w+)
- /taxonomies/(?P<taxonomy>\w+)/terms
- /taxonomies/(?P<taxonomy>\w+)/terms/(?P<term>\w+)

Bug Reference: #198
ea234ea
@kadamwhite kadamwhite Remove <type> argument from taxonomy routes
Per IRC conversation on 2014.05.26, the /taxonomies base route will take
precedence over the per-type endpoints. Removing the <type> parameter
will permit the existing routes to function, but moves towards having the
/taxonomies base routes become the canonical ones.
a04df28
@kadamwhite kadamwhite Update documentation to reflect /taxonomies routes
- Added routes/taxonomies.md defining existing taxonomy routes
- Updated guides/getting-started.md to clarify available methods (GET)
- Updated the schema.md file to reflect the base /taxonomy endpoints
b48eae9
Contributor

kadamwhite commented May 27, 2014

Docs added. At present all of the Taxonomy routes are WP_JSON_Server::READABLE only, so I've updated other related documentation to reflect that.

Per last night's conversations, I have also removed the property from the taxonomies routes. I realize however that this does prevent you from asking for the specific taxonomies related to a particular post type. Should we implement some sort of filter[]ing here, to permit things like "Show me all taxonomies for 'posts'"? I'm not sure the filtering metaphor works fully, since WP_Tax_Query seems to exist but isn't as documented as our ol' WP_Query itself. I'd copy what the .com API does, but they don't have arbitrary post types to worry about so there's nothing to go on ;)

Contributor

kadamwhite commented May 27, 2014

Also, I am going to need help writing the tests for these. I was able to get my environment up and running, but lack of familiarity with PHPUnit and PHP testing is going to slow me down a lot. The rest of y'all should be able to work through them much faster than I would.

nacin commented May 28, 2014

WP_Tax_Query is about querying posts with particular terms.

get_terms() (which will be a wrapper for an eventual WP_Term_Query) is about querying for terms.

Querying for which post types are associated with a taxonomy is get_object_taxonomies(). Note this gets tricky when dealing with attachments, see get_attachment_taxonomies().

kadamwhite added some commits May 28, 2014

@kadamwhite kadamwhite Add unit tests for /taxonomies routes 5f41e25
@kadamwhite kadamwhite Fix regression causing all taxonomies to be returned for all types
While the taxonomies routes are now type-independent, the post type
routes (e.g. /posts/types/post) return embedded taxonomy objects for
each taxonomy associated with that type. Removing the "types" argument
from the get_taxonomies method caused a regression where ALL
taxonomies would be returned for any post type endpoint.
f471152
@kadamwhite kadamwhite Add test for add_taxonomy_data regression
This adds a test to verify that get_taxonomies only returns taxonomies
for the provided post type if a type is passed into the function. This
ensures that the taxonomies embedded into post type endpoint responses
will be accurate.

It also DRYs up the test code a bit to avoid repeating the methods for
filtering a taxonomy collection down to only "public" members.
920a97d
Contributor

kadamwhite commented May 28, 2014

I took a crack at embedding type data into taxonomies: https://gist.github.com/kadamwhite/e941506b6cf8c9cce263

Because types have taxonomies embedded into them in turn, I was creating an infinite embedding loop that resulted in a 502 error. To prevent that, I've altered the add_taxonomy_data method to take another parameter that is used to avoid the circular embed chain. I left a more detailed comment in the linked gist above; thoughts on this approach would be welcome before I commit this.

@rmccue rmccue and 2 others commented on an outdated diff May 29, 2014

lib/class-wp-json-taxonomies.php
@@ -67,11 +83,11 @@ public function get_taxonomy( $type, $taxonomy ) {
* @param boolean $_in_collection Are we in a collection?
* @return array Taxonomy data
*/
- protected function prepare_taxonomy( $taxonomy, $type, $_in_collection = false ) {
@rmccue

rmccue May 29, 2014

Owner

I was wrong on removing $type, as this will now break backwards compatibility. Instead, let's deprecate the parameter instead and ignore the value.

@nacin

nacin May 29, 2014

We're going to at some point push 1.0 into a branch and keep master clean of deprecated stuff. If we "deprecate" this let's be sure it gets marked so we can yank it.

@rmccue

rmccue May 29, 2014

Owner

We're going to at some point push 1.0 into a branch and keep master clean of deprecated stuff. If we "deprecate" this let's be sure it gets marked so we can yank it.

We can use _deprecated_argument, but with some funky tricks to send headers. :)

@nacin

nacin May 29, 2014

Even just @deprecated is fine, just wanted to make it clear when I start hacking on the internals I'm not playing so nicely. :-)

@kadamwhite

kadamwhite May 29, 2014

Contributor

Just pushed up a commit (7911138) to reinstate $type for all methods, so that nothing breaks if a plugin written for 1.0 calls one of them. Internally, we no longer use $type in get_taxonomy (singular), prepare_taxonomy, get_terms, get_term, and prepare_term, and $type is passed as null whenever it needs to be passed to maintain argument order.

With the exception of the updated links (which still all point to /taxonomies/*), I believe that the old /posts/types//taxonomies/* endpoints should return the same data they did of old, and the methods have the same signature if being called directly.

kadamwhite added some commits May 29, 2014

@kadamwhite kadamwhite Reinstate $types parameter for back-compat with 1.0
$types is only necessary in `get_taxonomies` and `add_taxonomy_data`;
however, we need to leave it in the argumentss list for ALL methods in
order to avoid breaking any plugins built to interoperate with v1.0.

get_taxonomy (singular), prepare_taxonomy, get_terms, get_term,
and prepare_term should all have their $type parameter removed in 2.0.
7911138
@kadamwhite kadamwhite Add new methods for base /taxonomies routes & update tests
Because the new /taxonomies route have no $type parameter, and $type
cannot be removed without breaking back-compat, we need to add a new
set of route methods for the new, "correct" /taxonomies routes. To
minimize confusion, the old route method names were left as is, and
new methods added for each method which had previously taken a $type.
All the old methods are marked @deprecated and have a @see reference
to their successors.

New methods:

- `get_taxonomy` now wraps the new `get_taxonomy_object`
- `prepare_taxonomy` now wraps the new `prepare_taxonomy_object`
- `get_terms` now wraps the new `get_taxonomy_terms`
- `get_term` now wraps the new `get_taxonomy_terms`
- `prepare_term` now wraps the new `prepare_taxonomy_term`

The existing unit tests were changed to point to these new route methods.
66e1f98
Contributor

kadamwhite commented May 29, 2014

From 66e1f98:

Because the new /taxonomies route have no $type parameter, and $type cannot be removed without breaking back-compat, we need to add a new set of route methods for the new, "correct" /taxonomies routes. To minimize confusion, the old route method names were left as is, and new methods added for each method which had previously taken a $type. All the old methods are marked @deprecated and have a @see reference to their successors.

New methods:

  • get_taxonomy now wraps the new get_taxonomy_object
  • prepare_taxonomy now wraps the new prepare_taxonomy_object
  • get_terms now wraps the new get_taxonomy_terms
  • get_term now wraps the new get_taxonomy_terms
  • prepare_term now wraps the new prepare_taxonomy_term

The existing unit tests were changed to point to these new route methods.

@kadamwhite kadamwhite Embed associated post type records in Taxonomy objects
This is the first pass at embedding a list of associated post types in
taxonomy records, as described in #197. Types are NOT embedded if they
would be added to a taxonomy record that is itself embedded.

@rmccue has proposed changing how the $_in_taxonomy behavior works to
use a "$context" variable, as in other methods; this commit should be
considered a work-in-progress until he has signed off on it.
b3c628c
Contributor

kadamwhite commented May 30, 2014

@rmccue @rachelbaker I'll have more time to work on this over the weekend; I'd love your input on what needs to be changed in order to get this merged in

rmccue was assigned by rachelbaker May 30, 2014

Owner

rachelbaker commented May 30, 2014

@kadamwhite I am going to yield to @rmccue since I know he has been actively guiding the direction here.

One thing that would really help speed things along on our end is if you could pull from the master branch here and resolve any merge conflicts.

rachelbaker added some commits May 31, 2014

Owner

rachelbaker commented May 31, 2014

There is currently ~65% coverage on class-wp-json-taxonomies.php. We should get this to at least 90%.

rmccue added the Review label Jun 2, 2014

@kadamwhite @rmccue
Do you think we should specify that we are looking for the slug value from the GET /taxonomies response here? Within WordPress this is the $taxonomy->name, but it can be easily confused with $taxonomy->label that we actually call name in the GET /taxonomies response.

Owner

kadamwhite replied Jun 2, 2014

That is tricky... I think I'd be in favor of saying "slug," in order to keep how we refer to it consistent from the viewpoint of the API. How much would it confuse things if we put a note to developers clarifying the distinction?

@kadamwhite
After giving this more thought....
The best approach here is to just clarify the expectation in the docs and provide an example showing something like GET /taxonomy/category

The alternative of returning and accepting taxonomy objects with consistent object keys for label and name, doesn't fit with our approach for other endpoints like posts and users. (We prefer to keep naming within the API consistent within the front-end of WordPress, not the back-end.)

Owner

kadamwhite replied Jun 2, 2014

I am not sure I follow you. Are you proposing to, e.g. for tags, take /taxonomies/tag instead of /taxonomies/post_tag because the former is what is exposed to non-developers?

No, I just proposing adding a note about the expectation to the docs, so that API users know to send the request to: GET /taxonomy/post_tag (which in the taxonomy response is the slug)

Owner

kadamwhite replied Jun 2, 2014

Ah, yes, that makes sense—agreed. I'm not going to have much time this week to hack on this and will probably focus on unit tests in order to try to get this thing ready to review and merge, but @rachelbaker please feel free to update the docs notes in the way you've described.

Owner

rmccue commented Jun 3, 2014

The old routes now call _deprecated_function, and these errors are now returned via the X-WP-DeprecatedFunction and X-WP-DeprecatedParam headers. The default calls to trigger_error are also disabled for these, but can be reenabled by adding this to a plugin:

remove_filter( 'deprecated_function_trigger_error', '__return_false' );
remove_filter( 'deprecated_argument_trigger_error', '__return_false' );
Owner

rmccue commented Jun 3, 2014

Things that still need finishing up:

  • Test coverage
  • Calls to /posts/types/post/taxonomies embeds the post types; we should just give a list of type slugs here instead ($context = embed)
Contributor

kadamwhite commented Jun 4, 2014

Adding to that list:

  • The /posts/types/<type>/taxonomies route should be deprecated
Contributor

kadamwhite commented Jun 5, 2014

I've been trying to avoid embedding the post type in a taxonomy object within a post type object, and I"m having trouble with it. This is how things happen when you hit a post type object endpoint:

  • /posts/types/
    • Handler: WP_JSON_Posts::get_post_type
    • returns apply_filters( 'json_post_type_data', $data, $type, $_in_taxonomy );
      • Filter: WP_JSON_Taxonomies::add_taxonomy_data
      • calls $this->get_taxonomies( $type->name );
        • calls $this->prepare_taxonomy_object( $value, true );
        • returns apply_filters( 'json_prepare_taxonomy', $data, $taxonomy );
          • Filter: WP_JSON_Posts::add_post_type_data
            • Adds post type objects to taxonomy object

I only see two ways to proceed: Either pass something down all the way from WP_JSON_Posts::get_post_type to be checked in add_post_type_data at the end, or else to add another filter on json_post_type_data to run after add_taxonomy_data that would strip off the taxonomies again. Neither feels good. We're already tacking too many parameters on to the call chain in Posts as it is... Am I missing a third option?

Owner

rmccue commented Jun 8, 2014

I only see two ways to proceed: Either pass something down all the way from WP_JSON_Posts::get_post_type to be checked in add_post_type_data at the end, or else to add another filter on json_post_type_data to run after add_taxonomy_data that would strip off the taxonomies again. Neither feels good. We're already tacking too many parameters on to the call chain in Posts as it is... Am I missing a third option?

What we should do instead is pass the context down the chain and change what we add to the data.

Owner

rmccue commented Jun 8, 2014

Test coverage now at 100%!

@rmccue rmccue assigned rachelbaker and unassigned rmccue Jun 8, 2014

Owner

rmccue commented Jun 8, 2014

@rachelbaker #reviewmerge

@kadamwhite kadamwhite commented on an outdated diff Jun 8, 2014

lib/class-wp-json-posts.php
* @return array Post type data
*/
- public function get_post_type( $type, $_in_collection = false ) {
+ public function get_post_type( $type, $_in_collection = false, $_in_taxonomy = false ) {
@kadamwhite

kadamwhite Jun 8, 2014

Contributor

Should we use $context here as well, instead of $_in_collection and $_in_taxonomy?

@rmccue ReflectionMethod::setAccessible is PHP >= 5.3.2 only, so the build will always fail on PHP 5.2. http://php.net/manual/en/reflectionmethod.setaccessible.php

Collaborator

rmccue replied Jun 9, 2014

Ah nuts. Will solve in a different way then.

Owner

rmccue commented Jun 9, 2014

@rachelbaker Should be good to go! #reviewmerge

Owner

rachelbaker commented Jun 9, 2014

@rmccue @kadamwhite I am testing this now, and hope to have it merged before the end of my work day.

Owner

rachelbaker commented Jun 9, 2014

❗️ Merging

@rachelbaker rachelbaker added a commit that referenced this pull request Jun 9, 2014

@rachelbaker rachelbaker Merge pull request #211 from kadamwhite/add-taxonomy-base-route
Add new routes for taxonomies and terms.
a98607e

@rachelbaker rachelbaker merged commit a98607e into WP-API:master Jun 9, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

rachelbaker deleted the kadamwhite:add-taxonomy-base-route branch Jun 9, 2014

@kellbot kellbot pushed a commit to kellbot/WP-API that referenced this pull request Aug 1, 2014

@rachelbaker rachelbaker Merge pull request #211 from kadamwhite/add-taxonomy-base-route
Add new routes for taxonomies and terms.
8ee4390
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment