Skip to content

Adjust cycle implementation#4241

Merged
fabpot merged 1 commit intotwigphp:3.xfrom
smnandre:feat/clarify-cycle
Aug 30, 2024
Merged

Adjust cycle implementation#4241
fabpot merged 1 commit intotwigphp:3.xfrom
smnandre:feat/clarify-cycle

Conversation

@smnandre
Copy link
Copy Markdown
Contributor

Update code following #4158

  • return mixed
  • trigger when non countable
  • type the position argument

Add doc precisions

@smnandre smnandre force-pushed the feat/clarify-cycle branch from 97eb3fb to 79f8a67 Compare August 26, 2024 23:11
Copy link
Copy Markdown
Contributor

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Just CS review for now.


.. code-block:: twig

{% set fruits = ['apple', 'orange', 'citrus'] %}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like concrete examples instead of random ones. Let's revert to the fruits example.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I keep this open: i'll rework it later today.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No time nor inspiration these last day so ... back to fruits :)

@smnandre
Copy link
Copy Markdown
Contributor Author

smnandre commented Aug 27, 2024

All fixed

Documentation states it "cycles over" .. and we see it behaves more as a "static modulo-accessor".

I was wonderning if this function could return (or act as) an iterator ?

@stof
Copy link
Copy Markdown
Member

stof commented Aug 29, 2024

@smnandre the common use case is to use this cycle inside the body of a loop, using loop.index as position. The cycle function is not the one controlling the iteration behavior (if you want to iterate over the array passed as input, you don't need a function for that as the for loop can work with the array directly)

@smnandre
Copy link
Copy Markdown
Contributor Author

@smnandre the common use case is to use this cycle inside the body of a loop, using loop.index as position. The cycle function is not the one controlling the iteration behavior (if you want to iterate over the array passed as input, you don't need a function for that as the for loop can work with the array directly)

This is what i was thinking of .. would it be possible to not pass the "position" ?

{% for turtle in turtles %}

    🐢  {{ cycle([ '🔵', '🔴', '🟠', '🟣']) }}   

{% endfor %}

{# would render #}
🐢 🔵  🐢 🔴  🐢 🟠  🐢 🟣

@fabpot
Copy link
Copy Markdown
Contributor

fabpot commented Aug 30, 2024

@smnandre the common use case is to use this cycle inside the body of a loop, using loop.index as position. The cycle function is not the one controlling the iteration behavior (if you want to iterate over the array passed as input, you don't need a function for that as the for loop can work with the array directly)

This is what i was thinking of .. would it be possible to not pass the "position" ?

{% for turtle in turtles %}

    🐢  {{ cycle([ '🔵', '🔴', '🟠', '🟣']) }}   

{% endfor %}

{# would render #}
🐢 🔵  🐢 🔴  🐢 🟠  🐢 🟣

It already works:

{% for turtle in turtles %}
    🐢  {{ loop.cycle('🔵', '🔴', '🟠', '🟣') }}
{% endfor %}

@smnandre
Copy link
Copy Markdown
Contributor Author

With loop. yep ! And i really like this DX :)

Maybe this is something to highlight in the doc ? Or even deprecate the cycle function ?

@smnandre smnandre requested a review from fabpot August 30, 2024 17:22
@fabpot
Copy link
Copy Markdown
Contributor

fabpot commented Aug 30, 2024

With loop. yep ! And i really like this DX :)

Maybe this is something to highlight in the doc ? Or even deprecate the cycle function ?

I think both serve different purposes. I forgot to say that this is available as of 4.0. Docs are available: https://github.com/twigphp/Twig/blob/4.x/doc/tags/for.rst

@fabpot fabpot force-pushed the feat/clarify-cycle branch from 00c2439 to 7c3c161 Compare August 30, 2024 18:43
@fabpot
Copy link
Copy Markdown
Contributor

fabpot commented Aug 30, 2024

Thank you @smnandre.

@fabpot fabpot merged commit a39b6e1 into twigphp:3.x Aug 30, 2024
fabpot added a commit that referenced this pull request Nov 25, 2025
…cts (yoeunes)

This PR was merged into the 3.x branch.

Discussion
----------

Fix cycle() with non-countable ArrayAccess+Traversable objects

When using `cycle()` with an object that implements `\ArrayAccess` and `\Traversable` but is not `\Countable`, the function currently returns the object instance immediately after triggering the deprecation notice.

This prevents the value from being converted to an array, causing the cycle logic to fail or return the object itself.

This PR removes the early return to ensure `self::toArray()` is called, allowing these objects to be cycled correctly as expected.

**How to test**

```php
$seq = new class implements \ArrayAccess, \IteratorAggregate {
    public function offsetExists($offset): bool { return true; }
    public function offsetGet($offset): mixed { return 'val'; }
    public function offsetSet($offset, $value): void {}
    public function offsetUnset($offset): void {}
    public function getIterator(): \Traversable { yield 'odd'; yield 'even'; }
};

// Should return 'odd', currently returns the $seq object
$result = CoreExtension::cycle($seq, 0);
```

Related to #4241

Commits
-------

473653d [Core] Fix cycle() with non-countable ArrayAccess+Traversable objects
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.

3 participants