In the course of #2253237: Add proper classes for indexed items and search results, I have revamped the highlighting processor to use extra data on the result set object for passing the highlighted field data instead of overwriting the field values directly. View should be updated to (optionally) use this data when displaying fields of search results. (Doing the same when rendering whole entities/result objects would probably be a lot more difficult.)

CommentFileSizeAuthor
#9 2278433-9--views_highlighted_fields.patch37.88 KBdrunken monkey
#9 2278433-9--views_highlighted_fields--interdiff.txt5.9 KBdrunken monkey
#7 2278433-7--views_highlighted_fields.patch38.79 KBdrunken monkey
#7 2278433-7--views_highlighted_fields--interdiff.txt4.67 KBdrunken monkey
#4 2278433-4--views_highlighted_fields.patch35.28 KBdrunken monkey
#4 2278433-4--views_highlighted_fields--interdiff.txt11.95 KBdrunken monkey
#2 2278433-2--views_highlighted_fields.patch24.01 KBdrunken monkey
#2 2278433-2--views_highlighted_fields--ViewsPropertyExtractionTest--interdiff.txt8.22 KBdrunken monkey

Comments

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Issue tags: +release target
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Active » Needs review
Issue tags: +Vienna2017, +Needs change record
StatusFileSize
new 2278433-2--views_highlighted_fields--ViewsPropertyExtractionTest--interdiff.txt8.22 KB
new 2278433-2--views_highlighted_fields.patch24.01 KB

I finally have an implementation that looks like it might be working, plus some test coverage. Haven't actually tested it yet, though, since I'm on my laptop without an installation running. (Could probably also use an integration test, too.)
Probably the largest unknown: Whether this correctly bypasses Views sanitization for the highlighted field values (which, of course, contain HTML).
Anyways, please test/review!

Since I also noticed that we actually specified two ways to add the highlighted fields data, only one of which makes sense (and our Highlighting processor was actually using the other one), we should probably also add a change record for this.

Oh, and the tests build on those for #2911734: Passing processor-generated field values from search results to Views is broken – that's what the interdiff is for.

Status: Needs review » Needs work

The last submitted patch, 2: 2278433-2--views_highlighted_fields.patch, failed testing. View results

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs work » Needs review
StatusFileSize
new 2278433-4--views_highlighted_fields--interdiff.txt11.95 KB
new 2278433-4--views_highlighted_fields.patch35.28 KB
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

The code flows very clearly and I really like how it looks. Not sure about the part where we merge both arrays but looking at the comments you're not very confident about that either. No idea how to improve it though.

Nits:

  1. +++ b/src/Plugin/views/field/SearchApiFieldTrait.php
@@ -250,6 +261,13 @@ trait SearchApiFieldTrait {
+      '#title' => $this->t('Use highlighted field data, if available'),
+      '#description' => $this->t('Display field with matches of the search keywords highlighted, if available.'),

    I don't think we need to say "if available" in both the title and description, just in the description is enough I think. 

  2. +++ b/src/Plugin/views/field/SearchApiFieldTrait.php
@@ -840,28 +859,164 @@ trait SearchApiFieldTrait {
+   *   The Views result rows for which highlighted fiale values should be added

    /s/fiale//?

Status: Needs review » Needs work

The last submitted patch, 4: 2278433-4--views_highlighted_fields.patch, failed testing. View results

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs work » Needs review
StatusFileSize
new 2278433-7--views_highlighted_fields--interdiff.txt4.67 KB
new 2278433-7--views_highlighted_fields.patch38.79 KB

AS always, thanks a lot for your review!

The code flows very clearly and I really like how it looks. Not sure about the part where we merge both arrays but looking at the comments you're not very confident about that either. No idea how to improve it though.

Yeah, that part is pure mindfuck. I'm pretty sure it works as it should, though, and I don't think we can do without it either, so nothing for it. I hope at least the comments help understand why it's there and what it does.

I don't think we need to say "if available" in both the title and description, just in the description is enough I think.

I agree, thanks.

Status: Needs review » Needs work

The last submitted patch, 7: 2278433-7--views_highlighted_fields.patch, failed testing. View results

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs work » Needs review
StatusFileSize
new 2278433-9--views_highlighted_fields--interdiff.txt5.9 KB
new 2278433-9--views_highlighted_fields.patch37.88 KB

OK, I now tested this and, as feared, Views escaped our highlighting HTML. This should be fixed with this revision, which also fixes the tests and provides a new browser test for this functionality (making sure all components play together correctly).

Also: here's the change record.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

Anyone want to test/review? Otherwise I'll just commit it.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

I've reviewed the interdiffs and those look solid. Great work @drunken monkey!

  • drunken monkey committed 3c7295a on 8.x-1.x 
    Issue #2278433 by drunken monkey, borisson_: Added option to Views field...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

Great to hear, thanks a lot for reviewing!
Committed. (And even remembered to publish the release note!)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.