Add/rest pre insert filter terms #2377
Conversation
| } | ||
|
|
||
| if ( isset( $request['parent'] ) ) { | ||
| if ( ! is_taxonomy_hierarchical( $this->taxonomy ) ) { |
There was a problem hiding this comment.
I don't think prepare_item_for_database() should ever return a WP_Error. You can silently ignore the parent param if it's not set in the schema.
There was a problem hiding this comment.
Would silently ignoring the parent parma be setting it to -1 instead of throwing an error, or not setting it at all within the $prepared_term object?
Also, if I remove the returned WP_Error objects then should I remove these checks as well?
- https://github.com/WP-API/WP-API/pull/2377/files#diff-6a5f19de0f37a384ee52877de312ee67R409
- https://github.com/WP-API/WP-API/pull/2377/files#diff-6a5f19de0f37a384ee52877de312ee67R337
There was a problem hiding this comment.
Would silently ignoring the parent parma be setting it to -1 instead of throwing an error, or not setting it at all within the $prepared_term object?
Probably setting it to 0, as that's the closest representation to a WP Term object
Also, if I remove the returned WP_Error objects then should I remove these checks as well?
Yes.
* FIXED: The prepare_item_for_database function will now only return a valid WP_Term object
|
@danielbachhuber let if me if that new commit 5bc8309 is good for a merge. |
| if ( isset( $request['term_group'] ) ) { | ||
| $prepared_term->term_group = (int) $request['term_group']; | ||
| } | ||
|
|
There was a problem hiding this comment.
We don't support setting term_taxonomy_id through the REST API. term_taxonomy_id and term_id are the same in our eyes, and term_id is always exposed as id.
|
@kjbenk Better. Left comments on a few more things needing to be changed. |
* FIXED: Removed all term attributes that are either readonly or
irrelevant within the `prepare_item_for_database` function
* FIXED: Changes the `rest_pre_insert_` filter to account for all
taxonomies not just the generic `term`, so it is now
`rest_pre_insert_{$this->taxonomy}`
|
@danielbachhuber thanks for the feedback, I am learning a lot :) The next commit has passed so you are free to review it. |
|
🚢 🚢 🚢 |
Add/rest pre insert filter terms
2 participants
Fixes this issue #2368