Problem/Motivation

We were refactoring our list of fields to use only a few aggregate one and manage more easily the boost value like that.
So we start by adding some aggregate fields:
- for date
- for title
- for text

And like before, we want to have the Excerpt text of the search.
When removing the old fields and leaving only the aggregate fields, we didn't get any more the Excerpt from the result list.

We test many configuration from the Search API backend, also on Solr and on fields.
Then the next step is to check in code, and we found the problem to be in "src/Utility/FieldsHelper.php",
function "extractItemValues()".
In src/Utility/FieldsHelper.php line 245, the field are listed and to get the correct field, the property path is used.
But for aggregate fields, they are all with same datasource id and property path:
#datasourceId: null
#datasource: null
#propertyPath: "aggregated_field"

So it will just return the field aggregate field that is define and not the correct one in our case because aggregate for text is in 3 position.

Steps to reproduce

Get a drupal with search api and search api solr working on a test environment.

Create a media entity and a content entity with both title and text fields. And add both entity on the search api index.

Create 2 aggregates fields:
- the first on on "changed" date for media and content
- the second on the body text for media and content

Enable the excerpt on search api and the processor Highlight so it always return value.

Create a view of the search result page to display the entity list, with a full text input field has a filter.
Display the excerpt text in the result list.

Search on some text that are in the media and/or content and in the result list, no excerpt will be visible.

If you remove the "changed" aggregate field, excerpt will appear because the first aggregate field is now the body.
Or if you add the body text separately on media and content in the field list of the index, it will appear because it will use this fields for the excerpt.

Proposed resolution

Use also the Field Identifier in the problematic function so we really are using the correct aggregate field.

Problem in:

          // If a field with the right property path is already set on the item,
          // use it. This might actually make problems in case the values have
          // already been processed in some way, or use a data type that
          // transformed their original value. But that will hopefully not be a
          // problem in most situations.
          foreach ($this->filterForPropertyPath($item->getFields(FALSE), $datasource_id, $property_path) as $field) {
            $item_values[$combined_id] = $field->getValues();
            continue 2;
          }

Would be good to check if this problem is only in this class or in others places too.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Tomefa created an issue. See original summary.

Tomefa’s picture

Tomefa’s picture

Issue summary: View changes
Tomefa’s picture

Issue summary: View changes
Tomefa’s picture

Assigned: Tomefa » Unassigned
drunken monkey’s picture

Thanks a lot for reporting this problem, and even finding a pretty good fix. Sorry it took me a while to get back to you, I had an insane backlog of issues.

In any case, I still see two problems with your patch:

  • It only addresses the case when field values are already set on the result items (e.g., because the Solr backend was used with corresponding configuration), not when the values need to be newly extracted.
  • That $combined_id must match the field ID is actually not a requirement, and thus not necessarily the case. When fields are extracted for Views, for instance, I don’t think this holds. (Since Views also extracts values of properties that aren’t even fields on the index.) Therefore, this new code should only be used as a tie breaker if more than one field matches the given datasource and property path.

The attached patch should fix both of those problems and also includes a regression test. Please test/review!
And, again, thanks for your work!

PS: It seems you (like many others – it's really easy to misinterpret) are confused by the "Issue tags" field. As the guidelines state, they aren't meant for free text tags related to the issue, but only for specific categorization purposes, usually by module maintainers.
So, if you aren't sure your current usage is correct, please just leave the field empty.

The last submitted patch, 7: 3291409-7--excerpt_with_multiple_aggregated_fields--tests_only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 7: 3291409-7--excerpt_with_multiple_aggregated_fields.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

drunken monkey’s picture

Status: Needs work » Needs review

Test fail was a fluke, this is still up for testing/reviewing.

drunken monkey’s picture

@Tomefa: Could you please test the patch so I can commit?

Tomefa’s picture

Status: Needs review » Reviewed & tested by the community

Test the patch, and the excerpt is now actually better than with the previous patch.

Great work @drunken-monkey

  • drunken monkey committed a9328f9d on 8.x-1.x
    Issue #3291409 by drunken monkey, Tomefa: Fixed excerpts generated from...
drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Good to hear, thanks a lot for reporting back.
Merged. Thanks again!

Status: Fixed » Closed (fixed)

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