Closed GuySartorelli closed 1 month ago
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?
ArrayList
implements Sortable
and Filterable
, so it should be sortable and filterable.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.
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
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.
Do we need to have a meeting to discuss this one or can we get it merged?
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?
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.
ArrayData
that IMO should happen anyway.$Me
from ViewableData
into SSViewer_DataPresenter
which IMO should happen anyway.$Count
logic to be handled differently.
$Count
in SSViewer_DataPresenter
, which is probably the biggest piece of added complexity. It's 22 lines including comments, and all it does is
ViewableData
, Call XML_val('Count')
Count
on the item, use that
count($item)
null
ViewableData
and SSViewer_DataPresenter
to allow an <% if $MyIterable %>
to check the count of the iterable.
SSViewer
(and its scope) and ViewableData
so we would probably want to look at that deduping separately.ViewableData
and SSViewer_Scope
to wrap the iterable, which has to happen regardless of what we're wrapping it in$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:
ArrayList
so it doesn't throw exceptions when iterating over a non-associative arrayArrayData
or somewhere in the templating system to check if there's a _index
key in the item we're trying to:
$Me
to get the current item$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), orhasValue()
on ArrayData
so we're changing the way that works in PHP land as well), orgetItem()
or next()
on SSViewer_Scope
? Not sure), orI 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.
So again.
Do we need to have a meeting to discuss this one or can we get it merged?
Probably have a chat to discuss
I've quickly together a PR to demo the idea of just using an ArrayList https://github.com/silverstripe/silverstripe-framework/pull/11256, seems like it does the same thing with much less code changes required? (I've probably missed something though)
We've discussed offline - I had a faulty assumption about ArrayList
- we'll try with ArrayList
instead of ArrayIterator
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.
Had to rebase in the end to get kitchen sink CI to run happily. That CI run is linked in the issue.
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.
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 inArrayIterator
ArrrayList
when fetched fromViewableData::obj()
to respect the method signature.Not wrapping inArrayList
because that currently throws exceptions if indexed arrays are passed in, and if we changed that it would silently fail if you try to callFilter()
orSort()
orFind()
etc.Issues
11237