Values overwritten when multiple aggregated fields exist on same data source

Thanks a lot for reporting this problem and already providing a patch! My apologies that you had to wait so long for a response – didn’t manage to have much time for contrib in the last two months.

You are right, it makes sense that this is not only a problem for Views, but also for other places where we extract property values. (Though, at a glance, at least FieldsHelper::extractItemValues() only seems to be used for highlighting and for aggregated fields in such a broad manner.)
Your approach for solving this problem also seems sensible to me. As currently no code should be passing property paths containing | characters to FieldsHelper::extractItemValues(), this should also not break anything. We should just clearly document this capability in the method’s documentation. (Also, we might even want to create a change record for this? Might be overkill, though, for such a detail, that also shouldn’t break anything.)
However, when you already pass the field ID as part of the property path, you should just also use it in FieldsHelper::extractItemValues() – patch revision attached. This attached revision also takes care to catch any exception thrown in the newly added Highlight code.

In any case, as said, this looks like a sensible approach to me. However, more feedback from others would be very much appreciated. (Maybe I’ll add a hint to the next release notes …)
Also, tests for this new functionality are probably necessary before I can commit this – optimally both for FieldsHelper directly and for the usage in Highlight. Would you be able to work on that? (Tests should already exist for both methods/classes, you’d just have to add appropriate new data sets or test methods, probably with a bit of additional setup.)
Also, of course, please test/review my patch to see if it still works for you! (Sorry, no interdiff, as it was basically useless with all the whitespace changes.)

(Side note: By cirumventing the UI and just placing the appropriately suffixed property paths directly into the processor config via YAML (or similar), it seems this patch would even support selecting a specific field when aggregating configurable properties in aggregated fields.)

(Setting back to NW for the tests.)

I have tested the #7 patch. And it might be me but the highlighting didn't work like it used to.

After a lot of debugging, it seems like if the property path does not contain the separator, the variable property_path will be null and the field_id gets the value of the propery_path.
list ($property_path, $field_id) = Utility::splitPropertyPath($property_path, TRUE, '|');

I changed the arrangement of the property_path and the field_id, to prevent an empty property_path.
list ($field_id, $property_path) = Utility::splitPropertyPath($property_path, TRUE, '|');

I'm not sure if this is the correct solution, but the highlighting works for me with this approach.

I have also fixed some tests in the HighlightTest class. They should now pass again, except the testFieldExtraction-Test. I was not able to fix it but I will have a look on it again.

I would appreciate any feedback and suggestions for improvement.
Best Regards

Thanks a lot for chiming in here and helping polish this patch!

I changed the arrangement of the property_path and the field_id, to prevent an empty property_path.list ($field_id, $property_path) = Utility::splitPropertyPath($property_path, TRUE, '|');

You’re right that this line was buggy, thanks for finding that! I’m afraid, though, that your solution isn’t quite correct, either. I think I’ve got it now, though – please check the attached patch.

I also tried fixing the HighlightTest::testFieldExtraction() test, but didn’t manage it completely. It seems it gets a NULL value somewhere while drilling into the bar:foo nested property. Could you please debug further?

