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

Conversation

@rmccue
Copy link
Member

@rmccue rmccue commented Mar 2, 2016

Following on from #2257: in #2311, @danielbachhuber noticed we weren't using the variable for the check. In the process of correcting that check, I discovered that the code will be broken with that check anyway.

We used to use wp_get_object_terms to get the uncached terms, but as it turns out, that doesn't support include, exclude, hide_empty, search, or slug anyway. These should be supported in wp_get_object_terms IMO, but we can't change this code in the meantime.

We could alternatively add the code to our get_terms_for_post call, but it'll be a pain as this stuff really needs to be done in the SQL query.

PR adds a failing test to show that exclude doesn't work currently.

@rmccue rmccue added this to the 2.1 milestone Mar 2, 2016
@joehoyle
Copy link
Member

I think this could be resolved by #2780, maybe @boonebgorges can chime in on that?

@boonebgorges
Copy link
Contributor

I think this could be resolved by #2780, maybe @boonebgorges can chime in on that?

Yes. wp_get_object_terms() now supports all arguments available to get_terms()/WP_Term_Query. See https://core.trac.wordpress.org/changeset/38667

@kadamwhite
Copy link
Contributor

From the discussion, is this superseded by #2780? That PR looks like it's blocked until 4.7 ships, does that change any part of the equation?

@kadamwhite
Copy link
Contributor

@rmccue confirmed in slack that this should be superseded by #2780; that PR depends on 4.7 so will be merged when applicable. Closing this one out in the meantime to reduce noise.

@kadamwhite kadamwhite closed this Oct 17, 2016
@boonebgorges
Copy link
Contributor

Just to chime in that I agree with closing this ticket in favor of #2780.

If the core merge doesn't happen for 4.7, and if the plugin plans to continue to support WP 4.6, and if the team thinks that this feature is important enough, then it'd be possible to write a shim for 4.6. IMO it'd be more trouble than it's worth, and can be viewed as a progressive enhancement when running WP 4.7, but I wanted to throw it out there.

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.

5 participants