ENH Looping through arrays in templates

[GuySartorelli]

Description

Allows arrays to be passed into templates and iterated over.
Associative arrays are wrapped in ArrayData so their values can be accessed by key.
Indexed arrays are wrapped in ArrayIterator ArrrayList when fetched from ViewableData::obj() to respect the method signature.

Not wrapping in ArrayList because that currently throws exceptions if indexed arrays are passed in, and if we changed that it would silently fail if you try to call Filter() or Sort() or Find() etc.

Issues

• Ability to use arrays in templates #11237

[GuySartorelli]

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.

[GuySartorelli]

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.

[GuySartorelli]

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

[emteknetnz]

Can you help me understand what is meant by `Not wrapping in ArrayList because that currently throws exceptions if indexed arrays are passed in, and if we changed that it would silently fail if you try to call Filter() or Sort() or Find() etc.

Just doing a quick test on CMS 5 post merging the scalars PR

I can pass this in from a controller

    public function ArrayListTest()
    {
        // note the decending order
        return new ArrayList(['item3', 'item2', 'item1']);
    }

And render it in a template

<!-- works - item3, item2, item1 -->
<% loop $ArrayListTest %>
    $Me
<% end_loop %>

<!-- works -->
$ArrayListTest.Count

<!-- though this will never work instead it will "silently fail" - item3, item2, item1
     Sort requires a column -->
<% loop $ArrayListTest.Sort %>
    $Me
<% end_loop %>

Seem like the major reason for ArrayIterator is because methods like .Sort can't work, because we don't have ArrayData() or another object as the members in the ArrayList, and you'd like a hard error thrown rather than nothing happening to protected against developer error?

Though that seems like a strange reason to not use ArrayList? Because you can already construct an ArrayList full of scalars and use it in a template. It's just called Sort() on it with no parameters does nothing ... though that's what is supposed to happen?

I'm not sure what's so wrong with ArrayList's at we'd want to complicate things by using ArrayIterator() instead which increases the number of scenarios that need to be coded for / things that could go wrong.

I feel like I'm missing something important here?

[GuySartorelli]

Though that seems like a strange reason to not use ArrayList? Because you can already construct an ArrayList full of scalars and use it in a template. It's just called Sort() on it with no parameters does nothing ... though that's what is supposed to happen?

First of all I didn't say anything about no parameters - whether you pass parameters or not it'll have undefined behaviour and most likely fail silently. The assumption is that you can sort and filter arraylists. Remember this affects PHP-land logic as well, not just template logic.

If we want to wrap indexed arrays in ArrayList, we need to change the way ArrayList's iterator works so that it doesn't throw an exception.

We then need to decide what happens if someone calls sort(), filter(), find(), or any other methods that require keys to get values in order to operate on them?
- Do we throw exceptions for these? That's bad DX. ArrayList implements Sortable and Filterable, so it should be sortable and filterable.
- Do we allow sorting the underlying array in various ways (alphabetically, numerically, etc)? We then need to make sure the API for that is clear, and we're still left with answering what happens if someone tries to sort by a specific key,
- Do we allow sorting by value? If so we have the same problems as with sorting.
- Do we just let it fail silently? That's the worst possible solution IMO.

We also have to verify what will happen if you try to feed that into a gridfield, or anywhere else that currently expects ArrayList that works the way ArrayList works right now.

Basically trying to use ArrayList here opens a whole can of worms that I'm not willing to open as part of this issue. That's scope creep if ever I saw it. If you want to use ArrayList here you're welcome to open a follow-up card to do that after this PR is merged as a separate piece of work. I'm not doing it as part of this card.

[emteknetnz]

We then need to decide what happens if someone calls sort(), filter(), find(), or any other methods that require keys to get values in order to operate on them? ... We also have to verify what will happen if you try to feed that into a gridfield, or anywhere else that currently expects ArrayList that works the way ArrayList works right now.

It does seem a little odd to be concerned about all these things - as mentioned before in CMS 5 you can currently go new ArrayList(['item3', 'item2', 'item1']); - we don't seem to be too concerned about the fact you can't actually use this ArrayList in a GridField right now?

Based on what you're suggesting here seems like we may even want to actually restrict the constructor of ArrayList to disallow a index array scalars entirely since you can't use most of the ArrayList API on them? I am actually open to that idea as it would restrict the number of possible codepaths and scenarios that need to be considered ... though obviously it negates the whole idea of just return and array of scalars in a controller for template consumption.

Maybe this card is just a bad idea? I'm starting to now wonder what ArrayList methods with a list of values can and can't be used in a template. Count can, though sort and friends can't, though are there other that can? Should be making it so that arrays can have the exact same methods called on to match ArrayList? Feels like by allowing mixed types we're just making things worse... typically we want to go in the direction of being more restrictive to make things more robust e.g. strong typing, this feels like it's going in the wrong direction

[GuySartorelli]

It does seem a little odd to be concerned about all these things - as mentioned before in CMS 5 you can currently go new ArrayList(['item3', 'item2', 'item1']); - we don't seem to be too concerned about the fact you can't actually use this ArrayList in a GridField right now?

It's not so much about gridfield (that was just one example) as it is about the API for ArrayList not being respected or usable when you throw data into it which doesn't have keys to sort/filter/etc by.
Right now that's not really a problem because as soon as you try to iterate over the list it'll throw an exception at you anyway.

Based on what you're suggesting here seems like we may even want to actually restrict the constructor of ArrayList to disallow a index array scalars entirely since you can't use most of the ArrayList API on them? I am actually open to that idea as it would restrict the number of possible codepaths and scenarios that need to be considered ...

That would be great, as a separate issue. This issue isn't about deciding how to fix the problems with arraylist.

though obviously it negates the whole idea of just return and array of scalars in a controller for template consumption.

No it doesn't, as this PR avoids using ArrayList and allows you to return an array of scalars in a controller for template consumption.

Maybe this card is just a bad idea?

I think this card is a great idea, and I think we can worry about ArrayList separately.

I'm starting to now wonder what ArrayList methods with a list of values can and can't be used in a template. Count can, though sort and friends can't, though are there other that can? Should be making it so that arrays can have the exact same methods called on to match ArrayList? Feels like by allowing mixed types we're just making things worse... typically we want to go in the direction of being more restrictive to make things more robust e.g. strong typing, this feels like it's going in the wrong direction

With this PR, $Count will work in a template as an explicit and special variable, because it is common to want to know how many items are in your iterable variable.
Sorting and filtering of regular arrays can be done in a controller - if we want to add a way to do that from within the template that would be a neat enhancement - but we have to be able to use the arrays in a template in the first place before we can look at that.

[GuySartorelli]

Do we need to have a meeting to discuss this one or can we get it merged?

[emteknetnz]

My main concern here is that by not using ArrayList + ArrayData we're increasing the complexity of the number of scenarios that need to be managed.

As an alternate solution, would it work if arrays were converted right before going into the template to a combination of ArrayList and ArrayData with a single magical "_index" key used in the ArrayData if an index array was passed into ArrayList?

e.g.

['item1', 'item2'];

Automatically gets converted to

new ArrayList([
  new ArrayData['_index' => 'item1'],
  new ArrayData['_index' => 'item2'],
]);

And then have some logic where if the only key in the ArrayData is _index then use that for the value of $Me?

Seems like this would negate the need for all the extra logic to handle iterables / countables etc?

[GuySartorelli]

It's not only templates though, it's also PHP. So if you do that, you're also saying any time you pass a non-associative array to ArrayList it'll get this weird key and be wrapped in ArrayData. But if you iterate over that ArrayList in PHP it won't know magically that you're trying to get the value from the _index key so you have to account for that in your project code, which is a little gross.

If you're really insistent on this being handled using ArrayList, as I've mentioned, that opens a whole can of worms that will have to be thoroughly discussed. I don't want to do that, I want to just get something working in an intuitive way which IMO this PR does, and we can worry about ArrayList separately.

Looking at this PR again, there's not that much complexity being added to account for non-ArrayList iterables.

1. There's a small bugfix in ArrayData that IMO should happen anyway.
2. There's moving $Me from ViewableData into SSViewer_DataPresenter which IMO should happen anyway.
3. There's a small change in the parser to allow $Count logic to be handled differently.
   • This is just a simple conditional statement.
4. There's handling $Count in SSViewer_DataPresenter, which is probably the biggest piece of added complexity. It's 22 lines including comments, and all it does is
   1. If ViewableData, Call XML_val('Count')
   2. If there's a method or property called Count on the item, use that
      • This isn't strictly necessary, we could remove this if we're that concerned about the complexity but IMO it's not adding much maintenance burden.
   3. if the item is countable , use count($item)
   4. Everything else returns null
5. There's some logic in both ViewableData and SSViewer_DataPresenter to allow an <% if $MyIterable %> to check the count of the iterable.
   • This is just a simple conditional statement.
   • We could move this logic into a single place if we're concerned about the duplication - but there's quite a few spots like this between SSViewer (and its scope) and ViewableData so we would probably want to look at that deduping separately.
6. There's the logic in both ViewableData and SSViewer_Scope to wrap the iterable, which has to happen regardless of what we're wrapping it in
7. There's a tiny condition check for $this->item instanceof Iterable

Choosing to wrap this in ArrayList will remove 3, 4, and 5 above. The complexity of these is discussed above.
It will also probably add:

1. Changing ArrayList so it doesn't throw exceptions when iterating over a non-associative array
2. Possibly some new logic in sort/filter/find/etc to handle what happens with non-associative arrays
3. Possibly some logic either in ArrayData or somewhere in the templating system to check if there's a _index key in the item we're trying to:
   1. Call $Me to get the current item
   2. access a property or call a method (not just $Me, we need to be able to access any method or property on the item) - (currently handled by obj() on ArrayData so we're changing the way that works in PHP land as well), or
   3. check if it has a value (currently handled by hasValue() on ArrayData so we're changing the way that works in PHP land as well), or
   4. iterate over it (maybe this would go in getItem() or next() on SSViewer_Scope? Not sure), or
   5. Other scenarios? Are there edge cases we're missing that will result in new bugs we have to deal with down the line?

I think we'd quickly find trying to deal with ArrayList here becomes more complicated very quickly, and has much wider reaching changes than the ones I've made in this PR and therefore a higher chance of causing regressions or future unexpected behaviours.

[GuySartorelli]

So again.

Do we need to have a meeting to discuss this one or can we get it merged?

[emteknetnz]

Probably have a chat to discuss

I've quickly together a PR to demo the idea of just using an ArrayList #11256, seems like it does the same thing with much less code changes required? (I've probably missed something though)

[GuySartorelli]

We've discussed offline - I had a faulty assumption about ArrayList - we'll try with ArrayList instead of ArrayIterator

[GuySartorelli]

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.

[GuySartorelli]

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.

[GuySartorelli]

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.

[GuySartorelli]

Some of the added tests are no longer explicitly required for the new functionality - but they weren't being tested before so we're adding additional (if only tangentially related to the current issue) test coverage.

[emteknetnz]

Code changes look good, though this one should be run though a kitchen-sink CI run with the PR included since it's a pretty low level change

One thing I did notice is that while <% loop $Me %> worked as expected <% loop %> did not, possibly the PR just needs a rebase or merge-up though

Test code:

PageController

    public function OneList()
    {
        return ['abc', 'def'];
    }

    public function OneAssoc()
    {
        return ['Foo' => 'bar'];
    }

    public function TwoList()
    {
        return [
            ['abc', 'def'],
            ['ghi', 'xyz'],
        ];
    }

    public function TwoAssoc()
    {
        return [
            [
                'Food' => 'bars',
                'Bazz' => 'buzz',
            ],
        ];
    }

    public function ThreeList()
    {
        return [
            [
                ['123', '456'],
            ],
            [
                ['789', '000'],
            ]
        ];
    }

    public function ThreeAssoc()
    {
        return [
            [
                [
                    'Foody' => 'barsy',
                    'Bazzy' => 'buzzy',
                ],
            ],
        ];
    }

Page.ss

  <% loop $OneList %>
      $Me<br>
  <% end_loop %>

  $OneAssoc.Foo<br>

  <% loop $TwoList %>
      <% loop $Me %>
          $Me<br>
      <% end_loop %>
  <% end_loop %>

  <% loop $TwoAssoc %>
      $Me.Food<br>
      $Me.Bazz<br>
  <% end_loop %>

  <% loop $ThreeList %>
      <% loop $Me %>
          <% loop $Me %>
              $Me<br>
          <% end_loop %>
      <% end_loop %>
  <% end_loop %>

  <% loop $ThreeAssoc %>
      <% loop $Me %>
          $Me.Foody<br>
          $Me.Bazzy<br>
      <% end_loop %>
  <% end_loop %>

[GuySartorelli]

One thing I did notice is that while <% loop $Me %> worked as expected <% loop %> did not, possibly the PR just needs a rebase or merge-up though

This PR started before that work got done, so yeah that'd need to be merged up to 6 and then this would need to be rebased on top.
I'm not gonna bother doing that unless you explicitly ask for it because that doesn't really feel related to this work tbh.

[GuySartorelli]

Had to rebase in the end to get kitchen sink CI to run happily. That CI run is linked in the issue.

[GuySartorelli]

Regrettably, the Me deprecation in ViewableData caused other problems in sink, because against all reason it's being used in PHP contexts.

I've removed that from this PR and have opened a new issue for it.

That has also stopped the ArrayIterator tests in SSViewerTest from working (because there's no Me() method on that class) so I've removed those tests as well.

[GuySartorelli]

Wrapping arrays in ArrayData means it's no longer being cast with the old casting logic, which is causing problems in sink.
This points to a wider problem with how that casting is being done or relied on.

I've opened a separate card to look into all that but for now I've removed the logic wrapping arrays in ArrayData from this PR. We're now explicitly only dealing with list arrays.

[emteknetnz]

Tested locally, works well