Skip to content
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

[BUGFIX] Properly restore renderingContext in renderChildren() #970

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

s2b
Copy link
Contributor

@s2b s2b commented Aug 17, 2024

It has long been possible to render recursive templates with Fluid,
either by using partials or sections. In both cases, <f:render />
is used to initiate the recursive behavior.

For example, the following template creates a recursive main menu by
using sections:

<f:render section="menu" arguments="{items: mainMenu}" />

<f:section name="menu">
    <ul>
        <f:for each="{items}" as="item">
            <li>
                {item.title}
                <f:if condition="{item.children}">
                    <f:render section="menu" arguments="{items: item.children}" />
                </f:if>
            </li>
        </f:for>
    </ul>
</f:section>

However, this was only working because ForViewHelper was
implemented as static ViewHelper by using Fluid's trait
CompileWithRenderStatic. The trait changed Fluid's handling
of ViewHelpers in a way that variables stay consistent even
during recursions. This was made possible back in Fluid 1.0.5
with 819ca24.

However, this never worked properly with ViewHelpers not using
renderStatic() but rather the default render() method. In those
implementations, $this->renderChildren() is used to access and
render the ViewHelper's child nodes rather than executing the closure
directly passed to renderStatic().

To support recursions also for ViewHelpers not using renderStatic(),
this patch introduces new state to AbstractViewHelper to keep
track of rendering contexts during recursions. Note that this state
is only used for uncached templates, for cached templates this already
worked without any changes.

To explain the new behavior, let's take the example above and let's
also assume that ForViewHelper is implemented using render() rather
than renderStatic(). In uncached templates, the new behavior works
as follows:

  1. $view->render() gets called for the template
  2. TemplateParser creates nodes from template string, among them
    one ViewHelperNode for each ViewHelper.
  3. RenderViewHelper gets called by executing $viewHelperNode->execute(),
    which in turn calls ViewHelperInvoker, which in the end calls the render
    method of the ViewHelper (in this case RenderViewHelper::renderStatic())
  4. RenderViewHelper calls $view->renderSection(), which clones the
    current rendering context and puts the appropriate variables in it. The old
    rendering context gets saved to be restored later for the parent template.
  5. Inside the section, ForViewHelper gets called, we end up in
    ForViewHelper::render() (because we use our custom implementation without
    renderStatic())
  6. $this->renderChildren() is called for each {item}.
  7. If {item.children} is defined, things get interesting. RenderViewHelper
    calls $view->renderSection() again, which creates and saves another new
    rendering context, see 4. We're in a recursion now.
  8. With each recursion level finishing, the view restores the original
    rendering context, so that the variables before the recursion are available
    to the rest of the template file.

So the template/partial/section gets restored properly, but the
ForViewHelper still has the last rendering context from within the recursion
because Fluid didn't restore it to the previous state. In fact, Fluid couldn't
really do that from outside because $forViewHelper->renderChildren() initiated
the whole recursion and is still running (once for each recursion level).

To solve this dilemma, this patch implements a rendering context stack similarly
to the one used in the $view object, but on the ViewHelper level. Each ViewHelper
keeps track of recursive rendering contexts for itself – again, only for
uncached, non-static ViewHelpers.

In addition to that, the patch simplifies the logic of buildRenderChildrenClosure(),
which covers both cached/uncached and the contentArgumentName feature.

@s2b s2b force-pushed the bugfix/renderChildren branch 2 times, most recently from b20dc5d to 097d7dc Compare August 17, 2024 21:24
It has long been possible to render recursive templates with Fluid,
either by using partials or sections. In both cases, `<f:render />`
is used to initiate the recursive behavior.

For example, the following template creates a recursive main menu by
using sections:

```xml
<f:render section="menu" arguments="{items: mainMenu}" />

<f:section name="menu">
    <ul>
        <f:for each="{items}" as="item">
            <li>
                {item.title}
                <f:if condition="{item.children}">
                    <f:render section="menu" arguments="{items: item.children}" />
                </f:if>
            </li>
        </f:for>
    </ul>
</f:section>
```

However, this was only working because `ForViewHelper` was
implemented as static ViewHelper by using Fluid's trait
`CompileWithRenderStatic`. The trait changed Fluid's handling
of ViewHelpers in a way that variables stay consistent even
during recursions. This was made possible back in Fluid 1.0.5
with 819ca24.

However, this never worked properly with ViewHelpers not using
`renderStatic()` but rather the default `render()` method. In those
implementations, `$this->renderChildren()` is used to access and
render the ViewHelper's child nodes rather than executing the closure
directly passed to `renderStatic()`.

To support recursions also for ViewHelpers not using `renderStatic()`,
this patch introduces new state to `AbstractViewHelper` to keep
track of rendering contexts during recursions. Note that this state
is only used for uncached templates, for cached templates this already
worked without any changes.

To explain the new behavior, let's take the example above and let's
also assume that `ForViewHelper` is implemented using `render()` rather
than `renderStatic()`. In uncached templates, the new behavior works
as follows:

1. `$view->render()` gets called for the template
2. `TemplateParser` creates nodes from template string, among them
one `ViewHelperNode` for each ViewHelper.
3. `RenderViewHelper` gets called by executing `$viewHelperNode->execute()`,
which in turn calls `ViewHelperInvoker`, which in the end calls the render
method of the ViewHelper (in this case `RenderViewHelper::renderStatic()`)
4. `RenderViewHelper` calls `$view->renderSection()`, which clones the
current rendering context and puts the appropriate variables in it. The old
rendering context gets saved to be restored later for the parent template.
5. Inside the section, `ForViewHelper` gets called, we end up in
`ForViewHelper::render()` (because we use our custom implementation without
`renderStatic()`)
6. `$this->renderChildren()` is called for each `{item}`.
7. If `{item.children}` is defined, things get interesting. `RenderViewHelper`
calls `$view->renderSection()` again, which creates and saves another new
rendering context, see 4. We're in a recursion now.
8. With each recursion level finishing, the view restores the original
rendering context, so that the variables before the recursion are available
to the rest of the template file.

So the template/partial/section gets restored properly, but the
`ForViewHelper` still has the last rendering context from within the recursion
because Fluid didn't restore it to the previous state. In fact, Fluid couldn't
really do that from outside because `$forViewHelper->renderChildren()` initiated
the whole recursion and is still running (once for each recursion level).

To solve this dilemma, this patch implements a rendering context stack similarly
to the one used in the `$view` object, but on the ViewHelper level. Each ViewHelper
keeps track of recursive rendering contexts for itself – again, only for
uncached, non-static ViewHelpers.

In addition to that, the patch simplifies the logic of `buildRenderChildrenClosure()`,
which covers both cached/uncached and the contentArgumentName feature.
@s2b s2b merged commit 1ef0a14 into main Aug 20, 2024
8 checks passed
@s2b s2b deleted the bugfix/renderChildren branch August 20, 2024 17:05
@lolli42
Copy link
Member

lolli42 commented Aug 20, 2024

get. state. right.

s2b added a commit that referenced this pull request Aug 20, 2024
It has long been possible to render recursive templates with Fluid,
either by using partials or sections. In both cases, `<f:render />`
is used to initiate the recursive behavior.

For example, the following template creates a recursive main menu by
using sections:

```xml
<f:render section="menu" arguments="{items: mainMenu}" />

<f:section name="menu">
    <ul>
        <f:for each="{items}" as="item">
            <li>
                {item.title}
                <f:if condition="{item.children}">
                    <f:render section="menu" arguments="{items: item.children}" />
                </f:if>
            </li>
        </f:for>
    </ul>
</f:section>
```

However, this was only working because `ForViewHelper` was
implemented as static ViewHelper by using Fluid's trait
`CompileWithRenderStatic`. The trait changed Fluid's handling
of ViewHelpers in a way that variables stay consistent even
during recursions. This was made possible back in Fluid 1.0.5
with 819ca24.

However, this never worked properly with ViewHelpers not using
`renderStatic()` but rather the default `render()` method. In those
implementations, `$this->renderChildren()` is used to access and
render the ViewHelper's child nodes rather than executing the closure
directly passed to `renderStatic()`.

To support recursions also for ViewHelpers not using `renderStatic()`,
this patch introduces new state to `AbstractViewHelper` to keep
track of rendering contexts during recursions. Note that this state
is only used for uncached templates, for cached templates this already
worked without any changes.

To explain the new behavior, let's take the example above and let's
also assume that `ForViewHelper` is implemented using `render()` rather
than `renderStatic()`. In uncached templates, the new behavior works
as follows:

1. `$view->render()` gets called for the template
2. `TemplateParser` creates nodes from template string, among them
one `ViewHelperNode` for each ViewHelper.
3. `RenderViewHelper` gets called by executing `$viewHelperNode->execute()`,
which in turn calls `ViewHelperInvoker`, which in the end calls the render
method of the ViewHelper (in this case `RenderViewHelper::renderStatic()`)
4. `RenderViewHelper` calls `$view->renderSection()`, which clones the
current rendering context and puts the appropriate variables in it. The old
rendering context gets saved to be restored later for the parent template.
5. Inside the section, `ForViewHelper` gets called, we end up in
`ForViewHelper::render()` (because we use our custom implementation without
`renderStatic()`)
6. `$this->renderChildren()` is called for each `{item}`.
7. If `{item.children}` is defined, things get interesting. `RenderViewHelper`
calls `$view->renderSection()` again, which creates and saves another new
rendering context, see 4. We're in a recursion now.
8. With each recursion level finishing, the view restores the original
rendering context, so that the variables before the recursion are available
to the rest of the template file.

So the template/partial/section gets restored properly, but the
`ForViewHelper` still has the last rendering context from within the recursion
because Fluid didn't restore it to the previous state. In fact, Fluid couldn't
really do that from outside because `$forViewHelper->renderChildren()` initiated
the whole recursion and is still running (once for each recursion level).

To solve this dilemma, this patch implements a rendering context stack similarly
to the one used in the `$view` object, but on the ViewHelper level. Each ViewHelper
keeps track of recursive rendering contexts for itself – again, only for
uncached, non-static ViewHelpers.

In addition to that, the patch simplifies the logic of `buildRenderChildrenClosure()`,
which covers both cached/uncached and the contentArgumentName feature.
s2b added a commit that referenced this pull request Aug 20, 2024
…#980)

It has long been possible to render recursive templates with Fluid,
either by using partials or sections. In both cases, `<f:render />`
is used to initiate the recursive behavior.

For example, the following template creates a recursive main menu by
using sections:

```xml
<f:render section="menu" arguments="{items: mainMenu}" />

<f:section name="menu">
    <ul>
        <f:for each="{items}" as="item">
            <li>
                {item.title}
                <f:if condition="{item.children}">
                    <f:render section="menu" arguments="{items: item.children}" />
                </f:if>
            </li>
        </f:for>
    </ul>
</f:section>
```

However, this was only working because `ForViewHelper` was
implemented as static ViewHelper by using Fluid's trait
`CompileWithRenderStatic`. The trait changed Fluid's handling
of ViewHelpers in a way that variables stay consistent even
during recursions. This was made possible back in Fluid 1.0.5
with 819ca24.

However, this never worked properly with ViewHelpers not using
`renderStatic()` but rather the default `render()` method. In those
implementations, `$this->renderChildren()` is used to access and
render the ViewHelper's child nodes rather than executing the closure
directly passed to `renderStatic()`.

To support recursions also for ViewHelpers not using `renderStatic()`,
this patch introduces new state to `AbstractViewHelper` to keep
track of rendering contexts during recursions. Note that this state
is only used for uncached templates, for cached templates this already
worked without any changes.

To explain the new behavior, let's take the example above and let's
also assume that `ForViewHelper` is implemented using `render()` rather
than `renderStatic()`. In uncached templates, the new behavior works
as follows:

1. `$view->render()` gets called for the template
2. `TemplateParser` creates nodes from template string, among them
one `ViewHelperNode` for each ViewHelper.
3. `RenderViewHelper` gets called by executing `$viewHelperNode->execute()`,
which in turn calls `ViewHelperInvoker`, which in the end calls the render
method of the ViewHelper (in this case `RenderViewHelper::renderStatic()`)
4. `RenderViewHelper` calls `$view->renderSection()`, which clones the
current rendering context and puts the appropriate variables in it. The old
rendering context gets saved to be restored later for the parent template.
5. Inside the section, `ForViewHelper` gets called, we end up in
`ForViewHelper::render()` (because we use our custom implementation without
`renderStatic()`)
6. `$this->renderChildren()` is called for each `{item}`.
7. If `{item.children}` is defined, things get interesting. `RenderViewHelper`
calls `$view->renderSection()` again, which creates and saves another new
rendering context, see 4. We're in a recursion now.
8. With each recursion level finishing, the view restores the original
rendering context, so that the variables before the recursion are available
to the rest of the template file.

So the template/partial/section gets restored properly, but the
`ForViewHelper` still has the last rendering context from within the recursion
because Fluid didn't restore it to the previous state. In fact, Fluid couldn't
really do that from outside because `$forViewHelper->renderChildren()` initiated
the whole recursion and is still running (once for each recursion level).

To solve this dilemma, this patch implements a rendering context stack similarly
to the one used in the `$view` object, but on the ViewHelper level. Each ViewHelper
keeps track of recursive rendering contexts for itself – again, only for
uncached, non-static ViewHelpers.

In addition to that, the patch simplifies the logic of `buildRenderChildrenClosure()`,
which covers both cached/uncached and the contentArgumentName feature.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants