Instance vs. clone modifications on DataList?

[Ingo Schommer]

Hello Hamish, Sam, Stig, and other DataList afficionados :)

I've noticed two bugfixes due to misunderstanding how DataList works:

https://github.com/silverstripe-rebelalliance/sapphire/commit/058805c5231980e29a4e3e9f04ed2742c87a2ee9

https://github.com/silverstripe/sapphire/commit/c555256890b2cd0993402f775f5165aa04e72ae9

In general, it seems to be fairly arbitrary when DataList returns a clone,

and when it does inline modifications - which I think is very harmful to the API,

as it requires this "insider knowledge" from every dev.

$dataList->filter('Name', 'Bilbo'); // won't work, since its not assigned to new instance

$newList = $dataList->filter('Name', 'Bilbo'); // correct usage

$dataList->addFilter(array('Name' => 'Bilbo')); works, since it doesn't clone

The idea is that any state modification to the list produces a new list, correct?

In this case, the following methods should do that:

- join()

- limit()

- sort()

- addFilter()

- exclude()

- innerJoin()

- leftJoin()

- reverse()

There's a couple which do this already:

- filter()

- subtract()

That's a pretty major API change though. Hopefully I'm just misunderstanding the problem?

Ingo

[Andrew Short]

One other issue related to this is that the ArrayList API is also quite different - e.g. DataList->filter() returns a clone while ArrayList->filter() does the filtering in place. It would be good to make this consistent.

Andrew Short.

[Hamish Friedlander]

Oh, good.

I can't speak for Sam's original intention, but getting this consistent seems pretty important. I agree with you Ingo - ideally DataList instances should be seen as immutable, and any state change should return a new instance.

Could we make those methods that currently change internal state both retain current behaviour & return a new object as well? That way we can maintain backwards compatibility but allow developers to use the new "correct" API immediately without waiting for 3.1 to stabilise. Then in 3.1 we can either deprecate or disable the old behaviour.

We should definitely try and make ArrayList behave the same. Instances of the two should ideally be interchangeable for most use cases.

Hamish Friedlander

On 16 July 2012 20:13, Andrew Short <andrew...@gmail.com> wrote:

One other issue related to this is that the ArrayList API is also quite different - e.g. DataList->filter() returns a clone while ArrayList->filter() does the filtering in place. It would be good to make this consistent.

Andrew Short.

--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
To post to this group, send email to silverst...@googlegroups.com.
To unsubscribe from this group, send email to silverstripe-d...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/silverstripe-dev?hl=en.

[Ingo Schommer]

Created a ticket for this: http://open.silverstripe.org/ticket/7673

Currently assigned to 3.0.1 milestone - important enough to fix early,

before the ORM gains more usage (and more custom code makes assumptions), right?

Returning a clone as well as modifying the instance is a really good idea!

[Marcus Nyeholt]

Yeah, I've bumped into this issue a few times already; there are also a couple of places (I think I was needing to do something like an  orWhere type of thing) where DataList doesn't expose the API directly, you need to go directly to $list->dataQuery()->xxxMethod(), with the problem being that the xxxMethod() actually returns a clone of $this ($this being the data query object) which isn't reassigned to the $list's local dataQuery variable. 

So, does this imply that dataQuery should NOT be exposed via the $list, or should $list->dataQuery() actually create a clone of $this, as well as a clone of $this->dataQuery, and make the relevant reassignment? 

Cheers,

Marcus

--

You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.

To view this discussion on the web visit https://groups.google.com/d/msg/silverstripe-dev/-/ehnkE6ZsRl0J.

[Hamish Friedlander]

If we make DataLists immutable, allowing you to break the immutability by modifying the underlying query is probably bad. Better create a new clone of the list with the new query, like:

$query = $list->dataQuery();

$query = $query->xxxMethod();

$list = $list->withDataQuery($query);

Hamish Friedlander 

[Marcus Nyeholt]

Or $newList = $list->changeQuery()->xxxMethod() ?

Either way there's some reliance on the developer knowing to do-the-right-thing

Cheers,

Marcus

[Hamish Friedlander]

Initial pull request is at https://github.com/silverstripe/sapphire/pull/660

This doesn't make DataList or ArrayList immutable _yet_, but does allow you to use them like they are, and it's easy to turn off mutability in 3.1

This only affects the "query" methods - there are plenty of methods on DataList / ArrayList that perform persistant modifications, and they'll remain mutating. For example push, remove, etc

This will be going into 3.0.1, so if interested people could have a quick look as soon as poss, that'd be good. Especially Marcus what do you think of alterDataQuery?

Hamish Friedlander