Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
When the returned excerpt contains multiple highlighted words, the blank space between two highlighted words is accidentally removed. So the words are concatenated.
Example:
Presentation -Session 5 - [HIGHLIGHT]Addressing[/HIGHLIGHT] [HIGHLIGHT]emerging[/HIGHLIGHT] health threats and availability/therapeutic challenges
becomes
Presentation -Session 5 - Addressingemerging health threats and availability/therapeutic …
Proposed resolution
Blank space is at the end of the first snippet parts. "Presentation -Session 5 - [HIGHLIGHT]Addressing[/HIGHLIGHT] "
But removed by the usage of trim. $snippet = trim($snippet, "\00..\x2F:;=\x3F..\x40\x5B..\x60");
Then the snippet parts are concatenated by this code:
// Prepend the previous snippet if we're fusing.
if ($previous) {
$snippet = $previous . $snippet;
}
I suggest to add a ' ' (blank space) in between the concatenated snippet parts.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 3074587-4--excerpt_snippets_generation_overhaul.patch | 4.88 KB | drunken monkey |
| #4 | 3074587-4--space_between_excerpt_snippets.patch | 787 bytes | drunken monkey |
Comments
Comment #2
michel.settembrino CreditAttribution: michel.settembrino at European Commission and European Union Institutions, Agencies and Bodies commentedComment #3
michel.settembrino CreditAttribution: michel.settembrino at European Commission and European Union Institutions, Agencies and Bodies commentedComment #4
drunken monkeyThanks a lot for reporting this, and for your patch!
I can reproduce the problem and confirm that your patch fixes it. However, I spotted a few more niche cases in which snippet generation wouldn’t lead to the desired results. I first just amended a second mistake (see second patch and interdiff), but then chose to just rewrite the whole method to, hopefully, make more sense and eliminate a bunch of bugs in one go. (Hopefully not introducing any new ones, either.)
It would be great if you could test whether both patches provide correct excerpts for you in all cases (especially, of course, in the cases where you had problems before). An additional patch review would be even greater, of course.
Our lack of test coverage in D7 unfortunately means we can’t be really confident that such a rewrite of a whole method (but, indeed, also smaller changes like yours) don’t introduce new bugs, but as long as we test the main scenarios beforehand we should probably be fine.
Comment #5
michel.settembrino CreditAttribution: michel.settembrino at European Commission and European Union Institutions, Agencies and Bodies commented@drunken monkey,
Thanks a lot to work on this issue!
I tested your patches and I can confirm both of them works fine for me.
Regards,
Michel.
Comment #7
drunken monkeyGreat to hear, thanks a lot for testing!
Committed.
Thanks again for your work on this!