Codecov Report

Merging #1931 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1931   +/-   ##
=========================================
  Coverage     95.04%   95.04%           
  Complexity     1555     1555           
=========================================
  Files            48       48           
  Lines          3670     3670           
=========================================
  Hits           3488     3488           
  Misses          182      182

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 255e912...08b195c. Read the comment docs.

Codecov Report

Merging #1931 into master will not change coverage.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #1931   +/-   ##
=========================================
  Coverage     94.98%   94.98%           
  Complexity     1565     1565           
=========================================
  Files            49       49           
  Lines          3689     3689           
=========================================
  Hits           3504     3504           
  Misses          185      185

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b78e929...22af873. Read the comment docs.

Coverage remained the same at 94.322% when pulling 08b195c on docs/term into 255e912 on master.

Coverage remained the same at 94.324% when pulling 1a8aa94 on docs/term into e663dee on master.

It's much clearer now 👍

As I written in the comments I'm not sure about one thing in get_posts - if I would like to use:

term.posts( { posts_per_page: 5, orderby: 'menu_order' }, 'page', `myClass` )

and I have class page user might think it should works like this:

• post_type will be set to page
• postClass will be set to myClass
  but it won't. It will use:
• post_type post
• postClass page

I think there should be some extra logic:

if ( $PostClass == '' && class_exists($post_type) ) {
	$PostClass = $post_type;
}

and

if ( $post_type != '' ){
    $args['post_type'] = $post_type;
} elseif ( $post_type == '' && !isset($args['post_type']) ) {
    $args['post_type'] = 'any';
} 

or am I overthinking this?

@palmiak You are right! The logic that you described doesn’t work. I added some more tests to cover the different use cases. I think the reason that your use case fails is that in Timber, there’s a somewhat hidden logic that automatically uses a class for a post when it has the same name as the requested post type (as described in #799 (comment)).

So when there’s a class page and we select the post type page, Timber::get_posts() will always use the page class, no matter what we pass in the third parameter of Term::posts(). Even if we would change the logic with your suggestions. But this is probably a use case most uses will very rarely run into. To me, that behavior seems intended, though I think it’s too much magic.

It’s also something that might change with object factories, according to @pascalknecht.

@gchtr I fully understand. I think that for now we won't make it any better. Let's wait for @pascalknecht change and then we'll think should what to do next.

Great work as always Lukas 👍

Some tests failed because of some autoloading issue. I submitted #1940 to fix that.

@palmiak Now the tests work! I’d still need an approving review from you to be able to merge this 😉.

Could @jarednova or @palmiak approve this pull request so I can finally merge it? 😉

Thanks @gchtr and @palmiak !