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.

Sign up for GitHub

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

Jump to bottom

ENH Looping through arrays in templates #11244

Merged
merged 1 commit into from
May 24, 2024
Merged

ENH Looping through arrays in templates #11244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/ORM/ArrayList.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ public function exists()
public function getIterator(): Traversable
{
foreach ($this->items as $i => $item) {
if (is_array($item)) {
if (is_array($item) && !array_is_list($item)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Ultimately the way ArrayList works needs to be re-evaluated at some stage anyway - only converting in the iterator but not when fetching items directly via find() and other methods leads to unexpected results. So I think this is fine for now even though it allows a setup that used to cause an explicit error.

yield new ArrayData($item);
} else {
yield $item;
Expand Down
2 changes: 1 addition & 1 deletion src/View/ArrayData.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public function toMap()
public function getField($field)
{
$value = $this->array[$field];
if (is_object($value) && !$value instanceof ViewableData) {
if (is_object($value) && !($value instanceof ViewableData) && !is_iterable($value)) {
return new ArrayData($value);
Comment on lines -80 to 81
Copy link
Member Author

@GuySartorelli GuySartorelli May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is needed because otherwise ArrayIterator and other iterators get shoved inside ArrayData which is obviously not correct and results in not being able to get the count.

Arguably this is a bug fix

} elseif (ArrayLib::is_associative($value)) {
return new ArrayData($value);
Expand Down
7 changes: 1 addition & 6 deletions src/View/SSTemplateParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1886,8 +1886,6 @@ function PresenceCheck_Argument(&$res, $sub)
$res['php'] .= '((bool)'.$sub['php'].')';
} else {
$php = ($sub['ArgumentMode'] == 'default' ? $sub['lookup_php'] : $sub['php']);
// TODO: kinda hacky - maybe we need a way to pass state down the parse chain so
// Lookup_LastLookupStep and Argument_BareWord can produce hasValue instead of XML_val
$res['php'] .= str_replace('$$FINAL', 'hasValue', $php ?? '');
}
}
Expand Down Expand Up @@ -5292,8 +5290,6 @@ function Text__finalise(&$res)
$text = stripslashes($text ?? '');
$text = addcslashes($text ?? '', '\'\\');

// TODO: This is pretty ugly & gets applied on all files not just html. I wonder if we can make this
// non-dynamically calculated
$code = <<<'EOC'
(\SilverStripe\View\SSViewer::getRewriteHashLinksDefault()
? \SilverStripe\Core\Convert::raw2att( preg_replace("/^(\\/)+/", "/", $_SERVER['REQUEST_URI'] ) )
Expand Down Expand Up @@ -5332,8 +5328,7 @@ public function compileString($string, $templateName = "", $includeDebuggingComm

$this->includeDebuggingComments = $includeDebuggingComments;

// Ignore UTF8 BOM at beginning of string. TODO: Confirm this is needed, make sure SSViewer handles UTF
// (and other encodings) properly
// Ignore UTF8 BOM at beginning of string.
if (substr($string ?? '', 0, 3) == pack("CCC", 0xef, 0xbb, 0xbf)) {
$this->pos = 3;
}
Expand Down
9 changes: 9 additions & 0 deletions src/View/SSViewer_Scope.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use ArrayIterator;
use Countable;
use Iterator;
use SilverStripe\ORM\ArrayList;
use SilverStripe\ORM\FieldType\DBBoolean;
use SilverStripe\ORM\FieldType\DBText;
use SilverStripe\ORM\FieldType\DBFloat;
Expand Down Expand Up @@ -130,6 +131,12 @@ public function getItem()
if (is_scalar($item)) {
$item = $this->convertScalarToDBField($item);
}

// Wrap list arrays in ViewableData so templates can handle them
if (is_array($item) && array_is_list($item)) {
$item = ArrayList::create($item);
}

return $item;
}

Expand Down Expand Up @@ -308,6 +315,8 @@ public function next()
// Item may be an array or a regular IteratorAggregate
if (is_array($this->item)) {
$this->itemIterator = new ArrayIterator($this->item);
} elseif ($this->item instanceof Iterator) {
$this->itemIterator = $this->item;
} else {
Comment on lines 316 to 320
Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's worth adding this because:

  1. there's already logic here to handle native arrays, even though previously those couldn't be used at all and now they're wrapped in ArrayList before we get here... but this allows for them to be handled correctly if they somehow sneak in
  2. As with the array check, this allows iterators to be used correctly if they somehow sneak in. They shouldn't get here, but if they do it's better to just let them be iterated over rather than throw exceptions about it, IMO.

$this->itemIterator = $this->item->getIterator();

Expand Down
17 changes: 12 additions & 5 deletions src/View/ViewableData.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
use SilverStripe\Dev\Debug;
use SilverStripe\Dev\Deprecation;
use SilverStripe\ORM\ArrayLib;
use SilverStripe\ORM\ArrayList;
use SilverStripe\ORM\FieldType\DBField;
use SilverStripe\ORM\FieldType\DBHTMLText;
use SilverStripe\View\SSViewer;
Expand Down Expand Up @@ -537,7 +538,7 @@ protected function objCacheClear()
* @param array $arguments
* @param bool $cache Cache this object
* @param string $cacheName a custom cache name
* @return Object|DBField
* @return object|DBField
Comment on lines -540 to +541
Copy link
Member Author

Choose a reason for hiding this comment

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

It's probably the case that this used to mean the old SS3 Object which later became SS_Object which is effectively ViewableData now from a typing perspective.

That said, the way this is implemented, it really can return any object, so I'm going to treat this as though it was just a capital letter away from being correct initially.

Copy link
Member Author

@GuySartorelli GuySartorelli May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Update now that this PR uses ArrayList instead of ArrayIterator:
There are still other scenarios where this method could return arbitrary objects, because of the way it's been implemented. It might make more sense to tighten this up to require ViewableData to be returned but for now that's just not how it works - and doing that should be handled in a separate PR.

*/
public function obj($fieldName, $arguments = [], $cache = false, $cacheName = null)
{
Expand All @@ -558,6 +559,11 @@ public function obj($fieldName, $arguments = [], $cache = false, $cacheName = nu
$value = $this->$fieldName;
}

// Wrap list arrays in ViewableData so templates can handle them
if (is_array($value) && array_is_list($value)) {
$value = ArrayList::create($value);
}

// Cast object
if (!is_object($value)) {
// Force cast
Expand Down Expand Up @@ -601,7 +607,10 @@ public function cachedCall($fieldName, $arguments = [], $identifier = null)
public function hasValue($field, $arguments = [], $cache = true)
{
$result = $this->obj($field, $arguments, $cache);
return $result->exists();
if ($result instanceof ViewableData) {
return $result->exists();
}
return (bool) $result;
Comment on lines 609 to +613
Copy link
Member Author

Choose a reason for hiding this comment

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

This is still needed because as mentioned above the obj() method can return arbitrary objects with its current implementation. It's not related to the issue at hand but worth keeping IMO.

}

/**
Expand Down Expand Up @@ -671,10 +680,8 @@ public function getViewerTemplates($suffix = '')
/**
* When rendering some objects it is necessary to iterate over the object being rendered, to do this, you need
* access to itself.
*
* @return ViewableData
*/
public function Me()
public function Me(): static
{
return $this;
}
Expand Down
127 changes: 127 additions & 0 deletions tests/php/View/SSViewerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1025,6 +1025,44 @@ public function testIfBlocks()
);
}

public function provideIfBlockWithIterable(): array
{
$scenarios = [
'empty array' => [
'iterable' => [],
'inScope' => false,
],
'array' => [
'iterable' => [1, 2, 3],
'inScope' => false,
],
'ArrayList' => [
'iterable' => new ArrayList([['Val' => 1], ['Val' => 2], ['Val' => 3]]),
'inScope' => false,
],
];
foreach ($scenarios as $name => $scenario) {
$scenario['inScope'] = true;
$scenarios[$name . ' in scope'] = $scenario;
}
return $scenarios;
}

/**
* @dataProvider provideIfBlockWithIterable
*/
public function testIfBlockWithIterable(iterable $iterable, bool $inScope): void
{
$expected = count($iterable) ? 'has value' : 'no value';
$data = new ArrayData(['Iterable' => $iterable]);
if ($inScope) {
$template = '<% with $Iterable %><% if $Me %>has value<% else %>no value<% end_if %><% end_with %>';
} else {
$template = '<% if $Iterable %>has value<% else %>no value<% end_if %>';
}
$this->assertEqualIgnoringWhitespace($expected, $this->render($template, $data));
}

public function testBaseTagGeneration()
{
// XHTML will have a closed base tag
Expand Down Expand Up @@ -1340,6 +1378,88 @@ public function testCastingHelpers()
);
}

public function provideLoop(): array
{
return [
'nested array and iterator' => [
'iterable' => [
[
'value 1',
'value 2',
],
new ArrayList([
'value 3',
'value 4',
]),
],
'template' => '<% loop $Iterable %><% loop $Me %>$Me<% end_loop %><% end_loop %>',
'expected' => 'value 1 value 2 value 3 value 4',
],
'nested associative arrays' => [
'iterable' => [
[
'Foo' => 'one',
],
[
'Foo' => 'two',
],
[
'Foo' => 'three',
],
],
'template' => '<% loop $Iterable %>$Foo<% end_loop %>',
'expected' => 'one two three',
],
];
}

/**
* @dataProvider provideLoop
*/
public function testLoop(iterable $iterable, string $template, string $expected): void
{
$data = new ArrayData(['Iterable' => $iterable]);
$this->assertEqualIgnoringWhitespace($expected, $this->render($template, $data));
}

public function provideCountIterable(): array
{
$scenarios = [
'empty array' => [
'iterable' => [],
'inScope' => false,
],
'array' => [
'iterable' => [1, 2, 3],
'inScope' => false,
],
'ArrayList' => [
'iterable' => new ArrayList([['Val' => 1], ['Val' => 2], ['Val' => 3]]),
'inScope' => false,
],
];
foreach ($scenarios as $name => $scenario) {
$scenario['inScope'] = true;
$scenarios[$name . ' in scope'] = $scenario;
}
return $scenarios;
}

/**
* @dataProvider provideCountIterable
*/
public function testCountIterable(iterable $iterable, bool $inScope): void
{
$expected = count($iterable);
$data = new ArrayData(['Iterable' => $iterable]);
if ($inScope) {
$template = '<% with $Iterable %>$Count<% end_with %>';
} else {
$template = '$Iterable.Count';
}
$this->assertEqualIgnoringWhitespace($expected, $this->render($template, $data));
}

public function testSSViewerBasicIteratorSupport()
{
$data = new ArrayData(
Expand Down Expand Up @@ -2239,4 +2359,11 @@ public function testPrimitivesConvertedToDBFields()
$this->render('<% loop $Foo %>$Me<% end_loop %>', $data)
);
}

public function testMe(): void
{
$mockArrayData = $this->getMockBuilder(ArrayData::class)->addMethods(['forTemplate'])->getMock();
$mockArrayData->expects($this->once())->method('forTemplate');
$this->render('$Me', $mockArrayData);
}
}
28 changes: 28 additions & 0 deletions tests/php/View/ViewableDataTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use ReflectionMethod;
use SilverStripe\ORM\FieldType\DBField;
use SilverStripe\Dev\SapphireTest;
use SilverStripe\ORM\ArrayList;
use SilverStripe\View\ArrayData;
use SilverStripe\View\SSViewer;
use SilverStripe\View\Tests\ViewableDataTest\ViewableDataTestExtension;
Expand Down Expand Up @@ -278,4 +279,31 @@ public function testDynamicData()
$this->assertSame($obj, $viewableData->getDynamicData('abc'));
$this->assertSame($obj, $viewableData->abc);
}

public function provideWrapArrayInObj(): array
{
return [
'empty array' => [
'arr' => [],
'expectedClass' => ArrayList::class,
],
'fully indexed array' => [
'arr' => [
'value1',
'value2',
],
'expectedClass' => ArrayList::class,
],
];
}

/**
* @dataProvider provideWrapArrayInObj
*/
public function testWrapArrayInObj(array $arr, string $expectedClass): void
{
$viewableData = new ViewableData();
$viewableData->arr = $arr;
$this->assertInstanceOf($expectedClass, $viewableData->obj('arr'));
}
}
Loading