Allow excerpts to be generated in the case of an empty querystring

This looks good! We do need some tests for this new functionality. I did find some nits to pick.

1. +++ b/src/Plugin/search_api/processor/Highlight.php
   @@ -236,8 +243,14 @@ class Highlight extends ProcessorPluginBase implements PluginFormInterface {
   +    // Do not return an excerpt on an empty keyword if not requested by configuration.
   

   > 80 cols.

2. +++ b/src/Plugin/search_api/processor/Highlight.php
   @@ -459,6 +472,14 @@ class Highlight extends ProcessorPluginBase implements PluginFormInterface {
   +    // If no keys are given, return an excerpt from the beginning.
   

   I think this documentation can/should be improved.

   // When we don't have keys to search on, the excerpt should start at the beginning of the content.

Started writing browser tests.
Had some trouble properly getting the dummy content to index.

I also added a testing module (search_api_test_excerpt), setting up the view showing excerpts.

WIP patch and interdiff patch included.

This one still needs some work wrt caching, for steps to reproduce see this issue https://github.com/nickveenhof/drupal8-umami-search/issues/9

Note that:
- max-age doesn't bubble - see https://www.drupal.org/project/drupal/issues/2352009
- instead of using max-age we may want to provide a cache context (that has a lot of variations) - see https://www.drupal.org/project/drupal/issues/2232375

Thanks for posting!

To be honest, my immediate reaction was “too specific, can easily be done in custom code”. But as it's little code, it's already more or less done and you're even working on providing tests – sure, we can add this.
However, wouldn't a fallback to, say, the teaser (so, the item rendered with some view mode) be more practical? (On the other hand, this could easily be a follow-up, if enough people demand it and/or are willing to work on it.) That's how I would have imagined that feature at least, reading the initial description.

Apart from Joris' nit-picks, I just have one comment for now:

+++ b/src/Plugin/search_api/processor/Highlight.php
@@ -459,6 +472,14 @@ class Highlight extends ProcessorPluginBase implements PluginFormInterface {
+      $excerpt = implode($ellipses[1], [$out]) . $ellipses[2];

Won't the implode() just always return $out unchanged? I.e., isn't that completely useless?

Working on it.

Unassign, worked on another issue.

Alright, I have tested patch from comment #9, and it works for me as described. Also works correct with enabled new option in Highlight processor and disabled. And new option "excerpt_always" correctly exporting to configs.

This patch resolves the problem described. Slightly related, for results on phonetic matches, no keywords match but we could still show an excerpt. Could this patch be written in a way that it always returns an excerpt, in case of no keywords, or no keyword matches?

I've added a slight modification of patch #11, it now (if enabled) always renders an excerpt in the case no keywords are in the query, or no keywords match the returned excerpt. I believe anyone enabling this feature simply always wants excerpts!

Missing patch in #18.

1. +++ b/src/Plugin/search_api/processor/Highlight.php
   @@ -553,9 +566,20 @@ class Highlight extends ProcessorPluginBase implements PluginFormInterface {
   +    ¶
   

   Extraneous whitespace

2. +++ b/tests/src/Functional/EmptyQueryStringExcerptTest.php
   @@ -0,0 +1,99 @@
   + * Tests the option to get excerpts when there is an empty query string
   

   Missing a period at the end of the comment.

3. +++ b/tests/src/Functional/EmptyQueryStringExcerptTest.php
   @@ -0,0 +1,99 @@
   +  /**
   +   * @var \Drupal\search_api\ServerInterface
   +   */
   +  protected $server;
   +
   +  /**
   +   * @var \Drupal\search_api\IndexInterface
   +   */
   +  protected $index;
   

   These should have a description

4. +++ b/tests/src/Functional/EmptyQueryStringExcerptTest.php
   @@ -0,0 +1,99 @@
   +  /**
   +   * Tests the functionality with excerpt_always disabled
   +   */
   +  public function testExcerptAlwaysDisabled() {
   +    // @TODO
   +  }
   

   This obviously needs work ;)

5. +++ b/tests/src/Functional/EmptyQueryStringExcerptTest.php
   @@ -0,0 +1,99 @@
   +   * Tests the functionality with excerpt_always enabled
   

   Missing period at the end of this comment.

6. +++ b/tests/src/Functional/EmptyQueryStringExcerptTest.php
   @@ -0,0 +1,99 @@
   +    // Set the 'excerpt_always' setting
   

   Missing period

7. +++ b/tests/src/Functional/EmptyQueryStringExcerptTest.php
   @@ -0,0 +1,99 @@
   +    // Reindex
   +    $this->index->indexItems();
   

   This comment doesn't add anything, $this->index->indexItems(); basically says the same thing.

Thanks for everyone’s work in here. I agree with Legolasbo’s comments, but otherwise this looks pretty good already!
Adding a functional test, in particular, is a lot more than what I’d have hoped for. It even seems a little overkill for such a small feature, but sure, since it’s already there, mostly – why not? We just should also add a short test method for this to the existing HighlightTest.

Hi,

I fixed the little mistakes about periods and comment and check the test. The testExcerptAlwaysEnabled didn't be finished (it didn't check anything) but I'm sure that somewhere I have an incomplete code because the view (from $this->drupalGet('search-api-test-search-excerpt')) is empty. I mean, I debugged it and any content is show but I don't know how to finish it.

The testExcerptAlwaysDisabled method (that was empty) I'm not sure the correct approach (I'm so new with tests) but if someone can help me I will try!

Thanks in advance.

+++ b/src/Plugin/search_api/processor/Highlight.php
@@ -236,8 +243,14 @@ class Highlight extends ProcessorPluginBase implements PluginFormInterface {
+    $keys = $this->getKeywords($query);
+    // Do not return an excerpt on an empty keyword if not requested by configuration.
+    $excerpt_always = $this->configuration['excerpt_always'];
+    if (!$excerpt_always && !$keys) {

+ // Only return an excerpt on an empty keyword if requested by configuration.
+ $keys = $this->getKeywords($query);
+ $excerpt_always = $this->configuration['excerpt_always'];
+ if (!$excerpt_always && empty($keys)) {

Put the comment first;
Use active language without double negations;
Use empty() cfr line #479;

Fixed #25 comment.

I had to change the original view, I don't know why but the view doesn't run as expected. The view is always empty with and without the excerpt checkbox.

This is an screenshot of the result of the test view:

However, I replicated the view in a real environment (installing the module) and the view shows correctly two rows with the labels that I then look up in the code (to test the behaviour)

I don't know where is the mistake because the previous step in the test seems correct (The index has 2 different element indexed), It seems that the problem is in the test view. Can someone give me a hand with this? Thanks!

Several hours after all... I think that now it works.

I found some mistakes in the code, one in the view (the query type didn't seem correct) and one in the test (the test entity did not a bundle in the index process and the index doesn't work). So, I'm not sure if both changes are necessary but I think that yes.

The code now works as expected and the test works too. The changes are based on the ViewsTest.php code, but please review it and add comments if something is wrong.

The thing I could find is an übernit, I am really sorry about that. I am super proud of you for sticking with this patch, you did a really good job at it. Thank you so much.

+++ b/tests/src/Functional/EmptyQueryStringExcerptTest.php
@@ -54,15 +41,14 @@
+    ¶

This adds new indendation where it is not needed.

estoyausente++
borisson_ lovely übernit :D

Comments addressed in #29 & added an interdiff as well.

Removing the needs tests tag. Thanks!

First of all, thanks a lot for moving this along! Definitely further improvements, thanks for that!
There were several coding style and grammar problems, which should be fixed in the attached patch.

1. +++ b/src/Plugin/search_api/processor/Highlight.php
   @@ -554,8 +567,19 @@ class Highlight extends ProcessorPluginBase implements PluginFormInterface {
   +        $excerpt = implode($ellipses[1], [$out]) . $ellipses[2];
   

   See #13 – the implode() doesn’t do anything here. Fixed that, now, too.

2. +++ b/tests/src/Functional/EmptyQueryStringExcerptTest.php
   @@ -0,0 +1,127 @@
   +    // The text is label field in the view that is hide if the field
   +    // doesn't exist. The Excerpt value without any query string is always
   +    // "..." and is a bad string to search.
   +    $this->assertSession()->pageTextNotContains('Excerpt_label');
   

   Do I understand this correctly that the excerpts displayed in the test are actually empty?
   Doesn’t that mean that the exact opposite of what we want to test is true – namely, that the feature doesn’t work?

   (It also might mean that we want to make sure that the excerpt we generate won’t consist of just one ellipsis. Added that to the code, too.)

Finally, as stated in #21, this should also have a test method in the existing HighlightTest – which should be a lot simpler to implement than the functional test anyways.

Forgot to attach the patch, of course.

Aaand back to NW for the tests.

Posted an unfinished version of the patch, these are my last few edits (which I also talked about in the comment).

Just saying that I applied the patch and worked fine for me.
Once I get some time I will try to focus on your comments at this patch

Wanted to chime in. This is a useful patch and it applied well and works.

I changed the way the substring is created, to support languages with multi-byte characters, like greek, bulgarian etc.. in accordance to the issues that tackled such problems in the past.
For reference:
https://www.drupal.org/project/search_api/issues/2979316
https://www.drupal.org/project/search_api/issues/2867841

Otherwise the patch does what it says.
@dunken money What else is there left to be implemented for this?

Reuploading #41 with the D9 readiness key in the test module's info yml

@ Marios Anagnostopoulos: Thanks a lot for catching that! Would have been embarassing to introduce a multi-byte bug yet again in 2021 …

To finally get this over the finish line, I now also wrote a unit test.
Furthermore, I think in this case, like in the others, we don’t want to end the snippet in the middle of a word? Fixed that, too.

Applying #44 did not introduce any problems so far in my case. +1 to RTBC

Thanks for the feedback, great to hear!
Committed.
Thanks again, everyone!