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

v3.1.33 breaks {foreachelse} on foreach loops applied to objects #506

Closed
arkamax2k opened this issue Nov 6, 2018 · 4 comments · Fixed by #566
Closed

v3.1.33 breaks {foreachelse} on foreach loops applied to objects #506

arkamax2k opened this issue Nov 6, 2018 · 4 comments · Fixed by #566

Comments

@arkamax2k
Copy link

arkamax2k commented Nov 6, 2018

When fix for for #443 was provided, it disabled calculating $total when $from is an object. A line shortly further down sets $total to a value of 1, which further prevents $from from being set to null and triggering {foreachelse} inside the compiled template.

PoC:

<?php

require_once './vendor/autoload.php';

$templateContent = <<<TPL
{foreach \$items as \$item}
    {$item}<br>
{foreachelse}
    No items
{/foreach}
TPL;

$smarty = new Smarty();

$smarty->assign('items', new \ArrayIterator());

echo Smarty::SMARTY_VERSION . "<br>";

$smarty->display('string:' . $templateContent);

On v3.1.32, it outputs the expected "No items". On v3.1.33, it outputs nothing.

It doesn't appear that there is an easy way to determine if an object is empty or not without doing the count(), thus invoking the Countable interface. It would appear that this optimization is inapplicable to objects in $from where {foreachelse} is used, unless total is explicitly requested via a property, which would require editing existing affected templates, breaking backwards compatibility.

@ghost
Copy link

ghost commented Nov 30, 2018

What about this approach: Smarty should not be responsible for information that the underlying structure isn't able to provide (e.g.: the size of an (forward) iterator). Thus, don't calculate the total count and do not provide the properties that depends on it, if from is of a type that cannot be simply queried with the count() function. This is even more a problem for forward iterators that are broken when the total count is calculated due to a property, that requires it.
All in all this should be not such a big deal, because when I am using iterators somewhere else, I cannot get the total count from that iterator in advance, too. Therefore I cannot expect that from smarty.

The {foreachelse} block can be modified for this case like this: Count the items while actually iterating over the dataset and if that is still 0 after the iteration, show the {foreachelse} block. This should be used for all cases, so the template compiler do not have to handle all the cases separately.

If that sounds reasonable, I can gladly provide a patch.

@arkamax2k
Copy link
Author

Thank you for your response. The core of this issue for me is not so much in not having a total but rather in not executing a {foreachelse} branch - this breaks way too many things. Consequently, I think it'd be great if an empty foreach() execution could be caught and used as grounds for executing {foreachelse} - IMO this would address the core issue about without having to resort to counting.

@AnrDaemon
Copy link
Contributor

I think this could be fixed by addressing the way the output code is generated.
Instead of checking the total before the loop, test if the loop code was at all executed.
This may be a little bit fragile, but more compatible with users' expectations.

@ghost
Copy link

ghost commented Feb 11, 2019

That's exactly what the patch above does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants