Return [], instead of empty post collection to improve compatibility

[rubas]

Related: #2914

Issue

An empty array is falsy in PHP, which allow's Twig's if to tests if an array is empty Doc. This doesn't work, if you have a lazy Post Collection (get_posts) and not an array.

You need to use length or empty to check the Post Collection in Twig in this case.

Solution

We check, if there are no results and return [] in this case

This allows for:

{% set posts = get_posts({'post_type': 'not_existing'}) %}

{% if posts %}
  This is false
{% endif %}

{% if posts is not empty %}
  This is false
{% endif %}

Impact

I'm interested to get feedback, if this approach has downsides.

If approved, I'm happy to fix/extend the tests.

[nlemoine]

I have mixed feelings about this. The fact that Twig evaluate an empty collection to true isn't technically wrong. It would be the same on the PHP side and this encourages strong explicit checks rather than weaker type juggling to boolean. I personally always use posts|length against iterables.

On the other hand, I think it's in Twig philosophy to make things easier on the template side.

I guess this won't hurt. @timber/rangers thoughts?

[rubas]

@nlemoine
I do understand your reservation. And personally I don't disagree with you.

I found this "issue" tracking down some bugs after upgrading to Timber 2. Suddenly some empty checks are not working anymore 👀

Now you have to keep track, if you are working with an array or a Post Collection in the templates. That is annoying and will introduce unnecessary bugs.

Looking back, I would have preferred, if Twig would have enforced a array|length check, but hey it was 2009 ;-)

[Levdbas]

I ran into this issue myself a few times as well so all in favor of implementing this 👍 We only have to update the testcase it seems in https://github.com/timber/timber/blob/ac9ee1b5db0b9d283a069cedcdfc01899cff5ed6/tests/test-timber-post.php#L13C21-L13C43

[romainmenke]

I would actually prefer it if this wasn't changed.
I think it is unexpected that the return type suddenly changes when the count is zero.

[nlemoine]

The return type doesn't actually change. An empty array is still of iterable type.

[romainmenke]

I meant the actual and complete type of the returned object, not the declared return type in the function signature :)

---

Edit :

\Timber::get_posts has a return type of \Timber\PostCollectionInterface|null and that function calls from_wp_query, so returning an empty array would actually be breaking.

You can't call ->to_array() on an empty array.

[Levdbas]

@timber/rangers , thoughts on @romainmenke comment?

[rubas]

@romainmenke @Levdbas

\Timber::get_posts has a return type of \Timber\PostCollectionInterface|null and that function calls from_wp_query, so returning an empty array would actually be breaking.

You can't call ->to_array() on an empty array.

There is no perfect answer, as long as falsifable is not integrated in PHP.

Intention

The intention afaik of PostCollection was the ability to lazy load data. A decision has been made, that lazy loading is useful, enough, that a new return type compared to WP & Timber 1.0 was introduced - with some drawbacks.

Just, the lazy load argument is obsolete for an empty return.

In the spirit of Timber 2.0, I would change the return type and return [].

Continuity

If you look from a continuity perspective from Timber 2.0, I do understand this would be a breaking change, and you need to check the return type before you call ->to_array(). But how many times, do you really need to call ->to_array()?

Especially compared to using {% if posts %}.

Which solution is better / worse?

Calling ->to_array() on an Array breaks immediately.

Using {% if posts %} on an empty PostCollection does not fail and introduces a silent, sneaky bug, which you may or may not catch eventually.

---

PS: Internally we use a wrapper around the function and return null for an empty result. Not my preferred solution, though.

[rubas]

@Levdbas
If this change should be introduced, I suggest doing it with the 2.1 as this a sensitive topic for everyone that wants to update to Timber 2.0. I'm pretty sure, this had already and will continue to create a ton of silent bugs. So the earlier, the better ...

[romainmenke]

But how many times, do you really need to call ->to_array()?

Apparently 46 times 😄.
We can work to reduce this and add checks for the remainder.

I do agree with the intention here, I just am not yet convinced that an empty array is the best solution. But I don't have a strong opinion either way. Whatever works best for the most people is fine.

[gchtr]

To me, it looks like merging this pull request is a good solution at the moment and it would help developers transition to 2.x. It would help more than it would hurt.

In case we want this to be stronger typed in the future, we could still revert it.

And then, we should probably to recommend using empty instead of a simple if-check as a best practice.

{% if posts is not empty %}
{% endif %}

[Levdbas]

@rubas , @gchtr , I've merged in 2.x branch to make some tests succeed but there are quite some tests that fail. So I guess we have to update some of the tests as well.

[nlemoine]

Using {% if posts %} on an empty PostCollection does not fail and introduces a silent, sneaky bug, which you may or may not catch eventually.

Not a bug per say, this is expected.

I'm really not convinced with that change. I'd rather leave the way it is. If we intend to revert it later, this will create even more confusion. But I'll leave the other @timber/rangers decide 🙂