The "new results" check in \Drupal\search_api_saved_searches\Service\NewResultsCheck::getNewResults() currently uses a query without limits. While this will be fine most of the time, it can lead to out-of-memory errors in extreme cases, or at least to undesirably long e-mails in others.
But if we add a limit, we might as well make it configurable (in the type settings) right away, I'd say. After all, it will vary from site to site what "undesirably long" means, or what amoun of new results will overflow the available memory.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | 2955582-8--max_results_option.patch | 15.67 KB | drunken monkey |
Comments
Comment #2
drunken monkeyComment #3
drunken monkeyComment #4
dakwamineHello. :) Just wanted to say that it is indeed an interesting little feature, especially in edge cases such as when a large data set has been recently imported and a lot of those data matches the search criteria. For now, we have added a hard limit in code, but a config would have been useful.
Comment #5
drunken monkeyI realized that there are actually two facets to this: The maximum number of results retrieved from the server (too many might overload either the search or web server) and the maximum number of results sent to the user (too many might make the e-mail unreadable).
In case the new results are determined based on some date field, the two are actually the same, but in case of “Determine via result ID” we actually need to retrieve (potentially a lot) more results from the server than the new ones we report to the user. So, for this latter case, it would probably be best to provide two separate options to admins.
This is now implemented in the attached patch. Would be great if someone could test/review, especially regarding the UI. (Though that, on the other hand, is easiest to change later on. (Translators might disagree, however.))
What we could also think about is some way to detect if not all results have been sent to the user (i.e., number of result items is less than the result count on the search results) and allow admins to add an additional line to the e-mail in this case. But probably a follow-up issue.
Comment #7
alesbencina commentedthanks for the patch!
I've tested it and looks ok (was on dev branch)
Comment #8
drunken monkeyHad another positive feedback directly, just need to fix tests. If the attached passes, I’ll commit.
Comment #10
drunken monkeyCommitted.
Thanks a lot for testing!