Skip to content

2203/post terms api#2208

Merged
acobster merged 4 commits into2.x-term-factoriesfrom
2203/post_terms_api
Mar 6, 2020
Merged

2203/post terms api#2208
acobster merged 4 commits into2.x-term-factoriesfrom
2203/post_terms_api

Conversation

@jarednova
Copy link
Copy Markdown
Member

@jarednova jarednova commented Mar 1, 2020

Ticket: #2203

Issue

The API between Timber::get_terms and Post::terms() was out of whack. We used a query array attribute in one, but not the other.

Solution

Get Post::terms's API to match Timber::get_terms() which follows the overall Factories pattern.

Impact

No performance/query additions. The code is backwards compatibility, though I mark this section as deprecated.

Usage Changes

Yes, noted in docs. Previous usage:

$terms = $post->terms( [
      'query' => [
             'taxonomy' => 'custom_tax',
             'orderby'  => 'count',
         ],
         'merge'        => false,
        ] );

Revised usage:

$terms = $post->terms( [ 'taxonomy' => 'custom_tax',
                         'orderby'  => 'count' ], 
                       [ 'merge'        => false ] );

Considerations

As @gchtr we should also update Term::posts to match Factories as well and rectify that with #1931

Testing

Existing tests pass, added new test TestTimberPostTerms::testPostTermsUsingUsingFactories to validate new argument structure

@jarednova jarednova requested review from gchtr and palmiak as code owners March 1, 2020 20:42
@jarednova jarednova changed the base branch from master to 2.x-term-factories March 1, 2020 20:42
@jarednova jarednova requested review from acobster and removed request for palmiak March 1, 2020 20:42
@jarednova jarednova mentioned this pull request Mar 1, 2020
Merged
@jarednova jarednova added the 2.0 label Mar 1, 2020
@jarednova jarednova requested a review from pascalknecht as a code owner March 4, 2020 20:12
gchtr
gchtr approved these changes Mar 4, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@gchtr gchtr left a comment

Choose a reason for hiding this comment

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

This looks good to me! I only updated some of the DocBlock formatting.

As @gchtr mentioned we should also update Term::posts to match Factories as well and rectify that with #1931

Yes, let’s not forget that!

@jarednova
Copy link
Copy Markdown
Member Author

jarednova commented Mar 6, 2020

@gchtr is there a package/library you use to apply the correct formatting and line lengths? There's no way the method I'm using (manual indentation) is the most efficient.

@acobster since this is a PR into the 2.x-term-factories branch, can you merge if this looks good to you?

@acobster acobster mentioned this pull request Mar 6, 2020
Open
acobster
acobster approved these changes Mar 6, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@acobster acobster left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@acobster acobster merged commit d0d2fea into 2.x-term-factories Mar 6, 2020
@acobster acobster deleted the 2203/post_terms_api branch March 6, 2020 19:49
@acobster acobster restored the 2203/post_terms_api branch March 6, 2020 19:49
@jarednova jarednova deleted the 2203/post_terms_api branch March 7, 2020 00:39
@gchtr
Copy link
Copy Markdown
Member

gchtr commented Mar 9, 2020

@gchtr is there a package/library you use to apply the correct formatting and line lengths? There's no way the method I'm using (manual indentation) is the most efficient.

@jarednova I use PhpStorm’s auto formatting feature for DocBlocks. The only thing it won’t format properly are Parameters that are arrays. I don’t know of any CLI tool that could do it automatically 🤷‍♂️.

@jarednova
Copy link
Copy Markdown
Member Author

jarednova commented Mar 11, 2020

yeah, those arrays were the ones that were really killing me — oh well!

@gchtr gchtr mentioned this pull request Jun 6, 2020
Merged
gchtr added a commit that referenced this pull request Jun 6, 2020
@gchtr
#2208 Improve Upgrade Guide for Timber\Post::terms()
6b186a1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@acobster acobster acobster approved these changes

@gchtr gchtr gchtr approved these changes

@pascalknecht pascalknecht Awaiting requested review from pascalknecht

Assignees

@acobster acobster

Labels

2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jarednova @gchtr @acobster