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

Search Extender and tests #2483

Merged
merged 16 commits into from Feb 10, 2021
Merged

Search Extender and tests #2483

merged 16 commits into from Feb 10, 2021

Conversation

askvortsov1
Copy link
Sponsor Member

Part of #1891
Part of flarum/issue-archive#286
Part 3 of #2454

Changes proposed in this pull request:

  • All searchers now inherit from AbstractSearcher
  • Gambit managers no longer internally use a container
  • A SimpleFlarumSearch extender has been added
  • Events for Searching and gambits have been deprecated

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

@askvortsov1 askvortsov1 changed the base branch from master to as/use-filterer-for-ListPostController January 18, 2021 23:16
@askvortsov1 askvortsov1 force-pushed the as/use-filterer-for-ListPostController branch from 5d5b139 to fb1831e Compare January 19, 2021 23:53
@askvortsov1 askvortsov1 force-pushed the as/simple-flarum-search branch 2 times, most recently from 9dc049a to a8fbc4e Compare January 19, 2021 23:57
src/Search/SearchServiceProvider.php Outdated Show resolved Hide resolved
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@tankerkiller125 tankerkiller125 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested this code, but I've been following along with it and I see no glaring issues and in my opinion it's just as easy if not easier to read than the original code.

@askvortsov1
Copy link
Sponsor Member Author

Awesome, thanks guys! We'll merge this after the 2 dependency search refactor PRs

@askvortsov1
Copy link
Sponsor Member Author

askvortsov1 commented Feb 10, 2021

Actually looks like this doesn't depend on the search/filter split (or at least I don't believe so), so I'll try merging it tomorrow. That should make review for the search/filter split PR a bit easier since we'll have the search architecture right there to compare.

@askvortsov1 askvortsov1 changed the base branch from as/use-filterer-for-ListPostController to master February 10, 2021 14:50
@askvortsov1 askvortsov1 merged commit 76d6442 into master Feb 10, 2021
@askvortsov1 askvortsov1 deleted the as/simple-flarum-search branch February 10, 2021 14:59
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.

None yet

3 participants