Problem/Motivation
On #3075695: Move search_index() and related functions to a service, in the last couple of hours before it was committed, a change was rushed through that left Search module indexing in an inefficient state.
This should be fixed before the 8.8 release IMO.
Two problems:
a) The plugins that are calling ->index() are not passing the $needs_update in as FALSE, but they are still calling updateWordWeights(). For example, here is HelpSearch from that patch:
+ $words += $this->searchIndex->index($this->getType(), $item->sid, $langcode, $text);
+ }
}
}
}
+ finally {
+ $this->searchIndex->updateWordWeights($words);
+ }
So HelpSearch is keeping track of $words and calling updateWordWeights, but because it didn't pass in FALSE as the last argument, the ->index() function is updating itself:
+ public function index($type, $sid, $langcode, $text, $update_weights = TRUE) {
...
+ finally {
+ if ($update_weights) {
+ $this->updateWordWeights($current_words);
+ }
+ }
So, updateWordWeights() is being called on every index() call, and then again at the end. Inefficient and redundant.
b) The docs in index() telling you what function to call when you do pass in FALSE were removed in one of the interdiffs above. So people looking at SearchIndexInterface() don't have an explicit message that if they do pass in FALSE saying how they should update the word weights later.
Proposed resolution
a) Fix HelpSearch and NodeSearch to do the right thing with calling index() so it is more efficient.
b) Update the docs on SearchIndexInterface
Remaining tasks
Make a patch.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
None needed.
Comment | File | Size | Author |
---|---|---|---|
#2 | 3087407.patch | 2.28 KB | jhodgdon |
Comments
Comment #2
jhodgdonHere's an untested patch -- let's see what the bot says.
Comment #3
andypostI think RTBC but would be great to get another opinion
Comment #4
jhodgdonNote: In Slack @jibran suggested this needs a test.
My thought is that if we pass in FALSE (as in this patch) and the existing tests pass, then we know that everything is working correctly. I don't know of a way to have a test that we aren't being redundant in our calls to ->index() and ->updateWordWeights in both NodeSearch and HelpSearch plugins.
Comment #5
kim.pepperThanks for the quick fix.
Comment #6
andypostComment #7
andypostsure rtbc++
Comment #9
larowlanthanks - we should open a followup to remove the pass by reference here too (out of scope here)
Committed 1c4e7f7 and pushed to 9.0.x. Thanks!
Comment #11
andypostFiled follow-up #3087661: Remove pass by reference in NodeSearch plugin