Skip to content
This repository was archived by the owner on Sep 24, 2018. It is now read-only.

Add controller for a post's terms - #1216

Merged
danielbachhuber merged 19 commits into
developfrom
post-terms-controller
May 21, 2015
Merged

danielbachhuber merged 19 commits into
developfrom
post-terms-controller

Conversation

@joehoyle

@joehoyle joehoyle commented May 8, 2015

Copy link
Copy Markdown
Member

Should we allow DELETE /wp/v2/posts/1/terms/post_tag to remove all tags from the post?

Also should we allow POST /wp/v2/posts/1/terms/post_tag with body { ids: 1,2,3,4 } to assign more than 1 term to a post?

See #1199

joehoyle added 2 commits May 7, 2015 17:46
This allows you to create and delete post term relationships
@joehoyle

joehoyle commented May 8, 2015

Copy link
Copy Markdown
Member Author

@WP-API/amigos #review

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to pass taxonomy here too, as the route should be explicitly defined based on the taxonomy. See #1170

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 20621e8

@danielbachhuber

Copy link
Copy Markdown
Member

Some nits.

@joehoyle

Copy link
Copy Markdown
Member Author

Ready for another look!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check show_in_rest or public values for this taxonomy?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based off https://github.com/WP-API/WP-API/blob/develop/lib/endpoints/class-wp-rest-taxonomies-controller.php#L92-93 I think no (or at least that this point anyway). If we want to adopt show_in_rest for taxonomies, it should probably be applied universally and start at the taxonomies controller.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, maybe I need to check public though

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, turns out we don't need this now that the controller is a post_type / taxonomy pair. This checking is done already on route registration in plugin.php.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the previous ~10 lines of checks should be abstracted in some form.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 36c8073

@danielbachhuber

Copy link
Copy Markdown
Member

@joehoyle left some comments

joehoyle added 6 commits May 21, 2015 15:51
This is already accounted for by setting up the taxonomy
Conflicts:
tests/test-rest-terms-controller.php

It looks like you may be committing a merge.
If this is not correct, please remove the file
.git/MERGE_HEAD
and try again.
@joehoyle

Copy link
Copy Markdown
Member Author

@danielbachhuber want another bash?

danielbachhuber pushed a commit that referenced this pull request May 21, 2015
Add controller for a post's terms
@danielbachhuber
danielbachhuber merged commit 5e2c59a into develop May 21, 2015
@danielbachhuber
danielbachhuber deleted the post-terms-controller branch May 21, 2015 22:59
@danielbachhuber

Copy link
Copy Markdown
Member

🍰

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants