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

Roll filterable, sortable, and limitable into SS_List interface #8778

Closed
5 tasks done
maxime-rainville opened this issue Feb 1, 2019 · 7 comments
Closed
5 tasks done

Comments

@maxime-rainville
Copy link
Contributor

maxime-rainville commented Feb 1, 2019

Description

Lots of method that expect a list of object will declare that they expect an SS_List. Those methods usually implicitly assume that the list they are provided will be filterable, limitable and sortable.

In theory someone could define their own class that implements SS_List without implementing the Filterable, Sortable, limitable.

We had a long conversation about how to address this in #8609. The conclusion was that fixing this in SS 4 is not possible without introducing ambiguity.

The suggestion from that issue for SS 5 was to:

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

We've since decided that having the individual Filterable, Limitable, and Sortable interfaces on their own isn't helpful.

Acceptance criteria

  • The Filterable, Limitable, and Sortable interfaces are all rolled into SS_List
  • SS_List interface is updated to have appriopriate typhints
  • SS_List is updated to include and list methods we think should be absolutely included in all lists (e.g. excludeAny() is missing)
  • All old references are update in code and in doc
  • Changed is called out in upgrade doc

Kitchen Sink CI

CMS 6 PRs

CMS 5 PRs

@sminnee sminnee changed the title SS_List is assumed to be fitlerable, sortable, limitable Add new API defining filterable, sortable, and limitable SS_List Mar 8, 2019
@sminnee
Copy link
Member

sminnee commented Mar 8, 2019

This isn't a bug; the system is working as designed.

@maxime-rainville
Copy link
Contributor Author

There's a bunch of other problems with List that maybe we want to address while we are looking at CMS5. Just to name a few:

  • return types for all the method is not defined
  • removeAll is not defined in the interface

@maxime-rainville
Copy link
Contributor Author

We were just discussing this issue. We are thinking that maybe we don't need Filterable, Limitable and Sortable. It seems like a bit of over engineering since there's literally no implementation of SS_List that doesn't implement those as well.

@maxime-rainville
Copy link
Contributor Author

Not going to make it to CMS5.

@GuySartorelli
Copy link
Member

@emteknetnz I won't merge the PRs yet - you need CMS 5 PRs to deprecate the old interfaces. There's no way to add a notice, but we still need the @deprecated PHPDoc and a changelog entry.

@GuySartorelli
Copy link
Member

PRs merged but I noticed afterward that the CMS 5 PRs still weren't done.
Reassigning to Steve to deal with that.

@GuySartorelli
Copy link
Member

PRs merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants