[BUGFIX] Shallow copy RenderingContext before each ViewHelper

[NamelessCoder]

Fixes an issue where recursively calling a ViewHelper caused the RenderingContext reference to become incorrect after ViewHelper renders a nested instance of itself. Expressed for example by recursively rendering a section with a loop - after exiting the loop and returning to the parent section variables would have unexpected values because the RenderingContext was replaced.

Cloning (shallow copying) the ViewHelper instance before creating the render children closure solves this issue.

[IchHabRecht]

The problem is setting new renderingContext on one viewhelper instance over and over again. We should either use own instances for each rendering or introduce a renderingContextStash for viewhelper as well!

[IchHabRecht]

\TYPO3Fluid\Fluid\Core\Parser\SyntaxTree\ViewHelperNode::evaluate is using the same instance for the same viewhelper class. this means the renderingContext for one viewhelperclass is changed over and over again (vendor/typo3fluid/fluid/src/Core/ViewHelper/ViewHelperInvoker.php:73)

to ensure always an uninitialized instance is taken i changed the evaluation function to
return $renderingContext->getViewHelperInvoker()->invoke(clone $this->uninitializedViewHelper, $this->arguments, $renderingContext);
(added the clone)

[NamelessCoder]

We've also experimented with the clone operation - unfortunately it breaks f:cycle and possibly other ViewHelpers.

There is a bit of back story for that which may be relevant to know:

• Reuse ViewHelper instances #5 describes the reason for reusing ViewHelper instances. Originally I had constructed the standalone Fluid in a way that a new instance was created repeatedly (and would leave it up to for example a custom ViewHelperResolver implementation to return the same instance if reuse was desired).
• [BUGFIX] Let f:cycle use "as" argument as scope of cycled variable #25 was created to get around the need to do instance reuse, but meanwhile a change was made that reuses the instance from the ViewHelperNode. This pull request is likely to be abandoned.
• Meanwhile, and not reflected in this pull request, a test for the proper behavior of f:cycle was added and it breaks when adding this clone keyword.

I created this sequence diagram to try to illustrate the problem:

There clearly is an issue with the RenderingContext not being the expected one - but I'm sorry to say we are no closer to finding the solution.

[mneuhaus]

damnit claus, i was 1 sec away from writing almost the same message :P :D

yea, i have no clear idea how to go about it right now as well. i'm kinda leaning to say, screw cycle, let's do it right and find a "proper" solution to make cycle work again.

[NamelessCoder]

In my honest opinion: using a fresh instance for every call (and being smart about the cost of creating an instance, analysing arguments etc) like #25 tries to implement is, although less performant, a much more predictable solution. Since currently f:cycle is the only ViewHelper that would break from clone (no third party ones that I know of, either) we could argue that fixing f:cycle and removing instance reuse would be better.

There is also the somewhat long perspective that we currently have two modes of rendering where we should ideally be moving toward a single way (which is closer to the compiled mode than the uncompiled one, e.g. static calls). In this perspective, having instances become reused is not desired.

[NamelessCoder]

On a side note: doing so would affect TYPO3 CMS Fluid performance of uncompilable templates adversely, the main reason being the need to perform dependency injection on every instance creation. Once compiled (which almost every template can be now) the performance hit is gone again.

[mneuhaus]

i asked bastian for his 2 cents in the #25 cycle issue, to see if we can "resurrect" it :)

[IchHabRecht]

As I suggested there is even the other way to introduce a kind of stash for the viewhelpers renderingContext which adds one before rendering and removes it afterwards. Maybe a smarter way?

[NamelessCoder]

Did some voodoo.

[helhum]

I cannot follow the technical details, but can anybody who is deeper in the code explain to me why the cycle VH and recursive rendering worked in previous versions of Fluid? Can't we re-use this knowledge to fix this?

[NamelessCoder]

To define "voodoo": looks like something dark and mysterious happens when a rendering children closure is built and a variable is assigned to contain a reference to $this. Changing that reference to a shallow copy seems to preserve $this after exiting rendering of the same node inside that closure.

[NamelessCoder]

@helhum Due to the way templates were treated in the old Fluid I would induce that when sections were rendered recursively back then, the section's template source would actually be re-parsed every time, unless compilable - and this doesn't affect compiled templates. Now, we store the parsed template and use that same state each time rendering a section.

[IchHabRecht]

Just a quick proposal how to fix the cycleViewHelper IchHabRecht@469ded0

[NamelessCoder]

Thanks for the effort Nicole - but I think after this latest update to this patch, it should solve the issue in the right way without needing to change the cycle ViewHelper. If you could test and confirm this that would be great!

PS: we're ready to make a release after this.

[IchHabRecht]

This still fails for me. Either the tests and in TYPO3

[IchHabRecht]

Failed asserting that 'Item: 1.

    Item: 2.


    Item: 3.



                    Item: .' contains "Item: 4.".

[IchHabRecht]

And by reading the code it doesn't prevent setting different renderingContexts to the same object

[IchHabRecht]

okay, seems to work now. found the error (not up-to-date resources)

[NamelessCoder]

Excellent - I was just about to suggest taking a fresh checkout since I've force-pushed this a few times now.