Codecov Report

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

@@            Coverage Diff            @@
##             master    #1802   +/-   ##
=========================================
  Coverage     94.59%   94.59%           
  Complexity     1532     1532           
=========================================
  Files            48       48           
  Lines          3606     3606           
=========================================
  Hits           3411     3411           
  Misses          195      195

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 3239640...b272bc7. Read the comment docs.

Coverage remained the same at 93.999% when pulling b272bc7 on mdahlke:master into 3239640 on timber:master.

@mdahlke Thank you for your first contribution 👍 . I had a quick look at the code. One thing is missing: Whenever a new feature gets added we also require unit tests with them to confirm the code actually works as intended. Would you be up to adding these tests or do you require assistance?

@mdahlke Thanks for this pull request! I agree that we need this. Just today I ran into the same issue and had to solve it with a separate function. I’m very glad you raised this issue and provided a proposal for a solution 👍. It’s really great if we have so many developers who bring in their thoughts! 😎 But sometimes a little change like this can have a bigger impact than you initially thought or raise a bigger discussion.

Adding another parameter to the function might seem like the easiest solution. But then we’d have one more optional parameter in that function. I’m always hesitant about adding too many optional parameters, because then some questions arise. How should we define which parameter is the most important? If somebody wants to change only the last parameter, does he always need to check the defaults for the other parameters and pass them as well?
There are other areas in Timber where we have the same problem. I always propose to use an arguments array instead, because that seems like a good compromise in a situation like this. In this very particular case of getting terms, I’d even want to make a strong case for this:

When using Timber::get_terms(), we already need to pass the taxonomy along with other parameters. We use the same pattern that is used for WordPress’s get_terms() function. Couldn’t we use the same pattern here, so that we merge the tax and args into one array?

$terms = $post->terms( array(
    'taxonomy' => 'custom_tax',
    'orderby'  => 'count',
) );

I think it’s not important to know for a developer that in the back, Timber uses wp_get_post_terms(), which takes different parameters than get_terms().

Of course this would be a breaking change and we’d have to merge it into the 2.x branch instead. When we do that, I’d even go further and say that we use only one parameter:

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

An arguments array allows you to only pass the parameters that you want to change. Remember, we just merged in a pattern like this in #1778 for Timber\PostQuery:

$persons = new Timber\PostQuery( array(
    'query' => array(
        'post_type' => 'person',
    ),
    'post_class' => 'Person',
) );

Did I miss something? Any other opinions (@pascalknecht, @jarednova, @chrisvanpatten)?

@gchtr I totally agree. I like the pattern of #1778 where we have ~3x arguments. This could be implemented here in a way that doesn't break backwards compatibility via an adapter function (not sure if that's what it's called, but basically function overloading) that will run a modified function when that first argument is a two-dimensional array.

@mdahlke: is that something you'd be up for?

@jarednova I think that @gchtr solution is a much better way of implementing this than my quick and dirty solution. I say go that direction with it instead of just arbitrarily adding another argument.

@jarednova @mdahlke @pascalknecht Okay, I’m going to try to add this without breaking backwards compatibility 👍.

Thanks again for the PR @mdahlke! @gchtr ended up with a broader solution in #1845, but this is what got the ball rolling. So, while we're not going to merge in this PR know that it shall burn in our hearts forever — thanks again for contributing to 🌲 !