Nested Foreach attribute behavior changed

[dphadtare]

Smarty foreach attribute behavior is changed in the latest version that is 3.1.37.

Nested foreach behavioral change is observed in the latest version(Not sure from which version this has been changed but it is working as expected in version 3.1.21). Issue is, after the first iteration of parent foreach, item attribute of child foreach value does not update, to prove the behavioral change refer to the below snippet.

{$arrCounter = [ "FirstItem" => [ "First 1", "First 2", "First 3", "First 4" ], "SecondItem" => ["Second 1", "Second 2", "Second 3", "Second 4"] ]}

{foreach item=value1 from=$arrCounter}
	{foreach item=value from=$value1}
            {$value}
	{/foreach}
        <br>
{/foreach}

Value: {$value}

The output of above code is:

First 1 First 2 First 3 First 4
Second 1 Second 2 Second 3 Second 4
Value: First 4

Expected Output is:

First 1 First 2 First 3 First 4
Second 1 Second 2 Second 3 Second 4
Value: Second 4

The expected $value is Second 4 but it giving a value from the first iteration only.

Not sure what is cause for this behavioral change and did not found any updates/note in the changelog as well.

[wisskid]

That is interesting, I'll see if I can find where it originates.

[ophian]

How about changing L~342

        $output .= $foreachCompiler->compileRestore(1);

to

        $levels  = (!is_null($restore) && $restore > 1) ? $restore : 1;
        $output .= $foreachCompiler->compileRestore($levels);

in sysplugins/smarty_internal_compile_foreach.php ? Would that have any downsites elsewhere?

[wisskid]

Caused by a change in v3.1.28

[wisskid]

How about changing L~342

        $output .= $foreachCompiler->compileRestore(1);

to

        $levels  = (!is_null($restore) && $restore > 1) ? $restore : 1;
        $output .= $foreachCompiler->compileRestore($levels);

in sysplugins/smarty_internal_compile_foreach.php ? Would that have any downsites elsewhere?

This doesn't work. $restore appears to be 1 anyway (i.e. in my testcase)

[wisskid]

I have a fix:

diff --git a/libs/sysplugins/smarty_internal_runtime_foreach.php b/libs/sysplugins/smarty_internal_runtime_foreach.php
index badead16..d55ad576 100644
--- a/libs/sysplugins/smarty_internal_runtime_foreach.php
+++ b/libs/sysplugins/smarty_internal_runtime_foreach.php
@@ -56,12 +56,6 @@ class Smarty_Internal_Runtime_Foreach
         if (!isset($total)) {
             $total = empty($from) ? 0 : ($needTotal ? count($from) : 1);
         }
-        if (isset($tpl->tpl_vars[ $item ])) {
-            $saveVars[ 'item' ] = array(
-                $item,
-                $tpl->tpl_vars[ $item ]
-            );
-        }
         $tpl->tpl_vars[ $item ] = new Smarty_Variable(null, $tpl->isRenderingCache);
         if ($total === 0) {
             $from = null;

...but the unit tests now fail. There is a number of tests that explicitly test for (key and) item variables to be restored after the foreach loop. I wouldn't now why, but I'm hesitant to change behavior that appears to be intentional.

          array('{foreach $foo as $y => $x}{$y}=>{$x},{/foreach}-{$x}-{$y}', array(1,2,3), '0=>1,1=>2,2=>3,-x-y', 'saved loop variables', $i ++),
            array('{foreach $foo as $y => $x}{$y}=>{$x},{foreachelse}else{/foreach}-{$x}-{$y}', array(1,2,3), '0=>1,1=>2,2=>3,-x-y', 'saved loop variables', $i ++),
            array('{foreach $foo as $y => $x}{$y}=>{$x},{foreachelse}else{/foreach}-{$x}-{$y}', array(), 'else-x-y', 'saved loop variables', $i ++),

Commit logs or documentation do not provide any rationale for this behavior. @uwetews could you weigh in on this?

[ophian]

This doesn't work. $restore appears to be 1 anyway (i.e. in my testcase)

Given the failing example of first post it did work for me (restore==2) and it is caused by regression of 388993e9cae1ec472a88c09e568ebbe5625dd865 and 62d772e7345192134a62855f4bcf3387387afa78

But wait, I just reviewed and the failing still occurs. may be a caching event. (But I can confirm: Yours is the better fix! Can't say anything about the unit tests though)

[dphadtare]

Good to see that we found the version from where it started. When we are planning to release a new version for this fix?

[wisskid]

Not sure if we should “fix” this, as described above. It is admittedly a bit quirky, but fixing it would require breaking stuff that is actually tested for in the unit tests.

[ophian]

how do they fail, actually?
I did test them (the three you mentioned above) and all 3 worked well.

[wisskid]

how do they fail, actually? I did test them (the three you mentioned above) and all 3 worked well.

$ php7.1 vendor/phpunit/phpunit/phpunit --stop-on-failure --exclude-group slow tests/UnitTests/TemplateSource/TagTests/Foreach/CompileForeachTest.php
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

..........F

Time: 294 ms, Memory: 8.00MB

There was 1 failure:

1) CompileForeachTest::testForeach with data set #9 ('{foreach $foo as $y => $x}{$y...}-{$y}', array(1, 2, 3), '0=>1,1=>2,2=>3,-x-y', 'saved loop variables', 9)
testForeach - {foreach $foo as $y => $x}{$y}=>{$x},{/foreach}-{$x}-{$y} - saved loop variables
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'0=>1,1=>2,2=>3,-x-y'
+'0=>1,1=>2,2=>3,-3-y'

/smarty/wisskid/smarty/tests/UnitTests/TemplateSource/TagTests/Foreach/CompileForeachTest.php:50

FAILURES!
Tests: 11, Assertions: 11, Failures: 1.

[ophian]

including the patch both - php and smarty - result in 0=>1,1=>2,2=>3,-3-2, which is the correct result, isn't it?! It should be corrected in the unit result check, since ,-x-y just look like placeholders.

...and that PHPUnit returns -3-y could be a matter of being a "bug", ... you could ask for Sebastian Bergmann.

[dphadtare]

I agree with Ophian, if unit tests are written considering old foreach behavior, we should update that to correct one.