WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#18438 closed task (blessed) (fixed)

Create a new term in XMLRPC

Reported by: nprasath002 Owned by: westi
Milestone: 3.4 Priority: normal
Severity: normal Version:
Component: XML-RPC Keywords: has-patch
Focuses: Cc:

Description


Attachments (10)

wp.newTerm.patch (4.2 KB) - added by nprasath002 3 years ago.
wp.newTerm.2.patch (3.1 KB) - added by maxcutler 3 years ago.
taxes.combo.patch (12.6 KB) - added by maxcutler 2 years ago.
Combined patch with the 7 new methods for cycle 2.
taxes.combo.2.patch (13.0 KB) - added by maxcutler 2 years ago.
Refreshed patch after other recent changes.
term_unittests_new-edit.patch (12.1 KB) - added by markoheijnen 2 years ago.
1 test will fail due not returning id as a string
taxes.combo.3.patch (13.0 KB) - added by markoheijnen 2 years ago.
term_unittests_new-edit.2.patch (12.1 KB) - added by markoheijnen 2 years ago.
term_unittests_all.patch (27.7 KB) - added by markoheijnen 2 years ago.
18438-westi.diff (13.5 KB) - added by westi 2 years ago.
Updated version of combo 3 based on my changes so far to correct things while reviewing tests.
taxes.combo.5.patch (14.4 KB) - added by maxcutler 2 years ago.

Download all attachments as: .zip

Change History (37)

nprasath0023 years ago

maxcutler3 years ago

comment:1 maxcutler3 years ago

  • Cc max@… added

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

comment:2 westi2 years ago

  • Milestone changed from Awaiting Review to 3.4
  • Owner set to westi
  • Status changed from new to accepted
  • Type changed from feature request to task (blessed)

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

maxcutler2 years ago

Combined patch with the 7 new methods for cycle 2.

comment:3 maxcutler2 years ago

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

comment:4 markoheijnen2 years ago

  • Cc marko@… added

comment:5 maxcutler2 years ago

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

maxcutler2 years ago

Refreshed patch after other recent changes.

comment:6 maxcutler2 years ago

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

comment:7 follow-up: markoheijnen2 years ago

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

comment:8 in reply to: ↑ 7 nprasath0022 years ago

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

Version 1, edited 2 years ago by nprasath002 (previous) (next) (diff)

comment:9 follow-up: markoheijnen2 years ago

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?

markoheijnen2 years ago

1 test will fail due not returning id as a string

comment:10 markoheijnen2 years ago

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

comment:11 in reply to: ↑ 9 nprasath0022 years ago

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

comment:12 markoheijnen2 years ago

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.

markoheijnen2 years ago

comment:13 follow-ups: markoheijnen2 years ago

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.

comment:14 in reply to: ↑ 13 maxcutler2 years ago

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.

comment:15 in reply to: ↑ 13 westi2 years ago

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 :)

comment:16 westi2 years ago

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

comment:17 markoheijnen2 years ago

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.

westi2 years ago

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

comment:18 maxcutler2 years ago

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

comment:19 follow-up: markoheijnen2 years ago

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

comment:20 in reply to: ↑ 19 maxcutler2 years ago

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

comment:21 maxcutler2 years ago

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.

maxcutler2 years ago

comment:22 maxcutler2 years ago

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.

comment:23 follow-up: maxcutler2 years ago

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?

comment:24 in reply to: ↑ 23 westi2 years ago

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.

comment:25 westi2 years ago

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.

comment:26 westi2 years ago

In [20159]:

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

comment:27 maxcutler2 years ago

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

New bugs or enhancements on new tickets.

Note: See TracTickets for help on using tickets.