[BUGFIX] Handle generators and traversables correctly in ForViewHelper#1384
[BUGFIX] Handle generators and traversables correctly in ForViewHelper#1384Kanti wants to merge 1 commit into
Conversation
3c55772 to
772a0d2
Compare
Kanti
changed the title
772a0d2 to
2c0411e
Compare
Kanti
changed the title
2c0411e to
cf316b1
Compare
Kanti
changed the title
| $this->arguments['each'] = array_reverse($this->arguments['each'], true); | ||
| } else { | ||
| $items = []; | ||
| foreach ($this->arguments['each'] as $keyValue => $singleElement) { |
There was a problem hiding this comment.
I'm still not a fan of preemptively unwinding a Generator which may produce a lot of items. One point of generators is reducing memory usage which would be rendered useless here.
Still that's the behavior we always had so far so I guess it's acceptable.
There was a problem hiding this comment.
The generator can only be read once and cannot be reversed unless we collect all the items and reverse them.
Therefore, there is no other possible solution here.
The same problem applies if we need the count for each of the elements.
| } | ||
| } | ||
| if (isset($this->arguments['iteration'])) { | ||
| if ($items === null && !is_countable($this->arguments['each'])) { |
There was a problem hiding this comment.
This is rather hard to read and understand. We could instead always use $items as the result of a normalization from various input formats.
Also we completely skip Countable here. One can in fact implement a very memory friendly object which iterates using a generator and implement Countable. (Think e.g. SELECT * & COUNT(*) for DB data.)
There was a problem hiding this comment.
I could not find a solution where we could always use $items while keeping the generators streaming the items in case iteration and reverse are not used.
This sadly gives us two different ways to iterate.
I will try to make it more readable
There was a problem hiding this comment.
I think it's more readable now. What do you think?
Materialize traversables as key-value pairs when reverse iteration or iteration metadata requires looking ahead. This keeps duplicate yielded keys intact and allows iteration totals to work for non-countable traversables. Add coverage for non-countable traversables, generators, duplicate keys, and reverse rendering.
cf316b1 to
d9b986c
Compare
2 participants
What changed
This PR improves how
ForViewHelperhandles generators and other traversables.The ViewHelper sometimes has to look at all items before rendering them, for example when
reverse="true"is used or when{iterator.total}is needed.Until now, that was done with
iterator_to_array().That works for simple cases, but it loses entries when a generator yields the same key more than once.
The implementation now keeps materialized traversable items as key-value pairs instead.
That way, every yielded item is preserved, including duplicate keys.
Plain generator loops are still rendered directly whenever possible,
so we avoid consuming or buffering generators unless Fluid actually needs to know the full item list up front.
Why
PHP
foreachrenders every yielded generator item, even if keys are duplicated.ForViewHelpershould behave the same way.Without this change, using
iterationorreversecould silently drop entries from a valid generator.Tests
This adds test coverage for:
{iterator.total}{iterator.total}{iterator.total}reverse="true"reverse="true"reverse="true"and{iterator.total}reverse="true"and{iterator.total}