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

Fix registering custom searchers, allow searchers without fulltext #2755

Merged
merged 11 commits into from
Apr 19, 2021

Conversation

askvortsov1
Copy link
Sponsor Member

@askvortsov1 askvortsov1 commented Apr 4, 2021

Fixes #2712

Changes proposed in this pull request:

  • Fix registering custom searchers
  • Add integration tests
  • Require FulltextGambit as constructor arg for GambitManager

Confirmed

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

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.

if there doesn't exist a fulltext gambit for a model, then it's not really "searchable", and as such, a filterer should be used.

I agree, extension developers might try to use gambits when what they really need is filters instead. Throwing an exception somewhere -if we can- that makes it clear would be great as well.

@askvortsov1 askvortsov1 requested a review from SychO9 April 8, 2021 05:33
Copy link
Member

@clarkwinkelmann clarkwinkelmann left a comment

Choose a reason for hiding this comment

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

I have not tested locally, but this looks good to me.

I was worried that gambits might be resolved too early, but that doesn't seem to be an issue. The array is resolved in boot, but the actual classes are only container->make() inside of the container binding so it's fine.

My only issue is the comment below.

src/Search/SearchServiceProvider.php Outdated Show resolved Hide resolved
@askvortsov1 askvortsov1 force-pushed the as/fix-registering-custom-searchers branch from 6c989ae to dc07338 Compare April 9, 2021 00:12
@askvortsov1 askvortsov1 force-pushed the as/fix-registering-custom-searchers branch from 3de8fc9 to b10af17 Compare April 9, 2021 01:01
[ci skip] [skip ci]
@askvortsov1
Copy link
Sponsor Member Author

Reverted the exception as I was unable to catch it.

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.

Had a similar problem with trying to catch an exception from tests, it just wasn't possible.

@askvortsov1 askvortsov1 merged commit 5e2340b into master Apr 19, 2021
@askvortsov1 askvortsov1 deleted the as/fix-registering-custom-searchers branch April 19, 2021 20: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.

Extensions cannot register custom Searcher classes
4 participants