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.
Comments
Comment #2
Tomefa CreditAttribution: Tomefa as a volunteer commentedPatch added.
Comment #3
Tomefa CreditAttribution: Tomefa as a volunteer commentedComment #4
Tomefa CreditAttribution: Tomefa as a volunteer commentedComment #5
Tomefa CreditAttribution: Tomefa as a volunteer commentedComment #6
drunken monkeyThanks 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:
$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.
Comment #7
drunken monkeyOops.
Comment #10
drunken monkeyTest fail was a fluke, this is still up for testing/reviewing.
Comment #11
drunken monkey@Tomefa: Could you please test the patch so I can commit?
Comment #12
Tomefa CreditAttribution: Tomefa as a volunteer commentedTest the patch, and the excerpt is now actually better than with the previous patch.
Great work @drunken-monkey
Comment #14
drunken monkeyGood to hear, thanks a lot for reporting back.
Merged. Thanks again!