Still, in any case, this will also need an additional test (or another property in testFieldExtraction()) to make sure that the problem we’re trying to solve here is actually fixed. (Probably another method, though, to keep things clearly separated. testFieldExtraction() is complicated enough as it is.

I was able to get the patch to mostly apply to the current version of 8.x-1.35 and it seemed to have resolved the issue.

Attaching the patch that I updated. There are some changes to the FieldHelper utility that didn't apply cleanly but it looks like everything is working. So I'm not sure if another issue might of incorporated some of those changes.

I think this is the correctly rebased version of the patch.
Anyways, we use MRs for development now, so I created an MR containing that patch.
Still needs tests.

Hello,
I am wondering whether it makes sense to add the item language for the missing fields. Otherwise there might be uses cases were the returned values correspond to the "original" translation.
For example (Solr scenario):

1. Search index: Indexing auto_aggregated_fulltext_field (body and tags). Where tags are terms referencing vocabulary tags (translatable terms).
2. Views configured fulltext for: auto_aggregated_fulltext_field.
3. Highlight processor: configured to show body and tags.
4. Searching in the translated language might not show the translated string/excerpt.

This:

      if ($missing_fields) {
        $this->extractFields($item->getOriginalObject(), $missing_fields);
        foreach ($missing_fields as $property_fields) {
          foreach ($property_fields as $field) {
            $item_values[$field->getFieldIdentifier()] = $field->getValues();
          }
        }
      }

Becomes that:

      if ($missing_fields) {
        $this->extractFields($item->getOriginalObject(), $missing_fields, $item->getLanguage());
        foreach ($missing_fields as $property_fields) {
          foreach ($property_fields as $field) {
            $item_values[$field->getFieldIdentifier()] = $field->getValues();
          }
        }
      }

Since i am working using a custom patch combining #3031390: Excerpt shows all fields even when only one field has the searchterm and this issue's patch, I wonder if using $item->getLanguage() for missing fields can break some code that I am not aware of.

I've added the logger service in the setUpMockContainer() along with a method in the HighlightTest.php for creating the needed FieldItemDataDefinition.

So now we have this function createFieldItemDataDefinition for testFieldExtraction.php, which avoids the error:

Drupal\Tests\search_api\Unit\Processor\HighlightTest::testFieldExtraction
Error: Call to a member function getClass() on null

I've added the missing field data definition for the 'entity:test2', avoiding the error:

Drupal\Tests\search_api\Unit\Processor\HighlightTest::testFieldExtraction
TypeError: Drupal\search_api\Utility\FieldsHelper::retrieveNestedProperty(): Argument #1 ($properties) must be of type array, null given, called in /var/www/html/web/modules/contrib/search_api/src/Item/Field.php on line 483

This is the configuration for the test:

        [
          'entity:test1',
          [
            'bar' => new DataDefinition(),
            'foobar' => new DataDefinition(),
            'bar' => $this->createFieldItemDataDefinition('text'),
            'foobar' => $this->createFieldItemDataDefinition('text'),
          ],
        ],
        [
          'entity:test2',
          [
            'foobar' => $this->createFieldItemDataDefinition('text'),
          ],
        ],

Creating the entity:test2, along with the the logger service, is needed and it will solve the error:

Drupal\Tests\search_api\Unit\Processor\HighlightTest::testFieldExtraction
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "logger.channel.search_api".

Which it is expected in the Highlight.php since we will have this configuration:

• field1: Nested property path (entity:test1/bar:foo) - extracts from original object
• field2: Simple property path (entity:test2/foobar) - different datasource
• field3: Processor-generated field (foo) - uses ProcessorProperty
• field4: Non-text field (baz, FALSE) - should be excluded from highlighting
• field5: Pre-existing field values (entity:test1/foobar) - already on item

And then:

      try {
        $property = $field->getDataDefinition();
        if ($property instanceof ConfigurablePropertyInterface) {
          $path .= '|' . $field_id;
        }
      }
      catch (SearchApiException $e) {
        $this->logException($e);
      }

I have "rebased" the branch with the latest changes (it was around ~57 commits behind).

The only test failing is the one related to the issue #3536089: Replace deprecated REQUIREMENT_ERROR constant in search_api_db_defaults_requirements() for Drupal 11+ compatibility.

And my only concern is whether or not we should add the item language to when $missing fields (see my previous comment):

if ($missing_fields) {
        $this->extractFields($item->getOriginalObject(), $missing_fields, $item->getLanguage());

The issue is ready for review as soon as we decide whether or not to include the $item->getLanguage(), which I think we should to it.

Thanks a lot for your work on this, your changes look good to me.
Did you also test the MR on an actual site or did you just fix the tests? If you can confirm this works for you I think this would be RTBC.

However, in any case we have to wait for #3536089: Replace deprecated REQUIREMENT_ERROR constant in search_api_db_defaults_requirements() for Drupal 11+ compatibility (and possible #3523808: Fix failing pipelines?) to fix the pipelines.

If someone could confirm that the latest version of the MR works for them then I could merge this.

Sorry about the delay,
I've tested it in an actual site and it worked for me.

No worries, thanks for reporting back. And good to hear it works for you, too.
Merged.
Thanks again, everyone!