-
-
Notifications
You must be signed in to change notification settings - Fork 513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Return [], instead of empty post collection to improve compatibility #2919
base: 2.x
Are you sure you want to change the base?
Conversation
I have mixed feelings about this. The fact that Twig evaluate an empty collection to 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? |
@nlemoine 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 Looking back, I would have preferred, if Twig would have enforced a |
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 |
I would actually prefer it if this wasn't changed. |
The return type doesn't actually change. An empty array is still of iterable type. |
I meant the actual and complete type of the returned object, not the declared return type in the function signature :) Edit :
You can't call |
@timber/rangers , thoughts on @romainmenke comment? |
There is no perfect answer, as long as IntentionThe 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 ContinuityIf 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 Especially compared to using Which solution is better / worse?Calling Using PS: Internally we use a wrapper around the function and return |
@Levdbas |
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 {% if posts is not empty %}
{% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙂
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 caseThis allows for:
Impact
I'm interested to get feedback, if this approach has downsides.
If approved, I'm happy to fix/extend the tests.