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

When use_yield is true, CaptureNode fall in iterator_to_array pitfall regarding index overwrite #4029

Closed
TLG-Gildas opened this issue Apr 17, 2024 · 6 comments

Comments

@TLG-Gildas
Copy link
Contributor

CaptureNode use iterator_to_array to capture all yielded output in thier body.
But, when we have multiple block displayed in captured body, we have multiple yield from and we fall in the case of overwriting index describe in caution in this PHP doc page : https://www.php.net/manual/en/language.generators.syntax.php#:~:text=Caution,with%20iterator_to_array())

Without correction, the final captured body doesn't include all intended content.

Example of template :

{%- set tmp -%}
  {%- block foo 'foo' -%}
  {%- block bar 'bar' -%}
{%- endset -%}

We expect to have foobar in tmp, but we have only bar.

Preconized solution in PHP doc is to use the second parameter of iterator_to_array preserve_keys with value false.

the line concerned is l.47 :

$compiler->raw($useYield ? "implode('', iterator_to_array(" : '\\Twig\\Extension\\CoreExtension::captureOutput(');
if ($this->getAttribute('with_blocks')) {
$compiler->raw("(function () use (&\$context, \$macros, \$blocks) {\n");
} else {
$compiler->raw("(function () use (&\$context, \$macros) {\n");
}
$compiler
->indent()
->subcompile($this->getNode('body'))
->outdent()
->write("})() ?? new \EmptyIterator())")
;

@yellow1912
Copy link

yellow1912 commented Apr 17, 2024

I'm not sure what happened and if the issue is related, but since the new version (I accidentally updated just yesterday), my calls to ob_start returns empty string now.

Before, I have 2 twig functions, which are simplified like this:

public function startInline()
{
    ob_start();
}

public function endInline()
{
    $content = ob_get_clean();
}

My twig may look like this:

{{ startInline() }}
something here
{{ endInline() }}

It used to work, but now with the new twig it always returns an empty string. Downgrading to 3.8 fixes the issue so I'm pretty sure this has something to do with the new code. We need better document to explains how to update current code to work with the new 3.9.x.

@TLG-Gildas
Copy link
Contributor Author

@yellow1912 Your problem is indeed related to version 3.9 of twig, but not to the original subject of this issue.
To solve your problem, if I understand it correctly, you should use the set tag in its block form as indicated in the doc:
https://twig.symfony.com/doc/3.x/tags/set.html#:~:text=The%20set%20tag%20can,%3E%0A%7B%25%20endset%20%25%7D

The set tag can also be used to 'capture' chunks of text:

{% set foo %}
    <div id="pagination">
        ...
    </div>
{% endset %}

Once you use the set tag then you may run into the same issue that I described above.

If using the set tag does not meet your needs, then it might be best to open another issue.

@stof
Copy link
Member

stof commented Apr 17, 2024

@TLG-Gildas can you open a PR with the fix using the preserve_keys parameter (and an integration test reproducing the issue to prevent regressions) ?

@TLG-Gildas
Copy link
Contributor Author

@stof yes I can.
I'm going to try to do that today

@yellow1912
Copy link

@yellow1912 Your problem is indeed related to version 3.9 of twig, but not to the original subject of this issue. To solve your problem, if I understand it correctly, you should use the set tag in its block form as indicated in the doc: https://twig.symfony.com/doc/3.x/tags/set.html#:~:text=The%20set%20tag%20can,%3E%0A%7B%25%20endset%20%25%7D

The set tag can also be used to 'capture' chunks of text:

{% set foo %}
    <div id="pagination">
        ...
    </div>
{% endset %}

Once you use the set tag then you may run into the same issue that I described above.

If using the set tag does not meet your needs, then it might be best to open another issue.

Thank you. The set tag seems to be a possible solution, I could set the content to a variable then pass that variable to a twig method instead. That said, I wonder if I can still use the old code structure. I will open a new issue. Thank you.

TLG-Gildas pushed a commit to TLG-Gildas/Twig that referenced this issue Apr 18, 2024
@TLG-Gildas
Copy link
Contributor Author

@stof here PR to fix this issue : #4035

@fabpot fabpot closed this as completed in f7121a2 Apr 18, 2024
fabpot added a commit that referenced this issue Apr 18, 2024
… (TLG-Gildas)

This PR was merged into the 3.x branch.

Discussion
----------

fix: #4029 CaptureNode iterator_to_array preserveKeys false

Commits
-------

f7121a2 fix: #4029 when use_yield is true CaptureNode use iterator_to_array preserveKeys argument to false
fabpot added a commit that referenced this issue Apr 24, 2024
* 3.x:
  Bump version to 3.9.4-DEV
  Fix a warning
  Use ::class everywhere
  Auto-close PRs on subtree-splits
  Bump version
  Prepare the 3.9.3 release
  Update CHANGELOG
  Ensure Lexer:: is always initialized
  fix: #4033 add missing unwrap call when a TemplateWrapper instance can be present
  change extended DI extension class
  fix: #4029 when use_yield is true CaptureNode use iterator_to_array preserveKeys argument to false
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants