Create a new term in XMLRPC

[maxcutler]

Updated patch formatting and tightened the validation logic. Also fixed the return value which did not match the docstring.

[westi]

We are including this in our second cycle on XML RPC new features for 3.4 and will use it as the tracking ticket for this cycle's other tickets: #18439, #18440, #18441, #18442, #18443, and #18444

[maxcutler]

Combined patch with the 7 new methods for cycle 2.

[maxcutler]

As discussed on IRC, I've put together a patch that combines the methods from the seven taxonomy-related XML-RPC method tickets.

[maxcutler]

Updated the patch after the changes in [19848]. Going to start working on unit tests later in the week.

[maxcutler]

Refreshed patch after other recent changes.

[maxcutler]

I've updated the taxes.combo.2.patch file to apply cleanly after other recent commits to class-wp-xmlrpc-server.php.

[markoheijnen]

Was busy with creating unit tests for the patch and I wonder the following things:

wp_newTerm

• Check if taxonomy is set and is a string
• Why is there the following line: $taxonomy = (array) taxonomy?
• return term_id should have strval()

wp_editTerm

• should the current_user_can check $term_id as second parameter?
• error check op $parent_term but trying to show the error of $term
• Error message for parent variable is not the same as newTerm, the same for error code

[nprasath002]

Replying to markoheijnen:

Was busy with creating unit tests for the patch and I wonder the following things:

wp_newTerm

• Check if taxonomy is set and is a string

A new term must be associated with a taxonomy. So we check whether the input taxonomy is valid.

• Why is there the following line: $taxonomy = (array) taxonomy?

get_taxonomy() returns a taxonomy object.
We use $object->key to get the value from the object.
To use $objectkey? you must cast the object to an array.
Of course we can use $object-key through out.
But all of the other functions are coded in $objectkey? style.
For consistency i casted it to an array.

• return term_id should have strval()

+1

wp_editTerm

• should the current_user_can check $term_id as second parameter?

I think for terms we check capability in general. Not for a specific term like in posts.
AFAIK there is no 'edit_term' cap defined in the core.

• error check op $parent_term but trying to show the error of $term

+1. Should fix that

• Error message for parent variable is not the same as newTerm, the same for error code

+1

[markoheijnen]

Thanks for the answers. About the taxonomy check if you pass an array it will result in an error since an array can't be a key.
Not sure if this is something that should be checked tho.

About the error codes is there a page about which codes should apply to an error?

[markoheijnen]

1 test will fail due not returning id as a string

[markoheijnen]

Added some basic unit tests for creating and editing a term. First time I wrote something like this. Love to get some feedback.

[nprasath002]

Replying to markoheijnen:

Thanks for the answers. About the taxonomy check if you pass an array it will result in an error since an array can't be a key.
Not sure if this is something that should be checked tho.

About the error codes is there a page about which codes should apply to an error?

The error codes are based on http error codes,
​http://en.wikipedia.org/wiki/List_of_HTTP_status_codes

although some of the error codes in class-xmlrpc-server don't line up perfectly

[markoheijnen]

I thought so but wasn't sure. I think a lot of the 500 error code should be changed since in the XML-RPC we don't know the reason but the function that is given the error does. It returns an error message so error code 500 isn't the right one. 403 makes more sense then.

[markoheijnen]

Added a new patch and new unit tests.

• return term_id should have strval()
• error check op $parent_term but trying to show the error of $term
• Error message for parent variable is not the same as newTerm, the same for error code
• Changed some of the error messages so it is the same for all the term/taxonomy methods

Did wonder if someone can explain why there is an assign term check for wp_getTerm and wp_getTerms. Same for the edit taxonomy check for wp_getTaxonomy and wp_getTaxonomies. I think those checks can be removed.

[maxcutler]

Replying to markoheijnen:

Did wonder if someone can explain why there is an assign term check for wp_getTerm and wp_getTerms. Same for the edit taxonomy check for wp_getTaxonomy and wp_getTaxonomies. I think those checks can be removed.

I think that was an attempt to restrict access so that not just any user can fetch the taxonomy information. I'm not sure if or what a better cap check could be, or even what the data leakage concerns would be without a cap check.

The new wp.getPost and wp.getPosts methods check the edit_post cap for similar reasons, I assume.

[westi]

Replying to markoheijnen:

Added a new patch and new unit tests.

• return term_id should have strval()
• error check op $parent_term but trying to show the error of $term
• Error message for parent variable is not the same as newTerm, the same for error code
• Changed some of the error messages so it is the same for all the term/taxonomy methods

Did wonder if someone can explain why there is an assign term check for wp_getTerm and wp_getTerms. Same for the edit taxonomy check for wp_getTaxonomy and wp_getTaxonomies. I think those checks can be removed.

Thanks for the updates - I started reviewing the tests and patch yesterday and came accross some of these too.

I'll restart my review from the latest patches :)

[westi]

Unit Tests committed in ​[UT529] and ​[UT530] - There are a couple of bugs in them which I'm going to fix up post-commit.

[markoheijnen]

Damn, I uploaded the wrong unit test file. Uploaded here again which also include all the other tests.
Do wonder how we should check the data we get back.

[westi]

Updated version of combo 3 based on my changes so far to correct things while reviewing tests.

[maxcutler]

It might be worth adding another function akin to ​wp.suggestCategories that can work against any of the taxonomies.

[markoheijnen]

For the terms get method don't we need to set the arg hide_empty to false for the function get_terms() ? Or at least the ability to modify it?

Also for get_term() shouldn't we set the output to ARRAY_A and for casting it to an array use get_object_vars()? Like WordPress does if you look into the get_term() function

[maxcutler]

Replying to markoheijnen:

For the terms get method don't we need to set the arg hide_empty to false for the function get_terms() ? Or at least the ability to modify it?

I would be in favor of adding an optional $filter parameter that covers most of the arguments to get_terms, just like we did for wp.getPosts.

Also for get_term() shouldn't we set the output to ARRAY_A and for casting it to an array use get_object_vars()? Like WordPress does if you look into the get_term() function

+1

[maxcutler]

Cap check changes ​discussed yesterday on IRC:

• Only wp.editTerm needs to check the edit_terms cap. The wp.getTerm, wp.getTerms, wp.getTaxonomies, and wp.getTaxonomy methods should check assign_terms instead since they are read-only.
• nacin suggested wp.newTerm should also check assign_terms instead of edit_terms/manage_terms, since currently that's all that's required to create a new tag. He's going to follow up/confirm after doing some more research.

[maxcutler]

Refreshed patch with following changes:

• Tweaked caps on wp.getTaxonomy and wp.getTaxonomies per previous comment.
• Added optional filter parameter to wp.getTerms to support more scenarios, including hide_empty=0 and search (in lieu of a separate wp.suggestTerm method).
• Use get_object_vars to convert term object to array.

[maxcutler]

get_term/get_term_by return all field values as strings, even though some represent ints. For example, count and the various IDs (term_id, parent, term_group, term_taxonomy_id). Should we cast these fields to int in _prepare_term?

[westi]

Replying to maxcutler:

get_term/get_term_by return all field values as strings, even though some represent ints. For example, count and the various IDs (term_id, parent, term_group, term_taxonomy_id). Should we cast these fields to int in _prepare_term?

We should explicitly cast everything to whatever we want it returned as IMHO so as to future proof the code against internal api changes (in the past these have accidentally changed the datatypes returned by XMLRPC).

For anything where the db schema supports values larger than XML-RPC for Integers we should return them as strings to be safe.

[westi]

In [20137]:

XML-RPC: Initial implementation of Taxonomy and Term APIs.

Implements: wp.newTerm, wp.editTerm, wp.deleteTerm, wp.getTerm, wp.getTerms, wp.getTaxonomy and wp.getTaxonomies

See: #18438, #18439, #18440, #18441, #18442, #18443, and #18444 props maxcutler, markoheijnen and nprasath002.

[westi]

In [20159]:

XMLRPC: Start casting datatypes in _prepare_term so as to ensure consistent datatypes in our responses. See #18438.

[maxcutler]

New bugs or enhancements on new tickets.