fix: consider 'fields' value when returning terms from query

[jrathert]

Related:

• Ticket [BUG] Timber\Post->terms() throws error if 'fields' parameter has value other than 'all' #2802

Issue

A fields argument other than allor all_with_object_id lead to a fatal error, as TermFactory::from_wp_term_query() did expect the result of a query to always be of type WP_term[]

Solution

In TermFactory::from_wp_term_query() call TermFactory::build() only if fields is all or all_with_object_id, otherwise, return plain result of WP_Term_Query::get_terms() (i.e., int[]|string[]|string)

Considerations

One could - instead of examining the fields argument - check if the object returned from WP_Term_Query::get_terms() actually is a WP_Term[]. That would be future proof (if there are fields values added for which the query also returns an array of WP_Term) - but might have some impact on performance, so I decided to solve it this way.

Testing

No specific

[gchtr]

This feels like sensible approach to me.

I think we would also need tests for this. @jrathert Do you have testing set up and can you add tests? Or would you rather have us do it?

@nlemoine I’d be glad about your opinion on this.

[jrathert]

@jrathert Do you have testing set up and can you add tests?

I did not have it when I wrote the code, but now I have ;-). Can add some in the the next days.

[jrathert]

@gchtr Resolved your comments and added a number of tests covering all possible return values of WP_Term_Query::get_terms().

I tried to write the tests in a way that there is sort of an assertion problem with the old code that is now solved with my patch. I have to admit, I failed with that, as in the original code the root cause was the call of the build() function with inappropriate parameters. It is impossible to write a test that can detect that, IMHO.

[jrathert]

Did a few optimizaitons for the assertions (assertSame, not assertEquals etc)

[nlemoine]

Thank you @jrathert! We just need to take a decision on #2806 (comment)

And it will be good to go.

[jrathert]

Removed the unnecessary '_ - should be ready to merge now.

[gchtr]

I’m continuing the discussion we started in #2806 (comment) here.

Very good that the issue with count popped up here.

I would say: Timber::get_terms() should be used for getting objects or ids. If you need count, use get_terms. There's actually no benefit in using Timber::get_terms() for getting anything else than objects (event IDs) and keeping return types seems more important to me.

I agree with that.

Maybe in the fact that we allow a WP_Term_Query to be passed or maybe we should throw something when count is met, after all, it's a term factory, not a count term factory :) You should expect an iterable here.

We could use Timber\Helper::doing_it_wrong() here if count is found and advise to use get_terms() instead.

This could mean that we could keep iterable, right? Even though TermFactory::from() doesn’t have return types.

General error handling is still something we need to figure it in the future (#2210).

[gchtr]

I just talked to @nlemoine. We going to drop iterable and will probably come back to this once we can use union types, which is when we drop support for PHP 7.4 in a future version.

[gchtr]

Some tests only fail because of an issue with testing against WordPress 6.4. For now, we can ignore these tests. I’m merging this in.