TYPO3 Core

The bug can also be reproduced by simply nesting f:for loops

<ul>
    <f:for each="{categories}" as="cat1">
        <li>
            {cat1.uid}
                <ul>
                <f:for each="{categories}" as="cat2">
                    <li>{cat2.uid}</li>
                </f:for>
                </ul>
        </li>
    </f:for>
</ul>

The bug is caused by php passing objects by reference and can be fixed by having f:for always convert 'each' arguments that implements \Traversable to arrays.

This should cause no ill effects as this already happens when passing reverse="true" to the viewhelper ( this fixes the bug, but renders it in reverse )

in

https://forge.typo3.org/projects/typo3cms-core/repository/entry/typo3/sysext/fluid/Classes/ViewHelpers/ForViewHelper.php?rev=TYPO3_7-6

move lines 102-103 to line 99

seems a no-brainer fix

The reason is not so much the passing by reference - the reason is that array pointers work differently than Traversable pointers in two ways:

• An array is not copied by reference which means iteration receives a fresh pointer
• But more importantly, pointers are not reset when an iteration ends, so when you exit your inner iteration, the pointer will be at the end of the Traversable and the loop will exit.

It is the second piece of information that is vital knowledge here. It applies anywhere that you might attempt to do the same procedure!

You mention that converting the Traversable to an array fixes the problem. That is technically true, but it also has some rather severe side effects:

• Memory usage increases drastically if the Traversable is one that loads objects, e.g. a QueryResult.
• You lose the ability to use a custom Traversable and have the offsetGet etc. run in the loop - they will instead fire before and you lose control after that.
• You lose the ability to iterate a Generator correctly (and it increases the memory consumption of using a Generator in the first place)
• You potentially convert the same Traversable over and over
• You must convert every Traversable (or add a number of control mechanisms such as VH arguments to opt-in/out to such behavior)

Not to mention that any third-party implementations which declare they support Traversable and are passed a Traversable actually receive an array. Imagine the destructiveness that would have on third-party implementations of Traversable which include other functionality.

This explains why I personally would reject using this method to fix it. Yes, it fixes the issue, but the cost is quite severe - and after all this aims to solve an edge case that isn't possible in standard PHP either.

Now, having rejected the idea of solving it with copy to array, here are some options that you have:

• Convert the QueryResult to an array in your controller when you need to accommodate this edge case.
• Use any of the VHS ViewHelpers which modify arrays or Traversables (they always output arrays).
• From TYPO3v8 and onward and in Standalone Fluid, use `{myTraversable as array}` to automatically cast your edge-case afflicted Traversable to an array in one or both of the iterations you do.

Either of these are reasonable solutions but I think personally I would prefer converting it in the controller if you are on 7.6 LTS and have a controller. On 7.6 without a controller I would opt for the VHS method. And finally on anything else I would take advantage of the casting expression node and just cast the variable only when I access it in ways that require it to be an array.

Hope this explains it.

Almost forgot: VHS has `v:variable.convert` which also converts Traversable to array in a jiffy. https://fluidtypo3.org/viewhelpers/vhs/master/Variable/ConvertViewHelper.html

A couple of points;

• All of the doom and gloom you posted already happens - if you iterate in reverse, it seems very illogical that the iteration direction should produce inconsistent results.

• Not sure if rendering trees can really be considered an edge case

• arrays are copied by value that means each call to f:for would receive it's own personal copy, what nested calls do to their copy ( resetting iteration pointers or not ) is irrelevant to the caller as it's copy is not changed by this.

• finally this is about fluid in the core not the VHS extension

Fair point about casting the query result to array in the controller, but that does require you to know that nesting/recursion in fluid does not create a new scope ( which I don't think is documented anywhere )

To clarify; I don't particularly need for f:for to cast to an array ( this is just what already happens when asking for reverse iteration ) - a shallow copy of 'each' would work just as fine and allow any fancy custom traversible implementations to work still.

f:for just need to create a personal scope to avoid mucking with iterators that are not it's own.

An alternative could be to have it simply restore the iterator pointer upon exit, but I'm not sure that's advisable

All of the doom and gloom you posted already happens - if you iterate in reverse, it seems very illogical that the iteration direction should produce inconsistent results.

I'm not remotely a fan of that particular feature either and it may go away.

Not sure if rendering trees can really be considered an edge case

Trees - no. Same Traversable recursively - I would say so.

arrays are copied by value that means each call to f:for receives it's own personal copy, what nested calls do to their copy ( resetting iteration pointers or not ) is irrelevant to the caller as it's copy is not changed by this.

Correct. Copy by value is part of why I am concerned about this.

finally this is about fluid in the core not the VHS extension

Yes, and you are completely forgiven for not knowing this, but we are currently working on a very ambitious goal to convert much of VHS to a generic ViewHelper package that is likely to be included in TYPO3v9. This generic package will among other things contain all the iterator ViewHelpers (and this is one thing that would make the "reverse" argument redundant - that purpose is then solved by chaining a ViewHelper when passing the argument). So "it's VHS not core" will soon become a moot point. That, and the variable is possible to cast in TYPO3v8 and onward so if your case requires an array, you can demand an array.

And the point still stands that this procedure is not supported by PHP either, without explicitly converting one of the iterated instances to an array.

This then becomes a matter of accommodating 7.6 LTS and I've given you two options which would prevent this issue without forcing every single other LTS user into more memory consumption, broken Traversable predictability etc.

but that does require you to know that nesting/recursion in fluid does not create a new scope ( which I don't think is documented anywhere )

Actually, it's deeper than that: it requires that you know that you are passing a Traversable and that PHP foreach does what it does if you render a Traversable recursively; plus how arrays are different from this. You need to understand pointers and how they behave with Traversable, essentially. If you know this, you also know the use case you attempt simply is not possible in PHP either without explicit conversion - and then you know how to solve your problem (using any of the methods I recommended).

This is, in fact, pure PHP knowledge and does not pertain in particular to Fluid iterations - it is worth while knowing this about any PHP implementation you make, and I don't really see it as our obligation to document this, for that very reason along with this being an edge case (e.g. less than 5% of normal usage is affected). This is further illustrated by the number of times this has come up during the LONG life of Fluid.

That number is: exactly two. The first time is why I know what I now know about Traversable pointers and foreach.

An alternative could be to have it simply restore the iterator pointer upon exit, but I'm not sure that's advisable

Even PHP does not do this. We follow the way pointers behave in PHP so I would say very inadvisable. Resetting it also causes another issue if done in your use case... infinite loops.

Anywho, my conclusion is still that explicit conversion is the way to solve this. I hope you can use my suggestions and background knowledge to make your implementation work.

Testing this reveals that `clone $arguments['each']` solves the use case without conversion but it looks potentially breaking to me. Normally you just don't expect your object to be cloned when you iterate over it. I will consider this. If we change this it has some more impact in how we handle this with Standalone Fluid and TYPO3v8 (can't use same fix; would need to override f:for if behavior differs from standalone fluid - change would be considered potentially breaking there which again affects how we can update that dependency).