Issue #3828: Add sequence and mapping tests #3859
Conversation
|
Not 100% sure but I believe the fails reported by https://fabbot.io/report/twigphp/Twig/3859/508bc72c99bea1bfb69284610aaa154a3abeaf9a doesn't come from the code I have changed. |
src/Extension/CoreExtension.php
Outdated
| */ | ||
| function twig_test_sequence($value) | ||
| { | ||
| return $value instanceof \Traversable || (\is_array($value) && \array_is_list($value)); |
There was a problem hiding this comment.
Not all Traversable are sequences, they can return any key. Example https://3v4l.org/LJ0R2
The implementation of array_is_list could accept any iterable, a new name will be required as this is not identical to the PHP 8.1 function.
There was a problem hiding this comment.
The issue for an iterator is that this requires consuming the iterator, which might not be rewindable to consume it a second time (see generators)
src/Extension/CoreExtension.php
Outdated
| */ | ||
| function twig_test_mapping($value) | ||
| { | ||
| return \is_array($value) && ((0 === \count($value)) || !\array_is_list($value)); |
There was a problem hiding this comment.
Any stdClass is a mapping. That's the type returned by json_decode by default for objects. It's also the only way to have an empty mapping.
This test would be more useful if it was different than a <var> is not sequence.
Given this definition of this test in Jinja, the implementation should check that none of the keys are numeric.
mapping(value)
Return true if the object is a mapping (dict etc.).
There was a problem hiding this comment.
numeric keys that are not sequentials would still be a mapping IMO.
src/Extension/CoreExtension.php
Outdated
| * @return bool true if array is a list, false otherwise | ||
| */ | ||
| if (!function_exists('array_is_list')) { | ||
| function array_is_list(array $array) |
There was a problem hiding this comment.
symfony/polyfill-php81 should be used instead of creating the polyfill here.
But that will not be necessary if you change the implementation to support any iterable (previous comment).
There was a problem hiding this comment.
this should indeed depend on symfony/polyfill-php81 instead of providing its own untested polyfill.
|
+1 for this feature. Working on a Drupal site and this causes issues for rendering lists. |
pdureau
force-pushed
the
3828-add-sequence-mapping-tests
branch
from
508bc72
to
c1344ee
Compare
|
I have rebased my work with current 3.x branch, so my MR has changed. |
Add sequence and mapping tests from Jinja.