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.

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

API Add a GenericList interface to encapsulate a list that is fitlerable, sortable, limitable #8609

Closed

Conversation

maxime-rainville
Copy link
Contributor

This is admittedly a somewhat academic point, but there's quite a few places where we say we expect/return an SS_List when what we really mean is an SS_List that is Filterable, Sortable and/or Limitable.

I think we should be explicit and provide an interface that encapsulate all of those other interfaces.

Aside from the additional clarity it will also help with code completion.

I'm not overly fuss about that GenericList name. If someone can think of a more descriptive name, go for it.

@robbieaverill
Copy link
Contributor

It's been argued previously that adding interfaces can be a breaking change, we'd need to be careful with this

ping @silverstripe/core-team

@unclecheese
Copy link

@robbieaverill Can you articulate why this would be a breaking change?

@robbieaverill
Copy link
Contributor

I'm voicing a comment I remember from @dhensby, perhaps he can help :)

@robbieaverill
Copy link
Contributor

I guess if the interfaces don't have any new methods in them then it should be fine

@sminnee
Copy link
Member

sminnee commented Nov 15, 2018

It's more "CompleteList" or "FullFeaturedList" than "GenericList", but I agree that having a "union interface" probably makes sense.

I'd see this as a minor change but I struggle to see what it would break. Although changing a method that currently accepts an SS_List to accept a full-featured list would be a breaking change.

@maxime-rainville
Copy link
Contributor Author

I agree with Robbie that we should be careful about retroactively updating SS_List references to the new interface.

For example, if we update GridField_DataManipulator::getManipulatedData() to expect a complete list instead of an SS_List we could in theory break someone's custom SS_List implementation ... in practice their custom list is probably going to be broken any way because most GridField_DataManipulator components expect the list given to them to be sortable/fitlerable/limitable.

My suggestion would be that new APIs use the new interface and we wait for the SS5 to update old ones.

Updating ArrayList/DataList to the new interface is probably kosher because they are still implementing all the same interfaces.

CompleteList does sound better than GenericList.

@unclecheese
Copy link

+1. I only see this as a breaking change if we start consuming it in methods.

@tractorcow
Copy link
Contributor

It's only breaking if the new interface doesn't extend the old one. This does so it should be fine.

I dislike the name "Generic" also, because it doesn't convey any meaning. The SS_List has the same problem. It tries to be "the" list type for silverstripe. :)

We had a similar problem where we ended up adding Relation interface for relations, since not all relations could extend RelationList class.

We also have some ambiguity around item types used for these lists. DataList of course only works with DataObjects, but ArrayList is used for all viewabledata types, and even simple types.

I would prefer the name ObjectList and clarify that this list implies it contains ViewableData items only, and thus supports other list types that would work on objects only (e.g. Filterable, which requires objects).

@unclecheese
Copy link

ManipulableList?

Just thinking of getManipulatedData() and DataManipulator in GridField.

@tractorcow
Copy link
Contributor

I'd be forever spelling that wrong. :P

@unclecheese
Copy link

Fortunately, all you need to know are the first three letters, and the IDE will take it from there. :)

@robbieaverill
Copy link
Contributor

I already read it three times - is it a real word? =P

@ScopeyNZ
Copy link
Contributor

ScopeyNZ commented Nov 17, 2018

I prefer ManipulableList over GenericList but I'm not sure it's quite right 😖 . I can't think of anything better though:

ClassifiableList?
ManageableList?
TractableList? (@tractorcow 😉)

@maxime-rainville
Copy link
Contributor Author

I'll throw it in the backend dev channel and see what the masses comes back with.

@maxime-rainville
Copy link
Contributor Author

@jDolba suggested Collection which I quite like. It's short and punchy while still being descriptive of the purpose of the interface.

The only downside is that everything else is SomethingList. We might want to go with a pleonasm like CollectionList to make it clear it's related to ArrayList and DataList.

@sminnee
Copy link
Member

sminnee commented Nov 19, 2018

“Collection” means something In pre-existing patterns. It differs from “List” in that it doesn’t have an order, so you can’t iterate it. I think this is misleading - like how “DataObjectDecorator” wasn’t actually a decorator.

Calling it “CollectionList” is like calling something “CircleSquare”.

Can anyone elaborate on what problems this change actually solves? Do we have a lot of objects that explicitly require a list that has all 3 features?

Another option: FLSList - for filteable, limitable, sortable ;-)

@maxime-rainville
Copy link
Contributor Author

The specific examples that were bugging me were the GridField components and interfaces that proclaim they expect an SS_List object but then calls method specific to Limitable/Sortable/Filterable.

But in practice, pretty much every where we expect an SS_List we implicitly expect them to implement those 3 interfaces.

@maxime-rainville
Copy link
Contributor Author

As I mentioned in the original description, the point is somewhat academic ... I doubt there's many people out there creating their own list implementation from scratch.

The main problem is we don't have a way of saying: "this method needs a list that is sortable and filterable".

@robbieaverill
Copy link
Contributor

The specific examples that were bugging me were the GridField components and interfaces that proclaim they expect an SS_List object but then calls method specific to Limitable/Sortable/Filterable.

Yeah that bugs me too

@robbieaverill
Copy link
Contributor

If it helps, PSR-5 is talking about introducing a union type for phpdocs, but that doesn't help if you can't type hint it in the PHP argument for the method

@sminnee
Copy link
Member

sminnee commented Nov 20, 2018

I think that the API-breaking fix to this would be to push the features of these 3 interfaces into SS_List and find a different way of doing feature-detection.

Related, "Collection" would more closely match the current SS_List and "List" the full-featured version.

The original reason for this pattern was to allow for the implementation of other List implementations (e.g. providing integration with 3rd party APIs) that didn't necessarily provide sort, filter, and limit options. In other words, a form of feature-detection.

However, this could be done with a method such as $list->supports(SS_List::SORT_FEATURE) // boolean instead. Furthermore, it's rare that this capability is actually used.

These are all API breakages. So, if we were to go down this path, it would change in SS5.

So perhaps you could call it SS5List? ;)

@maxime-rainville
Copy link
Contributor Author

What about we tolerate the current situation, and in SS5 we:

  • rename the current SS_List to Collection and
  • introduce a new SS_List that encapsulate Collection, Filterable, Limitable and Filterable.

I'd rather get things right in the long run than introduce confusion now and then confuse people so more by renaming things around in in SS5.

@maxime-rainville
Copy link
Contributor Author

I've raised a matching issue for SS5 #8778.
I'll close this for now.

@maxime-rainville maxime-rainville deleted the pulls/4/genric-list branch February 1, 2019 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants