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.

Comments

marassa created an issue. See original summary.

marassa’s picture

StatusFileSize
new1.28 KB

Sorry, screwed up with patch numbering - it's the same patch.

marassa’s picture

StatusFileSize
new1.28 KB
marassa’s picture

drunken monkey’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks 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.

marassa’s picture

StatusFileSize
new1.29 KB

Thanks 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...

marassa’s picture

marassa’s picture

StatusFileSize
new3.31 KB

First attempt at a unit test.

marassa’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: 3125383-8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

marassa’s picture

StatusFileSize
new3.31 KB

Fixed some bugs.

marassa’s picture

Status: Needs work » Needs review
drunken monkey’s picture

Issue tags: -Needs tests
StatusFileSize
new6.78 KB
new6.68 KB

Great 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!

marassa’s picture

Status: Needs review » Reviewed & tested by the community

Thank you, works great for me!

  • drunken monkey committed a2f894c on 8.x-1.x authored by marassa
    Issue #3125383 by marassa, drunken monkey: Added negation support to the...
drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Good to hear, thanks for reporting back!
Committed.
Thanks a lot again!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.