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

Fix post taxonomy terms link urls #1447

Merged
merged 4 commits into from
Aug 5, 2015
Merged

Fix post taxonomy terms link urls #1447

merged 4 commits into from
Aug 5, 2015

Conversation

rachelbaker
Copy link
Member

Fixes #1383

This was missed in #1216, where the routes for the Taxonomies and Terms for a Post moved.

@rachelbaker
Copy link
Member Author

@WP-API/amigos #reviewmerge

@joehoyle joehoyle self-assigned this Aug 3, 2015
@@ -1155,13 +1155,11 @@ protected function prepare_links( $post ) {
}

if ( 'post_tag' === $tax ) {
Copy link
Member

Choose a reason for hiding this comment

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

We should remove this special case, I can pick up if you want @rachelbaker

Copy link
Member Author

Choose a reason for hiding this comment

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

@joehoyle thank you, please do.

rachelbaker added a commit that referenced this pull request Aug 5, 2015
Fix post taxonomy terms link urls
@rachelbaker rachelbaker merged commit f0e5d25 into develop Aug 5, 2015
@rachelbaker
Copy link
Member Author

Merged #1447

@rachelbaker rachelbaker deleted the fix-post-term-links branch August 5, 2015 23:19
@@ -1154,13 +1154,8 @@ protected function prepare_links( $post ) {
continue;
}

if ( 'post_tag' === $tax ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there going to be some alignment on "tag" (rest_base) vs "post_tag" (slug)? I get that "post_tag" is "ugly," but who really cares? The API should be data-driven, and not make special cases for personal preferences. The more the API special-cases "tag", the less consistent it becomes. Please, I beg you, use $tax->slug everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

@jdolan this issue is about using the rest base, so your concern is not specifically here. However, as far as the API exposes data, you'll only ever see tag, not a mix - so I don't know why it's a big issue. Tags are not just for posts, so the name is confusing, that's my 2c anyway. This change just fixes one small inconsistency where we didn't use rest_base (where we always should be doing).

Copy link
Contributor

Choose a reason for hiding this comment

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

It's an issue because rest_base is never exposed to the client. Nowhere in the JSON response for the taxonomy object does it include rest_base. It does, however, include the slug -- and slug is precisely what should be used to map to URIs. That's what slug is for. See #1381.

Copy link
Member

Choose a reason for hiding this comment

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

@jdolan #1381 still stands, that's not about what this fix was for. This is about using rest_base to build the url. If #1381 means we set rest_base to be post_tag then fine. This code is right either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I realize that. I just see more special casing to accommodate the rest_base mistake seeping into the code and it makes me cringe. This code doesn't even use rest_base from a constant or some other variable -- it just replicates it. If I'm a jerk for pointing this out, then I apologize.

Copy link
Member

Choose a reason for hiding this comment

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

@jdolan rest_base is not about post_tag we have to support it for custom taxonomies too. If anything, this commit supports you argument, it is getting rid of special casing the case.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants