Sandbox foreachVars in templates

[dtdesign]

Nesting the same template inside a foreach loop that is also accessed inside the nested call will overwrite the values from the outer template due to identical identifiers being used.

The sandbox did not protected $this->foreachVars despite being stateful.

See #4431
Fixes #4444

[TimWolla]

I'm not sure whether it is appropriate to tie this to the sandbox. The sandbox can be disabled on-demand, but we must always protect the foreachVars state, no?

[dtdesign]

Yes, you got a point there, but frankly I don't see a reasonable approach that does not rely on the sandbox. It is a design flaw of the template engine that cannot be overcome easily, especially not within the scope of a bugfix release.

I'm well aware that this is merely a bandaid for the problem, but considering that it never really surfaced since WCF 1.0, I'm deeming this to be sufficient for the time being.

[TimWolla]

Yes, you got a point there, but frankly I don't see a reasonable approach that does not rely on the sandbox.

I had extending compileIncludeTag in mind.

Before including the template:

$this->metadata[] = [
 'foreach' => $this->foreachVars,
];

After including:

['foreach' => $this->foreachVars] = \array_pop($this->metadata);

This is effectively a separate sandbox and should be reasonable even for a bugfix update?

In any case your PR LGTM in general. I just want to avoid needing to make yet another fix after this one.

[dtdesign]

Looking at compileIncludeTag() makes me feel a bit uneasy, because it has two somewhat different code paths. $metaData is also potentially unsafe, it originates from compileString() and can be supplied from the outside.

Recursively including the same template is not really a thing anymore since we started relying on nested iterators instead, therefore it should be fairly uncommon to do this. I'm leaning towards the "cheap" fix in this PR and replacing it in 5.5 with a proper solution, allowing us to have it reasonably tested before rolling it out to production.

[TimWolla]

I'm leaning towards the "cheap" fix in this PR and replacing it in 5.5 with a proper solution, allowing us to have it reasonably tested before rolling it out to production.

That's fair. Feel free to merge.