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

WIP: Split Search Filter and Search #2453

Closed
wants to merge 8 commits into from
Closed

Conversation

askvortsov1
Copy link
Member

@askvortsov1 askvortsov1 commented Nov 13, 2020

Start of flarum/issue-archive#286

Changes proposed in this pull request:

  • Split search and filter into separate endpoints
  • Add a "filterer" that applies filters and filter mutations

TODO:

  • Add filter equivalent of default search gambits
  • Convert bundled extensions to use this system
  • Clean up code / improve abstractions
  • Convert ListPostsController to use this system (in a separate PR)

Reviewers should focus on:

  • If we register things both as filters and as gambits for SimpleFlarumSearch, we're gonna have a lot of duplicated code. That being said, I'm not sure if there's anything we can do here if we want to truly separate the two
  • For simplicity and to reduce diff size, I omitted some of the abstractions present in the old search system for filtering (SearchCriteria, etc). I also reused some SimpleFlarumSearch utils / components, like SearchResults, ApplySearchParametersTrait, AbstractSearch. What do we want to do about this? IMO simpler is better, especially seeing that custom filtration drivers aren't going to be a thing, so unnecessary abstraction is unnecessary. Did I cut out too much? Also, I want to eventually move the shared stuff into a Query namespace, but didn't do so here to minimize diff.

What if we used one endpoint, and searched vs filtered based on the presence of the q parameter? And what if SimpleFlarumSearch internally used filtration as its gambits (and all gambits were applied as key-values on the filter param instead of as part of the q string?

@askvortsov1
Copy link
Member Author

Superceded by #2454

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.

1 participant