I'm trying to implement a "starts with" search when using Search API with database backend. It seems that this happens in Database.php on line 1947:
$db_or->condition('t.word', '%' . $this->database->escapeLike($word) . '%', 'LIKE');
I'm not sure if it's possible to alter the query in a hook or by overriding some method of some class.
But would it make sense to add an option to the Database server itself? Maybe another checkbox under "Search on parts of a word", something like Use "Starts with" to avoid ambiguous results or similar. With that selected the first "%" would be removed from the condition:
$db_or->condition('t.word', $this->database->escapeLike($word) . '%', 'LIKE');
I can provide a patch if this makes sense. Or is there a way to override this per view or index for example?
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 2971033-13--db_backend_starts_with.patch | 12.37 KB | drunken monkey |
Comments
Comment #2
merilainen commentedComment #3
drunken monkeyThanks for proposing this new feature!
I think it would make most sense to change the existing option to radios instead of a checkbox: does the user want whole-word matching, prefix matching or complete substring matching?
We'd need an update hook for that, then, but that should be simple enough. A bit more work would be the test coverage to ensure this works properly, and stays working. But all in all, not that hard, I think, and definitely something a lot of people might want. So, thanks again! If you could provide a patch, that would be great!
For doing this in a custom module, you could probably just use
hook_search_api_db_query_alter()to alter the query accordingly. Would need a bit of searching, though, to reliably find all such fulltext conditions – our queries tend to get pretty complex.Configuring this per-view or per-index would be a lot more work, that's why we also don't allow this for the current "partial matching" setting.
It would definitely be possible in some way, but I'd stay with this for now, unless you really need it.
(Per-index would be trivial to do already, though, by just having one search server per index. For a DB server, there's more or less no additional cost involved for creating multiple servers, after all.)
Comment #4
merilainen commentedI was thinking about the radio button element too, but it seems it would be a lot easier to implement an additional (conditional) checkbox element. It wouldn't break any exiting configuration or tests and changes to code would be minimal. Maybe logical naming would make it usable? Something like Match only words starting with given keywords.
Comment #5
merilainen commentedHere is a first stab with the additional checkbox approach, seems to work. Didn't include the condition for the checkbox yet, but that should be trivial.
Comment #6
merilainen commentedHere is a patch with the conditions for the form elements. Also added starts_with to the Database search server schema.
Comment #7
merilainen commentedFixed double key, doh.
Comment #8
drunken monkeyThanks a lot for the patch, looks pretty good!
However, in general, when posting new revisions of an existing patch, please include an interdiff. (It wasn't really necessary here, since I didn't look at the issue until the last patch was there, but probably still better to be on the safe side.)
Changing less code and not requiring and update hook are of course good arguments. However, the UI does not have to match the internal configuration – we can just have three radios in the UI, but use the same two boolean config keys as you currently do internally. We just need a
submitConfigurationForm()implementation to bridge the two.So, I don't know what the third follower's take on this is, but I think I would prefer radios there.
Also, an update hook and a few changed config files (which you already changed anyways) and tests wouldn't be that bad, either – and maybe not as confusing for developers.
I'm not sure which I prefer, to be honest.
In any case, disabling the first option once the second is enabled isn't a pattern we normally use, I think. When the user unchecks "Partial matching" and the second option gets hidden, that should usually be enough to clue them in that only enabling the second option doesn't work.
But, as said, please change to radios, unless you're strongly opposed.
Also,
viewSettings()could be adapted to show the matching strategy in a single item in any case. That's completely independent of the UI or the internal options.NOOO, it breaks the pretty pattern! T___T
But since that variable is also just used once, we might as well inline it … (Phew!)
And, finally, we'll still need test coverage for this new feature.
Just a few lines in
\Drupal\Tests\search_api_db\Kernel\BackendTestshould suffice – likeeditServerPartial()/searchSuccessPartial(), maybe just right after those.Comment #9
drunken monkeyComment #10
merilainen commentedI changed the form now to use radio buttons, added submitConfigurationForm() and changed viewSettings() to a single item. Also added editServerStartsWith() to BackendTest.php but I'm not familiar with writing tests so I would need some help with adding the test for that "starts with" matching. Included an interdiff this time too.
Comment #11
drunken monkeyThanks a lot, looks great!
I think we can keep the code a bit more concise, though – especially in
submitConfigurationForm()– see the attached patch. (Your version also failed to set all the other config values, or call the parent method.)This, by the way, also adds the required test method – I just copied
searchSuccessPartial()and adapted the expected return values and/or keywords based on the different semantics. (Worked surprisingly well, I didn't even need to consult\Drupal\Tests\search_api\Functional\ExampleContentTrait::insertExampleContent()on the actual contents of the indexed test entities.)Looking at the code, though, I begin to think we should really go with the string config value and update function – I'll post an adapted patch soon.
Comment #13
drunken monkeyAnd here is the promised adapted version with just a single option value.
I think we'll be happier with that going forward, since otherwise it's always a bit of a mental challenge converting between the UI options and the internal config values.
Anyone else want to weigh in on this?
I'd also be interested in other people's opinions of the radio button labels:
(The test "fail" is a testbot bug: #2973992: Permission issue in Nightwatch step marks all full testruns as unstable.)
Comment #15
merilainen commentedThis looks good to me and thanks for quick response! I won't mark this RTBC if you want to get a review from a fresh pair of eyes.
Comment #16
drunken monkeyYes, another review would be great, it is a larger change.
Comment #17
drunken monkeyComment #18
borisson_I haven't manually tested this, but the code looks great. Setting to rtbc based on that + the review in #15.
Comment #20
drunken monkeyGreat to hear, thanks a lot for reviewing!
Committed.
And thanks a lot again, mErilainen!