Skip to content

Make loop.last always available#4134

Merged
fabpot merged 1 commit into
twigphp:4.xfrom
fabpot:loop-last-always-available
Jul 12, 2024
Merged

Make loop.last always available#4134
fabpot merged 1 commit into
twigphp:4.xfrom
fabpot:loop-last-always-available

Conversation

@fabpot

@fabpot fabpot commented Jul 11, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@stof stof left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation breaks all other methods of the Iterator interface in the Loop class as they forward the call to the seq without considering it has been peeked

@fabpot

fabpot commented Jul 11, 2024

Copy link
Copy Markdown
Contributor Author

This implementation breaks all other methods of the Iterator interface in the Loop class as they forward the call to the seq without considering it has been peeked

Can you give me a Twig example where that could happen?

@stof

stof commented Jul 11, 2024

Copy link
Copy Markdown
Member

I'm not sure we have cases right now where we use the iterator API directly. AFAIK, Twig will only iterate over it with a foreach loop (which will use the Iterator API before isLast can be called on the LoopContext object) and the LoopContext does not expose it.
However, if we implement recursive loops as done in Jinja (see #3416), we might have to change that by using the iterator in more advanced ways (and at that point, having a broken iterator state after calling isLast until the next call to next might cause issues).
And if we want to implement loop.prevItem (which exists in Jinja), this might also be affected.

@fabpot

fabpot commented Jul 12, 2024

Copy link
Copy Markdown
Contributor Author

I'm not sure we have cases right now where we use the iterator API directly. AFAIK, Twig will only iterate over it with a foreach loop (which will use the Iterator API before isLast can be called on the LoopContext object) and the LoopContext does not expose it. However, if we implement recursive loops as done in Jinja (see #3416), we might have to change that by using the iterator in more advanced ways (and at that point, having a broken iterator state after calling isLast until the next call to next might cause issues). And if we want to implement loop.prevItem (which exists in Jinja), this might also be affected.

Agreed, and I have that locally but that will come as a next PR so that I can easily write tests via the new loop capabilities.

@fabpot fabpot merged commit 9499b2c into twigphp:4.x Jul 12, 2024
@fabpot fabpot deleted the loop-last-always-available branch July 12, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants