Problem/Motivation
No bundled parser currently supports negative keywords (= find stuff NOT containing the keyword). Negative keywords are pretty standard functionality nowadays starting from Google and ending with Drupal core search that supports negation out of the box. Preceding the negative keyword with a minus sign has become a sort of a de-facto standard that most users would expect (and try).
What's important, the Search API backends currently DO support negative keywords, it's just that no bundled parser would parse minus-preceded keywords into correct array structure for the backends to pick up.
Proposed resolution
A keyword preceded with a minus sign becomes a negative keyword, so that the index items containing the word are excluded from search. You can negate quoted multiple-word keys as well (make sure the minus is before the quote).
I hesitated between creating a new parser and adding the functionality to the existing "Multiple words" parser. Yes, theoretically it changes existing behavior but I still decided to modify the existing parser because:
- the functionality is pretty standard and intuitive and should work "out of the box" in my book. Currently it's one of the very few features that work in the core Search and don't in Search API
- to break some existing web site functionality, the site in question needs to 1) contain some meaningful keywords starting with the minus sign where the minus sign is important (can't come up with a single plausible use case, sorry) and 2) be already set up in a pretty unorthodox way as the current implementation of Tokenizer is hard-coded to remove minus signs from both search query and indexes (which is wrong in my book but that's whole 'nother story).
Remaining tasks
Reviews needed, tests probably needed.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 3125383-13--terms-parser-negation.patch | 6.68 KB | drunken monkey |
Comments
Comment #2
marassa commentedSorry, screwed up with patch numbering - it's the same patch.
Comment #3
marassa commentedComment #4
marassa commentedComment #5
drunken monkeyThanks a lot for proposing this and providing a patch!
You’re right, this is pretty standard functionality, so would be great if we could support that, too.
You’re also right, though, in that this needs tests. For instance, it looks to me like code as-is wouldn’t treat minus signs in quoted phrases correctly. (E.g.,
foo "bar -baz foobar".)Amazingly, though, it doesn’t seem like we currently have any test coverage for this parse mode, even though the code isn’t that trivial. So, all the more important to add test coverage, now. As you are already assigned, would you be able to work on that? Probably something like
\Drupal\Tests\search_api\Unit\TermsParseModeTest, with just a single test method and a data provider covering as many eventualities as possible.Comment #6
marassa commentedThanks for reviewing and finding a bug! The fix attached.
As for the tests, the only problem I foresee is that I have no idea how to write them ;)
I hoped it would only involve tweaking/extending an existing test (and learning something in the process) but now it seems like there's a whole lot more learning involved than I expected.
I am still willing to have a go at it but I'm afraid it's going to take me substantially more than a few minutes...
Comment #7
marassa commentedComment #8
marassa commentedFirst attempt at a unit test.
Comment #9
marassa commentedComment #11
marassa commentedFixed some bugs.
Comment #12
marassa commentedComment #13
drunken monkeyGreat job, thanks a lot once again!
On the whole, this looked pretty great again. I just added a few more test data sets, cleaned up the code and fixed one or two mini-bugs.
However, I then also couldn’t resist taking the chance to clean up the rest of that code, too, as it had various short-comings. (As one is bound to think, years after writing some code.) So, I fear the interdiff and patch file are now rather hard to review, at least for parser changes. Test changes are probably better.
Anyways, would still be great if you could take a look to see whether you find something wrong. Otherwise, I’ll commit.
And thanks again!
Comment #14
marassa commentedThank you, works great for me!
Comment #16
drunken monkeyGood to hear, thanks for reporting back!
Committed.
Thanks a lot again!